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

RFC: TensorBoard - Improved Plugin Extensibility #90

Merged
merged 1 commit into from
May 24, 2019

Conversation

stephanwlee
Copy link
Contributor

@stephanwlee stephanwlee commented Apr 11, 2019

Comment period ends 2019-05-19

TensorBoard: Improved Plugin Extensibility

Status Proposed
Author(s) Stephan Lee (Google), William Chargin (Google)
Sponsor Mani Varadarajan (Google)
Updated 2019-05-16

This RFC proposes a friction-free approach to enabling TensorBoard contributors to create and share TensorBoard plugins as installable extensions.

TensorBoard is composed of various plugins that show up as separate dashboards to users (e.g., scalars, graphs, hparams, histograms, etc.). Today. anyone can contribute a new plugin by forking TensorBoard, adding their plugin, and getting the PR merged. However, this can require a lengthy review to make sure it is architected correctly and is aligned with TensorBoard’s goals.

This RFC covers two changes that will make this process easier:
Plugins can be distributed as Python packages. Users can install them as extensions to TensorBoard.
Plugins are no longer limited to use Polymer but can instead use any frontend library or framework (e.g., React)

The intention is to enable any prospective contributor to easily be able to create or reuse a visualization with tools familiar to them, and enable users to try it out quickly. Plugins that are useful to many users should still be added to TensorBoard, but this removes the friction needed to start getting feedback or trying out something new.

Copy link
Contributor

@caisq caisq left a comment

Choose a reason for hiding this comment

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

Thanks for the doc. I'm very excited about making it easier for developers to
write plugins for TensorBoard, which I think will further increase the impact
and popularity of TensorBoard.

See my detailed comments below.

rfcs/20190411-tensorboard-improved-plugin-ext.md Outdated Show resolved Hide resolved
rfcs/20190411-tensorboard-improved-plugin-ext.md Outdated Show resolved Hide resolved
### Plugin distribution
A plugin will be distributed as a single Pip distribution package that includes both a Python backend and a web frontend.

The Python backend must export an object conforming to a standard interface that describes the plugin’s metadata and provides an option to load the plugin’s implementation. The exact shape is not yet specified, but it will be conceptually similar to a function returning a tuple containing a name, a description, and a [`tensorboard.plugins.base_plugin.TBLoader`][tb-loader] object.
Copy link
Contributor

Choose a reason for hiding this comment

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

... an object conforming to ...

Should it be an object or a function? The get_plugin example below is a function.

Choose a reason for hiding this comment

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

Functions are objects in Python.* We use the term “object” here
generically (as Python does).

* isinstance(lambda: None, object) # True

rfcs/20190411-tensorboard-improved-plugin-ext.md Outdated Show resolved Hide resolved

One critical drawback of all non-iframe approaches is that TensorBoard cannot enforce plugin authors from inadvertently mutating the global object. For instance, if a library that a plugin transitively depends polyfills and monkey-patches a global object, it can influence how other plugins behave.

**Pro**
Copy link
Contributor

Choose a reason for hiding this comment

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

See my comment above. The structure of the text around this point is somewhat hard to follow. It's unclear

  1. what the Pro and Con sections refer to
  2. why these sections exist for some of the options but not others

Copy link
Member

Choose a reason for hiding this comment

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

+1. What's confusing here is that the options 1, 2, and 3 are at different levels of the hierarchy. IIUC, the intent is:

  • Option A: Non-iFrame
    • Pro, con
    • Option A1
    • Option A2
  • Option B: iFrame
    • Pro, con

rfcs/20190411-tensorboard-improved-plugin-ext.md Outdated Show resolved Hide resolved

The Python backend must export an object conforming to a standard interface that describes the plugin’s metadata and provides an option to load the plugin’s implementation. The exact shape is not yet specified, but it will be conceptually similar to a function returning a tuple containing a name, a description, and a [`tensorboard.plugins.base_plugin.TBLoader`][tb-loader] object.

In order to be discovered by TensorBoard, the distribution should include an [`entry_points` entry][entry-points] declaring itself as a TensorBoard plugin. The entry point group is `tensorboard_plugins`. The entry point name is arbitrary, and ignored. The entry point object reference should refer to the standard interface described in the previous paragraph. As an example, the setup.py file for a hypothetical “widgets” plugin might look like:
Copy link
Contributor

Choose a reason for hiding this comment

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

standard interface describe in the previous paragraph

I can't find a detailed description of any interface in the preceding paragraph. Am I missing something? I see you said "The exact shape is not yet specified." But can you at least come up with an outline? There is already such a plugin registration method in the existing TensorBoard plugin system. Is the new interface expected to be similar or significantly different. I feel this RfC should at least include some rudimentary information on this topic.

Choose a reason for hiding this comment

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

Correct, the shape is not yet specified; the outline is, “conceptually
similar to a function returning a tuple containing a name, a
description, and a loader object”. Maybe it’ll just end up being a
function that returns a plugin loader, if we determine that we don’t
need any extra metadata.

The existing plugin system requires you to write your own main module
to register plugins, whereas this new system will not.

We haven’t given a ton of thought to the details of this interface,
because we expect it to be simple for anyone to implement. We’ll pick
something and specify it clearly, and people can just use it.


Instead, TensorBoard can provide a library one of which will be a `request`. The method will use `window.postMessage` to route the network request to the main frame and hide away the fact that it is using inter-frame communication at all. This solution, though easy for XMLHttpRequest based request, is difficult to apply for, for example, image, audio, video, CSS, web-fonts, EsModule, and WebSocket requests. To illustrate the problem more concretely, when `<img src="foo.png">` is created, a browser initiates the requests and it is not possible to route that request via the library. This will make a plugin like Images plugin difficult to write.

## Questions and Discussion Topics
Copy link
Contributor

Choose a reason for hiding this comment

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

Big missing pieces I see in this draft

  1. What is the signature of get_plugin(), i.e., how tensorboard discovers the 3p plugin? See my comment about the "standard interface" above.
  2. Apart from the standard interface, are there going to be any new protocols for any of the following?
  • data storage in the backend (e.g., tensorboard core can provide a unified database write/read API)
  • communication protocol between the plugin's frontend component and the backends (e.g., tensorboard can provide an NPM package that helps the frontend component manage the requests to the backend).

Copy link
Contributor

Choose a reason for hiding this comment

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

+1

Building on the discussion of the communication protocol, Does this illustration capture how you expect the pattern of communication to work? Or do you rather expect the TensorBoard FE to pass an address to the Plugin FE so that the Plugin FE can talk to the backend components directly?

image

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@bileschi thanks for the feedback. At this moment, we would like to confine the scope of design to redo the distribution strategy. The architecture you are describing has more profound impact than depicted in the picture and should be redesigned holistically (including the summary system and storage system. Note that the storage should also be pluggable: data format, storage mechanism, and etc...).

Before closing it, I would also like to note that close coupling between frontend and backend (and distributed as one unit) alleviates the concern of fragmentation raised below. Though hard to avoid duplication of features and varying level of quality, current plugin system allow us to do without API versioning and conflict resolutions.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the feedback! Can you provide a substitute diagram which describes the proposed communication strategy?

Copy link
Member

Choose a reason for hiding this comment

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

I also strongly believe that we should reconsider the architecture of providing plugins as coupled (Backend, Frontend) pairs.

At the same time, I think that problem is somewhat separable from the current proposal. That is, this RFC goes one step in the right direction; we'd like to discuss additional related steps soon, but they don't need to block this one.

In particular I think we can easily get consensus on these bits:

  • Plugins providing BE Python code are dynamically loaded at runtime.
  • BE plugins are distributed via PIP.

While these aspects of the current architecture (which this RFC retains) should remain open topics:

  • Plugins bundle FE code.
  • The TensorBoard backend is responsible for assembling the FE app from the available plugins and serving it.
  • Plugin network APIs are not formalized or versioned.

The reason this is coming up now is that most of this doc is specifically about how the plugin FEs should be assembled into the FE app that TB serves. Under a decoupled architecture in which the FE is built and served as an independent entity (presumably, from components distributed via npm, bundled with webpack or similar, etc.), the discussion around whether or not to use iFrames etc. would be very different.

The upshot is I think it's OK to go ahead with this proposal, because the dynamic BE plugin loading (and PIP distribution of those) is great in any case. It may turn out that the FE-assembly questions here will be obviated by a later design of broader scope; let's give that the attention it deserves in a separate RFC.

Copy link
Member

Choose a reason for hiding this comment

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

@bileschi As I understand it, the current proposal does not change the existing communication strategy, which is that a plugin FE makes requests only to routes that its matched BE exposes (or to standard routes that the TB core provides). See https://github.com/tensorflow/tensorboard/blob/master/tensorboard/http_api.md. There is no client-side TB code mediating those requests, and no client-side state shared between FE plugins. (Those are certainly good things to discuss; they're just outside the present scope.)

@bhack
Copy link
Contributor

bhack commented Apr 12, 2019

There were some general comments related to plugins/modularities in the Tensorflow addons RFC starting from #84 (comment).
I hope that we can include a solution to handle quality fragmentation also here.


**Con**
- global mutating libraries like Polymer app will not work
- can lead to an awkward UX: e.g., different version of a visualization library have different UI/UX
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure I follow-- can you explain this concern further or give an example?

- does not work in Colab
- generally larger binaries
- Cannot reuse components across plugins
- can lead to an awkward UX: e.g., different version of a visualization library have different UI/UX
Copy link
Member

Choose a reason for hiding this comment

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

This a Con of both options? Is there any alternative that avoids it, then?


Instead, TensorBoard can provide a library one of which will be a `request`. The method will use `window.postMessage` to route the network request to the main frame and hide away the fact that it is using inter-frame communication at all. This solution, though easy for XMLHttpRequest based request, is difficult to apply for, for example, image, audio, video, CSS, web-fonts, EsModule, and WebSocket requests. To illustrate the problem more concretely, when `<img src="foo.png">` is created, a browser initiates the requests and it is not possible to route that request via the library. This will make a plugin like Images plugin difficult to write.

## Questions and Discussion Topics
Copy link
Member

Choose a reason for hiding this comment

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

I also strongly believe that we should reconsider the architecture of providing plugins as coupled (Backend, Frontend) pairs.

At the same time, I think that problem is somewhat separable from the current proposal. That is, this RFC goes one step in the right direction; we'd like to discuss additional related steps soon, but they don't need to block this one.

In particular I think we can easily get consensus on these bits:

  • Plugins providing BE Python code are dynamically loaded at runtime.
  • BE plugins are distributed via PIP.

While these aspects of the current architecture (which this RFC retains) should remain open topics:

  • Plugins bundle FE code.
  • The TensorBoard backend is responsible for assembling the FE app from the available plugins and serving it.
  • Plugin network APIs are not formalized or versioned.

The reason this is coming up now is that most of this doc is specifically about how the plugin FEs should be assembled into the FE app that TB serves. Under a decoupled architecture in which the FE is built and served as an independent entity (presumably, from components distributed via npm, bundled with webpack or similar, etc.), the discussion around whether or not to use iFrames etc. would be very different.

The upshot is I think it's OK to go ahead with this proposal, because the dynamic BE plugin loading (and PIP distribution of those) is great in any case. It may turn out that the FE-assembly questions here will be obviated by a later design of broader scope; let's give that the attention it deserves in a separate RFC.

@stephanwlee
Copy link
Contributor Author

Due to uncertainty with the Colab + iframe story, we are going to push out the deadline few days til May 6th. Thanks for all the comments so far!

@stephanwlee
Copy link
Contributor Author

Sorry for the delay in closing the RFC. Prototype with Colab took longer than expected but everything is more or less looks good now. I have updated the proposal accordingly (the iframe based solution wins).

Since there was no major disagreement generally and especially since there wasn't much disagreement on how plugins are loaded, where we spent extra time prototyping with Colab, we will bake the proposal few more days and try to close it on May 19th. Thanks!

@stephanwlee
Copy link
Contributor Author

Alright, we have let it bake for awhile and I think it is good to close this.
@ewilderj can you help us with merging this? Thanks!

@ewilderj
Copy link
Contributor

Please push an update changing the status from Proposed to Accepted, and I will merge. Many thanks!

This RFC proposes a friction-free approach to enabling TensorBoard
contributors to create and share TensorBoard plugins as installable
extensions.

TensorBoard is composed of various plugins that show up as separate
dashboards to users (e.g., scalars, graphs, hparams, histograms, etc.).
Today. anyone can contribute a new plugin by forking TensorBoard, adding
their plugin, and getting the PR merged. However, this can require a
lengthy review to make sure it is architected correctly and is aligned
with TensorBoard’s goals.

This RFC covers two changes that will make this process easier: Plugins
can be distributed as Python packages. Users can install them as
extensions to TensorBoard.  Plugins are no longer limited to use Polymer
but can instead use any frontend library or framework (e.g., React)

The intention is to enable any prospective contributor to easily be able
to create or reuse a visualization with tools familiar to them, and
enable users to try it out quickly. Plugins that are useful to many
users should still be added to TensorBoard, but this removes the
friction needed to start getting feedback or trying out something new.
@stephanwlee
Copy link
Contributor Author

@ewilderj done! Thanks a lot!

@ewilderj ewilderj added RFC: Accepted RFC Design Document: Accepted by Review and removed RFC: Proposed RFC Design Document labels May 24, 2019
@ewilderj ewilderj merged commit 1b58758 into tensorflow:master May 24, 2019
stephanwlee added a commit to tensorflow/tensorboard that referenced this pull request May 29, 2019
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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes RFC: Accepted RFC Design Document: Accepted by Review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants