Skip to content
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

Group colours by regex of the experiment name. #6846

Merged
merged 13 commits into from
May 8, 2024

Conversation

AnuarTB
Copy link
Contributor

@AnuarTB AnuarTB commented May 3, 2024

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

@AnuarTB AnuarTB requested a review from hoonji May 3, 2024 05:51
@AnuarTB AnuarTB changed the title Regex feature Group colours by regex of the experiment name. May 3, 2024
@AnuarTB AnuarTB requested a review from roseayeon May 3, 2024 06:38
Copy link
Member

@hoonji hoonji left a 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)

@AnuarTB
Copy link
Contributor Author

AnuarTB commented May 3, 2024

@hoonji I will certainly try then remove the formatting of the BUILD files in this PR. Regarding the change you requested.

  1. at least split the made code into two separate PRs (e.g. redux code first, then the rest)

Does it just mean separate the formatting of the files and added feature code?

@hoonji
Copy link
Member

hoonji commented May 7, 2024

@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.

@roseayeon
Copy link
Contributor

I mean separating out the actions, effects, reducers and utils into a separate PR, and putting the component/container logic in a followup PR.

+1 This PR is quite large. It would be very helpful to review if you separated this PR into two parts.

@AnuarTB
Copy link
Contributor Author

AnuarTB commented May 7, 2024

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.

@roseayeon
Copy link
Contributor

roseayeon commented May 7, 2024

I should separate store, actions and reducers into first PR and then the views in the second PR?

Yeah Exactly

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.

How about putting all the other code in one PR, and the code under views/runs_table in a second PR? Does it sound okay?

@AnuarTB
Copy link
Contributor Author

AnuarTB commented May 7, 2024

Have separated Model and View parts to separate PRs. When this PR is submitted, will submit: #6847

Copy link
Contributor

@roseayeon roseayeon left a 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!

tensorboard/webapp/runs/actions/runs_actions.ts Outdated Show resolved Hide resolved
tensorboard/webapp/angular/BUILD Outdated Show resolved Hide resolved
tensorboard/webapp/runs/store/runs_reducers_test.ts Outdated Show resolved Hide resolved
@AnuarTB AnuarTB requested review from hoonji and roseayeon May 8, 2024 06:10
Copy link
Member

@hoonji hoonji left a 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!

tensorboard/webapp/runs/effects/runs_effects.ts Outdated Show resolved Hide resolved
tensorboard/webapp/runs/store/runs_reducers_test.ts Outdated Show resolved Hide resolved
Copy link
Contributor

@roseayeon roseayeon left a comment

Choose a reason for hiding this comment

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

LGTM!

tensorboard/webapp/runs/effects/runs_effects.ts Outdated Show resolved Hide resolved
@AnuarTB AnuarTB merged commit aa54272 into tensorflow:master May 8, 2024
13 checks passed
AnuarTB added a commit that referenced this pull request May 20, 2024
**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
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.

None yet

3 participants