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

chihuahua · 2018-03-05T23:29:54Z

We make the text plugin's is_active method return true if it detects
relevant data present within the multiplexer. Subsequently, there would
be no more need to check for data stored within plugin assets.

This solves an issue: The text plugin used to be inactive when the user
initially loads into it because the thread for computing whether the plugin
is active would not have completed executing yet.

Part of #625.

Test plan:

Run the text demo. Verify behavior. The text plugin should be active on
the first load.

We make the text plugin's is_active method short-circuit if it detects relevant data present within the multiplexer. Subsequently, there would be no more need to check for data stored within plugin assets.

nfelt

Thanks for the fix! I maybe would adjust the PR title slightly - the text plugin does already short-circuit in is_active(). What this PR fixes is that it always defaults to False on the first attempt; it can be smarter than that by checking the multiplexer first, since that part is cheap (it's just the plugin assets reads that aren't).

nfelt · 2018-03-07T00:05:50Z

tensorboard/plugins/text/text_plugin.py


+  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)


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.

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.

nfelt · 2018-03-07T00:14:58Z

tensorboard/plugins/text/text_plugin.py

    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


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.

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

nfelt · 2018-03-07T00:15:15Z

tensorboard/plugins/text/text_plugin.py

      else:
-        run_to_series[run] = []
+        run_to_series[run] += []


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

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

nfelt

LGTM, but text_plugin_test.py is failing:

======================================================================
FAIL: testPluginIsActiveWhenTextRuns (__main__.TextPluginTest)
The plugin should be active when there are runs with text.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/.bazel-output-base/bazel-sandbox/2750984358386572954/execroot/org_tensorflow_tensorboard/bazel-out/k8-fastbuild/bin/tensorboard/plugins/text/text_plugin_test.runfiles/org_tensorflow_tensorboard/tensorboard/plugins/text/text_plugin_test.py", line 372, in testPluginIsActiveWhenTextRuns
    self.assertIsActive(plugin, True)
  File "/home/travis/.bazel-output-base/bazel-sandbox/2750984358386572954/execroot/org_tensorflow_tensorboard/bazel-out/k8-fastbuild/bin/tensorboard/plugins/text/text_plugin_test.runfiles/org_tensorflow_tensorboard/tensorboard/plugins/text/text_plugin_test.py", line 336, in assertIsActive
    self.assertFalse(plugin.is_active())
AssertionError: True is not false

======================================================================
FAIL: testPluginTagsImpl (__main__.TextPluginTest)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/travis/.bazel-output-base/bazel-sandbox/2750984358386572954/execroot/org_tensorflow_tensorboard/bazel-out/k8-fastbuild/bin/tensorboard/plugins/text/text_plugin_test.runfiles/org_tensorflow_tensorboard/tensorboard/plugins/text/text_plugin_test.py", line 391, in testPluginTagsImpl
    self.assertEqual({}, self.plugin.tags_impl())
AssertionError: {} != {'fry': [u'message', u'vector'], 'leela': [u'message', u'vector']}
- {}
+ {'fry': [u'message', u'vector'], 'leela': [u'message', u'vector']}

----------------------------------------------------------------------

nfelt · 2018-03-10T06:25:39Z

tensorboard/plugins/text/text_plugin.py

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

+    if bool(self._multiplexer.PluginRunToTagToContent(metadata.PLUGIN_NAME)):


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

nfelt · 2018-03-10T06:27:30Z

tensorboard/plugins/text/text_plugin.py

      else:
-        run_to_series[run] += []
+        # The mapping should contain all runs among its keys.


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?

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.

nfelt · 2018-03-10T06:28:36Z

tensorboard/plugins/text/text_plugin.py

-    for (run, tags) in mapping.items():
-      run_to_series[run] += tags.keys()
-    return run_to_series
+    mapping = six.iteritems(


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.

Modified tests to check for correct behavior when the plugin detects relevant data within the multiplexer. Specifically, in that case, 1. The thread that seeks plugin assets data should not start. 2. The plugin should be active despite how that thread had not started. 3. The tags route should respond with multiplexer data.

Previously, text_plugin_test sometimes failed because tensorflow#1021 made the test check for complete dictionary equality, and tags may appear in different orders. This fix makes the test check for equality of content within lists, regardless of order. This makes the test parallel previous test logic for tags.

Previously, text_plugin_test sometimes failed because #1021 made the test check for complete dictionary equality, and tags may appear in different orders for each test run. This fix makes the test check for equality of content within lists, regardless of order. This makes the test parallel previous test logic for tags.

Make text plugin short-circuit

7e03f80

We make the text plugin's is_active method short-circuit if it detects relevant data present within the multiplexer. Subsequently, there would be no more need to check for data stored within plugin assets.

chihuahua added the plugin:text label Mar 5, 2018

chihuahua requested review from jart and nfelt March 5, 2018 23:29

nfelt approved these changes Mar 7, 2018

View reviewed changes

chihuahua changed the title ~~Make text plugin short-circuit~~ Make text plugin's is_active method check for data in multiplexer first before perusing plugin assets. Mar 10, 2018

chihuahua changed the title ~~Make text plugin's is_active method check for data in multiplexer first before perusing plugin assets.~~ Make text plugin's is_active method check for data in multiplexer first before checking plugin assets. Mar 10, 2018

Respond to comments

aa5e3b8

nfelt approved these changes Mar 10, 2018

View reviewed changes

chihuahua added 6 commits March 14, 2018 16:08

Respond to comments

9706380

Fix lint

296e44d

Import collections

e57b9d4

Remove unicode marker

96352d6

Remove collections import from text_plugin.py

83cb975

chihuahua merged commit 7f03e2a into tensorflow:master Mar 15, 2018

jart mentioned this pull request Mar 23, 2018

Nondeterminism in text_plugin_test #1072

Closed

chihuahua mentioned this pull request Mar 27, 2018

Check for content of lists within text_plugin_test #1078

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

chihuahua commented Mar 5, 2018 •

edited

Loading

nfelt left a comment

nfelt Mar 7, 2018

chihuahua Mar 10, 2018

nfelt Mar 7, 2018

chihuahua Mar 10, 2018

nfelt Mar 7, 2018

chihuahua Mar 10, 2018

nfelt left a comment

nfelt Mar 10, 2018

chihuahua Mar 14, 2018

nfelt Mar 10, 2018

chihuahua Mar 14, 2018

nfelt Mar 10, 2018

chihuahua Mar 14, 2018

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

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

Conversation

chihuahua commented Mar 5, 2018 • edited Loading

nfelt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nfelt left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chihuahua commented Mar 5, 2018 •

edited

Loading