program: restore default assets zip provider #4628
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Summary: This patch reverts the backward-incompatible changes to `program` made in #4588, while retaining the new functionality in that change. Specifically, we restore the calling convention of `TensorBoard` (reordering the parameters was a breaking change, as was removing the default value). For clarity, the data dependency to `webfiles.zip` lives in a new Python module, `//tensorboard:assets_lib`. The `program` module late-imports this only if no assets zip provider is given, in which case the caller is expected to have a build dep on `:assets_lib`. The main `//tensorboard` binary does so; `//tensorboard:dev` does not. Test Plan: Tested `//tensorboard`, `//tensorboard:dev`, and the Pip package in a new virtualenv. wchargin-branch: program-default-assets wchargin-source: b7dada87af060dc3ffbc5dd2ffee63652f1c1a23
wchargin-branch: program-default-assets wchargin-source: bdd62bf3774460c084ef2c9176a2570d939d69b8
Contributor
Author
|
(Oops; meant to send to @nfelt since we’d talked about this. |
stephanwlee
approved these changes
Jan 29, 2021
Comment on lines
+108
to
+114
| try: | ||
| from tensorboard import assets | ||
| except ImportError as e: | ||
| # `tensorboard.assets` is not a strict Bazel dep; clients are | ||
| # required to either depend on `//tensorboard:assets_lib` or | ||
| # pass a valid assets provider. | ||
| raise ImportError( |
Contributor
There was a problem hiding this comment.
No action required (or just relief):
I was a bit worried program.py would have hard dep on webfiles.zip but this of course make it better.
Contributor
Author
There was a problem hiding this comment.
Yeah—it took a bit of thinking to come up with a build/import structure
that I liked, but I think that it turned out okay.
nfelt
approved these changes
Jan 29, 2021
tensorboard/program.py
Outdated
| TBLoader instances or classes. If not specified, defaults to first-party | ||
| plugins. | ||
| assets_zip_provider: A function that provides a zip file containing assets to | ||
| the application. |
Contributor
There was a problem hiding this comment.
Maybe mention what happens if this is omitted/None?
wchargin-branch: program-default-assets wchargin-source: 7fa0230e73ba7d31194f53a124ebdd5ba4808622
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary:
This patch reverts the backward-incompatible changes to
programmadein #4588, while retaining the new functionality in that change.
Specifically, we restore the calling convention of
TensorBoard(reordering the parameters was a breaking change, as was removing the
default value). For clarity, the data dependency to
webfiles.ziplivesin a new Python module,
//tensorboard:assets_lib. Theprogrammodulelate-imports this only if no assets zip provider is given, in which case
the caller is expected to have a build dep on
:assets_lib. The main//tensorboardbinary does so;//tensorboard:devdoes not.Test Plan:
Tested
//tensorboard,//tensorboard:dev, and the Pip package in anew virtualenv. There is no build path from
:devto the prod assets:wchargin-branch: program-default-assets