-
Notifications
You must be signed in to change notification settings - Fork 3.2k
Specify ceiling versions for dependencies #1435
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds upper-bound version constraints to all Python dependencies in pyproject.toml to prevent new users from installing known-buggy releases (notably rerun-sdk v0.23).
- Introduce
<ceilings for each existing>=dependency. - Preserve exact pins where already declared (e.g., draccus, gymnasium).
Comments suppressed due to low confidence (2)
pyproject.toml:52
- [nitpick] Consider using a version range (e.g., ">=0.10.0,<0.11.0") instead of an exact pin for draccus to allow non-breaking patch updates.
"draccus==0.10.0",
pyproject.toml:56
- [nitpick] Consider relaxing the exact pin for gymnasium to a range (e.g., ">=0.29.1,<0.30.0") to allow non-breaking patch updates.
"gymnasium==0.29.1", # TODO(rcadene, aliberts): Make gym 1.0.0 work
|
@CarolinePascal Is there any chance for merging this? If so, I can rebase and do the same for the most recent changes. I experienced another scenario a few days ago that would've benefited from this as well--pi0 inference is broken on the latest transformers library: #1153 , https://discord.com/channels/1216765309076115607/1343589494485553222/1377399976979599420 |
|
Hi @ben-z , Sorry for the late reply. We actually discussed your PR with @imstevenpmwork but I did not get the time to answer properly. Ideally, we would like upper bounds on versions only two be specified when needed (bugs un revent versions, breaking changes, etc) and not everywhere. In this situation, it would mean only add ceiling version numbers for If you can modify your PR in that sense we would gladly merge it ! Best, Caroline. |
|
@CarolinePascal Thanks for the update! I can edit the PR to upper-bound only the 2 known broken packages. However, there will be similar cases in the future. Do we have a plan to handle those? uv is becoming the go-to package manager for lots of new Python projects. They use a lock file similar to what the nodejs package managers do. It seems that that is a better way to manage dependencies than plain pip. Do we want to switch to uv or another similar package manager in this repo? I think it would prevent a lot of headaches for new users. |
|
Hi @ben-z, Feel free to ping me when the PR is changed so I can proceed to merge it ;) For now, we'd like to promote higher flexibility and let the user update dependencies to their latest version, even though it means we are not immune to breaking changes. We actually also want to promote Best, Caroline. |
|
@CarolinePascal I see that there's a larger refactor of pyproject.toml at #1520 . Let's close this and use that instead! I'll make a suggestion on that PR to specify the ceil version for transformers.
Since the docs already mention this, I think we can simply replace the pip instructions with uv instructions, and leave conda as-is? |
|
Done in #1520 ! |
What this does
Rerun has a memory leak bug in
v0.23. And as discussed in #1404, new users can unknowingly install that version and experience unnecessary troubleshooting headaches. This PR adds ceiling versions to all dependencies, so that these issues don't happen in the future.In the future, we can explicitly upgrade dependencies when needed.
How it was tested
The floor versions haven't changed. The ceiling versions are chosen based on the latest versions of each dependency, except for
rerun. I haven't experienced any issues with these dependencies in my environment. Nonetheless, it's a non-breaking, safe change that strictly makes new user experience better.How to checkout & try? (for the reviewer)
Do the setup steps again (i.e.
pip install -e .) and see that all dependencies showRequirement already satisfied.For new users, they will install the newest version up to the ceiling specified.
cc @CarolinePascal for review since you are involved in #1404 :)