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
42 changes: 26 additions & 16 deletions tensorboard/plugins/text/text_plugin.py
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@
import numpy as np
# pylint: enable=g-bad-import-order

import six
import tensorflow as tf
from werkzeug import wrappers

Expand Down Expand Up @@ -234,14 +235,15 @@ def is_active(self):
if any(self._index_cached.values()):
return True

if any(self._multiplexer.PluginRunToTagToContent(metadata.PLUGIN_NAME)):
if bool(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.

I think having if bool(expr) is redundant - you can just do if expr.

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.

# 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()

return False

def _maybe_launch_index_impl_thread(self):
Expand Down Expand Up @@ -271,44 +273,52 @@ def _async_index_impl(self):

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.
name = 'tensorboard_text'
run_to_assets = self._multiplexer.PluginAssets(name)
for run, assets in run_to_assets.items():
if run in run_to_series:
# When runs conflict, the summaries created via the new method override.
continue

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
else:
run_to_series[run] += []
# The mapping should contain all runs among its keys.
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah ok, good catch. I feel like some plugins expect this (that index_impl() includes all runs, not just those with plugin data for that plugin) but not all of them? Do we have a set standard or is this just up to any plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

For all plugins, the "runs"/"tags" route should list all runs, not just those that include text summaries. Without this, the existing infrastructure breaks when it tries to look up non-plugin-specific runs in the run-to-tag mapping. There's a brief description within the internal change that removes the tf-sidebar-helper component.

run_to_series[run] = []

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)

# Augment the summaries created via the deprecated (plugin asset based)
# method with these summaries created with the new method. When they
# conflict, the summaries created via the new method overrides.
for (run, tags) in mapping.items():
run_to_series[run] += tags.keys()
return run_to_series
mapping = six.iteritems(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: strictly speaking this isn't a mapping, it's an iterable of dict items - maybe rename to "items" or conversely, move the six.iteritems() call to inside the dict comprehension.

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.

self._multiplexer.PluginRunToTagToContent(
metadata.PLUGIN_NAME))
return {
run: list(tag_to_content.keys())
for (run, tag_to_content)
in mapping
}

def tags_impl(self):
# Recompute the index on demand whenever tags are requested, but do it
# in a separate thread to avoid blocking.
self._maybe_launch_index_impl_thread()

# Use the cached index if present; if it's not, this route shouldn't have
# been reached anyway (since the plugin should be saying it's inactive)
# so just return an empty response.
return self._index_cached if self._index_cached else {}
# Use the cached index if present. If it's not, just return the result based
# on data from the multiplexer, requiring no disk read.
if self._index_cached:
return self._index_cached
else:
return self._fetch_run_to_series_from_multiplexer()

@wrappers.Request.application
def tags_route(self, request):
Expand Down