-
Notifications
You must be signed in to change notification settings - Fork 7.2k
Use torch.ao.quantization instead of torch.quantizaiton #4554
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
@z-a-f , are the functions / classes in |
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.
The changes look good to me but as Nicolas said, I think it's worth confirming that our references still work the way we expect them to work prior merging.
Edit: It seems there are some related failures. Often when new features are introduced, we need to wait until the latest nightly becomes available. Not sure if that's the case here as well.
is there any expectation that torchvision needs to work with arbitrary versions of pytorch (not just the latest version)? If yes, we will need a version check. If not, this is good. |
We expect the torchvision main branch to work with pytorch nightly, we don't need it to be compatible with previous (or even currently stable) versions |
The AO only changed the package name. The functionality is expected to be the same. |
This should be fine -- basic mapping of the AO migration is as follows:
Otherwise, the logic should stay the same. One note: the migration is currently an ongoing process and will not be part of the binary up until 1.11+ (if building from source, 1.11.dev10+). That means that if you have explicit dependency on earlier versions of pytorch, you will get errors. |
The reason https://app.circleci.com/pipelines/github/pytorch/vision/11088/workflows/c3d06578-6137-4d13-8f38-7342e02c2148/jobs/853515 is failing is because there is a dependency on 1.11.dev09+. |
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.
Approving given @z-a-f's comment that only the package changed location and everything else remains the same. Because we don't have great coverage on the reference scripts, I recommend testing the train and eval commands prior merging.
I tried 2 runs of
and got
On main I got
which is concistent with the current docs. So with @z-a-f 's confirmation that the changes are mostly just a renaming, I think the changes are fine. I'll merge once |
I tried to investigate why this is failing. It seems that cmake on Windows with GPU installs a very old nightly of PyTorch ( It's worth noting that the Windows Cmake with CPU picks up today's nightly ( Upon further investigation, it seems that the last available release of win-64 with CUDA 10.2 happened on 20210929. Since Cmake builds using CUDA 10.2, we are having issues. |
The issues should now hopefully be resolved after #4945 |
…4554) Summary: Co-authored-by: Vasilis Vryniotis <datumbox@users.noreply.github.com> Reviewed By: NicolasHug Differential Revision: D32759204 fbshipit-source-id: bdd11442acb0a9d8184d68b6392ae8b6fda00745
Same as #4551 but without black conflicts
I just did some brutal
sed -i "s/.../..."
so I might have done something wrong. @z-a-f please double-check that these are the intended changes :)