-
Notifications
You must be signed in to change notification settings - Fork 73
Pin torchao==0.12.0 to avoid PyTorch ABI warnings, also pin numpydoc>=1.6.0 and ty==0.0.1a21 for compatibility.
#417
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
base: main
Are you sure you want to change the base?
Conversation
|
This PR has been inactive for 10 days and is now marked as stale. |
|
Not stale. |
…pruna into pin-torchao-version
|
I've pinned the sphinx version to avoid a conflict with jinja. Currently, the janus test is failing since the version supporting it is not being installed while testing. Can I add a step like Secondly, the linting with ty seems to be failing due to updates in the newer version of ty, could we pin this package as well. WDYT? |
|
Could you please review? |
davidberenstein1957
left a comment
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.
@ParagEkbote, could you rebase on main_ Also, it seems one of the tests is failing because the Janus architecture is not being recognised? Perhaps we should see if this is caused by wrong transformer versions.
pyproject.toml
Outdated
| "hqq==0.2.7.post1", | ||
| "torchao", | ||
| "torchao==0.12.0", | ||
| "Sphinx>=4.5,<7.0", |
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.
why are we doing this?
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.
We need to pin the sphinx error to prevent a version conflict with jinja. The GH Action logs for the same:https://github.com/PrunaAI/pruna/actions/runs/18781274712/job/53587851292
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.
Furthermore, I have also pinned the ty version and transformers version, so the test and linting checks are passing. Could you please review?
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.
Sphinx is only a dev dependency so we shouldn't pin it here. I don't understand why we're pinning it in the first place and the logs are not available anymore (sorry for the delay), could you re-explain / re-run the logs please?
davidberenstein1957
left a comment
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.
hi @ParagEkbote sorry for the outcome of this PR. I feel that the resctrictions w.r.t. the versioning don't outweigh the benefits of skipping the warning message in this case. WDYT @johannaSommer ?
Can we consider updating the torch version so that a newer version of torchao can be used? |
Hey @ParagEkbote @davidberenstein1957 thanks for the progress so far! Only catching up now but definitely yes, we can upgrade the torch version and then we should be able to support also newer versions of torchao. Infact @gsprochette is currently working on this torch update, maybe he can give a quick heads up as to when this will be merged. |
torchao version to avoid warning of PyTorch ABItorchao version to avoid warning of PyTorch ABI, update transformers version and pin some deps
|
This PR has been inactive for 10 days and is now marked as stale. |
|
Not Stale. |
gsprochette
left a comment
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.
Thanks a lot for fixing the torchao version. I think all the pinned dependencies requires a justification. We should try not to restrict the environment too much, and especially for transformers, >=4.55.0 seems restrictive.
pyproject.toml
Outdated
| "coverage", | ||
| "docutils", | ||
| "ty", | ||
| "ty==0.0.1a20", |
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.
Is there a reason for pinning the specific version of ty?
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.
Since ty is a pre-release package, the number of rules and their enforcement levels vary. The pinned version helps to pass the majority of the linting checks in line with the configuration defined in pyproject.toml .
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.
Makes sense, can we pin it to 0.0.1a21 then? This is the version already in the current uv.lock
pyproject.toml
Outdated
| "torchmetrics[image]==1.7.4", | ||
| "requests>=2.31.0", | ||
| "transformers", | ||
| "transformers>=4.55.0", |
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.
why are we restraining the transformer version this much?
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.
As seen in this pipdeptree output, older versions of gliner had a minimum requirement of transformers>=4.51.0 , but newer versions support a higher version, should I update it and omit transformers v5 for now?
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.
I understand that pruna is compatible with transformers>=4.55.0 but here this is not the side that is restrained, you're declaring it's not compatible with transformers<4.55.0 and I don't understand why.
In the issue you linked it sounds like you want to use more recent model architecture, but this is a dependency external to pruna, right? I don't think this should justify restraining the compatibility of pruna with other packages.
pyproject.toml
Outdated
| "hqq==0.2.7.post1", | ||
| "torchao", | ||
| "torchao==0.12.0", | ||
| "Sphinx>=4.5,<7.0", |
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.
Sphinx is only a dev dependency so we shouldn't pin it here. I don't understand why we're pinning it in the first place and the logs are not available anymore (sorry for the delay), could you re-explain / re-run the logs please?
|
The logs for tests if we don't pin Sphinx are as follows: Could we pin sphinx as a dev dependency instead? WDYT? cc: @gsprochette |
gsprochette
left a comment
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.
For the Sphinx and Jinja2 issue, I would be in favor of pinning numpydoc>=1.6.0 which seems to be the first version with a pyproject.toml, the CI should already be using something above that in the uv.lock and numpydoc>=1.6.0 relies on Sphinx>=5. If you can leave a quick comment stating that this is fixing dependencies in numpydoc-validation that would be great :) Thanks a lot!
pyproject.toml
Outdated
| "torchmetrics[image]==1.7.4", | ||
| "requests>=2.31.0", | ||
| "transformers", | ||
| "transformers>=4.55.0", |
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.
I understand that pruna is compatible with transformers>=4.55.0 but here this is not the side that is restrained, you're declaring it's not compatible with transformers<4.55.0 and I don't understand why.
In the issue you linked it sounds like you want to use more recent model architecture, but this is a dependency external to pruna, right? I don't think this should justify restraining the compatibility of pruna with other packages.
pyproject.toml
Outdated
| "coverage", | ||
| "docutils", | ||
| "ty", | ||
| "ty==0.0.1a20", |
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.
Makes sense, can we pin it to 0.0.1a21 then? This is the version already in the current uv.lock
Pinning the numpydoc version has fixed the jinja version conflict and CI, thanks for getting back to me quickly 👍 cc: @gsprochette |
Based on the transformers >= 4.57.3, <5.0
WDYT? cc: @gsprochette |
|
I think you only see this requirement of transformer>=4.57.3 because gliner resolved to 0.2.24 on your side, but lower versions are compatible with the requirements of pruna. Running We should not constraint the transformers version unless the code in pruna is incompatible with these anterior versions, so I'm in favor of not touching the transformers version in this PR. |
Do we need to guard against a future transformers v5 for breaking changes or not touch that as well? cc: @gsprochette |
|
I don't think it's necessary for now to guard against the future transformers versions, we'll restrict compatibility if necessary when the version comes out |
torchao version to avoid warning of PyTorch ABI, update transformers version and pin some depstorchao==0.12.0 to avoid PyTorch ABI warnings, also pin numpydoc>=1.6.0 and ty==0.0.1a21 for compatibility.
|
I have unpinned transformers as suggested. Could you please review? cc: @gsprochette |
gsprochette
left a comment
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.
Looks good to me, thanks for your work :) @johannaSommer it would be cool to merge this soon to stabilize the CI
Description
As described in the issue, this PR pins the torchao version to 0.12.0. If we want to support a version higher, we will need to bump the torch version as well. After pinning to this version, the warning does not appear. Could you please review?
cc: @davidberenstein1957
Related Issue
Fixes #411
Type of Change
How Has This Been Tested?
Checklist
Additional Notes
Note
Pins
torchaoto version0.12.0to avoid the PyTorch ABI warning.Written by Cursor Bugbot for commit 474202c. This will update automatically on new commits. Configure here.