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

plugins: add sample dynamically loaded plugin #2354

Merged
merged 37 commits into from
Jun 24, 2019

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented Jun 17, 2019

Summary:
This plugin can eventually replace the tensorboard-plugin-example
repository; it’ll be easier for us to keep it up to date if it’s in the
same repository as main TensorBoard. And it has tests!

Test Plan:
Run bazel test //tensorboard/plugins/example:smoke_test to start.
Then, modify setup.py to remove the entry_points entry, and note
that the test fails. Separately, modify line 42 of __init__.py to
serve the JS under /not_index.js instead; the test should again fail.

Test the development workflow: Follow the instructions in README.md.
Then, edit widgets.js (say, add another exclamation point) and refresh
to see changes live without restarting TensorBoard:

screenshot of “Hello TensorBoard!” in example plugin

wchargin-branch: example-dynamic-plugin

Summary:
This plugin can eventually replace the `tensorboard-plugin-example`
repository; it’ll be easier for us to keep it up to date if it’s in the
same repository as main TensorBoard.

Test Plan:
Follow the instructions in `README.md`. Then, edit `widgets.js` (say,
add another exclamation point) and refresh to see changes live without
restarting TensorBoard:

[1]: https://user-images.githubusercontent.com/4317806/59641012-cb700380-9114-11e9-8323-c811388df31c.png

wchargin-branch: example-dynamic-plugin
Copy link
Contributor Author

@wchargin wchargin left a comment

Choose a reason for hiding this comment

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

Thanks. Will fix the Pip package data issue.

tensorboard/plugins/example/setup.py Show resolved Hide resolved
tensorboard/plugins/example/.gitignore Show resolved Hide resolved
wchargin-source: da430b7b6dd8c773371d770a38d3d33960e1c635
wchargin-branch: example-dynamic-plugin
Copy link
Contributor

@nfelt nfelt left a comment

Choose a reason for hiding this comment

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

Can we add some kind of minimal smoke test for this example plugin to ensure that it continues to be usable, unlike the old example repo?

wchargin added a commit that referenced this pull request Jun 18, 2019
Summary:
Prior to this change, defining a (possibly dynamically loaded) plugin
without this initializer would yield the rather inscrutable error

```
  File "/VIRTUAL_ENV/tensorboard/plugins/base_plugin.py", line 253, in load
    return self._plugin_class(context)
TypeError: object() takes no parameters
```

whose stack trace does not contain the offending plugin code at all.
(The error is because initializers are effectively inherited in Python.)

Test Plan:
Cherry-pick #2354, delete its example plugin’s `__init__`, and note that
the example still works. (You’ll have to build TensorBoard’s Pip package
and install it into a virtualenv.)

wchargin-branch: plugin-empty-initializer
wchargin added a commit that referenced this pull request Jun 18, 2019
Summary:
Prior to this change, defining a (possibly dynamically loaded) plugin
without this initializer would yield the rather inscrutable error

```
  File "/VIRTUAL_ENV/tensorboard/plugins/base_plugin.py", line 253, in load
    return self._plugin_class(context)
TypeError: object() takes no parameters
```

whose stack trace does not contain the offending plugin code at all.
(The error is because initializers are effectively inherited in Python.)

Test Plan:
Cherry-pick #2354, delete its example plugin’s `__init__`, and note that
the example still works. (You’ll have to build TensorBoard’s Pip package
and install it into a virtualenv.)

wchargin-branch: plugin-empty-initializer
@wchargin
Copy link
Contributor Author

wchargin commented Jun 18, 2019

Can we add some kind of minimal smoke test for this example plugin to
ensure that it continues to be usable, unlike the old example repo?

Yes.

Such a smoke test needs to depend on the TensorBoard Pip package, as
well as building the Pip package for the example plugin and installing
both into a new virtualenv.

I think that probably the cleanest way to do this is to split the
current //tensorboard/pip_package:build_pip_package, which both builds
the package and tests it, into an actual Bazel target for the two wheels
(a genrule depending on an sh_binary) and a second target to test
them (a sh_test, as currently*). Then the example plugin’s test can
depend on the TensorBoard wheels rather than rebuilding them.

I’m happy to do this, though it will take a bit of doing.

* EDIT: We currently have just an sh_binary, not an sh_test; we
could keep that or change it to an actual test.

Summary:
We plan to promote TensorBoard’s Pip packages to Bazel targets, but the
filenames of these targets can’t actually be statically determined: they
must include the TensorBoard version number, which is defined in a
Python module. Instead, we’ll build a tarball containing the two wheels.
Building tarballs deterministically with system tools is famously
troublesome (you can do it with GNU `tar` and a lot of flags, but not
portably), and it seems that the standard approach is to use Python’s
built-in `tarfile` library.

Implementation based off of <servo/servo#12244>,
but with changes to make it Python 3-compatible, to use our code style,
and to clean up some minor issues and fix some TODOs from the original.
The original source is Apache-2 licensed.

Test Plan:
End-to-end tests included. Replacing the body of `_run_tool` with

```python
    return subprocess.check_output([
        "/bin/sh",
        "-c",
        'x="$(readlink -f "$1")" && cd "$2" && tar czf "$x" .',
        "unused",
    ] + args)
```

causes the `test_invariant_under_mtime` test case to fail for the right
reason.

wchargin-branch: deterministic-tgz
Summary:
Running `bazel build //tensorboard/pip_package` now actually builds the
Pip package. Other targets can depend on the built wheels, which are
provided in a tarball. Smoke tests have been separated to a new
`:test_pip_package` target.

Test Plan:
Check that everything builds:

```
$ bazel build //tensorboard/pip_package 2>/dev/null
$ echo $?
0
$ tar tzf bazel-bin/tensorboard/pip_package/pip_packages.tar.gz
tensorboard-1.15.0a0-py2-none-any.whl
tensorboard-1.15.0a0-py3-none-any.whl
$ bazel run //tensorboard/pip_package:test_pip_package >/dev/null 2>&1
$ echo $?
0
```

Then:

  - Add an `assert False` to the `pb` function in `scalar/summary.py`,
    and verify that `:test_pip_package` fails with default options but
    passes with `--tf-version ""`.
  - Add an `assert False` to `tensorboard/main.py` and verify that the
    test fails even with `--tf-version ""`.
  - Add `def foo(): yield from None` to `tensorboard/main.py`, and
    verify that, when run in a Python 3 virtualenv, the test fails by
    default but passes with `--default-python-only`.

wchargin-branch: bazel-pip-package
wchargin-source: 309c6d9b6b7c08c652bcf87a224e299ebbae64e1
wchargin-branch: bazel-pip-package
wchargin-source: 2d3424900b3e3c7839a4f9a67999f75b8fe2ed0d
wchargin-branch: bazel-pip-package
wchargin-source: b90f6b91562638265cb95820aec6e29f9f3e45a9
wchargin-branch: example-dynamic-plugin
wchargin-source: b90f6b91562638265cb95820aec6e29f9f3e45a9
wchargin-branch: example-dynamic-plugin
@wchargin wchargin changed the base branch from master to wchargin-bazel-pip-package June 19, 2019 19:27
@wchargin
Copy link
Contributor Author

Thanks to both of you for pushing back on the Pip package data deps and
the testing, respectively. I’d planned to set those up in a later PR,
but now that they’re both here I’m much happier with this one.

wchargin-source: 991fdaa7a849ba49d062e9f640d6ddd12ffc703e
wchargin-branch: deterministic-tgz
wchargin-source: 33bf6658a701a3348e21fea5418d9825e4dc21b9
wchargin-branch: bazel-pip-package

