Relax protobuf<5.0.0 restriction
#6888
Conversation
groszewn
force-pushed
the
protobuf5
branch
3 times, most recently
from
de0d9c9 to
39460eb
Compare
| experiment_id, | ||
| request_proto, | ||
| ).run() | ||
| current_protobuf_version = importlib_version("protobuf") |
There was a problem hiding this comment.
Consider adding a util that we can use as a one-line call in all of these places, and abstract this detail away.
It's annoying that we'd need a separate file and all the overhead of the build file and all, but I think it's a more maintainable solution and results in cleaner code.
There was a problem hiding this comment.
There's already a plugin_util module, so I added it there (and added some tests).
There was a problem hiding this comment.
Sorry for not being clear. I was thinking more like a "proto_to_json" method, so the package checking is abstracted away from the code that just needs to convert the proto to a string, and the code elsewhere can be just a one-line method call. Sorry for the churn.
There was a problem hiding this comment.
I've updated to just have a proto_to_json method, but I've only used it for these two specific instances that use a deprecated field conditionally. There are probably other calls to MessageToJson throughout the codebase that don't need the conditional handling, but avoiding touching those as part of this PR.
| # NOTE: this version must be >= the protoc version in our WORKSPACE file. | ||
| # At the same time, any constraints we specify here must allow at least some | ||
| # version to be installed that is also compatible with TensorFlow's constraints: | ||
| # https://github.com/tensorflow/tensorflow/blob/25adc4fccb4b0bb5a933eba1d246380e7b87d7f7/tensorflow/tools/pip_package/setup.py#L101 | ||
| # 4.24.0 had an issue that broke our tests, so we should avoid that release: | ||
| # https://github.com/protocolbuffers/protobuf/issues/13485 | ||
| # 5.26.0 introduced a breaking change, so we restricted it for now. See issue #6808 for details. | ||
| protobuf >= 3.19.6, != 4.24.0, < 5.0.0 | ||
| protobuf >= 3.19.6, != 4.24.0 |
There was a problem hiding this comment.
Do you think we should add a TODO or a comment somewhere, to remove the packaging dependency and restrict this to >= 5.0.0 after it's been available for a while or something?
I worry that if at some point we remove this logic to check for protobuf version, we won't realize that we also could remove the packaging dependency, as it was only used there. Maybe there is some tooling that would let us know this... maybe we can add a small comment right next to that package. WDYT?
There was a problem hiding this comment.
Good call, added a comment specifying that if we bump protobuf >= 5.0.0, we can also remove the packaging dependency.
groszewn
force-pushed
the
protobuf5
branch
4 times, most recently
from
de4c2b1 to
5245f9b
Compare
Instead of restricting `protobuf<5.0.0` due to the removal to `including_default_value_fields` kwarg in `json_format.MessageToJson` (replaced by `always_print_fields_with_nopresence`), we can instead detect the currently installed protobuf version and use the appropriate kwarg. Contributes to tensorflow#6808 and should resolve tensorflow#6887.
Instead of restricting `protobuf<5.0.0` due to the removal to `including_default_value_fields` kwarg in `json_format.MessageToJson` (replaced by `always_print_fields_with_nopresence`), we can instead detect the currently installed protobuf version and use the appropriate kwarg. Contributes to tensorflow#6808 and should resolve tensorflow#6887.
Instead of restricting `protobuf<5.0.0` due to the removal to `including_default_value_fields` kwarg in `json_format.MessageToJson` (replaced by `always_print_fields_with_nopresence`), we can instead detect the currently installed protobuf version and use the appropriate kwarg. Contributes to #6808 and should resolve #6887.
# Description This PR relaxes the upper range of allowed protobuf versions so that IsaacLab can be installed in a modern python environment without downgrading or conflicting with modern protobuf versions. The original reason for pinning this to <5 was apparently due to transitive breakage in tensorboard, which also had this pinned to <5 - so pinning this in IsaacLab itself would not be necessary if both deps were composed together. Tensorboard has since (in Aug 2024) unpinned this here: tensorflow/tensorboard#6888 So, the original concern should, afaict, be obviated now. # Fixes This should repair any case where someone wants to install IsaacLab into a modern python environment that uses any of the Google ecosystem (gRPC/protobuf et al) without conflict or forced down-rev'ing to older versions (current version of protobuf is 6.31.1). ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
# Description This PR relaxes the upper range of allowed protobuf versions so that IsaacLab can be installed in a modern python environment without downgrading or conflicting with modern protobuf versions. The original reason for pinning this to <5 was apparently due to transitive breakage in tensorboard, which also had this pinned to <5 - so pinning this in IsaacLab itself would not be necessary if both deps were composed together. Tensorboard has since (in Aug 2024) unpinned this here: tensorflow/tensorboard#6888 So, the original concern should, afaict, be obviated now. # Fixes This should repair any case where someone wants to install IsaacLab into a modern python environment that uses any of the Google ecosystem (gRPC/protobuf et al) without conflict or forced down-rev'ing to older versions (current version of protobuf is 6.31.1). ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
Instead of restricting
protobuf<5.0.0due to the removal toincluding_default_value_fieldskwarg injson_format.MessageToJson(replaced byalways_print_fields_with_nopresence), we can instead detect the currently installed protobuf version and use the appropriate kwarg.Contributes to #6808 and should resolve #6887.