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

pip: fix smoke test double-TensorBoard dependency #2211

Merged
merged 3 commits into from
May 15, 2019

Conversation

wchargin
Copy link
Contributor

@wchargin wchargin commented May 7, 2019

Summary:
The smoke test doesn’t actually catch those things that it’s supposed to
catch, because it installs tf-nightly (or tf-nightly-2.0-preview),
which in turn installs tb-nightly. We then install the just-built
version of tensorboard, so the resulting virtualenv has both
tensorboard and tb-nightly: #1907 strikes again.

When --tf-version "" is passed, this is not an issue, but some smoke
checks only run when that is not passed.

It just so happens that this is currently benign: each of the direct
dependencies of :build_pip_package either has no smoke test, contains
an __init__.py file that is required to overwrite the Bazel-generated
empty one, because the smoke test references symbols directly in that
file, or is not yet in tb-nightly. But (a) starting with tomorrow’s
nightly, the mesh smoke test will meet none of those conditions, so its
smoke test will cease to be effective, and (b) I’d like to add an
hparams smoke test, which already falls into the same boat.

Test Plan:
Add an hparams smoke test, but remove the actual hparams dep:

diff --git a/tensorboard/pip_package/BUILD b/tensorboard/pip_package/BUILD
index c11e22e2..e63b790a 100644
--- a/tensorboard/pip_package/BUILD
+++ b/tensorboard/pip_package/BUILD
@@ -21,7 +21,6 @@ sh_binary(
         "//tensorboard:lib",  # User-facing overall TensorBoard API
         "//tensorboard:version",  # Version module (read by setup.py)
         "//tensorboard/plugins/beholder",  # User-facing beholder API
-        "//tensorboard/plugins/hparams",  # User-facing hparams API
         "//tensorboard/plugins/mesh:summary",  #  User-facing mesh summary API
         "//tensorboard/plugins/projector",  # User-facing projector API
         "//tensorboard/summary:tf_summary",  #  tf.summary API for TF 2.0
diff --git a/tensorboard/pip_package/build_pip_package.sh b/tensorboard/pip_package/build_pip_package.sh
index de778196..113724ef 100755
--- a/tensorboard/pip_package/build_pip_package.sh
+++ b/tensorboard/pip_package/build_pip_package.sh
@@ -172,6 +172,7 @@ import tensorboard as tb
 tb.summary.scalar_pb('test', 42)
 from tensorboard.plugins.beholder import Beholder, BeholderHook
 from tensorboard.plugins.mesh import summary
+from tensorboard.plugins.hparams.api import hparams_pb, KerasCallback
 "
   fi

Then run

bazel run //tensorboard/pip_package:build_pip_package

and note that the smoke test passes (incorrectly) before this commit and
fails (correctly) after it.

Note also that the pip freeze output from the smoke test includes both
tb-nightly and tensorboard before this change (e.g., on recent
Travis logs), but only tensorboard after it.

wchargin-branch: pip-smoke-2dep

Summary:
The smoke test doesn’t actually catch those things that it’s supposed to
catch, because it installs `tf-nightly` (or `tf-nightly-2.0-preview`),
which in turn installs `tb-nightly`. We then install the just-built
version of `tensorboard`, so the resulting virtualenv has both
`tensorboard` and `tb-nightly`: #1907 strikes again.

When `--tf-version ""` is passed, this is not an issue, but some smoke
checks only run when that is not passed.

It just so happens that this is currently benign: each of the direct
dependencies of `:build_pip_package` either has no smoke test, contains
an `__init__.py` file that is required to overwrite the Bazel-generated
empty one, because the smoke test references symbols directly in that
file, or is not yet in `tb-nightly`. But (a) starting with tomorrow’s
nightly, the mesh smoke test will meet none of those conditions, so its
smoke test will cease to be effective, and (b) I’d like to add an
hparams smoke test, which already falls into the same boat.

Test Plan:
Add an hparams smoke test, but remove the actual hparams dep:

```diff
diff --git a/tensorboard/pip_package/BUILD b/tensorboard/pip_package/BUILD
index c11e22e2..e63b790a 100644
--- a/tensorboard/pip_package/BUILD
+++ b/tensorboard/pip_package/BUILD
@@ -21,7 +21,6 @@ sh_binary(
         "//tensorboard:lib",  # User-facing overall TensorBoard API
         "//tensorboard:version",  # Version module (read by setup.py)
         "//tensorboard/plugins/beholder",  # User-facing beholder API
-        "//tensorboard/plugins/hparams",  # User-facing hparams API
         "//tensorboard/plugins/mesh:summary",  #  User-facing mesh summary API
         "//tensorboard/plugins/projector",  # User-facing projector API
         "//tensorboard/summary:tf_summary",  #  tf.summary API for TF 2.0
diff --git a/tensorboard/pip_package/build_pip_package.sh b/tensorboard/pip_package/build_pip_package.sh
index de778196..113724ef 100755
--- a/tensorboard/pip_package/build_pip_package.sh
+++ b/tensorboard/pip_package/build_pip_package.sh
@@ -172,6 +172,7 @@ import tensorboard as tb
 tb.summary.scalar_pb('test', 42)
 from tensorboard.plugins.beholder import Beholder, BeholderHook
 from tensorboard.plugins.mesh import summary
+from tensorboard.plugins.hparams.api import hparams_pb, KerasCallback
 "
   fi

```

Then run

    bazel run //tensorboard/pip_package:build_pip_package

and note that the smoke test passes (incorrectly) before this commit and
fails (correctly) after it.

Note also that the `pip freeze` output from the smoke test includes both
`tb-nightly` and `tensorboard` before this change (e.g., on recent
Travis logs), but only `tensorboard` after it.

wchargin-branch: pip-smoke-2dep
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.

Thanks for the fix.

tensorboard/pip_package/build_pip_package.sh Outdated Show resolved Hide resolved
wchargin-branch: pip-smoke-2dep
wchargin-branch: pip-smoke-2dep
@wchargin wchargin merged commit 2f1abca into master May 15, 2019
@wchargin wchargin deleted the wchargin-pip-smoke-2dep branch May 15, 2019 23:05
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