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

Add --plugins option to uploader #3402

Merged
merged 4 commits into from Mar 21, 2020
Merged

Conversation

bmd3k
Copy link
Contributor

@bmd3k bmd3k commented Mar 20, 2020

Note: This functionality has previously been merged then reverted. This PR includes a fix for the bug that required the revert.

Original: #3377
Revert: #3400

In addition to previous testing I also ran the following commands successfully:

bazel run //tensorboard -- dev upload --logdir <some log dir> --plugins scalars graphs
Note this only uploads scalars since the current prod TensorBoard.dev ignores "--plugins"

bazel run //tensorboard -- dev list
Creates experiment with id OzqFG8zQSwCP9q5ZWV2JsA

bazel run //tensorboard -- dev delete --experiment_id OzqFG8zQSwCP9q5ZWV2JsA

Motivation for features / changes
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.

Technical description of changes
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.

…ensorflow#3400)"

This reverts commit 2b2a976.
Subsequent commits will fix the issue that caused the original
revert.
Most tensorboard CLI subcommands do not define --plugins flag so we need
to explicitly check that FLAGS.plugins exists before using it.
@bmd3k bmd3k marked this pull request as ready for review March 20, 2020 13:24
@bmd3k bmd3k requested a review from wchargin March 20, 2020 13:24
Copy link
Contributor

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Thanks! Fix looks good to me (git diff e94fabf4..0777b730). I also
tested the export and auth revoke subcommands for good measure.

nit: Could you fix the code block/list Markdown in the PR description?
(Around the “bazel run…”.)

tensorboard/uploader/uploader_main.py Show resolved Hide resolved
tensorboard/uploader/uploader_main.py Outdated Show resolved Hide resolved
tensorboard/uploader/uploader_main.py Show resolved Hide resolved
@bmd3k bmd3k merged commit 3ba05e0 into tensorflow:master Mar 21, 2020
@bmd3k bmd3k deleted the upload-plugins-2 branch March 30, 2020 12:48
bileschi pushed a commit to bileschi/tensorboard that referenced this pull request Apr 14, 2020
Motivation for features / changes
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.

Technical description of changes
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.
@nfelt nfelt mentioned this pull request Apr 14, 2020
bileschi pushed a commit that referenced this pull request Apr 15, 2020
Motivation for features / changes
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.

Technical description of changes
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants