Modify dashboard mapping to take object values #459

chihuahua · 2017-09-02T00:12:19Z

Previously, logic related to dashboards would reference
TENSORBOARD_PLUGINS, a mapping between plugin name to dashboard element
name. Logic would also use the name of the plugin (the key of that
mapping) as the tab name displayed in TensorBoard.

This is suboptimal for several reasons - the plugin name is not
necessarily optimized for display purposes. For instance, take the
plugin name pr_curves. We would ideally remove the underscore from it.

We thus introduce the concept of DashboardData, which encapsulates data on a dashboard - the name of the element for the dashboard component (elementName), the name of the associated plugin (plugin), and the name to show within the navigation menu (tabName).

Renamed the _dashboards array within the tf-tensorboard component to
_dashboardInformationObjects to emphasize how that array is not a string
array, but rather contains objects.

Note that the underscore is now gone from the tab name of the PR curves dashboard.

Previously, logic related to dashboards would reference TENSORBOARD_PLUGINS, a mapping between plugin name to dashboard element name. Logic would also use the name of the plugin (the key of that mapping) as the tab name displayed in TensorBoard. This is suboptimal for several reasons - the plugin name is not necessarily optimized for display purposes. For instance, take the plugin name pr_curves. We would ideally remove the underscore from it. Renamed the _dashboards array within the tf-tensorboard component to _dashboardInformationObjects to emphasize how that array is not a string array, but rather contains objects.

wchargin

DashboardInformation and _dashboardInformationObjects are really mouthfuls. How about DashboardData and _dashboardData? This is a more common pattern, in my experience: for instance, we have PluginData, Summary[Meta]Data, etc. Names matter. :-)

wchargin · 2017-09-02T00:47:36Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

        if (!container) {
          // This dashboard doesn't exist. Nothing to do here.
+          console.log(`no exist - ${selectedDashboard}`);


Did you mean to remove this? If you want to keep it, (a) what value does it add? and (b) could you improve the message?

wchargin · 2017-09-02T00:49:37Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

@@ -696,7 +714,9 @@
          && selectedDashboard == null);
      },
      _computeShowNoSuchDashboardMessage(dashboards, selectedDashboard) {
-        return !!selectedDashboard && !dashboards.includes(selectedDashboard);
+        return !!selectedDashboard &&


Can we just write !!selectedDashboard && !dashboards.some(d => d.plugin === selectedDashboard)? The lodash boilerplate is really unnecessary here.

Yes - that boilerplate looks convoluted. Done.

wchargin · 2017-09-02T00:53:01Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

@@ -664,7 +679,10 @@
            return;
          }
          const activePlugins = result.value;
-          this._activeDashboards = this._dashboards.filter(d => activePlugins[d]);
+          this._activeDashboards = _.map(


Do you think that this is easier to read as this._dashboardInformationObjects.map(d => d.plugin).filter(p => activePlugins[p])?

Yes! Producing a list of strings and then filtering does seem clearer - the filtering logic only needs to observe the string value. Done.

wchargin · 2017-09-02T00:55:55Z

tensorboard/components/tf_tensorboard/default-plugins.ts

+    plugin: 'profile',
+    tabName: 'Profile',
+  },
+};


What do you think about adding the following after this line:

