hparams: thread experiment ID through data access #3418
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: Until now, `context.data_provider` was provided unless `generic_data` was explicitly set to `false`. Now, it’s provided in all cases (except the legacy DB modes). This was always expected to be safe, and has been live by default since TensorBoard 1.15.0. This is needed to write plugins that unconditionally assume that a data provider is available. A consequence of this is that `is_active`’s use of the experimental `list_plugins` method is now always on as well. This has been on by default for two months, but not yet in any release (other than the upcoming TensorBoard 2.2.0), so it’s slightly riskier, but it’s also expected to be safe. Test Plan: Run TensorBoard with `--generic_data false`. If pointing at an empty logdir, no plugins are shown as active. If pointing at a full plugin, all appropriate plugins are shown as active. The debugger plugin is still marked as active if `--debugger_port` is specified and as inactive if no relevant flags are given, so the fallback `is_active` calls are still working. wchargin-branch: backend-always-data-provider
Summary: Until now, the hparams plugin has been active only when hparams data is present *and* the scalars plugin is loaded and active. This cross-plugin dependency is unique and doesn’t fit well into the data provider world, so we remove it. The hparams plugin is now active iff hparams data is available, via the standard `data_plugin_names` API. Correspondingly, we loosen the error handling in parts of the plugin that rely on the scalars plugin being active. As long as the scalars plugin is included in the TensorBoard instance, this should not be a problem: TensorBoard core loads all plugins before accepting requests to any of them. If the hparams plugin is used in an installation with no scalars plugin, the errors will now be 404s instead of 500s. This seems acceptable, as running without a scalars plugin is not a common pattern. Test Plan: The hparams plugin is still marked as active in logdirs that contain hparams data and inactive in logdirs that don’t. wchargin-branch: hparams-data-provider-is-active wchargin-source: 19dedbb932881bbdb471b4d3d9796be47d1e8956
Summary: The hparams plugin makes three kinds of queries against the multiplexer: listing hparams metadata, listing scalars metadata, and reading scalar points. This commit encapsulates each such operation in a method so that we can migrate them to use data provider APIs. For now, the underlying implementations are unchanged. Test Plan: Running `git grep '\.multiplexer' tensorboard/plugins/hparams/` shows only matches in `backend_context.py` and tests. Existing tests pass, and all three views of the hparams plugin, including embedded scalar charts and the data download links, still work with and without `--generic_data true` (though in all cases the plugin actually still reads directly from the multiplexer). wchargin-branch: hparams-encapsulate-mux wchargin-source: a583e4f940723bd01dc433eb83e24776b686dc44
Summary: All data access in the hparams plugin is now performed via methods that require an experiment ID. The ID is currently unused except for an assertion. In a subsequent change, these methods will switch to using a data provider instead of the multiplexer, at which point the experiment ID will be required. Test Plan: Unit tests suffice for the internals. To check the wiring at the plugin level, note that all three views of the hparams plugin, including embedded scalar charts and the download links, still render properly. wchargin-branch: hparams-thread-eid wchargin-source: c82e1e20f220436002f95bf8f9575839b26c9293
nfelt
approved these changes
Mar 24, 2020
| """ | ||
| self._tb_context = tb_context | ||
| self._experiment_from_tag = None | ||
| self._experiment_from_tag = {} # experiment ID to Experiment proto |
Contributor
There was a problem hiding this comment.
This name doesn't really make sense any more now that it's a dict, IMO. Something like self._cached_experiments would be clearer.
For that matter though, this is now an ever-growing cache and I don't think it's actually needed (or at least not worth the risk of unbounded memory, even if in practice it probably wouldn't be a very fast leak). Could we just remove it?
Contributor
Author
There was a problem hiding this comment.
True about the name, and fair enough that it can probably just be
removed. Done.
Summary: Prior to this change, `read_scalars` (resp. `read_tensors`) delegated to `list_scalars` (resp. `list_tensors`) to find the set of time series to read. This is slower than it might sound, because `list_scalars` itself needs to scan over all relevant `multiplexer.Tensors` to identify `max_step` and `max_wall_time`, which are thrown away by `read_scalars`. (That `list_scalars` needs this full scan at all is its own issue; ideally, these would be memoized onto the event multiplexer.) When a `RunTagFilter` specifying a single run and tag is given, we optimize further by requesting individual `SummaryMetadata` rather than paring down `AllSummaryMetadata`. Resolves a comment of @nfelt on #2980: <#2980 (comment)> Test Plan: When applied on top of #3419, `:list_session_groups_test` improves from taking 11.1 seconds to taking 6.6 seconds on my machine. This doesn’t seem to fully generalize; I see only ~13% speedups in a microbenchmark that hammers `read_scalars` on a logdir with all the demo data, but the improvement on that test is important. wchargin-branch: data-read-without-list wchargin-source: bc728c60dcb0039a6f802eaf154205b7161e4796
wchargin-branch: hparams-encapsulate-mux wchargin-source: 118c7f3a1c7c29f5983b3ad2547669d9342e6091
wchargin-branch: hparams-thread-eid wchargin-source: 14bd159933cd83968f0a1fa891853695d7bcd954
wchargin-branch: data-read-without-list wchargin-source: d768ced329672f2b307bd25681f111ebe1b929a8
wchargin-branch: data-read-without-list wchargin-source: d768ced329672f2b307bd25681f111ebe1b929a8
wchargin-branch: hparams-encapsulate-mux wchargin-source: fb03f6591420b8a6f7f150dd4e30dd593d5f111e
wchargin-branch: hparams-thread-eid wchargin-source: f7768a2fe912914cc4dde2b22e8dd7be0d62912b
wchargin-branch: data-blob-sequence-tests wchargin-source: 664b9b53b60a76eacbd85ecca3335e62c172acf0
wchargin-branch: data-read-without-list wchargin-source: 674ec15dd632579245b7a4aed36a8cc654928123
wchargin-branch: hparams-encapsulate-mux wchargin-source: f1350f44bdb92a4f70430f30e54023c27ac42ca4
wchargin-branch: hparams-thread-eid wchargin-source: 518152451965bd3dadfe4737d888b9cdced56f85
wchargin
changed the base branch from
wchargin-hparams-encapsulate-mux
to
master
wchargin-branch: hparams-thread-eid wchargin-source: a85ac12889dc3d2bc3904c7240bf5734596c253d # Conflicts: # tensorboard/plugins/hparams/backend_context.py # tensorboard/plugins/hparams/list_session_groups.py # tensorboard/plugins/hparams/metrics.py
wchargin-branch: hparams-thread-eid wchargin-source: a85ac12889dc3d2bc3904c7240bf5734596c253d
bileschi
pushed a commit
to bileschi/tensorboard
that referenced
this pull request
Apr 15, 2020
Summary: All data access in the hparams plugin is now performed via methods that require an experiment ID. The ID is currently unused except for an assertion. In a subsequent change, these methods will switch to using a data provider instead of the multiplexer, at which point the experiment ID will be required. Test Plan: Unit tests suffice for the internals. To check the wiring at the plugin level, note that all three views of the hparams plugin, including embedded scalar charts and the download links, still render properly. wchargin-branch: hparams-thread-eid
bileschi
pushed a commit
that referenced
this pull request
Apr 15, 2020
Summary: All data access in the hparams plugin is now performed via methods that require an experiment ID. The ID is currently unused except for an assertion. In a subsequent change, these methods will switch to using a data provider instead of the multiplexer, at which point the experiment ID will be required. Test Plan: Unit tests suffice for the internals. To check the wiring at the plugin level, note that all three views of the hparams plugin, including embedded scalar charts and the download links, still render properly. wchargin-branch: hparams-thread-eid
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:
All data access in the hparams plugin is now performed via methods that
require an experiment ID. The ID is currently unused except for an
assertion. In a subsequent change, these methods will switch to using
a data provider instead of the multiplexer, at which point the
experiment ID will be required.
Test Plan:
Unit tests suffice for the internals. To check the wiring at the plugin
level, note that all three views of the hparams plugin, including
embedded scalar charts and the download links, still render properly.
wchargin-branch: hparams-thread-eid