Skip to content
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.

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

Make text plugin's is_active method check for data in multiplexer first before checking plugin assets. #1021

Merged
merged 8 commits into from
Mar 15, 2018
Merged
Changes from 1 commit
Commits
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
14 changes: 11 additions & 3 deletions tensorboard/plugins/text/text_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,11 @@ def is_active(self):
if any(self._index_cached.values()):
return True

if any(self._multiplexer.PluginRunToTagToContent(metadata.PLUGIN_NAME)):
# Text data is present in the multiplexer. No need to further check for
# data stored via the outdated plugin assets method.
return True

# We haven't conclusively determined if the plugin should be active. Launch
# a thread to compute index_impl() and return False to avoid blocking.
self._maybe_launch_index_impl_thread()
Expand Down Expand Up @@ -265,22 +270,25 @@ def _async_index_impl(self):
'TextPlugin index_impl() thread ending after %0.3f sec', elapsed)

def index_impl(self):
run_to_series = self._fetch_run_to_series_from_multiplexer()
# A previous system of collecting and serving text summaries involved
# storing the tags of text summaries within tensors.json files. See if we
# are currently using that system. We do not want to drop support for that
# use case.
run_to_series = collections.defaultdict(list)
name = 'tensorboard_text'
run_to_assets = self._multiplexer.PluginAssets(name)
for run, assets in run_to_assets.items():
if 'tensors.json' in assets:
tensors_json = self._multiplexer.RetrievePluginAsset(
run, name, 'tensors.json')
tensors = json.loads(tensors_json)
run_to_series[run] = tensors
run_to_series[run] += tensors
Copy link
Contributor

Choose a reason for hiding this comment

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

If we change these to += we probably want to do deduping, since I doubt we want duplicated tags if there are cases during the transition from legacy to new formats where the same tags were recorded in both places.

Copy link
Member Author

Choose a reason for hiding this comment

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

+1, now, new-style summaries override old-style ones per prior behavior.

else:
run_to_series[run] = []
run_to_series[run] += []
Copy link
Contributor

Choose a reason for hiding this comment

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

+= [] on a list has no effect, might as well just omit this whole else

Copy link
Member Author

Choose a reason for hiding this comment

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

Done - changed to = []. Added a comment.

return run_to_series

def _fetch_run_to_series_from_multiplexer(self):
run_to_series = collections.defaultdict(list)
# TensorBoard is obtaining summaries related to the text plugin based on
# SummaryMetadata stored within Value protos.
mapping = self._multiplexer.PluginRunToTagToContent(metadata.PLUGIN_NAME)
Copy link
Contributor

Choose a reason for hiding this comment

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

The "Augment the summaries..." comment a few lines down (grrrr github disallowing comments outside diffs) no longer applies here, it should probably be relocated - though I'm not actually sure introducing this helper is necessary? think the main thing is just checking the multiplexer in is_active(), otherwise index_impl() should work fine as is.

If we do keep the helper, the logic can also be simplified, this is pretty much just

return {run: list(tag_to_content.keys()) for run, tag_to_content in six.iteritems(mapping)}

I guess you're actually using the fact that it's a defaultdict(list) up in index_impl() but I might opt to avoid that dependency, since defaultdict is a bit subtle and relying on a given dict return value to actually be a defaultdict seems a bit hazardous unless clearly documented.

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, indeed! We can concisely write the expression like that. And yes, lets use a normal dict since runs with no data map to empty lists. I kept the helper since we also use it within tags_impl so that route is consistent with the plugin being active.

Also, removed the duplicate comment.

Expand Down