HParams: Add new experiments selector #6324
Conversation
JamesHollyer
left a comment
JamesHollyer
left a comment
There was a problem hiding this comment.
I would like to know how this is going to be used before giving the LGTM. Can you link to a branch or PR of a POC that shows how it is used?
| "//tensorboard/webapp/core/views:test_lib", | ||
| "//tensorboard/webapp/customization:customization_test_lib", | ||
| "//tensorboard/webapp/deeplink:deeplink_test_lib", | ||
| "//tensorboard/webapp/experiments/store:store_test_lib", |
| }); | ||
| }); | ||
|
|
||
| it('does not include experiments which are not found', () => { |
There was a problem hiding this comment.
This is already tested in the last test with the 'baz' id. Maybe this should test that an empty object is returned when none of the experiments are found?
There was a problem hiding this comment.
That's a good point. I already tested that empty object is returned when nothing is found in the test on line 80
There was a problem hiding this comment.
Line 80 tests an empty object is returned with nothing is PROVIDED. Which I would say is significantly different than if nothing is found.
Here's the CL that I showed you last week with everything being used cl/523834827 |
JamesHollyer
left a comment
JamesHollyer
left a comment
There was a problem hiding this comment.
Ah Ok. It is used inside that common selector just to add the experiment names to the object returned. Looks good.
| }); | ||
| }); | ||
|
|
||
| it('does not include experiments which are not found', () => { |
There was a problem hiding this comment.
Line 80 tests an empty object is returned with nothing is PROVIDED. Which I would say is significantly different than if nothing is found.
## Motivation for features / changes As part of the effort to bring hparams to the time series dashboard a handful of new selectors need to be created. In particular I will be creating a new one to handle runs filtering by moving the logic out of the runs_table_container into the selector layer. This will ensure filtered runs are consistent across the dashboard. Getting a mapping of experiment id to name is a prerequisite for this. Also the experiments tests weren't being run? ## Technical description of changes I created a new selector factory which takes in a list of experiment ids and creates a `Record<ExperimentId, ExperimentName>` ## Screenshots of UI changes None ## Detailed steps to verify changes work correctly (as executed by you) The tests should pass (note that the webapp experiments tests should be run)
## 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 1) `getRenderableRuns` A private selector to help with the readability of `getFilteredRenderableRuns` Gets all runs associated with the provided experiment ids 2) `getFilteredRenderableRuns` Gets all runs associated with the provided experiment ids while accounting for filters. 3) `getFilteredRenderableRunsFromRoute` Retrieves 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
2 participants
Motivation for features / changes
As part of the effort to bring hparams to the time series dashboard a handful of new selectors need to be created. In particular I will be creating a new one to handle runs filtering by moving the logic out of the runs_table_container into the selector layer.
This will ensure filtered runs are consistent across the dashboard.
Getting a mapping of experiment id to name is a prerequisite for this.
Also the experiments tests weren't being run?
Technical description of changes
I created a new selector factory which takes in a list of experiment ids and creates a
Record<ExperimentId, ExperimentName>Screenshots of UI changes
None
Detailed steps to verify changes work correctly (as executed by you)
The tests should pass (note that the webapp experiments tests should be run)