Extensible plugin infrastructural changes #2257

stephanwlee · 2019-05-21T18:12:14Z

This is the change based on tensorflow/community#90.

The change makes two improvements:

allow dynamic plugins (as spec in the RFC; uses entry_points to discover) to be loaded
change plugins_listing contract to include metadata about plugins including their respective FE entry point.

Notes on default_test: tried to get this tested end-to-end but setuptools was a bit cumbersome to test. For specifically, setuptools.setup cannot be invoked programmatically and it can lead to bad side-effects when not cleaned up properly.

wchargin

thanks! (I haven’t looked at the frontend changes yet.)

wchargin · 2019-05-21T19:16:53Z

tensorboard/http_api.md

@@ -35,13 +35,30 @@ The `logdir` argument is the path of the directory that contains events files.

 ## `data/plugins_listing`

-Returns a dict mapping from plugin name to a boolean indicating whether
+Returns a dict mapping from plugin name to an object that describes a plugin.


Previously, dashboard order was determined by plugin registration order,
which was in turn hard-coded in default-plugins.html. But now there is
nothing to determine plugin registration order. Should this be an array
of plugin objects instead?

Discussed a bit offline:

the order of plugins in TensorBoard UI is a UI consideration and plugins_listing probably should not care about that (sorting should be done on the frontend)

currently, the order in which plugins are listed is dictated by order of registerDashboard calls and it is defined in https://github.com/tensorflow/tensorboard/blob/master/tensorboard/components/tf_tensorboard/default-plugins.html.

For dynamic plugins, the frontend entry point will not appear in aforementioned default-plugins.html. In future FE changes, we will likely to introduce some business logic around how plugins are sorted in UI: plugins listed on the default-plugins will appear first then dynamic plugins will be alphabetically sorted.

tensorboard/http_api.md

tensorboard/default.py

tensorboard/http_api.md

tensorboard/plugins/base_plugin.py

tensorboard/backend/application.py

wchargin · 2019-05-24T17:21:29Z

tensorboard/default_test.py

+
+  @staticmethod
+  def create():
+    """Creates an instance off FakeEntryPoint.


sp.: off → of

wchargin · 2019-05-24T17:34:07Z

tensorboard/default_test.py

+
+if __name__ == "__main__":
+  test.main()
+


nit: Superfluous blank line.

tensorboard/default_test.py

tensorboard/BUILD

wchargin · 2019-05-24T17:43:54Z

tensorboard/default_test.py

+    actual_plugins = default.get_dynamic_plugins()
+
+    mock_iter_entry_points.assert_called_with('tensorboard_plugins')
+    self.assertEquals(actual_plugins, [FakePlugin])


The assertEquals function is deprecated; use assertEqual.

wchargin · 2019-05-24T17:45:36Z

tensorboard/default_test.py

+    return FakePlugin
+
+
+


nit: Just two blank lines between module members.

tensorboard/default_test.py

tensorboard/default.py

tensorboard/main.py

wchargin · 2019-05-24T23:06:28Z

tensorboard/BUILD

@@ -201,7 +201,9 @@ py_test(
    deps = [
        ":default",
        ":test",
-        "//tensorboard/plugins/scalar:scalars_plugin",
+	"//tensorboard:expect_pkg_resources_installed",


nit: indentation

wchargin · 2019-05-25T01:32:28Z

tensorboard/http_api.md

+The object contains a property `es_module_path` is an optional one that
+describes a path to the main JavaScript plugin module that will be
+loaded onto an iframe. For "v1" plugins whose JavaScript source is
+incorporated into webfiles.zip, the field is empty.


From the example below it looks like the field is null, right?

tensorboard/backend/application.py

wchargin · 2019-05-28T17:16:15Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

+        return this._dashboardData
+            .map((d) => d.plugin)
+            .filter((dashboardName) => {
+                return this._pluginsListing[dashboardName] &&


Did we want this frontend logic to be backward compatible with the
previous HTTP API?

The easiest way to make it backward compatible might be to transform the
response from {k: v} to {k: typeof v === 'boolean' ? v : v.enabled}
immediately after we receive it (in updatePluginsListing), allowing
most of the other logic to stay unchanged. Or you could store the actual
response data and case on it here, but it looks like as written this JS
will crash if given a response from the previous backend.

wchargin · 2019-05-28T17:18:17Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

-          const activePlugins = result.value;
-          this._activeDashboards = this._dashboardData.map(
-              d => d.plugin).filter(p => activePlugins[p]);
+          this._pluginsListing = result.value;


The renames/refactors in this commit make the tf-tensorboard logic
easier to follow; thanks!

tensorboard/default.py

wchargin · 2019-05-28T17:19:51Z

tensorboard/default_test.py

@@ -0,0 +1,74 @@
+# Copyright 2017 The TensorFlow Authors. All Rights Reserved.


wchargin · 2019-05-28T17:25:27Z

tensorboard/default.py

+      entry_point.load()
+      for entry_point in pkg_resources.iter_entry_points('tensorboard_plugins')
+  ]
+


nit: trailing blank line

wchargin · 2019-05-28T18:14:14Z

Mind rebasing/merging master to pull in #2267? Bazel 0.26 is out.

Fix the dependency issues with the change

wchargin · 2019-05-29T21:08:17Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

+                if (typeof maybeMetadata === 'boolean') {
+                  return maybeMetadata;
+                }
+                return this._pluginsListing[dashboardName].enabled;


nit: Replace this._pluginsListing[dashboardName] with maybeMetadata?

A brief comment about the reason for this business (namely, API compat)
would be reasonable imho, but I’ll leave that up to you.

wchargin self-requested a review May 21, 2019 19:14

wchargin reviewed May 21, 2019

View reviewed changes

stephanwlee changed the title ~~DO_NOT_SUBMIT: Extensible plugin backend changes~~ Extensible plugin infrastructural changes May 24, 2019

stephanwlee requested a review from wchargin May 24, 2019 16:59

wchargin reviewed May 24, 2019

View reviewed changes

stephanwlee changed the title ~~Extensible plugin infrastructural changes~~ [PAUSE] Extensible plugin infrastructural changes May 24, 2019

wchargin reviewed May 25, 2019

View reviewed changes

stephanwlee force-pushed the plugin branch from 86e5b0a to cac26b1 Compare May 28, 2019 15:14

stephanwlee changed the title ~~[PAUSE] Extensible plugin infrastructural changes~~ Extensible plugin infrastructural changes May 28, 2019

stephanwlee requested a review from wchargin May 28, 2019 17:16

wchargin reviewed May 28, 2019

View reviewed changes

stephanwlee added 7 commits May 29, 2019 10:57

Extensible plugin backend changes

f2e9878

fixup! Extensible plugin backend changes

82b8a6c

tests

1b4408a

Undo all the noob mistakes and copypasta; the best pasta

a913659

Fix the dependency issues with the change

review address

b9d28c3

Convey the null-ness

4050063

review address

7e8bc70

stephanwlee force-pushed the plugin branch from c49adf6 to 7e8bc70 Compare May 29, 2019 18:00

wchargin approved these changes May 29, 2019

View reviewed changes

comment about why it is maybe

a9711e9

stephanwlee merged commit af8a478 into tensorflow:master May 29, 2019

stephanwlee deleted the plugin branch May 29, 2019 22:36

stephanwlee mentioned this pull request Jun 18, 2019

[Dynamic plugin] tracking bug #2357

Open

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extensible plugin infrastructural changes #2257

Extensible plugin infrastructural changes #2257

stephanwlee commented May 21, 2019 •

edited

Loading

wchargin left a comment

wchargin May 21, 2019

stephanwlee May 24, 2019

wchargin May 24, 2019

wchargin May 24, 2019

wchargin May 24, 2019

wchargin May 24, 2019

wchargin May 24, 2019

wchargin May 25, 2019

wchargin May 28, 2019

wchargin May 28, 2019

wchargin May 28, 2019

wchargin May 28, 2019

wchargin commented May 28, 2019

wchargin May 29, 2019

		@@ -0,0 +1,74 @@
		# Copyright 2017 The TensorFlow Authors. All Rights Reserved.

Extensible plugin infrastructural changes #2257

Extensible plugin infrastructural changes #2257

Conversation

stephanwlee commented May 21, 2019 • edited Loading

wchargin 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

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

wchargin commented May 28, 2019

Choose a reason for hiding this comment

stephanwlee commented May 21, 2019 •

edited

Loading