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

refactor _replace_linear_8da4w #451

Merged
merged 3 commits into from
Jul 1, 2024
Merged

refactor _replace_linear_8da4w #451

merged 3 commits into from
Jul 1, 2024

Conversation

Hanxian97
Copy link
Contributor

@Hanxian97 Hanxian97 commented Jun 27, 2024

Summary:
Reimplement the _replace_linear_8da4w function using the more general util function _replace_with_custom_fn_if_matches_filter from torchao.quantization.quant_api to reduce code duplication of similar logic.

Test Plan:
python test/quantization/test_quant_api.py
python test/quantization/test_quant_primitives.py

Copy link

pytorch-bot bot commented Jun 27, 2024

🔗 Helpful Links

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

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

✅ No Failures

As of commit 2d4f772 with merge base dee13e1 (image):
💚 Looks good so far! There are no failures yet. 💚

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 Jun 27, 2024
Copy link
Contributor

@andrewor14 andrewor14 left a comment

Choose a reason for hiding this comment

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

Thanks, looks great overall, just a few small nits!

torchao/quantization/GPTQ.py Show resolved Hide resolved
if copy_weights and child.weight.device != torch.device("meta"):
new_linear.weight = child.weight
return new_linear
#setattr(module, name, new_linear)
Copy link
Contributor

Choose a reason for hiding this comment

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

Please delete the commented out code (this one and the block comment in 923)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

removed

@jerryzh168
Copy link
Contributor

_ No description provided. _

can you add a summary to talk about the context for the change?

@jerryzh168
Copy link
Contributor

jerryzh168 commented Jun 27, 2024

please make sure to fill in Summary and Test Plan, like: #389

@Hanxian97 Hanxian97 closed this Jun 27, 2024
@Hanxian97 Hanxian97 reopened this Jun 27, 2024
@Hanxian97
Copy link
Contributor Author

Summary:
Reimplement the _replace_linear_8da4w function using the more general util function _replace_with_custom_fn_if_matches_filter from torchao.quantization.quant_api to reduce code duplication of similar logic.

Test Plan:
python test/quantization/test_quant_api.py
python test/quantization/test_quant_primitives.py

@jerryzh168
Copy link
Contributor

Summary: Reimplement the _replace_linear_8da4w function using the more general util function _replace_with_custom_fn_if_matches_filter from torchao.quantization.quant_api to reduce code duplication of similar logic.

Test Plan: python test/quantization/test_quant_api.py python test/quantization/test_quant_primitives.py

you can edit the first message btw:
Screenshot 2024-06-27 at 17 29 46

Copy link
Contributor

@andrewor14 andrewor14 left a comment

Choose a reason for hiding this comment

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

Looks good to me! @jerryzh168 any other comments?

if _check_linear_int4_k(child.in_features, groupsize) or padding_allowed:
new_linear = linear_class(

#import the util function here to avoid circular dependency
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: please add a space between # and import

#import the util function here to avoid circular dependency
from torchao.quantization.quant_api import _replace_with_custom_fn_if_matches_filter

def filter_fn(child: torch.nn.Module, cur_fqn:str) -> bool:
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: spacing cur_fqn: str

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.

LGTM, thanks for addressing all the comments

@msaroufim
Copy link
Member

@pytorchbot merge

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@msaroufim
Copy link
Member

msaroufim commented Jun 29, 2024

@huydhn @clee2000 @jerryzh168 is it deliberate that we can no longer merge PRs manually now? Was this to enable ghstack? I'm not sure the tradeoff was worth it since most contributors are not using ghstack

I deliberately removed requirements to have the branch be up to date and authorized merging for anyone who is a maintainer and that makes merging contributor code simpler. Right now I'm just waiting on the bot to merge something that should be merged immediately

The required checks now don't have any tests either? The required checks are only some internal FB checks

image

image

EDIT: This seems to be an unrelated bug with branch protection rules internally, am following up

@jerryzh168
Copy link
Contributor

I'm not sure what happened, ghstack changes are not going to affect normal landing I think, this is unexpected. @huydhn do you know what is happening here?

@msaroufim msaroufim merged commit 39b02de into main Jul 1, 2024
13 checks passed
@msaroufim msaroufim deleted the hanxian_8da4w_trial branch July 1, 2024 16:11
dbyoung18 pushed a commit to dbyoung18/ao that referenced this pull request Jul 31, 2024
* refactor _replace_linear_8da4w

* clean up version

---------
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. Merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants