executorch icon indicating copy to clipboard operation
executorch copied to clipboard

Qualcomm AI Engine Direct - Add rewrite function of observer

Open chunit-quic opened this issue 9 months ago • 8 comments

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

chunit-quic avatar Apr 11 '25 00:04 chunit-quic

:link: Helpful Links

:test_tube: 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.

:x: 1 New Failure

As of commit 3caeaab6615362dcc6af3898eaa3bc840930473f with merge base 95a1db5b797e4be6198cea2821069175fd9e61bd (image):

NEW FAILURE - The following job has failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

pytorch-bot[bot] avatar Apr 11 '25 00:04 pytorch-bot[bot]

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 avatar Apr 11 '25 00:04 chunit-quic

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

cccclai avatar Apr 15 '25 22:04 cccclai

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

cccclai avatar Apr 15 '25 22:04 cccclai

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.

chunit-quic avatar Apr 16 '25 00:04 chunit-quic

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 avatar Apr 24 '25 00:04 sxu

Maybe @jerryzh168 can confirm?

sxu avatar Apr 24 '25 00:04 sxu

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

cccclai avatar May 01 '25 09:05 cccclai

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

cccclai avatar May 23 '25 17:05 cccclai

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

Done! Thank you.

chunit-quic avatar May 26 '25 00:05 chunit-quic

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

facebook-github-bot avatar May 26 '25 23:05 facebook-github-bot

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

cccclai avatar May 28 '25 00:05 cccclai

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

chunit-quic avatar May 28 '25 01:05 chunit-quic

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

cccclai avatar May 28 '25 03:05 cccclai

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

chunit-quic avatar May 28 '25 04:05 chunit-quic

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

chunit-quic avatar May 28 '25 04:05 chunit-quic

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

facebook-github-bot avatar May 29 '25 17:05 facebook-github-bot

There seems to be a lint error, and that might be reason merging fails. Since https://github.com/pytorch/executorch/pull/11049 is landed, can you apply the according changes and see if the error is gone?

cccclai avatar May 29 '25 17:05 cccclai

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!

chunit-quic avatar Jun 02 '25 00:06 chunit-quic

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

facebook-github-bot avatar Jun 02 '25 01:06 facebook-github-bot

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.

kimishpatel avatar Jun 02 '25 13:06 kimishpatel