data: expose data_location integration point
#2627
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
Test Plan: Unit tests included. wchargin-branch: data-value-types
Summary:
The `data_location` field will soon be made pluggable on the data
provider, which will muddy the semantics of the `mode` field. Given that
it’s unused, we can just remove it.
Test Plan:
There are no other uses of the removed `getMode` method:
$ git grep -ho 'getMode\w*' | sort -u
getModelClass
getModelHeaderClass
getModelName
getModelName_
The frontend still seems to work fine in both logdir and DB modes.
wchargin-branch: remove-environ-mode
wchargin-branch: remove-environ-mode wchargin-source: ce7609bebd9c2a9d759829abdda95202619bc9cb
wchargin-branch: remove-environ-mode wchargin-source: ce7609bebd9c2a9d759829abdda95202619bc9cb
Summary: Generic data providers can now control the “data location” string in the TensorBoard UI, which appears at the bottom of the runs selector: ![Screenshot of the runs selector and its data location string][ss] As this is only a cosmetic change, it’s provided as an optional method on the `DataProvider` interface. [ss]: https://user-images.githubusercontent.com/4317806/64572837-cf7b7e00-d31d-11e9-8fd6-d81d36774172.png Test Plan: TensorBoard still seems to work properly with `--generic_data` set to `false`, `auto`, or `true`, in both logdir and DB modes. Conspicuously changing the `MultiplexerDataProvider` to always `return "tada"` induces the appropriate change in the frontend, so the wiring is hooked up. wchargin-branch: generic-data-location wchargin-source: 34124f25978fa827704b106283c03c00c653198c
wchargin-branch: remove-environ-mode wchargin-source: 60569ec99fede5f819f23b09631ba27a7adb3367
wchargin
changed the base branch from
wchargin-remove-environ-mode
to
master
wchargin-branch: generic-data-location wchargin-source: 8c168ac45dbbcecf1b4cc2925cc573b0cbb95104
nfelt
approved these changes
Sep 12, 2019
Contributor
nfelt
left a comment
nfelt
left a comment
There was a problem hiding this comment.
Sorry for the delay, forgot I hadn't reviewed yet.
Contributor
Author
|
Thanks! It wasn’t urgent. |
wchargin-branch: generic-data-location wchargin-source: 96c7f9577e32de041c8dc53e7a9ab5728c23e64e # Conflicts: # tensorboard/plugins/core/core_plugin.py
wchargin-branch: generic-data-location wchargin-source: 96c7f9577e32de041c8dc53e7a9ab5728c23e64e
wchargin
added a commit
that referenced
this pull request
Oct 6, 2019
Summary: This commit changes the frontend experiment ID plumbing from using the query string to using a path component: experiment URLs are now of the form `http://localhost:6006/experiment/123/`. This supersedes the short-term changes from #2580, #2613, and #2627. Test Plan: Apply the below patch to the multiplexer data provider, then launch TensorBoard with `--generic_data=true` and verify that the correct experiment ID is printed for each call, at `/` and at `/experiment/123`. Verify also that no request has an `experiment` query parameter. Patch: ```diff diff --git a/tensorboard/backend/event_processing/data_provider.py b/tensorboard/backend/event_processing/data_provider.py index 0b354aa..1c00b930 100644 wchargin-source: c5b1a893a67cb82286f9de0f5b2a0638dc372a54 --- a/tensorboard/backend/event_processing/data_provider.py +++ b/tensorboard/backend/event_processing/data_provider.py @@ -57,10 +57,12 @@ class MultiplexerDataProvider(provider.DataProvider): return None def data_location(self, experiment_id): + logger.warn("data_location(%r)", experiment_id) del experiment_id # ignored return str(self._logdir) def list_runs(self, experiment_id): + logger.warn("list_runs(%r)", experiment_id) del experiment_id # ignored for now return [ provider.Run( @@ -72,6 +74,7 @@ class MultiplexerDataProvider(provider.DataProvider): ] def list_scalars(self, experiment_id, plugin_name, run_tag_filter=None): + logger.warn("list_scalars(%r, ...)", experiment_id) del experiment_id # ignored for now run_tag_content = self._multiplexer.PluginRunToTagToContent(plugin_name) result = {} @@ -103,6 +106,7 @@ class MultiplexerDataProvider(provider.DataProvider): def read_scalars( self, experiment_id, plugin_name, downsample=None, run_tag_filter=None ): + logger.warn("read_scalars(%r, ...)", experiment_id) # TODO(@wchargin): Downsampling not implemented, as the multiplexer # is already downsampled. We could downsample on top of the existing # sampling, which would be nice for testing. ``` wchargin-branch: eid-frontend
wchargin
added a commit
that referenced
this pull request
Oct 8, 2019
Summary: This commit changes the frontend experiment ID plumbing from using the query string to using a path component: experiment URLs are now of the form `http://localhost:6006/experiment/123/`. This supersedes the short-term changes from #2580, #2613, and #2627. Test Plan: Apply the below patch to the multiplexer data provider, then launch TensorBoard with `--generic_data=true` and verify that the correct experiment ID is printed for each call, at `/` and at `/experiment/123`. Verify also that no request has an `experiment` query parameter. Patch: ```diff diff --git a/tensorboard/backend/event_processing/data_provider.py b/tensorboard/backend/event_processing/data_provider.py index 0b354aa..1c00b930 100644 --- a/tensorboard/backend/event_processing/data_provider.py +++ b/tensorboard/backend/event_processing/data_provider.py @@ -59,2 +59,3 @@ class MultiplexerDataProvider(provider.DataProvider): def data_location(self, experiment_id): + logger.warn("data_location(%r)", experiment_id) del experiment_id # ignored @@ -63,2 +64,3 @@ class MultiplexerDataProvider(provider.DataProvider): def list_runs(self, experiment_id): + logger.warn("list_runs(%r)", experiment_id) del experiment_id # ignored for now @@ -74,2 +76,3 @@ class MultiplexerDataProvider(provider.DataProvider): def list_scalars(self, experiment_id, plugin_name, run_tag_filter=None): + logger.warn("list_scalars(%r, ...)", experiment_id) del experiment_id # ignored for now @@ -105,2 +108,3 @@ class MultiplexerDataProvider(provider.DataProvider): ): + logger.warn("read_scalars(%r, ...)", experiment_id) # TODO(@wchargin): Downsampling not implemented, as the multiplexer ``` wchargin-branch: eid-frontend
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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:
Generic data providers can now control the “data location” string in the
TensorBoard UI, which appears at the bottom of the runs selector:
As this is only a cosmetic change, it’s provided as an optional method
on the
DataProviderinterface.Test Plan:
TensorBoard still seems to work properly with
--generic_dataset tofalse,auto, ortrue, in both logdir and DB modes. Conspicuouslychanging the
MultiplexerDataProviderto alwaysreturn "tada"inducesthe appropriate change in the frontend, so the wiring is hooked up.
wchargin-branch: generic-data-location