Skip to content

Hparam: Add selectors to retrieve data written by the new hparams effects #6544

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.

Sign up for GitHub

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

Merged
merged 11 commits into from
Aug 21, 2023
Merged

Hparam: Add selectors to retrieve data written by the new hparams effects #6544

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
11 commits
Select commit Hold shift + click to select a range
93a76c2
add new selector and use it to inject hparams data into the runs sele…
rileyajones Aug 14, 2023
5bf172f
add tests for new selectors
rileyajones Aug 15, 2023
85ad5c5
remove fit and fix remaining tests
rileyajones Aug 15, 2023
3887ef6
rename new selector
rileyajones Aug 16, 2023
6a95104
ensure dashboard hparam data cannot be injected on the experiment lis…
rileyajones Aug 16, 2023
e0a06b7
rename getRunsFromExperimentIds to getDashboardRuns. getDashboardRuns…
rileyajones Aug 17, 2023
63783af
Merge branch 'master' into hparam-selectors
rileyajones Aug 17, 2023
fd0ecab
address some comments and return to using state builders in tests
rileyajones Aug 18, 2023
c408707
add missing properties to app state and resolve circular dependency
rileyajones Aug 18, 2023
d9c509e
revert some more tests to using state builders
rileyajones Aug 19, 2023
71370ab
fix lint error, rename some helpers
rileyajones Aug 21, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions tensorboard/webapp/hparams/_redux/BUILD
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -168,6 +168,7 @@ tf_ts_library(
"//tensorboard/webapp/runs/data_source:testing",
"//tensorboard/webapp/runs/store:testing",
"//tensorboard/webapp/testing:utils",
"//tensorboard/webapp/util:types",
"//tensorboard/webapp/webapp_data_source:http_client_testing",
"@npm//@ngrx/effects",
"@npm//@ngrx/store",
Expand Down
56 changes: 55 additions & 1 deletion tensorboard/webapp/hparams/_redux/hparams_selectors.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,13 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/
import {createFeatureSelector, createSelector} from '@ngrx/store';
import {DiscreteFilter, HparamAndMetricSpec, IntervalFilter} from '../types';
import {
DiscreteFilter,
HparamAndMetricSpec,
HparamValue,
IntervalFilter,
RunToHparamsAndMetrics,
} from '../types';
import {combineHparamAndMetricSpecs} from './hparams_selectors_utils';
import {HparamsState, HPARAMS_FEATURE_KEY} from './types';
import {
Expand Down Expand Up @@ -178,3 +184,51 @@ export const getExperimentsHparamsAndMetricsSpecs = createSelector(
);
}
);

export const getDashboardHparamsAndMetricsSpecs = createSelector(
getHparamsState,
(state: HparamsState) => {
return state.dashboardSpecs;
}
);

export const getDashboardRunsToHparamsAndMetrics = createSelector(
getHparamsState,
(state): RunToHparamsAndMetrics => {
const runToHparamsAndMetrics: RunToHparamsAndMetrics = {};

for (const sessionGroup of state.dashboardSessionGroups) {
const hparams: HparamValue[] = Object.entries(sessionGroup.hparams).map(
(keyValue) => {
const [hparam, value] = keyValue;
return {name: hparam, value};
}
);

for (const session of sessionGroup.sessions) {
runToHparamsAndMetrics[session.name] = {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It probably doesn't matter since this is all going to get rewritten, but in case you decide to carry over this logic: I don't think this is desirable. session.name does not guarantee even being the name of a run.

I don't think runs_data_source does this but I only took a quick look.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is essentially what runs_data_source does on lines 171-188
In practice Session.name seems to always be a run name. If no mapping between Session and run exists we cannot display hparams by runs.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is in runs_data_source.ts:L171-188. Those lines of code only generate runToHparamsAndMetrics keys from combination of session.name + metricValue.name.group (with some special handling for when group is empty).

This statement here is generating keys from session.name alone.

To illustrate this, the test at runs_data_source_test.ts:L63 does not generate a runToHparamsAndMetrics entry with key 'eid/run_name_2'.

In practice Session.name seems to always be a run name.

It is not. Sometimes it is a run name. Sometimes it is a common prefix of a set of run names. Sometimes it is an experiment id. Sometimes it is empty.

Even though this will be refactored soon, it would be reassuring if we could agree on this before merging the PR, especially since it's possible I'll be OOO during the refactor.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We discussed offline and it seems like we are on the same page. We will not generate data for runs that do not exist - more specifically we do not generate data based solely on session name.

metrics: [],
hparams,
};

for (const metricValue of session.metricValues) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I recall in a recent discussion that we decided we didn't want metrics from the /session_groups response to appear in the Time Series runs table. You would instead be using the metric values from elsewhere in the data model - from the /timeSeries response perhaps?

We thought that we could therefore turn off metric calculation for /session_groups requests for the dashboard. We'd save a bunch of unneccesary processing and reduce latency.

With that in mind, is there any need for parsing the session metric values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an offline discussion:

  • We will stop relying on this portion of the response for data.
  • We should calculate these values if they exist in the store particularly considering this is essentially the same as the logic found in the runs data source.

const runId = metricValue.name.group
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On the other hand, without metric values from the /session_groups requests, you wouldn't be able to calculate runIds in this way - you'd have to find a different approach. Perhaps get a list of runs for each experiment (I believe you get this elsewhere, too) and then determine which session groups they belong to?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an offline discussion:

  • We will calculate this mapping using existing data on the front end (probably from the /runs response) paired with the Session.name value.

? `${session.name}/${metricValue.name.group}`
: session.name;

const hparamsAndMetrics = runToHparamsAndMetrics[runId] || {
metrics: [],
hparams,
};
hparamsAndMetrics.metrics.push({
tag: metricValue.name.tag,
trainingStep: metricValue.trainingStep,
value: metricValue.value,
});
runToHparamsAndMetrics[runId] = hparamsAndMetrics;
}
}
}
return runToHparamsAndMetrics;
}
);
241 changes: 241 additions & 0 deletions tensorboard/webapp/hparams/_redux/hparams_selectors_test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ See the License for the specific language governing permissions and
limitations under the License.
==============================================================================*/

