New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Automatically update metric plots for in-progress runs #2099 #5017
Changes from 13 commits
4552061
42bb25e
8af2942
18b9fac
e4a22cb
a91c53c
dbc980a
7777420
70408f2
164eb69
90c976b
df7ad32
c0895fe
c41d94f
712f9a7
037cefb
2bb4b62
a5ed069
ebad40f
643e2ce
f246b63
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -2,11 +2,11 @@ import React from 'react'; | |
import { connect } from 'react-redux'; | ||
import Utils from '../../common/utils/Utils'; | ||
import RequestStateWrapper from '../../common/components/RequestStateWrapper'; | ||
import { getMetricHistoryApi } from '../actions'; | ||
import { getMetricHistoryApi, getRunApi } from '../actions'; | ||
import PropTypes from 'prop-types'; | ||
import _ from 'lodash'; | ||
import { MetricsPlotView } from './MetricsPlotView'; | ||
import { getRunTags } from '../reducers/Reducers'; | ||
import { getRunTags, getRunInfo } from '../reducers/Reducers'; | ||
import { | ||
MetricsPlotControls, | ||
X_AXIS_WALL, | ||
|
@@ -22,10 +22,16 @@ import { getUUID } from '../../common/utils/ActionUtils'; | |
export const CHART_TYPE_LINE = 'line'; | ||
export const CHART_TYPE_BAR = 'bar'; | ||
|
||
// Polling interval | ||
export const METRICS_PLOT_POLLING_INTERVAL_MS = 5000; | ||
// Stop polling when the polling duration exceeds this value | ||
export const METRICS_PLOT_POLLING_DURATION_MS = 3600 * 1000; // 1 hour | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should only stop polling when there's no new data. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @dbczumar I remember we discussed that we should set an appropriate polling threshold when a run never ends, but not setting such a threshold sounds ok to me because runs end in most cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Offline discussion: check the timestamp of the last metric, and if it's more than 1 week, then we won't refresh. |
||
|
||
export class MetricsPlotPanel extends React.Component { | ||
static propTypes = { | ||
experimentId: PropTypes.string.isRequired, | ||
runUuids: PropTypes.arrayOf(PropTypes.string).isRequired, | ||
completedRunUuids: PropTypes.arrayOf(PropTypes.string).isRequired, | ||
metricKey: PropTypes.string.isRequired, | ||
// A map of { runUuid : { metricKey: value } } | ||
latestMetricsByRunUuid: PropTypes.object.isRequired, | ||
|
@@ -34,6 +40,7 @@ export class MetricsPlotPanel extends React.Component { | |
// An array of { metricKey, history, runUuid, runDisplayName } | ||
metricsWithRunInfoAndHistory: PropTypes.arrayOf(PropTypes.object).isRequired, | ||
getMetricHistoryApi: PropTypes.func.isRequired, | ||
getRunApi: PropTypes.func.isRequired, | ||
location: PropTypes.object.isRequired, | ||
history: PropTypes.object.isRequired, | ||
runDisplayNames: PropTypes.arrayOf(PropTypes.string).isRequired, | ||
|
@@ -71,11 +78,65 @@ export class MetricsPlotPanel extends React.Component { | |
popoverX: 0, | ||
popoverY: 0, | ||
popoverRunItems: [], | ||
focused: true, | ||
}; | ||
this.displayPopover = false; | ||
this.intervalId = null; | ||
this.mountedAt = null; | ||
this.loadMetricHistory(this.props.runUuids, this.getUrlState().selectedMetricKeys); | ||
} | ||
|
||
onFocus = () => { | ||
this.setState({ focused: true }); | ||
}; | ||
|
||
onBlur = () => { | ||
this.setState({ focused: false }); | ||
}; | ||
|
||
clearIntervalIfExists = () => { | ||
if (this.intervalId) { | ||
clearInterval(this.intervalId); | ||
} | ||
}; | ||
|
||
allRunsCompleted = () => { | ||
return this.props.completedRunUuids.length === this.props.runUuids.length; | ||
}; | ||
|
||
exceedsPollingDuration = () => { | ||
if (!this.mountedAt) { | ||
return false; | ||
} | ||
return new Date().getTime() - this.mountedAt >= METRICS_PLOT_POLLING_DURATION_MS; | ||
}; | ||
|
||
componentDidMount() { | ||
this.mountedAt = new Date().getTime(); | ||
window.addEventListener('focus', this.onFocus); | ||
window.addEventListener('blur', this.onBlur); | ||
|
||
if (!this.allRunsCompleted()) { | ||
this.intervalId = setInterval(() => { | ||
if (this.state.focused) { | ||
const { completedRunUuids, runUuids } = this.props; | ||
const uncompletedRuns = _.difference(runUuids, completedRunUuids); | ||
this.loadMetricHistory(uncompletedRuns, this.getUrlState().selectedMetricKeys); | ||
this.loadRuns(uncompletedRuns); | ||
} | ||
if (this.exceedsPollingDuration() || this.allRunsCompleted()) { | ||
this.clearIntervalIfExists(); | ||
} | ||
}, METRICS_PLOT_POLLING_INTERVAL_MS); | ||
} | ||
} | ||
|
||
componentWillUnmount() { | ||
window.removeEventListener('focus', this.onFocus); | ||
window.removeEventListener('blur', this.onBlur); | ||
this.clearIntervalIfExists(); | ||
} | ||
|
||
getUrlState() { | ||
return Utils.getMetricPlotStateFromUrl(this.props.location.search); | ||
} | ||
|
@@ -149,6 +210,16 @@ export class MetricsPlotPanel extends React.Component { | |
return requestIds; | ||
}; | ||
|
||
loadRuns = (runUuids) => { | ||
const requestIds = []; | ||
runUuids.forEach((runUuid) => { | ||
const id = getUUID(); | ||
this.props.getRunApi(runUuid); | ||
requestIds.push(id); | ||
}); | ||
return requestIds; | ||
}; | ||
|
||
getMetrics = () => { | ||
/* eslint-disable no-param-reassign */ | ||
const state = this.getUrlState(); | ||
|
@@ -471,6 +542,8 @@ export class MetricsPlotPanel extends React.Component { | |
return ( | ||
<div className='metrics-plot-container'> | ||
<MetricsPlotControls | ||
numRuns={this.props.runUuids.length} | ||
numCompletedRuns={this.props.completedRunUuids.length} | ||
distinctMetricKeys={distinctMetricKeys} | ||
selectedXAxis={selectedXAxis} | ||
selectedMetricKeys={selectedMetricKeys} | ||
|
@@ -534,6 +607,9 @@ const mapStateToProps = (state, ownProps) => { | |
return latestMetrics ? Object.keys(latestMetrics) : []; | ||
}); | ||
const distinctMetricKeys = [...new Set(metricKeys)].sort(); | ||
const completedRunUuids = runUuids.filter( | ||
(runUuid) => getRunInfo(runUuid, state).status !== 'RUNNING', | ||
); | ||
|
||
const runDisplayNames = []; | ||
|
||
|
@@ -561,9 +637,10 @@ const mapStateToProps = (state, ownProps) => { | |
latestMetricsByRunUuid, | ||
distinctMetricKeys, | ||
metricsWithRunInfoAndHistory, | ||
completedRunUuids, | ||
}; | ||
}; | ||
|
||
const mapDispatchToProps = { getMetricHistoryApi }; | ||
const mapDispatchToProps = { getMetricHistoryApi, getRunApi }; | ||
|
||
export default withRouter(connect(mapStateToProps, mapDispatchToProps)(MetricsPlotPanel)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we increase this to 10 seconds? 5 seems aggressive.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In general, what happens if the refresh fails? Does the page crash?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when-request-fails.mov