Skip to content

Hparams: Change some handling/generation of hparams with discrete domains. #6489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Jul 13, 2023

Conversation

bmd3k
Copy link
Contributor

@bmd3k bmd3k commented Jul 11, 2023

This addresses several issues and cleanup related to how we generate and handle hparams with discrete domains:

  1. From the backend, always return the list of discrete domain values for a string hparam. Previously we were returning no discrete domain values when the number of values exceeded 10 but this was breaking assumptions in the Angular hparam logic.
  2. In the polymer UI, generate a "regexp" filter for discrete string hparams with greater than 10 values.
  3. Fix regexp filter display logic in the polymer UI. We were never successfully showing the regexp filter. The check [[hparam.filter.regexp]] was incorrect since it would evaluate to false when regexp is the empty string. Instead we must check that hparam.filter.regex !== undefined.
  4. Simplify the logic for generating discrete domain filters in the polymer UI - we just need to do it in a single place now that we can assume that the backend will always return the list of discrete domain values (thanks to item (1) and to previous work done in go/tbpr/6393)

@bmd3k bmd3k requested a review from JamesHollyer July 11, 2023 20:43
Copy link
Contributor

@JamesHollyer JamesHollyer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These changes look good and should fix the issue. I believe this will potentially break the filter control in the runs table as it was not built to accommodate potentially hundreds of discrete values. So we need to make sure we fix that before launching.

I would also like to understand when the domain would be unpopulated.

@@ -88,8 +88,7 @@ message HParamInfo {
// every instance of this hyperparameter will hold a value from this set. It
// is used by the UI to allow filtering so that only session groups (see
// below) whose associated hyperparameter value "passes" the filter are
// displayed. If this is not populated, the domain is assumed to be the
// entire domain of the type of the hyperparameter.
// displayed. If this is not populated, the domain is assumed to be empty.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure I understand this what is meant by an empty domain. I would expect this to always be populated.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible, for example, in hparam summary api to specify that an hparam exists but not provide any values for it for any of the runs.

@rileyajones
Copy link
Contributor

These changes look good and should fix the issue. I believe this will potentially break the filter control in the runs table as it was not built to accommodate potentially hundreds of discrete values. So we need to make sure we fix that before launching.

I would also like to understand when the domain would be unpopulated.

I am still not ready to send #6488 for review so I'll find some way to accommodate this issue prior to large numbers of discrete values prior to sending it for review.

@bmd3k bmd3k merged commit 060fecb into tensorflow:master Jul 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants