-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Group colours by regex of the experiment name. #6846
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This PR does not follow our review best practices. Can you please first 1) remove (or split out) the unnecessary changes like BUILD file reordering and 2) at least split the made code into two separate PRs (e.g. redux code first, then the rest) (go/small-cls)
@hoonji I will certainly try then remove the formatting of the BUILD files in this PR. Regarding the change you requested.
Does it just mean separate the formatting of the files and added feature code? |
@AnuarTB I mean separating out the actions, effects, reducers and utils into a separate PR, and putting the component/container logic in a followup PR. I think it's silly to spend too much effort just splitting code, but we should prefer it in cases like this where a split is easy and natural. |
+1 This PR is quite large. It would be very helpful to review if you separated this PR into two parts. |
Ok so If I understood correctly, I should separate store, actions and reducers into first PR and then the views in the second PR? If that's so I will separate now, I was just confused on how should i separate this CL, as it is one big coherent block of code. |
Yeah Exactly
How about putting all the other code in one PR, and the code under |
Have separated Model and View parts to separate PRs. When this PR is submitted, will submit: #6847 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Congratulations on adding a new feature to TB!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks solid to me. Thanks for applying this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
**NOTE**: This PR is a continuation of #6846. ## Motivation for features / changes Users want to color runs by the regex string of the corresponding to runs experiment names. ## Technical description of changes - Added new GroupBy type, REGEX_BY_EXP. - Added a dropdown in dialog window, so users could select between regex for run name or experiment name. ## Screenshots of UI changes (or N/A) N/A since internal change. ## Detailed steps to verify changes work correctly (as executed by you) - Run tensorboard.corp server. - Click on color grouping icon and select `Regex`. - Select `Experiment Name` in dropdown. ## Alternate designs / implementations considered (or N/A) N/A
There was a problem when user tried to 1) enable coloring by experiment name, 2) trying to add more experiments. It stemmed from not implementing the serialization and deserialization of REGEX_BY_EXP in the query pararms of the route. To followup the tensorflow#6846 and tensorflow#6847 I have added the 'regex_exp:' as a query parameter for coloring by experiment name.
There was a problem when user tried to 1) enable coloring by experiment name, 2) trying to add more experiments. It stemmed from not implementing the serialization and deserialization of REGEX_BY_EXP in the query pararms of the route. To followup the tensorflow#6846 and tensorflow#6847 I have added the 'regex_exp:' as a query parameter for coloring by experiment name.
There was a problem when user tried to 1) enable coloring by experiment name, 2) trying to add more experiments. It stemmed from not implementing the serialization and deserialization of REGEX_BY_EXP in the query pararms of the route. To followup the tensorflow#6846 and tensorflow#6847 I have added the 'regex_exp:' as a query parameter for coloring by experiment name.
There was a problem when user tried to 1) enable coloring by experiment name, 2) trying to add more experiments. It stemmed from not implementing the serialization and deserialization of REGEX_BY_EXP in the query pararms of the route. To followup the #6846 and #6847 I have added the 'regex_exp:' as a query parameter for coloring by experiment name. ## Motivation for features / changes To fix the issue with the new feature. ## Technical description of changes Added `regex_exp:` query param for serialization and deserialization of `REGEX_BY_EXP` GroupBy. It ensures that whenever user share the URL the filter will remain in place. ## Screenshots of UI changes (or N/A) ## Detailed steps to verify changes work correctly (as executed by you) 1) Enable the coloring by experiment name. 2) Try to add more experiments via pressing `Add more experiments` in the top panel. ## Alternate designs / implementations considered (or N/A)
Motivation for features / changes
Users want to color runs by the regex string of the corresponding to runs experiment names.
Technical description of changes
Screenshots of UI changes (or N/A)
N/A since internal change.
Detailed steps to verify changes work correctly (as executed by you)
Regex
.Experiment Name
in dropdown.Alternate designs / implementations considered (or N/A)
N/A