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

core: stabilize and use plugin_metadata #2304

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Jun 4, 2019

Summary:
This patch switches first-party plugins from calling registerDashboard
on the frontend to implementing plugin_metadata on the backend.

This requires reworking the “dashboard containers stamped” logic, whose
purpose is to introduce a Polymer data dependency from the dom-repeat
stamping state to the dashboard rendering. Previously, all containers
were stamped simultaneously, but this may no longer be the case; we must
instead track exactly which containers have been stamped.

Plugin ordering was also previously defined implicitly on the frontend;
we shift that ordering to the backend.

Test Plan:
Check that basic functionality still works (dashboards display as
intended and in the correct order). Click through every dashboard to
verify that the proper web component is rendered. Verify that navigating
to /#wat displays a “no such dashboard” message, but navigating to
/#scalars shows no such message even before the plugins listing loads.
Verify that TensorBoard works properly when pointed at an empty logdir.

Then, check that all plugins were migrated (except for the core plugin,
which has no frontend):

$ git grep -l registerDashboard
tensorboard/components/tf_tensorboard/registry.ts
$ comm -3 \
> <(git grep -l 'class .*TBPlugin' ':!*test.py' | sort) \
> <(git grep -l 'def frontend_metadata' ':!*test.py' | sort) \
> ;
tensorboard/plugins/core/core_plugin.py

wchargin-branch: core-use-pluginmetadata

@wchargin wchargin requested review from stephanwlee and removed request for stephanwlee June 4, 2019 19:56
Summary:
This patch switches first-party plugins from calling `registerDashboard`
on the frontend to implementing `plugin_metadata` on the backend.

This requires reworking the “selected dashboard stamped” logic, whose
purpose is to introduce a Polymer data dependency from the `dom-repeat`
stamping state to the dashboard rendering. Previously, all containers
were stamped simultaneously, but this may no longer be the case; we must
instead track exactly which containers have been stamped.

Plugin ordering was also previously defined implicitly on the frontend;
we shift that ordering to the backend.

Test Plan:
Check that basic functionality still works (dashboards display as
intended and in the correct order). Click through every dashboard to
verify that the proper web component is rendered. Verify that navigating
to `/#wat` displays a “no such dashboard” message, but navigating to
`/#scalars` shows no such message even before the plugins listing loads.
Verify that TensorBoard works properly when pointed at an empty logdir.

Then, check that all plugins were migrated (except for the core plugin,
which has no frontend):

```
$ git grep -l registerDashboard
tensorboard/components/tf_tensorboard/registry.ts
$ comm -3 \
> <(git grep -l 'class .*TBPlugin' ':!*test.py' | sort) \
> <(git grep -l 'def frontend_metadata' ':!*test.py' | sort) \
> ;
tensorboard/plugins/core/core_plugin.py
```

wchargin-branch: core-use-pluginmetadata
@wchargin wchargin force-pushed the wchargin-core-use-pluginmetadata branch from d35f434 to d8d6bb4 Compare June 4, 2019 20:04
@wchargin wchargin requested a review from stephanwlee June 4, 2019 20:04
wchargin added a commit that referenced this pull request Jun 4, 2019
Summary:
The `plugins_listing` response has long included a `"core"` key. Prior
to #2299 and #2304, this did nothing; now, it properly emits a warning,
because the frontend does not know how to render such a plugin. There’s
no reason that the frontend needs to know about this plugin at all.

Test Plan:
Observe that the “Plugin has no loading mechanism and no baked-in
registry entry: core” warning no longer appears in the JS console.

wchargin-branch: core-verb
Copy link
Contributor

@stephanwlee stephanwlee left a comment

Choose a reason for hiding this comment

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

I think I finally understand what is going on.

In the near future, I would like to make some minor improvements by either:

  1. create the container myself using DOM API (i.e., document.createElement)
  2. Force render on dom-repeat! https://github.com/Polymer/polymer/blob/1.x/src/lib/template/dom-repeat.html#L427-L439

@wchargin
Copy link
Contributor Author

wchargin commented Jun 5, 2019

We could create the containers ourselves, yes. I’m not sure whether
Polymer would continue to manage the selected$="[[…]]" binding, or
whether we’d have to do that ourselves, but I suppose that we could also
do that.

Forcing render on the dom-repeat sounds viable; presumably we’d do
that at the top of _ensureDashboardContainersStamped?

@wchargin wchargin merged commit 1992f58 into master Jun 5, 2019
@wchargin wchargin deleted the wchargin-core-use-pluginmetadata branch June 5, 2019 18:48
wchargin added a commit that referenced this pull request Jun 5, 2019
Summary:
The `plugins_listing` response has long included a `"core"` key. Prior
to #2299 and #2304, this did nothing; now, it properly emits a warning,
because the frontend does not know how to render such a plugin. There’s
no reason that the frontend needs to know about this plugin at all.

Test Plan:
Observe that the “Plugin has no loading mechanism and no baked-in
registry entry: core” warning no longer appears in the JS console.

wchargin-branch: core-verb
wchargin added a commit that referenced this pull request Jun 11, 2019
Summary:
Plugins whose backends are provided via “loaders” rather than the
plugins themselves may fail to load. We still want to allow those
plugins to display a message in the frontend indicating what must be
done to enable them (e.g., update TensorFlow, start a `tfdbg` debugger),
which requires registering their frontends unconditionally.

This is a short-term hack to retain functionality prior to #2304 (with
the minor change that these plugins will now always appear at the end of
the list). We’ll want to replace this with a better mechanism when time
permits.

Fixes #2338.

Test Plan:
Launch TensorBoard without TensorFlow installed and with no flags other
than `--logdir`. Note that the beholder, debugger, hparams, what-if
tool, and profile dashboards all have valid entries in the “inactive”
dropdown, displaying appropriate help messages.

Verify that of the plugins listed in `default.py`, all those provided
via loaders now have `registerDashboard` statements.

wchargin-branch: register-conditional-plugins
wchargin added a commit that referenced this pull request Jun 12, 2019
Summary:
Plugins whose backends are provided via “loaders” rather than the
plugins themselves may fail to load. We still want to allow those
plugins to display a message in the frontend indicating what must be
done to enable them (e.g., update TensorFlow, start a `tfdbg` debugger),
which requires registering their frontends unconditionally.

This is a short-term hack to retain functionality prior to #2304 (with
the minor change that these plugins will now always appear at the end of
the list). We’ll want to replace this with a better mechanism when time
permits.

Fixes #2338.

Test Plan:
Launch TensorBoard without TensorFlow installed and with no flags other
than `--logdir`. Note that the beholder, debugger, hparams, what-if
tool, and profile dashboards all have valid entries in the “inactive”
dropdown, displaying appropriate help messages.

Verify that of the plugins listed in `default.py`, all those provided
via loaders now have `registerDashboard` statements.

wchargin-branch: register-conditional-plugins
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants