HParams: Update scalar card to read runs with respect for hparams filters #6377
Conversation
rileyajones
force-pushed
the
scalar-card-common-selectors
branch
2 times, most recently
from
f04ca97 to
0630796
Compare
rileyajones
force-pushed
the
scalar-card-common-selectors
branch
from
0630796 to
5f059ee
Compare
| getMetricsCardRangeSelectionEnabled, | ||
| getRun, | ||
| getRunColorMap, | ||
| getCurrentRouteRunSelection, |
There was a problem hiding this comment.
Why did this get moved?
There was a problem hiding this comment.
I removed it and reimported it and expected prettier to put it in the same place 🤷
Something definitely seems off with prettier lately
| } | ||
| ); | ||
|
|
||
| export const factories = { |
There was a problem hiding this comment.
AFAICT this is only used in tests. Why is it not in the TEST_ONLY section?
There was a problem hiding this comment.
In order for the tests to work properly they need to spyOn the same object as the is being used by the code they are testing. There are a few oddities when it comes to testing selector factories, I want to add some additional utilities to help with that in the future
| initialState: appStateFromMetricsState(buildMetricsState()), | ||
| }), | ||
| ], | ||
| providers: [provideMockTbStore()], |
| end: null, | ||
| }); | ||
| store.refreshState(); | ||
| tick(); |
There was a problem hiding this comment.
👎. I am not completely sure why all of these are needed. But I am assuming they are.
There was a problem hiding this comment.
I don't like this either, it does only add ~2ms to the test runtime, but it feels janky. It has something to do with the underlying selector needing to be recreated. I have an idea on how to solve this sort of issue, but I'd like to handle that in a separate PR
| selectors.getCurrentRouteRunSelection, | ||
| new Map([['run1', true]]) | ||
| ); | ||
| store.overrideSelector( |
There was a problem hiding this comment.
I wish these could all go in to before each sections. However, I understand that the run is set here and it does make sense to do this after that.
There was a problem hiding this comment.
I definitely thought about it, but as you suspected I wanted it to mirror the run selection override.
…nTimeSeries` is `true` (#6399) ## Motivation for features / changes We have been discovering issues with the way the hparam domains are defined. This has lead to issues due to #6377 causing scalar cards to appear empty when one of these issues occurs. I attempted to fix this in #6393 but missed the situation where a string type hparam with more than 10 values returns an empty domain (Googlers see tb/3183400180427144287 for an example). I am moving the new filtering logic added in by #6377 behind a flag while we fix this issue on the backend. ## Screenshots of UI changes (or N/A) Screenshots for Googlers only Before: https://screenshot.googleplex.com/AELb2oHqhxa8UNF After: Feature Flag Enabled: https://screenshot.googleplex.com/AcPxsbNNXSSFGx9 Feature Flag Disabled: https://screenshot.googleplex.com/3aEXpfHrGZdzYgZ
2 participants
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)