HParams: Create hparams effects to trigger data to be fetched from the hparams plugin #6540

rileyajones · 2023-08-10T22:39:45Z

Motivation for features / changes

This is a natural continuation of the work done in #6535

I added a new effects file which fires a newly created action whenever the runs table is shown, a navigation occurs, or the user reloads the data.

The data loading is then filtered and throttled by the existence of experiment ids.

Finally the raw response from the hparams_data_source is written to the redux state. In a future PR I will add a series of selectors to remap and inject that data into the runs metadata.

Alternate designs / implementations considered (or N/A)

I've opted to include the effects and data source in the hparams module but hide the functionality behind the feature flag. This should make it easier to develop changes downstream from this pr. In the future I will gate the existing behavior behind the inverse of this flag and that should allow us to test the full flow without needing to remove the old code (I have tested removing the old code).

The reducer change could have been split out from this but it's quite small and I thought including it made it easier to see the entire write path.

Because I added two new attributes to the hparams redux state there are a few changes to test utilities that are not strictly necessary but are useful to have.

bmd3k · 2023-08-11T14:11:08Z

tensorboard/webapp/hparams/_redux/types.ts

@@ -43,6 +45,8 @@ export const HPARAMS_FEATURE_KEY = 'hparams';

 export interface HparamsState {
  specs: ExperimentToHparams;
+  currentSpecs: HparamAndMetricSpec;


Could you elaborate a bit on what the final data model is going to look like? Introduction of "currentSpecs" and "sessionGroups" isn't clearly going in the direction that I hoped this would. It could just be a naming issue, though.

Ultimately I think the data model should look something like:

export interface HparamsState { listSpecs: Record<string, HparamAndMetricSpec> ; listSessionGroups: Record<string, SessionGroup[]>; listFilters: Record<string, Filters>; dashboardSpecs: HparamAndMetricSpec; dashboardSessionGroups: SessionGroup[]; dashboardFilters: Filters; }

Notably, there is no notion of a single "current" spec since (1) there is also no notion of a "previous" spec and (2) you could have multiple active lists at the same time - multiple in the experiment list and one in the dashboard.

I do think this is a naming issue. I agree that dashboardSpecs seems like a better name as it helps clarify the usease. I chose ccurrentSpecs to indicate that it's value would change as the experiment ids changed, but in hindsight that is implicit with redux properties.

bmd3k · 2023-08-11T14:17:07Z

tensorboard/webapp/hparams/_redux/hparams_effects.ts

+  ) {}
+
+  private readonly runTableShown$: Observable<string[]> = this.actions$.pipe(
+    ofType(runsActions.runTableShown),


If I understand correctly, runTableShown is fired when any run table is shown - either in experiment list or in dashboard, right?

If so, then loading any runs table ultimately dispatches an instance of hparamsFetchSessionGroupsSucceeded which populates the same state over and over - overwriting whatever was there before. The action doesn't clarify which run table you've loaded data for - is it one of the tables in the experiment list or is it the table in the dashboard?

I'm unsure how we can differentiate between them. This does remind me that I had intended to ensure the events are only dispatched on the dashboard routes. How would you feel about that solution?

I think you could encode the information in the action that triggers the logic. For example:

have two different actions runTableShownInExperimentList and runTableShownInDashboard

or make it a property of the runTableShown action. Maybe an enum of type RunTableLocation.

On the other hand, using route information for this seems ok. But just to confirm: Do you intend that this logic will also handle the run tables in the experiment list? And, if so, do you plan to somehow differentiate the two cases when you dispatch the action after retrieving the data - perhaps add a property to hparamsFetchSessionGroupsSucceeded (again maybe an enum type RunTableLocation).

At the moment I do not intend for this to be active on the experiment list at all. Once this portion of the code is stable I plan to do a refactor of the data fetching logic on the experiment list though and at that point I may try to merge these. If that happens I will probably add an enum to the runTableShown action payload.

JamesHollyer · 2023-08-11T17:59:11Z

tensorboard/webapp/hparams/_redux/hparams_effects.ts

+      filter(
+        ([, getEnableHparamsInTimeSeries]) => getEnableHparamsInTimeSeries
+      ),
+      throttleTime(10),


Is the reason for this because when we navigate to timeseries we also trigger runTableShown? So we are just avoiding a duplicate call?

Yeah, that's exactly it.

JamesHollyer · 2023-08-11T18:18:04Z

tensorboard/webapp/hparams/_redux/types.ts

@@ -43,6 +45,8 @@ export const HPARAMS_FEATURE_KEY = 'hparams';

 export interface HparamsState {
  specs: ExperimentToHparams;


Nit: To avoid confusion I think this should be name experimentListSpecs or something like that.

I agree but I'd like to refactor the experiment list portion of the state in a separate PR once the dashboard flow is fully checked in. I think this new flow is better and I am hopeful that we can remove a lot of the old code.

JamesHollyer · 2023-08-11T18:26:00Z

tensorboard/webapp/util/types.ts

@@ -19,3 +19,9 @@ limitations under the License.
 export type DeepReadonly<T> = {
  readonly [P in keyof T]: DeepReadonly<T[P]>;
 };
+
+export type DeepPartial<T> = T extends object


I'm surprised to discover that you can branch type definitions based on conditions on the type parameters . Kind of neat.

bmd3k · 2023-08-14T17:08:50Z

A minor error in the description:

I added a new actions file which fires whenever the runs table is shown, a navigation occurs, or the user reloads the data.

actions file => effects file

bmd3k · 2023-08-14T17:11:11Z

tensorboard/webapp/util/types.ts

@@ -19,3 +19,9 @@ limitations under the License.
 export type DeepReadonly<T> = {
  readonly [P in keyof T]: DeepReadonly<T[P]>;
 };
+
+export type DeepPartial<T> = T extends object


I'm surprised to discover that you can branch type definitions based on conditions on the type parameters . Kind of neat.

bmd3k · 2023-08-14T17:19:44Z

tensorboard/webapp/hparams/_redux/hparams_effects.ts

+  ) {}
+
+  private readonly runTableShown$: Observable<string[]> = this.actions$.pipe(
+    ofType(runsActions.runTableShown),


I think you could encode the information in the action that triggers the logic. For example:

have two different actions runTableShownInExperimentList and runTableShownInDashboard

or make it a property of the runTableShown action. Maybe an enum of type RunTableLocation.

On the other hand, using route information for this seems ok. But just to confirm: Do you intend that this logic will also handle the run tables in the experiment list? And, if so, do you plan to somehow differentiate the two cases when you dispatch the action after retrieving the data - perhaps add a property to hparamsFetchSessionGroupsSucceeded (again maybe an enum type RunTableLocation).

bmd3k · 2023-08-14T17:22:58Z

tensorboard/webapp/hparams/_redux/hparams_actions.ts

@@ -50,3 +54,11 @@ export const hparamsMetricFilterChanged = createAction(
    includeUndefined: boolean;
  }>()
 );
+
+export const hparamsFetchSessionGroupsSucceeded = createAction(


Do you have plans to introduce the other actions we typically also have for data source fetches?

hparamsFetchSessionGroupsRequested
hparamsFetchSessionGroupsFailed

(I'd ok - in fact, I prefer - if these come in a different PR. I just want to make sure they are on your radar).

Yeah, I'll make sure these get added soon. I wanted to get the MVP out as soon as possible to unblock James.

tensorboard/webapp/hparams/_redux/hparams_effects.ts

bmd3k · 2023-08-14T17:37:27Z

tensorboard/webapp/hparams/_redux/hparams_effects_test.ts

+        routeKind: RouteKind.EXPERIMENT,
+        params: {},
+      });
+      store.overrideSelector(selectors.getExperimentIdsFromRoute, ['exp1']);


Should we rename to perhaps 'expFromRoute' to make it a bit clearer where the navigated, reload, and manualReload tests are getting this from?

tensorboard/webapp/hparams/_redux/hparams_effects_test.ts

…ects (#6544) ## Motivation for features / changes This is a continuation of #6540 Now that data is being retrieved from the hparams plugin and written to the store I am adding a pair of selectors to retrieve it and inject it into the the data retrieved from `getRuns` and `getRunsForExperimentId`. The logic used to translate `SessionGroups` to `Record<RunId/Group, Value>` is very similar to that being used by the old code in `runs_data_source`. I could imagine this being refactored to retrieve the run/group relationships from another part of the store in the future in order to better integrate with our plans to stop relying on the metrics portion of the hparams payload. ## Note For Reviewer(s) A large majority of the lines changed in this PR are indent changes resulting from changing the method used to mock the state during tests. I have opted not to add additional test to the runs or common selectors (though I did have to fix 16) as the intention is for this to be a drop in replacement with no new features added. The number of test failures which I had to fix left me feeling confident of the existing test coverage.

…e hparams plugin (tensorflow#6540) ## Motivation for features / changes This is a natural continuation of the work done in tensorflow#6535 I added a new effects file which fires a newly created action whenever the runs table is shown, a navigation occurs, or the user reloads the data. The data loading is then filtered and throttled by the existence of experiment ids. Finally the raw response from the `hparams_data_source` is written to the redux state. In a future PR I will add a series of selectors to remap and inject that data into the runs metadata. ## Alternate designs / implementations considered (or N/A) I've opted to include the effects and data source in the hparams module but hide the functionality behind the feature flag. This should make it easier to develop changes downstream from this pr. In the future I will gate the existing behavior behind the inverse of this flag and that should allow us to test the full flow without needing to remove the old code (I have tested removing the old code). The reducer change could have been split out from this but it's quite small and I thought including it made it easier to see the entire write path. Because I added two new attributes to the hparams redux state there are a few changes to test utilities that are not strictly necessary but are useful to have.

…ects (tensorflow#6544) ## Motivation for features / changes This is a continuation of tensorflow#6540 Now that data is being retrieved from the hparams plugin and written to the store I am adding a pair of selectors to retrieve it and inject it into the the data retrieved from `getRuns` and `getRunsForExperimentId`. The logic used to translate `SessionGroups` to `Record<RunId/Group, Value>` is very similar to that being used by the old code in `runs_data_source`. I could imagine this being refactored to retrieve the run/group relationships from another part of the store in the future in order to better integrate with our plans to stop relying on the metrics portion of the hparams payload. ## Note For Reviewer(s) A large majority of the lines changed in this PR are indent changes resulting from changing the method used to mock the state during tests. I have opted not to add additional test to the runs or common selectors (though I did have to fix 16) as the intention is for this to be a drop in replacement with no new features added. The number of test failures which I had to fix left me feeling confident of the existing test coverage.

## Motivation for features / changes Now that we are fetching the hparams data using the newly created hparams_effects (#6540) we no longer need to fetch them using the old method. Note that the experiment list SHOULD still use the old method.

rileyajones added 2 commits August 10, 2023 22:32

add hparams effects and two new attributes to the hparams redux store

b39d0e0

reformat build files

26f2531

rileyajones requested a review from JamesHollyer August 10, 2023 22:45

rileyajones marked this pull request as ready for review August 10, 2023 22:45

bmd3k self-requested a review August 11, 2023 14:19

bmd3k reviewed Aug 11, 2023

View reviewed changes

rileyajones added 2 commits August 11, 2023 17:47

prefix new redux properties with dashboard

6c9a82b

do not dispatch requests when on the experiments list route

4b44222

rileyajones requested a review from bmd3k August 11, 2023 17:57

JamesHollyer approved these changes Aug 11, 2023

View reviewed changes

bmd3k approved these changes Aug 14, 2023

View reviewed changes

address pr comments

b19357c

rileyajones merged commit d4df603 into tensorflow:master Aug 14, 2023

rileyajones mentioned this pull request Aug 15, 2023

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

Merged

rileyajones mentioned this pull request Aug 28, 2023

HParams: Stop fetching hparams data using run effects #6561

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

HParams: Create hparams effects to trigger data to be fetched from the hparams plugin #6540

HParams: Create hparams effects to trigger data to be fetched from the hparams plugin #6540

rileyajones commented Aug 10, 2023 •

edited

Loading

bmd3k Aug 11, 2023

rileyajones Aug 11, 2023

bmd3k Aug 11, 2023

rileyajones Aug 11, 2023

bmd3k Aug 14, 2023

rileyajones Aug 14, 2023

JamesHollyer Aug 11, 2023

rileyajones Aug 11, 2023

JamesHollyer Aug 11, 2023

rileyajones Aug 11, 2023

JamesHollyer Aug 11, 2023

bmd3k Aug 14, 2023

bmd3k commented Aug 14, 2023

bmd3k Aug 14, 2023

bmd3k Aug 14, 2023

bmd3k Aug 14, 2023

rileyajones Aug 14, 2023

bmd3k Aug 14, 2023

		@@ -43,6 +45,8 @@ export const HPARAMS_FEATURE_KEY = 'hparams';

		export interface HparamsState {
		specs: ExperimentToHparams;

HParams: Create hparams effects to trigger data to be fetched from the hparams plugin #6540

HParams: Create hparams effects to trigger data to be fetched from the hparams plugin #6540

Conversation

rileyajones commented Aug 10, 2023 • edited Loading

Motivation for features / changes

Alternate designs / implementations considered (or N/A)

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bmd3k commented Aug 14, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rileyajones commented Aug 10, 2023 •

edited

Loading