-
Notifications
You must be signed in to change notification settings - Fork 360
generalize torch compatibility check #3042
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
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/3042
Note: Links to docs will display an error until the docs builds have been completed. ✅ No FailuresAs of commit 3fdc4c4 with merge base 1591603 ( This comment was automatically generated by Dr. CI and updates every 15 minutes. |
2f28062 to
d7f042f
Compare
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! Just left a few minor questions
15066eb to
a17ecb7
Compare
c5c4a3e to
39bbab1
Compare
torchao/__init__.py
Outdated
| if _parse_version(torch.__version__) == _parse_version( | ||
| "2.9.0.dev" | ||
| ) and _parse_version(__version__) == _parse_version("0.14.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 is this only torch 2.9.0 nightly and torchao == 0.14.0 instead of something like torch >= 2.8 and torchao >= 0.13.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.
we want to do stricter checks like == instead of >=
if we did torch >= x and torchao >= y we can have false positives if torchao version is y+2 but torch is x+1 when y+2 is built against torch x+2 (so not guaranteed compatible with torch x+1)
we also only need to check the latest torchao version against the latest torch version because this code will never run for a previous torchao version (it is redundant to consider the case when torchao == 0.13.0 because if that were the case, this code would not exist)
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.
if we did torch >= x and torchao >= y we can have false positives if torchao version is y+2 but torch is x+1 when y+2 is built against torch x+2 (so not guaranteed compatible with torch x+1)
I think changing to >= for both would skip all future inclusions of this? e.g. torchao >= 0.14.0.dev and torch >= 2.9.0.dev (this will be satisfied for all current and future torch and torchao versions)
explanation for 0.13.0 and below makes sense
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.
also to clarify, current version for torchao should be 0.14.0.dev I think, not sure if you intended to check 0.14.0 or not
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 think you should spell out all the versions here:
if torch_version == X and torchao_version == Y:
...
elif torch_version == X+1 and torchao_version == Y+1:
...
else:
...this removes all confusion about what is supported or not. It's ok that if torch_version == X and torchao_version == Y will become dead code, IMO more important to make it crystal clear what's going on. IMO having the entire support matrix here is the best way to do that.
If you really don't want dead code, I'd still have a comment very clearly explaining why only the latest thing is here and what the behavior was before.
| # Install compatible version of torch (nightly 2.9.0dev) | ||
| pip install --pre torch --index-url https://download.pytorch.org/whl/nightly/cu129 | ||
| # Installs nightly torchao (0.14.0dev...) | ||
| pip install --pre torchao --index-url https://download.pytorch.org/whl/nightly/cu129 |
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.
should we test more combinations? e.g.
torchao = (0.13, 0.14.dev)
pytorch = (2.8, 2.9.dev, 2.9 (future))
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.
adding more test, but don't need to test previous versions i think (see #3042 (comment), #3042 (comment))
5c47e19 to
500b3f9
Compare
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.
comments inline
ea007fc to
e35ee3f
Compare
e35ee3f to
77f3af6
Compare
.github/scripts/test_torch_version_torchao_version_compatibility.sh
Outdated
Show resolved
Hide resolved
77f3af6 to
1ecd3b6
Compare
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.
thank you for improving this!
5f1a4bd to
3fdc4c4
Compare
Summary
per #2919, torchao .so files built using PyTorch 2.8.0 are not ABI compatible with PyTorch 2.9+. this PR modifies the check added in #2908 for future versions of torchao. we keep the checks for current stable versions for clarity and add a check for most recent versions (torch 2.9.0.dev and torchao 0.14.0dev). this check should be updated with every release to correctly reflect compatibility.
Testing
bash script
./scripts/test_torch_version_torchao_version_compatibility.sh. this script was ran locally to test and should be updated and run before every release/version bump.