8000 Qualcomm AI Engine Direct - Add rewrite function of observer by chunit-quic · Pull Request #10093 · pytorch/executorch · GitHub
[go: up one dir, main page]
Skip to content

Qualcomm AI Engine Direct - Add rewrite function of observer #10093

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jun 2, 2025
Merged

Qualcomm AI Engine Direct - Add rewrite function of observer #10093

merged 2 commits into from
Jun 2, 2025

Conversation

chunit-quic
Copy link
Contributor
@chunit-quic chunit-quic commented Apr 11, 2025 •
edited
Loading
  • Add function to rewrite observer after prepare_pt2e
  • Add corresponding test case

8000

@chunit-quic chunit-quic requested a review from cccclai as a code owner April 11, 2025 00:25
@pytorch-bot PyTorch Bot
Copy link
pytorch-bot bot commented Apr 11, 2025 •
edited
Loading

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/10093

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

❌ 1 New Failure

As of commit 3caeaab with merge base 95a1db5 (image):

NEW FAILURE - The following job has 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 Apr 11, 2025
@chunit-quic
Copy link
Contributor Author

Hi @cccclai,

This PR is for the requested functionality to overwrite a quanization parameters after calibration. It can handle shared observer too. Thank you!

@chunit-quic
Copy link
Contributor Author
chunit-quic commented Apr 16, 2025 •
edited
Loading

Mind sharing a bit more details for the request? I probably miss it

No problem. :)
We received some feature requests in a mail thread regarding quantization requirements. This particular request is for the following purpose:

The QNN Quantizer should allow users to override quantization parameters for specific tensors, regardless of the data ranges observed during calibration or QAT. This override must respect the transitive closures established by SharedQuantizationSpec.

After a brief discussion your team, we concluded that the rewriting stage should occur after calibration but before conversion. That's essentially the background.

@sxu
Copy link
Contributor
sxu commented Apr 24, 2025 •
edited
Loading

Looks like this would work to me, however it would be great if someone from AO can confirm this kind of overwriting activation postprocessing submodule is enough and there's no other references to them.

@sxu
Copy link
Contributor
sxu commented Apr 24, 2025

Maybe @jerryzh168 can confirm?

@chunit-quic chunit-quic force-pushed the dev_rewrite_qconfig branch from 2c9c79c to b92c102 Compare May 26, 2025 00:47
@chunit-quic
Copy link
Contributor Author

Hey sorry for being late, can you rebase this PR?

Done! Thank you.

@facebook-github-bot
Copy link
Contributor

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@chunit-quic
Copy link
Contributor Author

No problem. Just rebased. Pleasefel free to let me know if there is any issue. Thank you!

jerryzh168
jerryzh168 reviewed May 28, 2025
View reviewed changes
backends/qualcomm/tests/test_qnn_delegate.py
qscheme=torch.per_tensor_affine,
)

rewrite_prepared_observer(prepared, {"activation_post_process_2": new_obs})
Copy link
Contributor
@jerryzh168 jerryzh168 May 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

@sxu does your callback work for this? maybe you can share your example

@chunit-quic chunit-quic force-pushed the dev_rewrite_qconfig branch from b92c102 to 943eb09 Compare May 28, 2025 04:07
@chunit-quic
Copy link
Contributor Author

Ops, Sorry I rebased locally without push. Thank you for pointing out! Done.

1 similar comment
@chunit-quic
Copy link
Contributor Author

Ops, Sorry I rebased locally without push. Thank you for pointing out! Done.

@facebook-github-bot
Copy link
Contributor

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

Chun-I Tsai added 2 commits June 2, 2025 08:26
Qualcomm AI Engine Direct - Add rewrite function of observer
678cd29 
- Add function to rewrite observer after prepare_pt2e
- Add corresponding test case
Fix torch.ao to torchao error from rebase
3caeaab
@chunit-quic chunit-quic force-pushed the dev_rewrite_qconfig branch from 943eb09 to 3caeaab Compare June 2, 2025 00:42
@chunit-quic
Copy link
Contributor Author

There seems to be a lint error, and that might be reason merging fails. Since #11049 is landed, can you apply the according changes and see if the error is gone?

Fixed. Thanks a lot for pointing out the error!

@facebook-github-bot
Copy link
Contributor

@cccclai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

@facebook-github-bot facebook-github-bot merged commit d5c4ba7 into pytorch:main Jun 2, 2025
96 of 97 checks passed
@kimishpatel
Copy link
Contributor

Mind sharing a bit more details for the request? I probably miss it

No problem. :) We received some feature requests in a mail thread regarding quantization requirements. This particular request is for the following purpose:

The QNN Quantizer should allow users to override quantization parameters for specific tensors, regardless of the data ranges observed during calibration or QAT. This override must respect the transitive closures established by SharedQuantizationSpec.

After a brief discussion your team, we concluded that the rewriting stage should occur after calibration but before conversion. That's essentially the background.

I dont quite follow why we cannot do this by allowing module specific customization? FOr example can I not say quanitzer.set_module_qconfig(....). And @jerryzh168 I really think we should make this part of the base class. Quantizers that dont implement this will fallback to baseclass which will just raise error.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jerryzh168 jerryzh168 jerryzh168 left review comments

@sxu sxu sxu approved these changes

@billmguo billmguo billmguo approved these changes

Assignees
No one assigned
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. release notes: qualcomm Changes to the Qualcomm backend delegate
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@chunit-quic @sxu @facebook-github-bot @kimishpatel @jerryzh168 @billmguo
0