Update Protobuf Bazel Workspace#7094
Conversation
arcra
left a comment
There was a problem hiding this comment.
As discussed offline, I'd like some optional and somewhat independent changes to be done in separate PRs, ideally.
Also, not sure what issues you were finding with the urllib and six packages, but there were a couple related PRs: #7016 and #7013 (we vendor the urllib3 package, IIRC, and according to that second PR, the six library shouldn't be necessary anymore).
| sudo apt-get update | ||
| sudo apt-get install -y libgbm-dev libxss1 libasound2 | ||
| sudo apt-get install -y "${py_abi}-dev" libgbm-dev libxss1 libasound2 | ||
| # Bazel's system_python repository resolves headers relative to the |
There was a problem hiding this comment.
Is this a problem that started from upgrading to bazel version 7.7? Or from running in our self-hosted container? Or with the new protobuf version?
Perhaps this comment explains it... although it's not super clear to me either. Or maybe an incompatibility of the setup-python action in a self-hosted runner, as described here.
Generally, this seems hacky for something that I expect should be easily supported (setting up python).
Having said this, I think the environment is fairly complex, so if this works, I guess I'm ok with it, but it will just be complex to maintain. If we could make it work in a simpler way, that would be my preference.
Perhaps we can try either:
- Using the env var mentioned in the "advance usage" documentation of the setup-python action (linked above)
- Using a newer setup-python that might handle this better
- Remove the setup-python action, and just installing the specific python version via shell commands directly in the container.
We can leave this as a follow-up, but if possible, I'd like this change to be submitted as a standalone PR.
There was a problem hiding this comment.
This is primarily a CI environment issue:
- Bazel's
system_pythonrepo resolves headers relative to the interpreter exposed bysetup-python - In the self-hosted container, that include path does not already contain
Python.h - The fix is needed for Bazel repo loading/builds in that environment
This is not caused by protobuf logic directly.
It is hacky and reasonable to split later with:
- newer
setup-python - shell-installed Python in container
- alternate Bazel python toolchain wiring
There was a problem hiding this comment.
Sg, we can try to clean up this setup later. Although, if possible, it would still be nice if this could be merged separately from other changes.
I expect that "update the way we setup python for CI" can be independent of everything else, or at least (if necessary) as part of a "bazel 7 upgrade" PR, but without the changes for the protobuf dependencies upgrade.
I know you've mentioned it's been hard to get a stable version to work, but I don't know if you've already attempted to do the bazel upgrade without doing the protobuf upgrade. If that's possible, that could be a cleaner way of merging these changes.
| # in our WORKSPACE file. | ||
| BAZEL_VERSION: '6.5.0' | ||
| BAZEL_SHA256SUM: 'a40ac69263440761199fcb8da47ad4e3f328cbe79ffbf4ecc14e5ba252857307' | ||
| BAZEL_VERSION: '7.7.0' |
There was a problem hiding this comment.
FWIW, my suggestion to upgrade Bazel version to 7.7 was to use bazel modules, which should ideally help with dependency management.
Are we getting any benefit from doing this upgrade? Does it help updating some dependencies or something?
There was a problem hiding this comment.
- TensorFlow 2.21 uses Bazel 7.7.0
- The upgrade unblocked protobuf 6.31.1 and related dependency alignment
- This branch already relies on several custom repository overrides, patches, and local repos
- Moving to bzlmod would be a larger dependency-management refactor than the minimum needed to align with TF 2.21, but this could be considered in a future split.
There was a problem hiding this comment.
The version of Bazel that TF uses should not have any effect on what we use.
I don't really see how this helps at this point, but not opposed to it. We will want to update to use Bazel modules eventually (and I had hoped that could have helped with this protobuf upgrade), but since this is working as is. I'm ok with moving forward with this.
| def protobuf_java_export(**kwargs): | ||
| java_export( | ||
| javacopts = JAVA_RELEASE_OPTS, | ||
| - # https://github.com/bazelbuild/rules_jvm_external/issues/1245 |
There was a problem hiding this comment.
Can we add comments for why we are patching these changes? (all of these files)
There was a problem hiding this comment.
Adding comments to this files
| @@ -2,6 +2,11 @@ | |||
|
|
|||
| We use [patch-package](https://www.npmjs.com/package/patch-package) to apply | |||
There was a problem hiding this comment.
Is this still accurate? (i.e. are we still using patch-package somewhere?) Or should we remove this comment?
It's unclear with the current phrasing. If this is still used, can you rephrase to make it clear that both methods are used for different libraries? (and on what this depends or why we're using two methods?)
There was a problem hiding this comment.
patch-package is still the authoring/generation mechanism for npm patches
this branch applies the generated patch artifacts through Bazel yarn_install(post_install_patches=...)
that avoids the less reliable install-time invocation inside the repository rule
There was a problem hiding this comment.
When you say "this branch", are you referring to a git branch?
This PR shows to me as attempting to merge into master. Did you intend it to go into the 2.21 branch?
Can we update the comment to say that the patches are generated using patch-package, but they are applied using yarn_install(post_install_patches=...) with a reason for why we do this? What is "less reliable" about the other method?
There was a problem hiding this comment.
I refer to the branch used in this PR "psamanoelton:protobuf", the merge can be either master or 2.21 branch.
Taking note to update with a comment
| srcs = ["__init__.py"], | ||
| srcs_version = "PY3", | ||
| # Keep the package's real __init__.py on this target instead of depending | ||
| # on :compat so Bazel 7 test runfiles expose the lazy tf/tf2 exports from |
There was a problem hiding this comment.
Is this bazel-7-specific? We might forget to update this if we update the project to use a new bazel version. Could we just say "bazel", without the specific version?
There was a problem hiding this comment.
This is a real fix, not cosmetic:
- Bazel test runfiles were exposing a synthesized/incorrect package layout
- that broke
from tensorboard.compat import tf/tf2 - the failure reproduced in both Bazel tests and wheel/runtime smoke tests
There was a problem hiding this comment.
This comment specifically was about saying "bazel 7" in the comment.
I guess the point was to communicate that "this started breaking with bazel 7". Consider rephrasing, but not a big deal.
Like I've mentioned before, I'd like it if we could make this change separately either "in preparation" of upgrading to Bazel 7, or perhaps as a "Bazel 7 upgrade" without upgrading the protobuf dependencies and any changes necessary for that (assuming that is possible, not sure if you attempted this already).
There was a problem hiding this comment.
I can use part of this PR to create a smaller one that only focus in Bazel 7
| path = "third_party/safe_html_types", | ||
| ) | ||
|
|
||
| # rules_closure's Soy toolchain still expects safe-html-types classes that are |
There was a problem hiding this comment.
Can this comment be above the previous lines where the local repository is defined?
| @@ -53,19 +84,108 @@ py_repositories() | |||
|
|
|||
| http_archive( | |||
There was a problem hiding this comment.
I thought you'd use the tb_http_archive rule here. Why is it only used in some places?
| ) | ||
|
|
||
| java_import_external( | ||
| name = "com_google_template_soy", |
There was a problem hiding this comment.
Can you add comments for why we added all of these dependencies? What needs this template soy package? And why are we adding the extra_build_file_content?
| package_json_remove = ["scripts.postinstall"], | ||
| patch_args = ["-p1"], | ||
| # Under Bazel 7.7.0 on this stack, invoking patch-package from the | ||
| # repository rule is fragile. Apply the existing git-format patches |
There was a problem hiding this comment.
What does "fragile" mean in this context? It would sometimes not work correctly?
| omit_com_google_common_html_types = True, | ||
| omit_com_google_protobuf = True, | ||
| omit_com_google_protobuf_js = True, | ||
| omit_com_google_template_soy = True, |
There was a problem hiding this comment.
Does this mean the rules_closure_dependencies are the ones that require the new dependencies above?
There was a problem hiding this comment.
I will answer all the WORKSPACE comments in this one...
safe_html_typesis the main true vendored code in this PR- it is a build-time Java dependency for the existing Closure/Soy toolchain, not TensorBoard runtime code
- the local copy is used because this branch needs a protobuf-6-compatible adjusted version of those classes, not just any upstream release
- the
omit_*entries are exactly what stoprules_closure_dependenciesfrom re-introducing older/conflicting transitive versions of the same Java deps tb_http_archiveis used where we needed common mirror/patch/link behavior; plainhttp_archiveremains where that helper was not yet applied or where we stayed closer to upstream declarations
Suggested actions:
- Keep vendoring for now unless we can prove an exact upstream archive works unchanged with protobuf 6.31.1.
- Improve comments in
WORKSPACE:- move the vendor rationale above
local_repository - add one comment before
com_google_template_soy - add one comment before
rules_closure_dependencies(...)explaining theomit_*linkage
- move the vendor rationale above
- Expand
third_party/safe_html_types/README.md:- what is vendored
- why it is needed
- why it is safe
- why it is local_repository instead of a plain archive in this branch
There was a problem hiding this comment.
I'd like us to try to find a version that works for this, for which we can just use an http_archive, instead of vendoring it. If we can't find one, then it's fine to proceed as is.
Summary
This PR updates TensorBoard’s local Bazel/protobuf build stack to stay compatible with the TensorFlow 2.21 ecosystem changes that this branch needs:
The goal of this PR is to keep the smallest working set of changes needed to make the branch build, test, and package correctly with the upgraded toolchain.
What changed
Bazel / workspace updates
.bazelversionand Bazel version guards to 7.7.0WORKSPACE) rather than switching this PR to bzlmodProtobuf 6.31.1 alignment
rules_closure / Soy compatibility
third_party/safe_html_typesas a build-time Java dependency override for the existing Soy toolchainrules_closure_dependencies(...)from re-introducing conflicting older transitive copiesPackaging and test fixes
tensorboard.compatpackage exports working correctly in Bazel runfiles and wheel/runtime smoke teststest_pip_packageand local Bazel test flows working under the upgraded toolchainVendored code
This PR vendors
third_party/safe_html_types, which is the main true vendored code in this branch.Why it is needed:
Why this is safe:
We investigated avoiding vendoring here, but the straightforward
http_archivealternatives we tried did not work for this branch’s current dependency combination.Review-driven cleanup in this PR
In response to review, this PR now also:
safe_html_typesdependency more explicitlyValidation
Validated through the local/container workflow used for this branch, including:
test_pip_packagesmoke validationFollow-up work
The following are better handled in follow-up PRs rather than expanding this one further:
third_party/repo.bzl