-
Notifications
You must be signed in to change notification settings - Fork 1.7k
hparams: Include hparams without filterable values in session group requests #5971
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
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
nfelt
approved these changes
Oct 13, 2022
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.
Thanks for the detailed description and test plan! 🦀
qihach64
pushed a commit
to qihach64/tensorboard
that referenced
this pull request
Dec 19, 2022
…equests (tensorflow#5971) === Motivation for features / changes === There are bugs in the hparams dashboard (Googlers, see b/239749612 and b/239749837) where the root cause appears to be the fact that some hparams are excluded from session_groups requests. Attempting to sort by the excluded column or any column to the right of it, column "i", would in fact sort by column "i+1". We have an "off-by-one" error. The code that excludes the column from the session_groups request is here: * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/hparams/tf_hparams_query_pane/tf-hparams-query-pane.ts;l=822-829;drc=dc4754cf27d55c289b85a5ae77591f8d96fb682b * The column is being excluded because its metadata does not include a filter. Digging further, we see that this particular column is excluded because it is deemed to have too many possible discrete values: * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/hparams/backend_context.py;l=270;drc=049f875d96c19afe439d0ee4d13f78c7b599169a === Technical description of changes === The fix proposed here is to delete the code that omits these columns from the request. Trying to guess the history here, it seems like the code was added when this was a clear error condition but later we added cases where this is expected. Examining the backend code, this seems like a safe change. * The backend code already handles the case where a column does not contain a filter: * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/hparams/list_session_groups.py;l=439-474;drc=6996d71b9e0bad19a895a37a8bea8d7807775c39 * A column missing a filter indicates that no values should be filtered. This is the same behavior as just not including the column at all. * The backend code would already return a column in the response even if that column was not specified in the request. So including the column in the request when it previously was not included does not seem to have any impact on the response. === Detailed steps to verify changes work correctly (as executed by you) === 1. I generated an hparams data set with this colab notebook: * https://colab.research.google.com/drive/1FoMTeuFdoeMTnlKrb6vrvSOmLTjkX6ga#scrollTo=TB0wBWfcVqHz * It contains one column (kernel_init) with 12 discrete values. 2. Load a tensorboard with the logs and: * Verify kernel_init is now being included in the request and the data being returned in the response is still the complete set of hparams (that is, we aren't somehow accidentaly filtering results we weren't before). * Specify a filter for some other columns and verify that the response contains the expected rows. * Verify the kernel_init column can be sorted. * Verify that other columns can also be sorted and that there is no longer an off-by-one column error. * Specify a filter and download the data as CSV and ensure the correct data is included in the CSV (data download depends on the session_groups code). 3. Repeat the same verification with a TensorBoard.corp instance.
yatbear
pushed a commit
to yatbear/tensorboard
that referenced
this pull request
Mar 27, 2023
…equests (tensorflow#5971) === Motivation for features / changes === There are bugs in the hparams dashboard (Googlers, see b/239749612 and b/239749837) where the root cause appears to be the fact that some hparams are excluded from session_groups requests. Attempting to sort by the excluded column or any column to the right of it, column "i", would in fact sort by column "i+1". We have an "off-by-one" error. The code that excludes the column from the session_groups request is here: * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/hparams/tf_hparams_query_pane/tf-hparams-query-pane.ts;l=822-829;drc=dc4754cf27d55c289b85a5ae77591f8d96fb682b * The column is being excluded because its metadata does not include a filter. Digging further, we see that this particular column is excluded because it is deemed to have too many possible discrete values: * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/hparams/backend_context.py;l=270;drc=049f875d96c19afe439d0ee4d13f78c7b599169a === Technical description of changes === The fix proposed here is to delete the code that omits these columns from the request. Trying to guess the history here, it seems like the code was added when this was a clear error condition but later we added cases where this is expected. Examining the backend code, this seems like a safe change. * The backend code already handles the case where a column does not contain a filter: * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/hparams/list_session_groups.py;l=439-474;drc=6996d71b9e0bad19a895a37a8bea8d7807775c39 * A column missing a filter indicates that no values should be filtered. This is the same behavior as just not including the column at all. * The backend code would already return a column in the response even if that column was not specified in the request. So including the column in the request when it previously was not included does not seem to have any impact on the response. === Detailed steps to verify changes work correctly (as executed by you) === 1. I generated an hparams data set with this colab notebook: * https://colab.research.google.com/drive/1FoMTeuFdoeMTnlKrb6vrvSOmLTjkX6ga#scrollTo=TB0wBWfcVqHz * It contains one column (kernel_init) with 12 discrete values. 2. Load a tensorboard with the logs and: * Verify kernel_init is now being included in the request and the data being returned in the response is still the complete set of hparams (that is, we aren't somehow accidentaly filtering results we weren't before). * Specify a filter for some other columns and verify that the response contains the expected rows. * Verify the kernel_init column can be sorted. * Verify that other columns can also be sorted and that there is no longer an off-by-one column error. * Specify a filter and download the data as CSV and ensure the correct data is included in the CSV (data download depends on the session_groups code). 3. Repeat the same verification with a TensorBoard.corp instance.
dna2github
pushed a commit
to dna2fork/tensorboard
that referenced
this pull request
May 1, 2023
…equests (tensorflow#5971) === Motivation for features / changes === There are bugs in the hparams dashboard (Googlers, see b/239749612 and b/239749837) where the root cause appears to be the fact that some hparams are excluded from session_groups requests. Attempting to sort by the excluded column or any column to the right of it, column "i", would in fact sort by column "i+1". We have an "off-by-one" error. The code that excludes the column from the session_groups request is here: * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/hparams/tf_hparams_query_pane/tf-hparams-query-pane.ts;l=822-829;drc=dc4754cf27d55c289b85a5ae77591f8d96fb682b * The column is being excluded because its metadata does not include a filter. Digging further, we see that this particular column is excluded because it is deemed to have too many possible discrete values: * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/hparams/backend_context.py;l=270;drc=049f875d96c19afe439d0ee4d13f78c7b599169a === Technical description of changes === The fix proposed here is to delete the code that omits these columns from the request. Trying to guess the history here, it seems like the code was added when this was a clear error condition but later we added cases where this is expected. Examining the backend code, this seems like a safe change. * The backend code already handles the case where a column does not contain a filter: * https://cs.opensource.google/tensorflow/tensorboard/+/master:tensorboard/plugins/hparams/list_session_groups.py;l=439-474;drc=6996d71b9e0bad19a895a37a8bea8d7807775c39 * A column missing a filter indicates that no values should be filtered. This is the same behavior as just not including the column at all. * The backend code would already return a column in the response even if that column was not specified in the request. So including the column in the request when it previously was not included does not seem to have any impact on the response. === Detailed steps to verify changes work correctly (as executed by you) === 1. I generated an hparams data set with this colab notebook: * https://colab.research.google.com/drive/1FoMTeuFdoeMTnlKrb6vrvSOmLTjkX6ga#scrollTo=TB0wBWfcVqHz * It contains one column (kernel_init) with 12 discrete values. 2. Load a tensorboard with the logs and: * Verify kernel_init is now being included in the request and the data being returned in the response is still the complete set of hparams (that is, we aren't somehow accidentaly filtering results we weren't before). * Specify a filter for some other columns and verify that the response contains the expected rows. * Verify the kernel_init column can be sorted. * Verify that other columns can also be sorted and that there is no longer an off-by-one column error. * Specify a filter and download the data as CSV and ensure the correct data is included in the CSV (data download depends on the session_groups code). 3. Repeat the same verification with a TensorBoard.corp instance.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation for features / changes
There are bugs in the hparams dashboard (Googlers, see b/239749612 and b/239749837) where the root cause appears to be the fact that some hparams are excluded from session_groups requests. Attempting to sort by the excluded column or any column to the right of it, column "i", would in fact sort by column "i+1". We have an "off-by-one" error.
The code that excludes the column from the session_groups request is here:
Digging further, we see that this particular column is excluded because it is deemed to have too many possible discrete values:
Technical description of changes
The fix proposed here is to delete the code that omits these columns from the request. Trying to guess the history here, it seems like the code was added when this was a clear error condition but later we added cases where this is expected.
Examining the backend code, this seems like a safe change.
Detailed steps to verify changes work correctly (as executed by you)