Improve Bazel build handling of protobuf dependency situation #6185
Comments
nfelt
added a commit
that referenced
this issue
Feb 9, 2023
This PR changes how we depend on the Python protobuf runtime library when running under Bazel. Now, we follow the same approach we use for depending on TensorFlow, numpy, etc and expect the package to be importable from the environment, non-hermetically. We introduce an `:expect_protobuf_installed` dummy target to facilitate syncing to our internal repository. (Note: this PR has no effect on the actual generated pip package, which has always imported `protobuf` from the environment in this way.) Previously, when running under Bazel (e.g. `bazel test` or `bazel run`), the Python protobuf runtime was provided hermetically as part of the build via the target `@com_google_protobuf//:protobuf_python`. This target was used in two different ways: - Our generated Python protobuf code (produced by `protoc` via compilation of the .proto files) automatically includes this target in the `deps` attribute of the resulting `py_library()`, because it uses our `tb_proto_library()` Starlark macro which encapsulated this behavior - A smaller number of normal Python targets explicitly depend on this target, because they [`import google.protobuf` directly](https://cs.opensource.google/search?ss=tensorflow%2Ftensorboard&q=import.*protobuf%7Cprotobuf.*import%20lang:py) (typical use cases would be to use a well-known common proto like `timestamp` or a utility like `text_format`) The problem with the hermetic approach is that it forces our Python protobuf runtime to match our `protoc` version, since they come from the same `@com_google_protobuf` repository. That poses an issue because we don't want to advance our `protoc` version past 3.19.6 for the reasons described in [this comment](https://github.com/tensorflow/tensorboard/blob/22becc02379375785c1ad3b61f712050843aea5c/WORKSPACE#L117-L131), but we depend on TensorFlow which has [recently increased its minimum protobuf runtime to 3.20.3](tensorflow/tensorflow@84f4092#diff-f526feeafa1000c4773410bdc5417c4022cb2c7b686ae658b629beb541ae9112). As a result, trying to run our Bazel tests against TF at HEAD causes protobuf compatibility failures for the TF generated protos due to an insufficient runtime version. This PR resolves this skew by allowing the protobuf runtime to "float" to whatever version the pip requirements dictate, at the cost of increased non-hermeticity. We might ultimately be able to resolve this hermetically - for all our `expect_<name>_installed` deps - by depending on newer versions of protobuf Starlark rules and/or migrating our pip-specified dependencies to use rules_python. I've filed #6185 as a tracking issue for trying to improve this situation longer-term.
yatbear
pushed a commit
to yatbear/tensorboard
that referenced
this issue
Mar 27, 2023
…low#6186) This PR changes how we depend on the Python protobuf runtime library when running under Bazel. Now, we follow the same approach we use for depending on TensorFlow, numpy, etc and expect the package to be importable from the environment, non-hermetically. We introduce an `:expect_protobuf_installed` dummy target to facilitate syncing to our internal repository. (Note: this PR has no effect on the actual generated pip package, which has always imported `protobuf` from the environment in this way.) Previously, when running under Bazel (e.g. `bazel test` or `bazel run`), the Python protobuf runtime was provided hermetically as part of the build via the target `@com_google_protobuf//:protobuf_python`. This target was used in two different ways: - Our generated Python protobuf code (produced by `protoc` via compilation of the .proto files) automatically includes this target in the `deps` attribute of the resulting `py_library()`, because it uses our `tb_proto_library()` Starlark macro which encapsulated this behavior - A smaller number of normal Python targets explicitly depend on this target, because they [`import google.protobuf` directly](https://cs.opensource.google/search?ss=tensorflow%2Ftensorboard&q=import.*protobuf%7Cprotobuf.*import%20lang:py) (typical use cases would be to use a well-known common proto like `timestamp` or a utility like `text_format`) The problem with the hermetic approach is that it forces our Python protobuf runtime to match our `protoc` version, since they come from the same `@com_google_protobuf` repository. That poses an issue because we don't want to advance our `protoc` version past 3.19.6 for the reasons described in [this comment](https://github.com/tensorflow/tensorboard/blob/22becc02379375785c1ad3b61f712050843aea5c/WORKSPACE#L117-L131), but we depend on TensorFlow which has [recently increased its minimum protobuf runtime to 3.20.3](tensorflow/tensorflow@84f4092#diff-f526feeafa1000c4773410bdc5417c4022cb2c7b686ae658b629beb541ae9112). As a result, trying to run our Bazel tests against TF at HEAD causes protobuf compatibility failures for the TF generated protos due to an insufficient runtime version. This PR resolves this skew by allowing the protobuf runtime to "float" to whatever version the pip requirements dictate, at the cost of increased non-hermeticity. We might ultimately be able to resolve this hermetically - for all our `expect_<name>_installed` deps - by depending on newer versions of protobuf Starlark rules and/or migrating our pip-specified dependencies to use rules_python. I've filed tensorflow#6185 as a tracking issue for trying to improve this situation longer-term.
dna2github
pushed a commit
to dna2fork/tensorboard
that referenced
this issue
May 1, 2023
…low#6186) This PR changes how we depend on the Python protobuf runtime library when running under Bazel. Now, we follow the same approach we use for depending on TensorFlow, numpy, etc and expect the package to be importable from the environment, non-hermetically. We introduce an `:expect_protobuf_installed` dummy target to facilitate syncing to our internal repository. (Note: this PR has no effect on the actual generated pip package, which has always imported `protobuf` from the environment in this way.) Previously, when running under Bazel (e.g. `bazel test` or `bazel run`), the Python protobuf runtime was provided hermetically as part of the build via the target `@com_google_protobuf//:protobuf_python`. This target was used in two different ways: - Our generated Python protobuf code (produced by `protoc` via compilation of the .proto files) automatically includes this target in the `deps` attribute of the resulting `py_library()`, because it uses our `tb_proto_library()` Starlark macro which encapsulated this behavior - A smaller number of normal Python targets explicitly depend on this target, because they [`import google.protobuf` directly](https://cs.opensource.google/search?ss=tensorflow%2Ftensorboard&q=import.*protobuf%7Cprotobuf.*import%20lang:py) (typical use cases would be to use a well-known common proto like `timestamp` or a utility like `text_format`) The problem with the hermetic approach is that it forces our Python protobuf runtime to match our `protoc` version, since they come from the same `@com_google_protobuf` repository. That poses an issue because we don't want to advance our `protoc` version past 3.19.6 for the reasons described in [this comment](https://github.com/tensorflow/tensorboard/blob/22becc02379375785c1ad3b61f712050843aea5c/WORKSPACE#L117-L131), but we depend on TensorFlow which has [recently increased its minimum protobuf runtime to 3.20.3](tensorflow/tensorflow@84f4092#diff-f526feeafa1000c4773410bdc5417c4022cb2c7b686ae658b629beb541ae9112). As a result, trying to run our Bazel tests against TF at HEAD causes protobuf compatibility failures for the TF generated protos due to an insufficient runtime version. This PR resolves this skew by allowing the protobuf runtime to "float" to whatever version the pip requirements dictate, at the cost of increased non-hermeticity. We might ultimately be able to resolve this hermetically - for all our `expect_<name>_installed` deps - by depending on newer versions of protobuf Starlark rules and/or migrating our pip-specified dependencies to use rules_python. I've filed tensorflow#6185 as a tracking issue for trying to improve this situation longer-term.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
This is a tracking issue for trying to improve how we work with protobuf in our Bazel build. Currently, it's complex and challenging to understand. Some particular problems we have:
PATHchanges, a longstanding issue: Bazel build recompiles protoc every time PATH changes #3213Also, we recently managed, at last, to update our protoc dep to 3.19.6 (#6147 resolving #5708, which was branched out of the user-facing issue #5703). Eventually, it would be good to drop support for the 3.x protobuf python runtime and migrate completely to 4.21+ (which for protoc is just called 21+, due to a confusing versioning system that is only lightly described in https://protobuf.dev/news/2022-08-03/).
To resolve this, we should talk to the protobuf and Bazel folks to try to understand how Bazel + google-internal projects are expected to manage this. It sounds like there might be some changes in this area that could lead to a clearer set of standard practices we can follow.
The text was updated successfully, but these errors were encountered: