HParams: Add common selector to get runs filtered by hparams #6366
Conversation
rileyajones
force-pushed
the
common-selectors
branch
from
0eb9afb to
bdbb475
Compare
| return createSelector( | ||
| getRunsFromExperimentIds(experimentIds), | ||
| getExperimentNames(experimentIds), | ||
| getCurrentRouteRunSelection, |
There was a problem hiding this comment.
I am a bit confused here. This selector returns null when current route is not about an experiment. So how does this work in OSS?
There was a problem hiding this comment.
In OSS the route is always an experiment. see getExperimentIdsFromRouteParams
in tensorboard/webapp/app_routing/internal_utils.ts. TLDR it returns DEFAULT_EXPERIMENT_ID e.g. defaultExperimentId when no experiment ids are found in the route
| } | ||
| if (filter.type === DomainType.DISCRETE) { | ||
| // (upcast to work around bad TypeScript libdefs) | ||
| const values: Readonly<Array<typeof filter.filterValues[number]>> = |
There was a problem hiding this comment.
this type is confusing me. What does the "number" thing do? DiscreteHparamValues are supposed to be arrays of numbers, strings, or booleans.
There was a problem hiding this comment.
This is confusing to me too (I didn't write it) but it seems to work and I don't really want to change it.
| expect( | ||
| selectors.TEST_ONLY.utils.filterRunItemsByRegex( | ||
| runTableItems, | ||
| 'run1', |
There was a problem hiding this comment.
If you make the 3rd run run3 I think it will serve this test well to make it 'run[13]'.
tensorboard/webapp/metrics/views/main_view/common_selectors_test.ts
Outdated
Show resolved
Hide resolved
| ).toEqual(0); | ||
| }); | ||
|
|
||
| it('filters by hparams using interval filters', () => { |
There was a problem hiding this comment.
Not sure we need to test the different types of hparam or metric filters as that is already tested in the matchfilter test. I am ok with leaving them in though.
| const spy = spyOn( | ||
| selectors.TEST_ONLY.utils, | ||
| 'filterRunItemsByHparamAndMetricFilter' | ||
| ).and.callThrough(); |
There was a problem hiding this comment.
I think it might be nice to ensure it returns the result of the filtering. Maybe mock it to return one of the runs and then test that it is in the results? Also, ok with this if you feel it is not necessary.
…ters (#6377) ## Motivation for features / changes As part of the effort to bring hparams data to the time series dashboard we are expanding the hparams filtering functionality in the runs table to effect the runs being shown in rest of the dashboard Now that #6366 is merged, the rest of the dashboard can now access the list of runs with respect for any hparam filters a user has applied. ## Screenshots of UI changes (or N/A) No Filters  Several HParam Filters Applied  Regex and HParam Filter Applied  Only a Regex Filter Applied  All Runs Selected  1 Run Deselected  ## Detailed steps to verify changes work correctly (as executed by you) 1) Start tensorboard with a logdir which contains hparam data 2) Navigate to http://localhost:6006?enableHparamsInTimeSeries 3) Filter the runs table by regex and ensure scalar cards reflect this 4) Unselect some runs and ensure scalar cards reflect this 5) Filter the runs table by an hparam and ensure scalar cards reflect this.
2 participants
Motivation for features / changes
The runs table currently contains the logic to filter runs by hparams, unfortunately, this logic is not shared with the rest of the dashboard and so filtering runs this way does not actually effect what is shown on the dashboard.
Technical description of changes
I moved out a lot of utility functions from the runs table container into common selectors. I then created three selectors
getRenderableRunsA private selector to help with the readability of
getFilteredRenderableRunsGets all runs associated with the provided experiment ids
getFilteredRenderableRunsGets all runs associated with the provided experiment ids while accounting for filters.
getFilteredRenderableRunsFromRouteRetrieves the experiment ids from the route then gets the filtered runs associated with them
NOTE: The utility functions are encapsulated in a utils object so that they can be spied on during testing
See the PRs leading up to this
#6340 #6360 #6361 #6323 #6324
Screenshots of UI changes (or N/A)
N/A