Object.entries(PLUGIN_TO_DASHBOARD_INFORMATION).forEach(([plugin, data]) => { if (data.plugin !== plugin) { throw new Error(`Plugin name mismatch for key: ${plugin}`); } }

This free check seems like it might save us some trouble down the line.

Good idea. Done.

wchargin · 2017-09-02T00:57:44Z

tensorboard/components/tf_tensorboard/default-plugins.ts

+limitations under the License.
+==============================================================================*/
+
+export interface DashboardInformation {


How about DashboardData?

wchargin · 2017-09-02T00:58:20Z

tensorboard/components/tf_tensorboard/default-plugins.ts

+
+// Maps plugin name (must match the backend) to DashboardInformation.
+const PLUGIN_TO_DASHBOARD_INFORMATION
+    : {[key: string]: DashboardInformation} = {


It looks like you don't need this line break, and it's certainly nicer to read without it.

chihuahua

I verified that demos WAI (same screenshots).

chihuahua · 2017-09-02T01:04:07Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

        if (!container) {
          // This dashboard doesn't exist. Nothing to do here.
+          console.log(`no exist - ${selectedDashboard}`);


chihuahua · 2017-09-02T01:06:47Z

tensorboard/components/tf_tensorboard/default-plugins.ts

+limitations under the License.
+==============================================================================*/
+
+export interface DashboardInformation {


chihuahua · 2017-09-02T01:07:04Z

tensorboard/components/tf_tensorboard/default-plugins.ts

+
+// Maps plugin name (must match the backend) to DashboardInformation.
+const PLUGIN_TO_DASHBOARD_INFORMATION
+    : {[key: string]: DashboardInformation} = {


chihuahua · 2017-09-02T01:10:39Z

tensorboard/components/tf_tensorboard/default-plugins.ts

+    plugin: 'profile',
+    tabName: 'Profile',
+  },
+};


Good idea. Done.

chihuahua · 2017-09-02T01:14:12Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

@@ -664,7 +679,10 @@
            return;
          }
          const activePlugins = result.value;
-          this._activeDashboards = this._dashboards.filter(d => activePlugins[d]);
+          this._activeDashboards = _.map(


Yes! Producing a list of strings and then filtering does seem clearer - the filtering logic only needs to observe the string value. Done.

chihuahua · 2017-09-02T01:15:45Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

@@ -696,7 +714,9 @@
          && selectedDashboard == null);
      },
      _computeShowNoSuchDashboardMessage(dashboards, selectedDashboard) {
-        return !!selectedDashboard && !dashboards.includes(selectedDashboard);
+        return !!selectedDashboard &&


Yes - that boilerplate looks convoluted. Done.

wchargin · 2017-09-02T01:23:52Z

tensorboard/components/tf_tensorboard/tf-tensorboard.html

         *
         * @type {!Array<Object>}
         */
-        _dashboardInformationObjects: {
+        _dashboardDataObjects: {


The other half of my earlier comment (perhaps just implied) was that the Objects is totally redundant here. _dashboardData is easier to work with, and the "data" bit clarifies that it's not just the dashboard names (a goal that you noted in your original description, and with which I agree).

True. Objects seems redundant. We now use Data to refer to a list of Datums. Thus, renamed PLUGIN_TO_DASHBOARD_DATA to PLUGIN_TO_DASHBOARD_DATUM

Cool. I was considering that; either's fine with me.

wchargin

You have my approval.

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

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 requested review from wchargin and jart September 2, 2017 00:12

chihuahua force-pushed the chihuahua-add-tab-name branch from 58464ce to 30109ef Compare September 2, 2017 00:16

chihuahua force-pushed the chihuahua-add-tab-name branch from 30109ef to 85e29e3 Compare September 2, 2017 00:18

wchargin reviewed Sep 2, 2017

View reviewed changes

Respond to comments

91aa444

chihuahua commented Sep 2, 2017

View reviewed changes

wchargin reviewed Sep 2, 2017

View reviewed changes

chihuahua added 4 commits September 1, 2017 18:32

Respond to comments. A.

8938228

s/Data/Datum

4af34f6

Some more e s/Data/Datum

a27b211

Fix doc

a845b91

wchargin approved these changes Sep 2, 2017

View reviewed changes

chihuahua merged commit 7d1d430 into master Sep 2, 2017

chihuahua deleted the chihuahua-add-tab-name branch September 2, 2017 05:34

chihuahua mentioned this pull request Sep 3, 2017

Update version of TensorBoard tensorflow/tensorboard-plugin-example#13

Closed

jart mentioned this pull request Oct 4, 2017

Use registration pattern for dashboard tabs #611

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Modify dashboard mapping to take object values #459

Modify dashboard mapping to take object values #459

chihuahua commented Sep 2, 2017 •

edited

wchargin left a comment

wchargin Sep 2, 2017

chihuahua Sep 2, 2017

wchargin Sep 2, 2017

chihuahua Sep 2, 2017

wchargin Sep 2, 2017

chihuahua Sep 2, 2017

wchargin Sep 2, 2017

chihuahua Sep 2, 2017

wchargin Sep 2, 2017

chihuahua Sep 2, 2017

wchargin Sep 2, 2017

chihuahua Sep 2, 2017

chihuahua left a comment

chihuahua Sep 2, 2017

chihuahua Sep 2, 2017

chihuahua Sep 2, 2017

chihuahua Sep 2, 2017

chihuahua Sep 2, 2017

chihuahua Sep 2, 2017

wchargin Sep 2, 2017

chihuahua Sep 2, 2017

wchargin Sep 2, 2017

wchargin left a comment

Modify dashboard mapping to take object values #459

Modify dashboard mapping to take object values #459

Conversation

chihuahua commented Sep 2, 2017 • edited

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

chihuahua 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

wchargin left a comment

Choose a reason for hiding this comment

chihuahua commented Sep 2, 2017 •

edited