Skip to content

migrate prototype/awq to configs #1853

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

Merged
merged 16 commits into from
Mar 8, 2025
Merged

migrate prototype/awq to configs #1853

merged 16 commits into from
Mar 8, 2025

Conversation

vkuzo
Copy link
Contributor

@vkuzo vkuzo commented Mar 7, 2025

Summary:

As titled

Test Plan:

// note: this fails on weights only load, but the failure happens
// after my changes, and already exists on main branch
pytest test/prototype/test_awq.py -s -x

Reviewers:

Subscribers:

Tasks:

Tags:

vkuzo added 3 commits March 7, 2025 06:48
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
Copy link

pytorch-bot bot commented Mar 7, 2025

🔗 Helpful Links

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

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.

@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 Mar 7, 2025
vkuzo added a commit that referenced this pull request Mar 7, 2025
Summary:

As titled

Test Plan:

```
// note: this fails on weights only load, but the failure happens
// after my changes, and already exists on main branch
pytest test/prototype/test_awq.py -s -x
```

Reviewers:

Subscribers:

Tasks:

Tags:

ghstack-source-id: a6dceb5
ghstack-comment-id: 2706766649
Pull Request resolved: #1853
@vkuzo vkuzo added the topic: not user facing Use this tag if you don't want this PR to show up in release notes label Mar 7, 2025
This was referenced Mar 7, 2025
[ghstack-poisoned]
vkuzo added 6 commits March 7, 2025 11:39
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
Copy link
Contributor

@danielvegamyhre danielvegamyhre left a comment

Choose a reason for hiding this comment

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

Change itself looks good to me but this doesn't sound good:

// note: this fails on weights only load, but the failure happens
// after my changes, and already exists on main branch
pytest test/prototype/test_awq.py -s -x

If the bug exists on main and is unrelated to these changes, how has the test been passing CI? Sounds like we should create a bug to track this and maybe the feature owner can take a look

Copy link
Contributor

@drisspg drisspg left a comment

Choose a reason for hiding this comment

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

Is the model change purposeful

@vkuzo
Copy link
Contributor Author

vkuzo commented Mar 7, 2025

Is the model change purposeful

ah, that snuck in. No, will remove.

@vkuzo
Copy link
Contributor Author

vkuzo commented Mar 7, 2025

Change itself looks good to me but this doesn't sound good:

// note: this fails on weights only load, but the failure happens
// after my changes, and already exists on main branch
pytest test/prototype/test_awq.py -s -x

If the bug exists on main and is unrelated to these changes, how has the test been passing CI? Sounds like we should create a bug to track this and maybe the feature owner can take a look

in general I agree, but given that this is the prototype folder the responsibility to do what you suggested would be on the owner of awq - I'm not sure who that is :) In general for prototype folder there aren't guarantees that we'll spend our time to migrate things, if people want their thing to continue to keep working they should work on moving the feature out of prototype.

vkuzo added 3 commits March 8, 2025 06:15
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
vkuzo added 3 commits March 8, 2025 12:16
[ghstack-poisoned]
[ghstack-poisoned]
[ghstack-poisoned]
@vkuzo vkuzo changed the base branch from gh/vkuzo/56/head to main March 8, 2025 20:17
@vkuzo vkuzo merged commit 49694e3 into main Mar 8, 2025
12 checks passed
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. topic: not user facing Use this tag if you don't want this PR to show up in release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants