Skip to content
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

Make version check equal to or more than instead of more than #485

Closed
wants to merge 11 commits into from

Conversation

msaroufim
Copy link
Member

@msaroufim msaroufim commented Jul 8, 2024

EDIT: This PR is causing a bunch of IMA issues on PyTorch 2.3 which is unexpected

This PR solves 2 issues

  1. Our current version guard checks were incorrect TORCH_VERSION_AFTER_2_3(2.3) would return false for 2.3 and that's because we intended to say (see below for more detail) and adding .dev was obscuring the actual intent
  2. The second issue is the name we were using was confusing so renamed TORCH_VERSION_AFTER_X_X to TORCH_VERSION_AT_LEAST_X_X,
import torch
torch.__version__ # prints 2.3.0
from torchao.utils import torch_version_at_least
torch_version_at_least("2.3.0.dev) # was returning false for 2.3.0 but True for 2.3.1

But after changes in this PR

torch_version_at_least("2.3.0.dev") # True
torch_version_at_least("2.3.0") # True

Also tested with 3.5

Python 3.10.14 (main, May  6 2024, 19:42:50) [GCC 11.2.0] on linux
Type "help", "copyright", "credits" or "license" for more information.
>>> from torchao.utils import torch_version_at_least
>>> from torchao.utils import torch_version_at_least
KeyboardInterrupt
>>> torch_version_at_least("2.5.0.dev")
True
>>> torch_version_at_least("2.5.0")
True
>>> import torch
>>> torch.__version__
'2.5.0.dev20240708+cu121'
>>> 
>>> from torchao.utils import TORCH_VERSION_AFTER_2_5
>>> TORCH_VERSION_AFTER_2_5

```python
import torch
torch.__version__ # prints 2.3.0
from torchao.utils import torch_version_at_least
torch_version_at_least("2.3.0.dev) # was returning false for 2.3.0
```

But after changes in this PR

```python
torch_version_at_least("2.3.0.dev) # True
torch_version_at_least("2.3.0") # True
```
Copy link

pytorch-bot bot commented Jul 8, 2024

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/485

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 New Failures

As of commit fbb61fc with merge base ab4ec43 (image):

NEW FAILURES - The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jul 8, 2024
@msaroufim msaroufim requested a review from jerryzh168 July 8, 2024 17:22
torchao/utils.py Outdated Show resolved Hide resolved
@msaroufim msaroufim changed the title Fix incorrect version check logic Make version check equal to or more than instead of more than Jul 8, 2024
Copy link
Contributor

@jerryzh168 jerryzh168 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thanks!

@msaroufim
Copy link
Member Author

msaroufim commented Jul 9, 2024

Looks like there's some weird caching issue where Github is running older code and failing

EDIT: @kit1980 figured out I just needed to rebase

@gau-nernst
Copy link
Collaborator

Found some issues with the current TORCH_VERSION_AFTER_X_X. In CI

  • torch 2.2.2: TORCH_VERSION_AFTER_2_2 return True
  • torch 2.3: TORCH_VERSION_AFTER_2_3 return False

So I think changing it to TORCH_VERSION_AT_LEAST_X_X is the way to go.

The failing test in this PR is probably because people expect the old behavior? To make CI pass, probably some places you need to replace TORCH_VERSION_AFTER_2_2 with TORCH_VERSION_AT_LEAST_2_3 for example.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants