-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Extensible plugin infrastructural changes #2257
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks! (I haven’t looked at the frontend changes yet.)
tensorboard/http_api.md
Outdated
@@ -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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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/default_test.py
Outdated
|
||
@staticmethod | ||
def create(): | ||
"""Creates an instance off FakeEntryPoint. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sp.: off → of
tensorboard/default_test.py
Outdated
|
||
if __name__ == "__main__": | ||
test.main() | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Superfluous blank line.
tensorboard/default_test.py
Outdated
actual_plugins = default.get_dynamic_plugins() | ||
|
||
mock_iter_entry_points.assert_called_with('tensorboard_plugins') | ||
self.assertEquals(actual_plugins, [FakePlugin]) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The assertEquals
function is deprecated; use assertEqual
.
tensorboard/default_test.py
Outdated
return FakePlugin | ||
|
||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: Just two blank lines between module members.
tensorboard/BUILD
Outdated
@@ -201,7 +201,9 @@ py_test( | |||
deps = [ | |||
":default", | |||
":test", | |||
"//tensorboard/plugins/scalar:scalars_plugin", | |||
"//tensorboard:expect_pkg_resources_installed", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation
tensorboard/http_api.md
Outdated
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the example below it looks like the field is null
, right?
return this._dashboardData | ||
.map((d) => d.plugin) | ||
.filter((dashboardName) => { | ||
return this._pluginsListing[dashboardName] && |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
const activePlugins = result.value; | ||
this._activeDashboards = this._dashboardData.map( | ||
d => d.plugin).filter(p => activePlugins[p]); | ||
this._pluginsListing = result.value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The renames/refactors in this commit make the tf-tensorboard
logic
easier to follow; thanks!
@@ -0,0 +1,74 @@ | |||
# Copyright 2017 The TensorFlow Authors. All Rights Reserved. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: 2019
tensorboard/default.py
Outdated
entry_point.load() | ||
for entry_point in pkg_resources.iter_entry_points('tensorboard_plugins') | ||
] | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: trailing blank line
Mind rebasing/merging master to pull in #2267? Bazel 0.26 is out. |
if (typeof maybeMetadata === 'boolean') { | ||
return maybeMetadata; | ||
} | ||
return this._pluginsListing[dashboardName].enabled; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
This is the change based on tensorflow/community#90.
The change makes two improvements:
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.