infra: create dev target #4588
infra: create dev target #4588
Conversation
tensorboard/BUILD
Outdated
There was a problem hiding this comment.
So I agree this is where the data dep belongs, but right now the code in main.py actually reads it from program.get_default_assets_zip_provider(). It used to live in default but was moved to program back in #1631 (not really sure why, at this point).
IMO better at this point to just inline it into main.py and remove the fallback logic in program.TensorBoard() that calls it, so that assets_zip_provider must be specified by the caller. That way we don't have skew between where the data dep is defined in the BUILD file, and where the code actually expects it to exist. WDYT?
There was a problem hiding this comment.
Makes sense. Done.
tensorboard/BUILD
Outdated
There was a problem hiding this comment.
I assume the idea is that we'll replace this fairly soon with something more substantially different? Otherwise, would be good to have a comment saying to keep the srcs/deps in sync with the non-dev assets.
There was a problem hiding this comment.
Yup, #4589 gives you glimpse of what the difference might be.
New `tensorboard:dev` target provides a development instance of the application which optimizes for development experience. While the changes are omitted in this commit for brevity, we are looking to stream line the FE building to optimize for its incremental build time.
nfelt
left a comment
nfelt
left a comment
There was a problem hiding this comment.
Pip package smoke tests seems to be broken by this change :/ If I had to guess, maybe either moving the data dependency, or moving the webfiles.zip reference (I would think referencing from program.py would work the same as from main.py but maybe for some strange reason it's different).
tensorboard/program.py
Outdated
| TBLoader instances or classes. If not specified, defaults to first-party | ||
| plugins. | ||
| assets_zip_provider: Delegates to TBContext or uses default if None. | ||
| assets_zip_provider: A function that provides a zip file containing assets to |
There was a problem hiding this comment.
Move up one to match arg order?
| from tensorboard.plugins.core import core_plugin | ||
|
|
||
|
|
||
| def fake_asset_provider(): |
There was a problem hiding this comment.
If we wanted to avoid the boilerplate here we could always make the argument still optional, but if omitted, we would generate a dummy zip file that just contains index.html with Hello TensorBoard or something. Probably fine to make it required, but just a thought.
There was a problem hiding this comment.
I prefer things to be a bit more explicit so I will keep it as is.
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
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. There is no build path from `:dev` to the prod assets: ``` $ bazel query 'somepath(//tensorboard:dev, //tensorboard:webfiles.zip)' INFO: Empty results ``` wchargin-branch: program-default-assets
2 participants
New
tensorboard:devtarget provides a development instance of theapplication which optimizes for development experience. While the
changes are omitted in this commit for brevity, we are looking to stream
line the FE building to optimize for its incremental build time.