-
Notifications
You must be signed in to change notification settings - Fork 320
fix incorrect torch version test #2786
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/2786
Note: Links to docs will display an error until the docs builds have been completed. ❌ 3 New FailuresAs of commit 05edf52 with merge base e6b38bb ( NEW FAILURES - The following jobs have failed:
This comment was automatically generated by Dr. CI and updates every 15 minutes. |
torchao/utils.py
Outdated
|
||
# Parser for local identifiers | ||
current_version = re.sub(r"\+.*$", "", torch.__version__) | ||
return parse_version(current_version) >= parse_version(min_version) |
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 have some impression that we don't want to do this, but @msaroufim would have more context here
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.
seems fine to merge altho we probably want to delete the compare_versions function
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.
compare_versions & parse_version
are unavailable to used here because parse_version
only extracts \d+\.\d+\.\d+
(e.g., 2.5.0→[2, 5, 0]). Therefore, we can inject more parsers (e.g., a0, dev) into parse_version
, but I am not certain because check_cpu_version & check_xpu_version
are chained with them.
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.
btw we also have
Line 1002 in 43b4106
def is_package_at_least(package_name: str, min_version: str): |
I vaguely remember at some point that we don't want to use parse version, but don't remember why though
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.
btw we also have
Line 1002 in 43b4106
def is_package_at_least(package_name: str, min_version: str): , should we just reuse that? not sure if this works for pre-releases as well, could you check?
I vaguely remember at some point that we don't want to use parse version, but don't remember why though
Installation
# 2.8.0 stable
pip install torch --index-url https://download.pytorch.org/whl/cpu
# 2.9.0 pre-release (nighty)
pip install torch --index-url https://download.pytorch.org/whl/nightly/cpu
Test code
test_cases = ["2.8.0a0+git9f17037", "2.8.0", "2.9.0a0+git9f17037", "2.9.0", "2.10.0a0+git9f17037", "2.10.0"]
print(torch.__version__)
print(torch.version)
for test in test_cases:
print(torch_version_at_least(test), is_package_at_least(package_name="torch", min_version=test))
Result
# stable
2.8.0+cpu
<module 'torch.version' from 'ao/.venv/lib/python3.12/site-packages/torch/version.py'>
True False
True True
False False
False False
False True
False True
# pre-release (nighty)
2.9.0.dev20250821+cpu
<module 'torch.version' from 'ao/.venv/lib/python3.12/site-packages/torch/version.py'>
True True
True True
True False
False True
False True
False True
It seems that is_package_at_least()
doesn't work for both stable/pre-release becauseversion()
returns the module object; __version__()
returns the actual version string. In my naive guess, torch_version_at_least
and is_package_at_least
can be consolidated because they share "return true if installed <package/torch> is higher than required" (with PyTorch pre-release detection). How about consolidating them?
Hi @namgyu-youn, thanks for fixing this. I think we should actually fix
Basically always return a list of 3 numbers, and -1 represents a pre-release. Then we can just compare as follows:
To me this is simpler than the existing |
Sounds good to me. Customized |
…versions - Co-authored-by: andrewor14 <[email protected]>
…versions - Co-authored-by: andrewor14 <[email protected]>
torchao/utils.py
Outdated
version = re.sub(r"\+.*$", "", version_string) | ||
|
||
# Check for pre-release indicators (including all common patterns) | ||
is_prerelease = bool(re.search(r"(a|b|dev)", version)) |
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.
Seems like this is simpler:
is_prerelease = bool(re.search(r"(git|dev)", version_string))
match = re.match(r"(\d+)\.(\d+)\.(\d+)", version_string)
any other patterns we need to search for? @jerryzh168
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.
git and dev should be enough I think, we have git locally and dev in nightly
Examples: "2.5.0.dev20240708+cu121" -> [2, 5, -1], "2.5.0" -> [2, 5, 0] | ||
""" | ||
# Check for pre-release indicators | ||
is_prerelease = bool(re.search(r"(git|dev)", version_string)) |
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.
please also verify the version string for pytorch, if you build it from source
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.
git and dev should be enough I think, we have git locally and dev in nightly
Aren't the above comments mentioning git
and dev
enough for a pre-release pattern? Let me know if I am missing something.
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.
Yeah should be git
if you build from source, this is mine: 2.9.0a0+git1f19003
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.
yeah I was referring to torchao as well but then realized this is for torch
Thanks @namgyu-youn, this looks good to me! The test failures are unrelated. |
Summary: version semantics is fixed in pytorch#2786 and we are updating the version check logic accordingly Test Plan: python test/integration/test_integration.py -k test_autoquant_hp_float Reviewers: Subscribers: Tasks: Tags:
Summary: version semantics is fixed in #2786 and we are updating the version check logic accordingly Test Plan: python test/integration/test_integration.py -k test_autoquant_hp_float Reviewers: Subscribers: Tasks: Tags:
Summary:
PyTorch pre-release/dev versions have
a0/dev
in their name. Therefore, the right order is the following:For correct order, this PR applies true order within stable and pre-release/dev.
torch_version_at_least
semantics are incorrect #2722Test plan: CI