-
Notifications
You must be signed in to change notification settings - Fork 2k
Removes protobuf upper version pin #2726
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
Conversation
400af1b
to
93f5eb4
Compare
I'm hitting this error when updating protobuf to the latest, so it seems like there may still be some compatibility issues there?
|
hey @kellyguo11, thanks for taking a look. how are you getting to that error? |
I tried manually upgrading protobuf to the latest on windows and ran any of the training scripts and hit this. seemed to be coming from wandb. |
@kellyguo11 hmm, it looks like
so resolving if you are just doing e.g. |
btw, @kellyguo11 I am investigating the test failure in the is the CI failure for that a known issue? I am not seeing how my change could be related to that - and the second test appears to be flaky (did not fail the first CI run before I rebased). let me know if you can see how or why that might be failing and I'm happy to dig into it. |
Yes agreed when we install as a transitive step, we do not end up pulling the latest protobuf, but that means even if we extend the protobuf version support in lab configs, we will not be able to support the latest protobuf out of the box, so I'm not sure if it makes sense for us to relax the version in the lab dependencies. For the unit test, you can also try running it locally with |
@kellyguo11 I'm not sure I follow when you say "we will not be able to support the latest protobuf out of the box". what dependency does IsaacLab itself have on protobuf that causes it to fail with the latest protobuf? and how does it specifically fail? I'm not actually seeing the protobuf dep being used in the project sources (maybe I'm overlooking something) - but it sounded from your message like this was scoped to 3rd party dependencies (presumably for tensorboard). if that is the case, those 3rd party deps (e.g. tensorboard or wandb) should be the thing specifying the upper bounds for protobuf transitively in order to get the correct behavior - and it appears to already do that. if IsaacLab itself doesn't directly use protobuf, it should ultimately omit that from it's requirements and let the transitive deps manage that - or at a minimum, unpin the upper range to be more compatible with modern python environments.
doesn't appear to be:
I see the same failure on other PRs, so I'm pretty sure it's unrelated to my change. |
6f4a37d
to
d6e11fe
Compare
Lab shouldn't have a direct dependency on protobuf, but rather the dependencies of lab prevents it from being compatible with the latest version. It looks like it's still able to pull in an older version that's compatible with wandb with these changes, so it shouldn't cause any issues merging this. I'm just not convinced that these changes alone will allow someone to run isaac lab out of the box with the latest version of protobuf. |
gotcha. well we are working on doing exactly this, so I'll report back or circle back with more PRs if we hit issues. thank you! btw, do you know when the next release is planned? |
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
Checklist
pre-commit
checks with./isaaclab.sh --format
config/extension.toml
fileCONTRIBUTORS.md
or my name already exists there