[go: nahoru, domu]

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

build: update protobuf to 3.19.6 and grpc to 1.48.2 #6147

Merged
merged 3 commits into from
Jan 19, 2023

Conversation

nfelt
Copy link
Collaborator
@nfelt nfelt commented Jan 14, 2023

Fixes #5708 and #5703.

This updates our protobuf dependency to 3.19.6 in an attempt to address #5708, and provide a cleaner solution to #5703.

The choice of 3.19.6 is meant to satisfy two competing constraints:

  • Current Python protobuf runtimes (the 4.x series) only support generated code from protoc versions 3.19.0+, as discussed in https://protobuf.dev/news/2022-05-06/. As a result, prior to this change, TensorBoard's pip package had to force its pip package dependency to protobuf < 4 to avoid the errors seen in TensorBoard's generated pb2.py files are incompatible with protobuf 4.21.0 #5703. This PR lifts that restriction.

  • Current TensorFlow is still stuck on protobuf 3.x, the same as we have been, and as a result pins its pip package dependency using protobuf < 3.20 (this could presumably be relaxed to < 4 but that would require new TF releases). As a result, we must support at least one protobuf runtime version that also works with TF's constraints.

Our previous attempt at this upgrade (to ~3.18 or so) caused test failures for Keras (which depends on TB, via TF, for the summary API code), apparently due to a protobuf runtime that was too old for our generated code. At the time, this was puzzling because they were pip-installing a protobuf runtime version that should have been recent enough - but I suspect now that this was a red herring, and bazel test was actually getting the protobuf runtime from the protobuf build dependency, not from the installed Python packages. If we see this failure mode again, we'll have to get Keras to update the protobuf Python runtime available in bazel tests.

Lastly, this upgrade lets us clean up some additional issues we had to work around:

@nfelt nfelt marked this pull request as ready for review January 19, 2023 22:25
@nfelt nfelt requested a review from yatbear January 19, 2023 22:25
@nfelt
Copy link
Collaborator Author
nfelt commented Jan 19, 2023

cc @vam-google FYI

Copy link
Member
@yatbear yatbear left a comment

Choose a reason for hiding this comment

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

Awesome! Thanks!

@nfelt nfelt merged commit 78f97c2 into tensorflow:master Jan 19, 2023
@nfelt nfelt deleted the proto-upgrade branch January 19, 2023 23:56
nfelt added a commit that referenced this pull request Feb 8, 2023
We inadvertently resolved #6115 via #6147, so we no longer need to pin
to a maximum Bazel version of 5.4.0.

That said, I think it's still a good idea to retain a maximum compatible
version, since Bazel major version upgrades have reliably resulted in
our build breaking in the past. That way, when Bazel 7.0.0 arrives,
we'll get a clean error message, and if we then try to remove the pin,
we'll either be pleasantly surprised that it's fine, or we'll be
expecting a build failure and know the cause (rather than encountering a
build failure during a routine build, and then wasting time debugging
before realizing Bazel was silently upgraded).

Putting this in place now also ensures this protection extends to older
commits, rather than doing it reactively, which then relies on people
syncing past that commit before trying to build.

Tested via `USE_BAZEL_VERSION=6.0.0 bazel run tensorboard` (w/ bazel set
to bazelisk) and confirmed it builds successfully and runs apparently
normally.
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
Fixes tensorflow#5708 and tensorflow#5703.

This updates our protobuf dependency to 3.19.6 in an attempt to address
tensorflow#5708, and provide a cleaner solution to tensorflow#5703.

The choice of 3.19.6 is meant to satisfy two competing constraints:

- Current Python protobuf runtimes (the 4.x series) only support
generated code from protoc versions 3.19.0+, as discussed in
https://protobuf.dev/news/2022-05-06/. As a result, prior to this
change, TensorBoard's pip package had to force its pip package
dependency to `protobuf < 4` to avoid the errors seen in tensorflow#5703. This PR
lifts that restriction.

- Current TensorFlow is still stuck on protobuf 3.x, the same as we have
been, and as a result pins its pip package dependency using `protobuf <
3.20` (this could presumably be relaxed to `< 4` but that would require
new TF releases). As a result, we must support at least one protobuf
runtime version that also works with TF's constraints.

Our previous attempt at this upgrade (to ~3.18 or so) caused test
failures for Keras (which depends on TB, via TF, for the summary API
code), apparently due to a protobuf runtime that was too old for our
generated code. At the time, this was puzzling because they were
pip-installing a protobuf runtime version that should have been recent
enough - but I suspect now that this was a red herring, and bazel test
was actually getting the protobuf runtime from the protobuf build
dependency, not from the installed Python packages. If we see this
failure mode again, we'll have to get Keras to update the protobuf
Python runtime available in bazel tests.

