Add --plugins option to uploader #3377
Merged
Conversation
This file contains hidden or 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
wchargin
reviewed
Mar 17, 2020
Contributor
wchargin
left a comment
wchargin
left a comment
There was a problem hiding this comment.
Looks good, given that we’re deferring --exclude_plugins or equivalent
to a future change.
nit: OSS commit style is imperative with no trailing punctuation with
optional section prefix, so: “Add --plugins option to uploader”, or:
“uploader: add --plugins option”. Could you please update the PR title
accordingly?
* Why? For consistency; this is the style of commits generated by Git
itself, as in git revert, git merge, etc.
bmd3k
changed the title
Fix other commands broken by --plugins implementation.
* Clarify type of upload_plugins arguments. * Log request.
* --plugins flag now has default value of []. Can never be none. * create_server_info implementation reflects this, too.
wchargin
approved these changes
Mar 17, 2020
Contributor
wchargin
left a comment
wchargin
left a comment
There was a problem hiding this comment.
Looks great; thank you!
Contributor
|
This breaks |
wchargin
added a commit
that referenced
this pull request
Mar 19, 2020
This reverts commit 343456b. Test Plan: Running `bazel run //tensorboard -- dev list` now works. Previously, it failed with: ``` File ".../tensorboard/uploader/uploader_main.py", line 750, in _get_server_info server_info = server_info_lib.fetch_server_info(origin, flags.plugins) AttributeError: 'Namespace' object has no attribute 'plugins' ``` wchargin-branch: revert-uploader-plugins-flag
wchargin
added a commit
that referenced
this pull request
Mar 20, 2020
This reverts commit 343456b. Test Plan: Running `bazel run //tensorboard -- dev list` now works. Previously, it failed with: ``` File ".../tensorboard/uploader/uploader_main.py", line 750, in _get_server_info server_info = server_info_lib.fetch_server_info(origin, flags.plugins) AttributeError: 'Namespace' object has no attribute 'plugins' ``` wchargin-branch: revert-uploader-plugins-flag
bmd3k
added a commit
to bmd3k/tensorboard
that referenced
this pull request
Mar 20, 2020
…ensorflow#3400)" This reverts commit 2b2a976. Subsequent commits will fix the issue that caused the original revert.
This was referenced Apr 2, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
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.
Allow uploader users to specify the plugins for which data should be uploaded. It has two-fold purpose:
(1) Allow users to specify "experimental" plugins that would otherwise not be uploaded.
(2) Allow users to list a subset of launched plugins, if they do not want to upload all launched plugin data.
Add "--plugins" command line option for upload subcommand. The information is sent in the ServerInfoRequest (aka the handshake) and may impact the list of plugins returned in ServerInfoResponse.plugin_control.allowed_plugins.
Sample usage:
tensorboard dev upload --logdir --plugins scalars graphs histograms
Outside of these specific changes:
It's expected that supported servers will evaluate the list of plugins and decide whether it is valid. If valid, the server will respond with the entire list of plugins or perhaps a sublist. If the list is invalid then it will respond with a CompatibilityVerdict of VERDICT_ERROR and a useful detail message to print to console.
If --plugins is not specified then the server is expected to respond with a default list of plugins to upload.