import {SessionGroup} from '../types';
import * as selectors from './hparams_selectors';
import {
buildDiscreteFilter,
Expand All @@ -21,6 +22,8 @@ import {
buildHparamsState,
buildIntervalFilter,
buildMetricSpec,
buildMetricsValue,
buildSessionGroup,
buildSpecs,
buildStateFromHparamsState,
} from './testing';
Expand Down Expand Up @@ -466,6 +469,244 @@ describe('hparams/_redux/hparams_selectors_test', () => {
});
});

describe('#getDashboardHparamsAndMetricsSpecs', () => {
it('returns dashboard specs', () => {
const state = buildStateFromHparamsState(
buildHparamsState({
dashboardSpecs: {
hparams: [buildHparamSpec({name: 'foo'})],
metrics: [buildMetricSpec({tag: 'bar'})],
},
})
);

expect(selectors.getDashboardHparamsAndMetricsSpecs(state)).toEqual({
hparams: [buildHparamSpec({name: 'foo'})],
metrics: [buildMetricSpec({tag: 'bar'})],
});
});
});

describe('#getDashboardRunsToHparamsAndMetrics', () => {
let mockSessionGroups: SessionGroup[];
beforeEach(() => {
mockSessionGroups = [
buildSessionGroup({
name: 'session_group_1',
hparams: {
hp1: 1,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use quotes for keys in 'hparams': 'hp1', 'hp2', etc...
You will otherwise get lint errors when you import. (The import would still suceed but might as well keep it as clean as we can when we see things in advance).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The OSS linter doesn't like that.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The definition of 'hparams' is:

  hparams: {
    [hparamName: string]: boolean | number | string;
  };

It says the keys should be strings.

Perhaps there is confusion because I quoted 'hparams' as well. I perhaps should have used hparams instead to try to make it clearer that you should just quote the keys.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Step 1:
image

Step 2:
image

Step 3:
image

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. Sorry, my bad. It's surprising to me but I guess makes sense when I realize that prettier is not the typescript compiler and probably doesn't know what has string keys and what does not.

I could have tried this out myself which would have saved some back and forth.

I looked briefly into prettier option "--quote-props=preserve", which would allow us to quote the property names. But perhaps its best to leave things alone and see how this quote issue plays out when prettier is adopted internally.

hp2: true,
hp3: 'foo',
},
sessions: [
{
name: 'exp1/run1',
metricValues: [
buildMetricsValue({name: {tag: 'foo', group: '1'}, value: 2}),
buildMetricsValue({name: {tag: 'bar', group: '2'}, value: 103}),
],
},
{
name: 'exp1/run2',
metricValues: [
buildMetricsValue({name: {tag: 'foo', group: '1'}, value: 3}),
buildMetricsValue({name: {tag: 'bar', group: '2'}, value: 104}),
buildMetricsValue({name: {tag: 'baz', group: '3'}, value: 201}),
],
},
],
}),
buildSessionGroup({
name: 'session_group_2',
hparams: {
hp1: 2,
hp2: false,
hp3: 'bar',
},
sessions: [
{
name: 'exp1/run3',
metricValues: [
buildMetricsValue({name: {tag: 'foo', group: '1'}, value: 4}),
buildMetricsValue({name: {tag: 'bar', group: '2'}, value: 105}),
],
},
],
}),
buildSessionGroup({
name: 'session_group_3',
hparams: {
hp4: 'hyperparameter4',
},
sessions: [
{
name: 'exp1/run4',
metricValues: [],
},
],
}),
buildSessionGroup({
name: 'session_group_4',
hparams: {
hp1: 7,
hp2: false,
hp3: 'foobar',
},
sessions: [
{
name: 'exp2/run1',
metricValues: [
buildMetricsValue({name: {tag: 'foo', group: '1'}, value: 4}),
buildMetricsValue({name: {tag: 'bar', group: '2'}, value: 105}),
buildMetricsValue({
name: {tag: 'baz', group: '2'},
value: 1000,
}),
],
},
],
}),
];
});

it('contains entry for each runId', () => {
const state = buildStateFromHparamsState(
buildHparamsState({
dashboardSessionGroups: mockSessionGroups,
})
);

expect(
selectors.getDashboardRunsToHparamsAndMetrics(state)['exp1/run4']
).toEqual({
metrics: [],
hparams: [{name: 'hp4', value: 'hyperparameter4'}],
});
});

it('contains entry for each runId/group', () => {
const state = buildStateFromHparamsState(
buildHparamsState({
dashboardSessionGroups: mockSessionGroups,
})
);

expect(selectors.getDashboardRunsToHparamsAndMetrics(state)).toEqual({
'exp1/run1': {
metrics: [],
hparams: [
{name: 'hp1', value: 1},
{name: 'hp2', value: true},
{name: 'hp3', value: 'foo'},
],
},
'exp1/run1/1': {
metrics: [{tag: 'foo', trainingStep: 0, value: 2}],
hparams: [
{name: 'hp1', value: 1},
{name: 'hp2', value: true},
{name: 'hp3', value: 'foo'},
],
},
'exp1/run1/2': {
metrics: [{tag: 'bar', trainingStep: 0, value: 103}],
hparams: [
{name: 'hp1', value: 1},
{name: 'hp2', value: true},
{name: 'hp3', value: 'foo'},
],
},
'exp1/run2': {
metrics: [],
hparams: [
{name: 'hp1', value: 1},
{name: 'hp2', value: true},
{name: 'hp3', value: 'foo'},
],
},
'exp1/run2/1': {
metrics: [{tag: 'foo', trainingStep: 0, value: 3}],
hparams: [
{name: 'hp1', value: 1},
{name: 'hp2', value: true},
{name: 'hp3', value: 'foo'},
],
},
'exp1/run2/2': {
metrics: [{tag: 'bar', trainingStep: 0, value: 104}],
hparams: [
{name: 'hp1', value: 1},
{name: 'hp2', value: true},
{name: 'hp3', value: 'foo'},
],
},
'exp1/run2/3': {
metrics: [{tag: 'baz', trainingStep: 0, value: 201}],
hparams: [
{name: 'hp1', value: 1},
{name: 'hp2', value: true},
{name: 'hp3', value: 'foo'},
],
},
'exp1/run3': {
metrics: [],
hparams: [
{name: 'hp1', value: 2},
{name: 'hp2', value: false},
{name: 'hp3', value: 'bar'},
],
},
'exp1/run3/1': {
metrics: [{tag: 'foo', trainingStep: 0, value: 4}],
hparams: [
{name: 'hp1', value: 2},
{name: 'hp2', value: false},
{name: 'hp3', value: 'bar'},
],
},
'exp1/run3/2': {
metrics: [{tag: 'bar', trainingStep: 0, value: 105}],
hparams: [
{name: 'hp1', value: 2},
{name: 'hp2', value: false},
{name: 'hp3', value: 'bar'},
],
},
'exp1/run4': {
metrics: [],
hparams: [{name: 'hp4', value: 'hyperparameter4'}],
},
'exp2/run1': {
metrics: [],
hparams: [
{name: 'hp1', value: 7},
{name: 'hp2', value: false},
{name: 'hp3', value: 'foobar'},
],
},
'exp2/run1/1': {
metrics: [{tag: 'foo', trainingStep: 0, value: 4}],
hparams: [
{name: 'hp1', value: 7},
{name: 'hp2', value: false},
{name: 'hp3', value: 'foobar'},
],
},
'exp2/run1/2': {
metrics: [
{tag: 'bar', trainingStep: 0, value: 105},
{tag: 'baz', trainingStep: 0, value: 1000},
],
hparams: [
{name: 'hp1', value: 7},
{name: 'hp2', value: false},
{name: 'hp3', value: 'foobar'},
],
},
});
});
});

it('does not use default filters when includeDefaults is false', () => {
const state = buildStateFromHparamsState(
buildHparamsState({
Expand Down
Loading