# Conflicts:
#	tensorboard/pip_package/build_pip_package.sh
wchargin-source: 33bf6658a701a3348e21fea5418d9825e4dc21b9
wchargin-branch: bazel-pip-package
wchargin-source: f17f6e5093ebf6f8d9d56bdc8dc9623a4dd5ab19
wchargin-branch: example-dynamic-plugin
wchargin-source: 30e936b83964ab4b78b9c9755fc62849b182c3d2
wchargin-branch: deterministic-tgz
wchargin-source: 30e936b83964ab4b78b9c9755fc62849b182c3d2
wchargin-branch: deterministic-tgz
wchargin-source: 30e936b83964ab4b78b9c9755fc62849b182c3d2
wchargin-branch: bazel-pip-package
wchargin-source: 30e936b83964ab4b78b9c9755fc62849b182c3d2
wchargin-branch: example-dynamic-plugin
wchargin-source: 54daa09db50cca2bc6b41c933fc40e7b124495ac
wchargin-branch: deterministic-tgz
wchargin-source: 54daa09db50cca2bc6b41c933fc40e7b124495ac
wchargin-branch: bazel-pip-package
wchargin-source: 54daa09db50cca2bc6b41c933fc40e7b124495ac
wchargin-branch: example-dynamic-plugin
wchargin-source: 065ea1bdcfbd0621f5998a8e310b9e2e7f1f6a5f
wchargin-branch: deterministic-tgz
wchargin-source: 065ea1bdcfbd0621f5998a8e310b9e2e7f1f6a5f
wchargin-branch: bazel-pip-package
wchargin-source: 065ea1bdcfbd0621f5998a8e310b9e2e7f1f6a5f
wchargin-branch: example-dynamic-plugin
wchargin-source: a410c619117892bb8945f20c5c277d748801e342
wchargin-branch: deterministic-tgz
wchargin-source: a410c619117892bb8945f20c5c277d748801e342
wchargin-branch: deterministic-tgz
wchargin-source: a410c619117892bb8945f20c5c277d748801e342
wchargin-branch: bazel-pip-package
wchargin-source: a410c619117892bb8945f20c5c277d748801e342
wchargin-branch: example-dynamic-plugin
wchargin-source: 639878d4e6349a2977efc1ff82d6f6c3d103eaf5
wchargin-branch: bazel-pip-package
wchargin-source: 639878d4e6349a2977efc1ff82d6f6c3d103eaf5
wchargin-branch: bazel-pip-package
wchargin-source: 64e82cba51f08cc9733fc20469f3b37efd9eff53
wchargin-branch: example-dynamic-plugin
wchargin-source: d75820f22a6622d33b775213271b94f8f15ae9a4
wchargin-branch: bazel-pip-package
wchargin-source: 51ab1d5e05bb1df1edb2c372f44bf34671fbcf90
wchargin-branch: example-dynamic-plugin
wchargin-source: c16653dbed2cb17f4909fe0b568fb9fb009bf4ee
wchargin-branch: bazel-pip-package
wchargin-source: 9ae1b734041ee8d510914fd6914b790bd88b139c
wchargin-branch: example-dynamic-plugin
wchargin-source: 243330e9aadb74dd31360a8bcae168c13953b205
wchargin-branch: bazel-pip-package
wchargin-source: 5aadd23f332ea61486a059de08cde95861ad09ec
wchargin-branch: example-dynamic-plugin
wchargin-source: 7ccbb3d00964617280946b9364a64a668e44d92b
wchargin-branch: example-dynamic-plugin
@wchargin wchargin changed the base branch from wchargin-bazel-pip-package to master June 24, 2019 17:03
wchargin-source: 285ad2a1fcc514909d130946f23e13d5fbc427d8
wchargin-branch: example-dynamic-plugin
@wchargin wchargin merged commit afecb68 into master Jun 24, 2019
@wchargin wchargin deleted the wchargin-example-dynamic-plugin branch June 25, 2019 01:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants