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.

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 2 commits into from
Jun 2, 2025

Conversation

chunit-quic
Copy link
Collaborator

@chunit-quic chunit-quic commented Apr 11, 2025

  • Add function to rewrite observer after prepare_pt2e
  • Add corresponding test case

@chunit-quic chunit-quic requested a review from cccclai as a code owner April 11, 2025 00:25
Copy link

pytorch-bot bot commented Apr 11, 2025

🔗 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
Collaborator 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!

@cccclai
Copy link
Contributor

cccclai commented Apr 15, 2025

@chunit-quic thanks for putting up the pr. cc: @sxu @billmguo

@cccclai
Copy link
Contributor

cccclai commented Apr 15, 2025

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

@chunit-quic
Copy link
Collaborator Author

chunit-quic commented Apr 16, 2025

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.

@cccclai cccclai requested review from sxu and billmguo April 17, 2025 19:35
@sxu
Copy link
Contributor

sxu commented Apr 24, 2025

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?

@cccclai
Copy link
Contributor

cccclai commented May 1, 2025

@kirklandsign can you help checking the CI signal for this one too? I remember there are some errors.

@cccclai
Copy link
Contributor

cccclai commented May 23, 2025

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

@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
Collaborator 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.

@cccclai
Copy link
Contributor

cccclai commented May 28, 2025

Hey there some errors likely due to the pytorch pin update. Mind rebasing again? Sorry for the inconvinience

@chunit-quic
Copy link
Collaborator Author

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

qscheme=torch.per_tensor_affine,
)

rewrite_prepared_observer(prepared, {"activation_post_process_2": new_obs})
Copy link
Contributor

@jerryzh168 jerryzh168 May 28, 2025

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

@cccclai
Copy link
Contributor

cccclai commented May 28, 2025

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

Did you rebase? Seems like there is no commit..

@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
Collaborator Author

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

1 similar comment
@chunit-quic
Copy link
Collaborator 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.

@cccclai
Copy link
Contributor

cccclai commented May 29, 2025

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?

@cccclai cccclai added the release notes: qualcomm Changes to the Qualcomm backend delegate label May 29, 2025
Chun-I Tsai added 2 commits June 2, 2025 08:26
- Add function to rewrite observer after prepare_pt2e
- Add corresponding test case
@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
Collaborator 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
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
Development

Successfully merging this pull request may close these issues.

7 participants