Bug Fix: update hparams plugin to generate domains for boolean hparams #6393
Conversation
rileyajones
force-pushed
the
boolean-domains
branch
3 times, most recently
from
e862eb2
to
0c4c8c9
Compare
rileyajones
force-pushed
the
boolean-domains
branch
from
0c4c8c9
to
fdbe1ef
Compare
There was a problem hiding this comment.
The change makes sense to me. However, it is worth noting that the hparams dashboard code already works around this backend limitation:
Can/did you:
- Check to see how this change impacts the existing hparams dashboard?
- Consider changing the hparams dashboard code to read the domain from the backend result rather than generating the domain itself?
| } | ||
| """ | ||
| ctxt = backend_context.Context( | ||
| self._mock_tb_context, max_domain_discrete_len=1 |
There was a problem hiding this comment.
Probably clearer not to specify max_domain_discrete_len since it has no impact on the result.
| """ | ||
| self.session_2_start_info_ = """ | ||
| hparams:[ | ||
| {key: 'batch_size' value: {bool_value: false}} |
There was a problem hiding this comment.
What's interesting about the logic you've added is that the domain is [True, False] no matter what the values are in *start_info_. I think it would be worth acknowledging that by changing this test case (or adding new ones) so that only a single value (one of True or one of False) is specied in *start_info_.
There was a problem hiding this comment.
That's a good point, I'll make them both True here so that it is more self evident
I check the hparams dashboard and added more screenshots to the cl I linked in the description. TLDR it does effect it but not in a meaningful way (the filter is just moved) I will try to remember to update the hparams dashboard the next time that I am on call. |
Perhaps the best way to remember is to create a bug and assign it to yourself. Or add it to your hparams-in-timeseries task list and assign it to yourself. |
b/283462407 |
…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
Motivation for features / changes
This has been a bug for a while that we only noticed now that hparams are being used to filter runs in the time series dashboard.
Googlers see https://chat.google.com/room/AAAA03izhrk/VyyPgojcNvY for context on how this was discovered
Technical description of changes
Boolean HParams were not having a domain set. This lead to the ui treating them as intervals, however, this lead to incorrect filter conditions. We currently treat all non number values being filtered this way as not matching (the alternative would be to treat them all as being true which also seems wrong). This leads to all runs with a value for a boolean hparam being filtered out.
Screenshots of UI changes (or N/A)
Googlers see cl/532932348
Alternate designs / implementations considered (or N/A)
This could have been done on the client but ideally the bug would be fixed in other places where the api is being used.