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

Hparams: fix metrics loading when group name is "." #6822

Merged
merged 2 commits into from
Apr 8, 2024

Conversation

yatbear
Copy link
Member

@yatbear yatbear commented Apr 4, 2024

In the Hparams plugin, when fetching metrics (scalar data) for multiple experiments in the comparison view, we can't replace group name . with an empty string.

Googlers, see b/332308243 for more details.

Tested internally: cl/582334596

#hparams #oncall

Copy link
Contributor

@bmd3k bmd3k left a comment

Choose a reason for hiding this comment

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

This (mostly) makes sense to me. Is this the correct interpretation?

During /session_groups calls: The call to normpath was modifying run names from self._tb_context.data_provider.read_last_scalars. I thought this was a good idea originally (#6791) but, no, it turns out it was not.

During /metric_evals calls: The reason it was not a good idea was that we ended up passing the wrong run names to self._scalars_plugin_instance.scalars_impl. The run names were invalid. There is basically no way to reliably reverse the modifications from the normpath() call back to the original run name.

So your solution is to avoid modifying run names. Instead you change the rest of the code to special case the "." run name case.

@yatbear
Copy link
Member Author

yatbear commented Apr 5, 2024

This (mostly) makes sense to me. Is this the correct interpretation?

During /session_groups calls: The call to normpath was modifying run names from self._tb_context.data_provider.read_last_scalars. I thought this was a good idea originally (#6791) but, no, it turns out it was not.

During /metric_evals calls: The reason it was not a good idea was that we ended up passing the wrong run names to self._scalars_plugin_instance.scalars_impl. The run names were invalid. There is basically no way to reliably reverse the modifications from the normpath() call back to the original run name.

So your solution is to avoid modifying run names. Instead you change the rest of the code to special case the "." run name case.

During /session_groups calls: The run names were already normalized incorrectly before your change:

(run, tag) = metrics.run_tag_from_session_and_metric(
and #6791 did fix most other cases when the group name isn't ".", this is a pre-existing issue where run name "." wasn't accounted for. I reverted #6791 because the path normalization in the metrics util needs to be removed.

@yatbear yatbear merged commit 309771d into tensorflow:master Apr 8, 2024
13 checks passed
@yatbear yatbear deleted the hparams branch April 8, 2024 13:39
AnuarTB pushed a commit to AnuarTB/tensorboard that referenced this pull request May 3, 2024
In the Hparams plugin, when fetching metrics (scalar data) for multiple experiments in the comparison view, we can't replace group name `.` with an empty string.

Googlers, see b/332308243 for more details.

Tested internally: cl/582334596

#hparams #oncall
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants