Use registration pattern for dashboard tabs #611

jart · 2017-10-04T01:27:16Z

The plugin list no longer needs to be specified at all. The HTML imports
themselves are now the plugin list. Special properties, e.g. custom tab names,
are now defined as properties on the Polymer components themselves.

The tradeoff is that each plugin's dashboard must call a registration function,
providing the plugin name and the name of the custom element.

See also: #459

chihuahua · 2017-10-04T06:25:52Z

After thinking about this refactoring, I'm not sure of the benefits. I'm not adamantly opposed (more indifferent) - I just don't see the win here of moving where information is stored.

Previously, plugin info (name, element, title) were stored in 1 place (a JSON list). To register a plugin, a developer would just add an entry (JSON object) to a list and import its dashboard.

Now, that same info is stored across individual dashboards. Because polymer lacks interfaces, it's hard to document + enforce properties each dashboard must provide. And each dashboard has to call registerDashboard, which is a non-trivial function. And we maintain this complex registry ...

The previous system seems more systematic and understandable (and concise enough).

chihuahua · 2017-10-04T18:00:38Z

To be clear, I think we want to at least prevent constructing all the dashboards if we're to go forward. At the same time, a stronger justification for what we get out of this refactoring would be nice.

jart · 2017-10-04T18:27:12Z

The advantage is that lines 38 through 55 in https://github.com/tensorflow/tensorboard-plugin-example/blob/b095516b8070fe3001d428f957990719560d869c/greeter_tensorboard/index.html can be deleted. This is preferable to needing to copy the big central const data structure introduced by #459 into that repository.

chihuahua · 2017-10-04T18:46:33Z

Ah k. Despite being big, the const data structure did offer type/property-checking and clarity that I think we want to try and preserve with this change.

Could we

Somehow avoid creating every dashboard (even inactive ones) on this line: https://github.com/tensorflow/tensorboard/pull/611/files#diff-4401f5045c6476b17ecdc75b1605b3fbR62
For instance, can we make tabName static? Or just an optional argument to registerDashboard?
Could we briefly add some docs to
https://github.com/tensorflow/tensorboard-plugin-example/blob/master/README.md
to clarify how to use registerDashboard? For instance, if the tab name is not provided, we default to using the plugin name? Otherwise, I'm not sure if developers would naturally look at registry.ts.

jart · 2017-10-04T19:01:32Z

I preserved the benefits of your previous change. We still have type checking and all that goodness. The only difference now is that the definitions themselves are localized (rather than centralized.)

I loaded TensorBoard and ran document.createElement('vz-projector-dashboard') and it took 2ms. Chrome seems to do a reasonable job making this cheap, so there shouldn't be much downside to doing this on load for each plugin.
The plugin example repository would need to be updated after this change is merged.

chihuahua · 2017-10-04T19:16:04Z

Could we avoid placing the property on the dashboard component (which lacks type-checking)? Just add an argument to registerDashboard?

nfelt · 2017-10-04T19:59:31Z

A few questions:

It looks like the current mechanism for plugin registration in the tensorboard repo diverges from the TENSORBOARD_PLUGINS approach described in the tensorboard-plugin-example repo (at least, I don't see any code within tensorboard master that would read that constant dict). Is that the case right now? Does that mean the plugin-example instructions are currently out of date?
Is there any use case for giving a plugin a different dashboard display name than one that's hardcoded within the plugin component itself? E.g. say that someone writes their own scalars plugin, and they want to name it "scalars" and the existing default scalars one something like "legacy scalars" or "default scalars"? It seems like this approach limits the ability to do that without doing something like adding your own plugin that wraps the existing one (not sure if that is actually possible/easy either?).
Same question but for the backend plugin-identifier string. Any use case for a user making that something other than hardcoded? E.g. if someone wanted to run two copies of a plugin on the backend with slightly different data but use the same frontend for both?
It looks like the reason we couldn't do something like include a tiny bit of javascript that crawls the link rel=import tags and auto-detects the plugins there (without any changes to plugin code) is that everything gets vulcanized and those link tags disappear. Is that correct?

jart · 2017-10-04T20:16:16Z

The divergence happened a month ago in Modify dashboard mapping to take object values #459, which caused problems when updating the example repo. This change is meant to take the good stuff from Modify dashboard mapping to take object values #459 and make separate-repo TB builds even easier to do than they were before.
I can't think of a use case for a third party to want to rename first party plugins. That might cause confusion for users.
Not sure.
Yes. HTML imports get compiled away by vulcanize.

jart · 2017-10-04T20:20:13Z

@chihuahua You're going to love love love what I just did. Not only did I take tabName off the Polymer objects, but I also took isReloadDisabled off them too. Now the definitions for these things are all in one central place.

The plugin list no longer needs to be specified at all. The HTML imports themselves are now the plugin list. Special properties, e.g. custom tab names, are now defined as properties on the Polymer components themselves. The tradeoff is that each plugin's dashboard must call a registration function, providing the plugin name and the name of the custom element. See also: tensorflow#459

chihuahua

This seems like something that works nicely. We have a Dashboard type that specifies properties. Lets try it out both internally and with greeter plugin.

@nfelt, we indeed should update greeter plugin. I don't think we have a need to differentiate between plugins with the same name at the moment.

chihuahua · 2017-10-04T20:54:40Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

@@ -324,6 +326,10 @@
    import {fetchRuns} from "../tf-backend/runsStore.js";
    import * as paginatedViewStore from "../tf-paginated-view/paginatedViewStore.js";

+    if (Object.keys(dashboardRegistry).length === 0) {


nit: Since we imported lodash, we could optionally use _.isEmpty(dashboardRegistry)

chihuahua · 2017-10-04T20:57:16Z

tensorboard/components/tf_tensorboard/registry.ts

+    throw new Error(`Plugin already registered: ${dashboard.plugin}`);
+  }
+  if (!dashboard.tabName) {
+    dashboard.tabName = capitalize(dashboard.plugin);


There's no need to capitalize in JS because we do so using CSS:

tensorboard/tensorboard/components/tf_tensorboard/tf-tensorboard.html

Line 239 in 3d8b6dc

text-transform: uppercase;

That's true but I wasn't quite sure how comfortable I felt making assumptions about the CSS design here. Done.

chihuahua · 2017-10-04T21:01:10Z

The main subsequent challenge is to smoothly update other logic that had depended on our previous API for registering plugins.

jart requested review from nfelt and chihuahua October 4, 2017 01:27

jart force-pushed the index-shell branch from 3ca125b to 74b2be0 Compare October 4, 2017 18:38

jart added 2 commits October 4, 2017 13:21

Round of review

1004992

jart force-pushed the index-shell branch from 969b6c9 to 1004992 Compare October 4, 2017 20:21

chihuahua approved these changes Oct 4, 2017

View reviewed changes

Round of review no. 2

9138ad1

jart merged commit bb57d4c into tensorflow:master Oct 4, 2017

jart deleted the index-shell branch October 4, 2017 21:47

jart mentioned this pull request Oct 5, 2017

Tidy up the tensorboard.main API #620

Merged

wchargin mentioned this pull request Feb 9, 2019

notebook: add Colab integration shell #1822

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use registration pattern for dashboard tabs #611

Use registration pattern for dashboard tabs #611

jart commented Oct 4, 2017

chihuahua commented Oct 4, 2017

chihuahua commented Oct 4, 2017

jart commented Oct 4, 2017

chihuahua commented Oct 4, 2017

jart commented Oct 4, 2017

chihuahua commented Oct 4, 2017

nfelt commented Oct 4, 2017

jart commented Oct 4, 2017

jart commented Oct 4, 2017

chihuahua left a comment

chihuahua Oct 4, 2017

jart Oct 4, 2017

chihuahua Oct 4, 2017

jart Oct 4, 2017

chihuahua commented Oct 4, 2017

Use registration pattern for dashboard tabs #611

Use registration pattern for dashboard tabs #611

Conversation

jart commented Oct 4, 2017

chihuahua commented Oct 4, 2017

chihuahua commented Oct 4, 2017

jart commented Oct 4, 2017

chihuahua commented Oct 4, 2017

jart commented Oct 4, 2017

chihuahua commented Oct 4, 2017

nfelt commented Oct 4, 2017

jart commented Oct 4, 2017

jart commented Oct 4, 2017

chihuahua left a comment

Choose a reason for hiding this comment

chihuahua Oct 4, 2017

Choose a reason for hiding this comment

jart Oct 4, 2017

Choose a reason for hiding this comment

chihuahua Oct 4, 2017

Choose a reason for hiding this comment

jart Oct 4, 2017

Choose a reason for hiding this comment

chihuahua commented Oct 4, 2017