Lastly, this upgrade lets us clean up some additional issues we had to
work around:

- We can also upgrade gRPC now, to 1.48.2. I selected this version since
it appears to be the most recent version prior to gRPC adopting protobuf
4.x (see
grpc/grpc@41ec08c)
- We can revert the backported fixes to protobuf and grpc from
tensorflow#5793, since the upgraded
dependencies don't require patching
- We can back out rules_apple reintroduction from
tensorflow#5561 that we only needed
for gRPC
yatbear pushed a commit to yatbear/tensorboard that referenced this pull request Mar 27, 2023
…flow#6183)

We inadvertently resolved tensorflow#6115 via tensorflow#6147, so we no longer need to pin
to a maximum Bazel version of 5.4.0.

That said, I think it's still a good idea to retain a maximum compatible
version, since Bazel major version upgrades have reliably resulted in
our build breaking in the past. That way, when Bazel 7.0.0 arrives,
we'll get a clean error message, and if we then try to remove the pin,
we'll either be pleasantly surprised that it's fine, or we'll be
expecting a build failure and know the cause (rather than encountering a
build failure during a routine build, and then wasting time debugging
before realizing Bazel was silently upgraded).

Putting this in place now also ensures this protection extends to older
commits, rather than doing it reactively, which then relies on people
syncing past that commit before trying to build.

Tested via `USE_BAZEL_VERSION=6.0.0 bazel run tensorboard` (w/ bazel set
to bazelisk) and confirmed it builds successfully and runs apparently
normally.
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
Fixes tensorflow#5708 and tensorflow#5703.

This updates our protobuf dependency to 3.19.6 in an attempt to address
tensorflow#5708, and provide a cleaner solution to tensorflow#5703.

The choice of 3.19.6 is meant to satisfy two competing constraints:

- Current Python protobuf runtimes (the 4.x series) only support
generated code from protoc versions 3.19.0+, as discussed in
https://protobuf.dev/news/2022-05-06/. As a result, prior to this
change, TensorBoard's pip package had to force its pip package
dependency to `protobuf < 4` to avoid the errors seen in tensorflow#5703. This PR
lifts that restriction.

- Current TensorFlow is still stuck on protobuf 3.x, the same as we have
been, and as a result pins its pip package dependency using `protobuf <
3.20` (this could presumably be relaxed to `< 4` but that would require
new TF releases). As a result, we must support at least one protobuf
runtime version that also works with TF's constraints.

Our previous attempt at this upgrade (to ~3.18 or so) caused test
failures for Keras (which depends on TB, via TF, for the summary API
code), apparently due to a protobuf runtime that was too old for our
generated code. At the time, this was puzzling because they were
pip-installing a protobuf runtime version that should have been recent
enough - but I suspect now that this was a red herring, and bazel test
was actually getting the protobuf runtime from the protobuf build
dependency, not from the installed Python packages. If we see this
failure mode again, we'll have to get Keras to update the protobuf
Python runtime available in bazel tests.

Lastly, this upgrade lets us clean up some additional issues we had to
work around:

- We can also upgrade gRPC now, to 1.48.2. I selected this version since
it appears to be the most recent version prior to gRPC adopting protobuf
4.x (see
grpc/grpc@41ec08c)
- We can revert the backported fixes to protobuf and grpc from
tensorflow#5793, since the upgraded
dependencies don't require patching
- We can back out rules_apple reintroduction from
tensorflow#5561 that we only needed
for gRPC
dna2github pushed a commit to dna2fork/tensorboard that referenced this pull request May 1, 2023
…flow#6183)

We inadvertently resolved tensorflow#6115 via tensorflow#6147, so we no longer need to pin
to a maximum Bazel version of 5.4.0.

That said, I think it's still a good idea to retain a maximum compatible
version, since Bazel major version upgrades have reliably resulted in
our build breaking in the past. That way, when Bazel 7.0.0 arrives,
we'll get a clean error message, and if we then try to remove the pin,
we'll either be pleasantly surprised that it's fine, or we'll be
expecting a build failure and know the cause (rather than encountering a
build failure during a routine build, and then wasting time debugging
before realizing Bazel was silently upgraded).

Putting this in place now also ensures this protection extends to older
commits, rather than doing it reactively, which then relies on people
syncing past that commit before trying to build.

Tested via `USE_BAZEL_VERSION=6.0.0 bazel run tensorboard` (w/ bazel set
to bazelisk) and confirmed it builds successfully and runs apparently
normally.
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.

tracking issue for upgrading protobuf to 3.19.0+ (and gRPC as well)
2 participants