projector: read run names from data provider #4494
Merged
Conversation
This file contains 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: The projector plugin no longer depends on the event multiplexer. It finds run directories by calling `list_runs` to get run names and adjoining those to the value of the `--logdir` flag. This still only works with a local logdir (as was also the case before this patch), but now it works even when the data provider is not backed by a multiplexer. The data provider interface does not mandate that run names be valid relative paths under a logdir, but that does hold for the two data providers that we care about: the multiplexer provider and the RustBoard provider. Making the projector plugin truly portable will require much deeper changes, so we don’t bother with anything more principled here. This will certainly not work with `--logdir_spec`; the old version may or may not have worked. The docs for `--logdir_spec` clearly disclaim that it may cause some features to stop working, so I’m okay with this. Test Plan: Tested on a logdir whose `projector_config.pbtxt` is under plugin assets and one whose config is in the main run directory. You can tell that it’s properly reading the config because otherwise the sprite sheet won’t show up. The projector plugin no longer contains any occurrences of the word `multiplexer`, and it still works under `--load_fast`, which does not create a multiplexer at all. wchargin-branch: projector-data-provider-run-names wchargin-source: aa06452ae259240118ffe91585b9ab6b1e43975b
Summary: This rolls forward #4479 (see that commit for context) with changes due to Google-internal, non-TensorBoard code that monkey patches some of the projector internals: - The `_run_paths` field is now updated only during `_update_configs`, to ensure that each change to `_run_paths` gives `_configs` the opportunity to update. - Configs/run paths are no longer updated in `__init__`, since that may be expensive and involve file access (even with a multiplexer). - The `_run_paths` field is now always a `dict` rather than `None`, fixing an existing bug wherein accessing `configs` on a plugin with no multiplexer would raise an `AttributeError`. This reverts commit 4c9a699, and therefore reinstates commit 638014e. Test Plan: Test plan from #4479 still passes, and a test sync now passes as well. Also tested pointing the projector at a logdir with no runs but with checkpoint data at root, as fixed in #3694; this worked in the original version of this change (#4479), but it works now, too. wchargin-branch: projector-fix-aliasing wchargin-source: 0cb7e9c3917d5dc9f00616e35aa5bf756a51dea0
wchargin
changed the base branch from
master
to
wchargin-projector-fix-aliasing
wchargin-branch: projector-data-provider-run-names wchargin-source: 0bdafa0a24cd27648ed0a3fd421f4e96ac49bff2 # Conflicts: # tensorboard/plugins/projector/projector_plugin.py
wchargin-branch: projector-data-provider-run-names wchargin-source: 0bdafa0a24cd27648ed0a3fd421f4e96ac49bff2
psybuzz
approved these changes
Jan 5, 2021
| @@ -524,7 +533,8 @@ def _append_plugin_asset_directories(self, run_path_pairs): | |||
| metadata.PLUGIN_ASSETS_NAME, | |||
There was a problem hiding this comment.
nit: maybe replace this with plugin_assets_name, since we've extracted it in L525?
There was a problem hiding this comment.
Ah, indeed. Thanks.
wchargin-branch: projector-data-provider-run-names wchargin-source: f2ef64702fbff9fd0daff7b2780d18e58902f31d # Conflicts: # tensorboard/plugins/projector/projector_plugin.py
wchargin-branch: projector-data-provider-run-names wchargin-source: f2ef64702fbff9fd0daff7b2780d18e58902f31d
wchargin-branch: projector-data-provider-run-names wchargin-source: f353faf4468fdcf4cb733130ba5ee949c13cdee6
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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:
The projector plugin no longer depends on the event multiplexer. It
finds run directories by calling
list_runsto get run names andadjoining those to the value of the
--logdirflag. This still onlyworks with a local logdir (as was also the case before this patch), but
now it works even when the data provider is not backed by a multiplexer.
The data provider interface does not mandate that run names be valid
relative paths under a logdir, but that does hold for the two data
providers that we care about: the multiplexer provider and the RustBoard
provider. Making the projector plugin truly portable will require much
deeper changes, so we don’t bother with anything more principled here.
This will certainly not work with
--logdir_spec; the old version mayor may not have worked. The docs for
--logdir_specclearly disclaimthat it may cause some features to stop working, so I’m okay with this.
Test Plan:
Tested on a logdir whose
projector_config.pbtxtis under plugin assetsand one whose config is in the main run directory. You can tell that
it’s properly reading the config because otherwise the sprite sheet
won’t show up. The projector plugin no longer contains any occurrences
of the word
multiplexer, and it still works under--load_fast, whichdoes not create a multiplexer at all. Test sync also passes.
wchargin-branch: projector-data-provider-run-names