Skip to content

uploader: add PluginControl handshake protos #3299

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.

Sign up for GitHub

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
wchargin merged 4 commits into from
Feb 27, 2020
Merged

uploader: add PluginControl handshake protos #3299

Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
f7b79df
uploader: add `PluginControl` handshake protos
wchargin Feb 26, 2020
dd9c64b
[update patch]
wchargin Feb 27, 2020
548d55c
[update diffbase]
wchargin Feb 27, 2020
7265308
[update patch]
wchargin Feb 27, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions tensorboard/uploader/proto/server_info.proto
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,20 @@ message ServerInfoResponse {
ApiServer api_server = 2;
// How to generate URLs to experiment pages.
ExperimentUrlFormat url_format = 3;
// For which plugins should we upload data? (Even if the uploader is
// structurally capable of uploading data from many plugins, we only actually
// upload data that can be currently displayed in TensorBoard.dev. Otherwise,
// users may be surprised to see that experiments that they uploaded a while
// ago and have since shared or published now have extra information that
// they didn't realize had been uploaded.)
//
// The client may always choose to upload less data than is permitted by this
// field: e.g., if the end user specifies not to upload data for a given
// plugin, or the client does not yet support uploading some kind of data.
//
// If this field is omitted, there are no upfront restrictions on what the
// client may send.
PluginControl plugin_control = 4;
}

enum CompatibilityVerdict {
Expand Down Expand Up @@ -59,3 +73,9 @@ message ExperimentUrlFormat {
// (See docs for `template` field.)
string id_placeholder = 2;
}

message PluginControl {
Copy link
Member

@davidsoergel davidsoergel Feb 27, 2020

Choose a reason for hiding this comment

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

I like how generic this message is. We could in principle extend it with rate limits per plugin, informational messages per plugin, whatever makes sense later.

A slightly different design might prepare us for that eventuality even better. Instead of one PluginControl for all plugins, you could do repeated PluginControl plugin_controls; where for now PluginControl contains just string plugin_name. Or do that in a nested manner with AllPluginControls where you have it, with repeated SinglePluginControl inside (mod naming).

(Just to consider; I'm OK with it either way)

Copy link
Contributor Author

@wchargin wchargin Feb 27, 2020

Choose a reason for hiding this comment

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

I like how generic this message is. We could in principle extend it
with rate limits per plugin, informational messages per plugin,
whatever makes sense later.

Yep, exactly. Rate limits were something that I had in mind, as was a
flag “bool allow_all_plugins = 2;” for general-purpose storage (e.g.,
if people run their own instances, they might want that).

A slightly different design might prepare us for that eventuality even
better […]

A downside of repeated PluginControl plugin_controls; is that it
doesn’t admit distinguishing between an old server that doesn’t know
about PluginControls and a server that is denying all uploads. If we
did that, then rolling back the server to a pre-PluginControl version
would be dangerous once we’d pushed a new uploader, so I like having the
wrapper message.

An AllPluginControls with repeated SinglePluginControls does resolve
this, and indeed is more flexible along the plugin axis. But it’s a bit
less flexible on other axes: e.g., I could imagine wanting

message ServerInfoResponse {
  // ...
  PluginControl plugin_control = 4;
  repeated RateLimit rate_limit = 5;
}

message RateLimit {
  oneof what {
    // Rate-limit a specific plugin.
    string plugin_name = 1;
    // Rate-limit all data of a given data class.
    .tensorboard.DataClass data_class = 2;
  }
  // Send requests at most once every this often.
  Duration min_request_period = 3;
}

in which case the SinglePluginControls aren’t obviously the right
place to put the rate limiting.

Given that both options seem reasonable and the worst-case costs aren’t
too high (we can either deprecate the old fields entirely, or use a
hybrid solution like denormalizing plugin names or using the plugin
control ordering as an index; neither is perfect, but they’re not
maintenance nightmares), I’m going to keep this as is, because this way
is simpler right now. Thank you for the suggestion!

// Only send data from plugins with these names. These are plugin names as
// stored in the the `SummaryMetadata.plugin_data.plugin_name` proto field.
repeated string allowed_plugins = 1;
}