-
Notifications
You must be signed in to change notification settings - Fork 228
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
migrates prototype/mixed_precision to configs #1854
Conversation
🔗 Helpful Links🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/ao/1854
Note: Links to docs will display an error until the docs builds have been completed. This comment was automatically generated by Dr. CI and updates every 15 minutes. |
Summary: Note: had to remove int4/int8 functionality to simplify the refactor. Whoever uses this script next and needs that functionality can add this back. Test Plan: ``` pytest test/prototype/test_mixed_precision.py -s -x ``` Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 2b0f46d458e69e6c16aefe4a8644c44094b584cb ghstack-comment-id: 2706815985 Pull Request resolved: #1854
|
||
|
||
# for bc | ||
intN_weight_only = IntNWeightOnlyConfig |
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.
Imo these prototype folders we should just break BC
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 agree, but it's actually easier to keep BC here (to not change callsites) - if someone wants to change the callsites of all these prototype features, that would sgtm in a separate PR.
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 guess, but also changing callsites within a repro is - I find - not that hard and ultimately helps create a more unified api as we move everything to configs + registrations
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.
ok will do
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.
@drisspg , I take it back. See how the scope of #1851 expanded from removing the bc name. Some of these other prototypes have even more callsites throughout the codebase, some of those callsites aren't easily testable, etc - I just don't think it's worth the effort to tie removing old names to this PR stack since we have to keep BC anyways for some of the APIs.
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.
See comment
Summary:
Note: had to remove int4/int8 functionality to simplify the refactor.
Whoever uses this script next and needs that functionality can add this
back.
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags: