rust: implement data location #4751
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
Summary: This patch implements `GetExperiment` for the Rust data server and the Python data provider. The server only populates the data location field, because we don’t actually have any other experiment metadata. Test Plan: Tested with `--load_fast` using a local logdir and a GCS logdir. wchargin-branch: rust-data-location wchargin-source: 899ee9d107546bf60cbb3ba41fb0ebc850a9c606
nfelt
approved these changes
Mar 9, 2021
| self._stub = stub | ||
|
|
||
| def data_location(self, ctx, *, experiment_id): | ||
| return "grpc://%s" % (self._addr,) |
Contributor
There was a problem hiding this comment.
No action needed, just musing that it was kind of useful actually to see this when testing Rustboard (easy to remember which grpc port it was on). I wonder if maybe it'd be worth formatting the data location as something like <logdir> served by grpc://<address>?
Contributor
Author
There was a problem hiding this comment.
Yeah, I also miss it. I feel weakly inclined to keep it to just the
logdir to avoid confusing users. It’s nice to be able to say that
--load_fast is meant to be a straight upgrade.
You can get the address from --verbosity 0 or RUST_LOG, and I’d be
happy to patch /data/environment to return str(data_provider) for
debug use. Could even let that str be rendered on hover, if we want to
be really aggressive. That doesn’t cover the use case of “a user sent us
only a screenshot and we want to divine what data provider they’re
using”, but perhaps it gets most of the value—what do you think?
Contributor
There was a problem hiding this comment.
I like the idea of patching /data/environment. If we want user screenshots to be more useful probably the first thing we want is a version number anyway, so it's not critical that it be visible. But nice if the frontend has some way to indicate to a curious soul "what DataProvider am I actually talking to".
wchargin
added a commit
that referenced
this pull request
Mar 9, 2021
Summary: In #4751, we implemented `data_location` support for RustBoard, which means that the location no longer includes the gRPC address. But while this is an improvement for users, the address was convenient for development, and there’s no other way to get it from the UI. This patch augments `/data/environment` to dump some basic debug info: the TensorBoard version, the data provider, and the command-line flags. This behavior can be turned off. It’s enabled in `tensorboard(1)`, but disabled by default on the `CorePlugin` and `CorePluginLoader` APIs. Thus, users who build their own TensorBoards will not find themselves unexpectedly leaking information about their setup. For normal users of `tensorboard(1)`, the flags are fine to render; the only potentially sensitive thing is the logdir path, which is of course already prominent. Sample response, passed through `jq .`: ``` { "data_location": "logs/mnist/", "window_title": "", "debug": { "version": "2.5.0a0", "data_provider": "GrpcDataProvider(addr='localhost:42127')", "flags": { "logdir": "logs/mnist/", "logdir_spec": "", "host": null, "bind_all": true, "port": null, "reuse_port": false, "load_fast": true, "grpc_creds_type": "local", "grpc_data_provider": "", "purge_orphaned_data": true, "db": "", "db_import": false, "inspect": false, "version_tb": false, "tag": "", "event_file": "", "path_prefix": "", "window_title": "", "max_reload_threads": 1, "reload_interval": 5, "reload_task": "auto", "reload_multifile": null, "reload_multifile_inactive_secs": 86400, "generic_data": "auto", "samples_per_plugin": { "scalars": 0, "images": 0 }, "custom_predict_fn": "", "wit_data_dir": "", "__tensorboard_subcommand": "serve" } } } ``` Test Plan: Manually navigated to `/data/environment` and verified that the flags render and the data provider appears for both `--load_fast` and the legacy multiplexer. Checked that when the change to `default.py` is reverted, no debug data shows up. wchargin-branch: environment-debug-info wchargin-source: 772a3783d6f19e4060b37fe43076b963cfd8d96e
wchargin
added a commit
that referenced
this pull request
Mar 11, 2021
Summary: In #4751, we implemented `data_location` support for RustBoard, which means that the location no longer includes the gRPC address. But while this is an improvement for users, the address was convenient for development, and there’s no other way to get it from the UI. This patch augments `/data/environment` to dump some basic debug info: the TensorBoard version, the data provider, and the command-line flags. This behavior can be turned off. It’s enabled in `tensorboard(1)`, but disabled by default on the `CorePlugin` and `CorePluginLoader` APIs. Thus, users who build their own TensorBoards will not find themselves unexpectedly leaking information about their setup. For normal users of `tensorboard(1)`, the flags are fine to render; the only potentially sensitive thing is the logdir path, which is of course already prominent. Sample response, passed through `jq .`: ``` { "version": "2.5.0a0", "data_location": "logs/mnist/", "window_title": "", "debug": { "data_provider": "GrpcDataProvider(addr='localhost:32877')", "flags": { "logdir": "logs/mnist/", "logdir_spec": "", "host": null, "bind_all": true, "port": null, "reuse_port": false, "load_fast": true, "grpc_creds_type": "local", "grpc_data_provider": "", "purge_orphaned_data": true, "db": "", "db_import": false, "inspect": false, "version_tb": false, "tag": "", "event_file": "", "path_prefix": "", "window_title": "", "max_reload_threads": 1, "reload_interval": 5, "reload_task": "auto", "reload_multifile": null, "reload_multifile_inactive_secs": 86400, "generic_data": "auto", "samples_per_plugin": {}, "custom_predict_fn": "", "wit_data_dir": "", "__tensorboard_subcommand": "serve" } } } ``` Test Plan: Manually navigated to `/data/environment` and verified that the flags render and the data provider appears for both `--load_fast` and the legacy multiplexer. Checked that when the change to `default.py` is reverted, no debug data shows up. wchargin-branch: environment-debug-info
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 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.
Summary:
This patch implements
GetExperimentfor the Rust data server and thePython data provider. The server only populates the data location field,
because we don’t actually have any other experiment metadata.
Test Plan:
Tested with
--load_fastusing a local logdir and a GCS logdir.wchargin-branch: rust-data-location