Skip to content

Qualcomm AI Engine Direct - Quantizer refine for qat #6747

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

Conversation

chunit-quic
Copy link
Collaborator

Follow the instruction to resubmit the PR after PR6513 is reverted.

Copy link

pytorch-bot bot commented Nov 11, 2024

🔗 Helpful Links

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

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

❗ 1 Active SEVs

There are 1 currently active SEVs. If your PR is affected, please view them below:

✅ No Failures

As of commit 844acda with merge base ecdc007 (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 Nov 11, 2024
@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.

get_16a8w_qnn_ptq_config,
get_default_16bit_qnn_ptq_config,
Copy link
Contributor

Choose a reason for hiding this comment

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

Removing get_default_16bit_qnn_ptq_config causing internal failure...can we do it in a the following way

  1. This PR includes both new config and the old config
  2. I submit a PR internally to remove the old config call site
  3. You submit a new PR to remove the old config

Then we can land it safely...

Copy link
Collaborator Author

@chunit-quic chunit-quic Nov 13, 2024

Choose a reason for hiding this comment

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

Hi Chen,
Thanks for pointing out what to fix.
It seems to fail again. Shuold I also add get_default_8bit_qnn_ptq_config back ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry what is fail again?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Two checks in CI chcek list below
image

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh hmm I feel like the Meta Internal-Only check isn't very accurate...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No problem. Let me know what should be fix if any. :D

Copy link
Contributor

Choose a reason for hiding this comment

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

Let me import and check CI again...

@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 Nov 13, 2024

Oh looks like some internal failure is resolved! There is one more left but I think it's already there. I'll submit a PR to fix that and then merge this change.

@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 Nov 14, 2024

Hey can you rebase this change? I landed the internal PR...



# TODO move to torch/ao/quantization/observer.py.
class PerChannelParamObserver(UniformQuantizationObserverBase):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you add some comments here as to why chose the min/max ranges in the way being done here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @kimishpatel, just add the comment. If it's not clear enought please feel free to let me know. Thanks

@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 Nov 18, 2024

Hey wonder if you can rebase this PR, I've been failing to import this PR and the error is following

    The Pull Request could not be imported cleanly from GitHub. This usually happens for one of two reasons:
    1. The Pull Request is out of date, if you rebase the PR on the latest commit on the GitHub branch and find merge conflicts this is probably the reason
    2. ShipIt is broken for this repo, check bunnylol `oss org/repo`, click on the ShipIt tab, and see if there are any alerts. Fix those and retry the import.

Joey Tsai added 6 commits November 18, 2024 11:23
- Reorginize qualcomm/quantizer
- Split quantizer/utils.py to
-- qconfig
-- annotators
-- observers directory
- Change coresponding callees
- Rename get_default_Nbit_qnn_ptq_config to get_NaNw_qnn_ptq_config
- Add 16a4w conv test* (It is not compared with original model)
- Move and rename param_observer.py to per_channel_param_observer.py
- Add todo to merge qconfig
- Add todo for per_channel_param_observer.py
@chunit-quic chunit-quic force-pushed the dev1/chunit/qat_quantizer_refine branch from dd19c66 to b353d4c Compare November 18, 2024 03:23
@chunit-quic
Copy link
Collaborator Author

Hey wonder if you can rebase this PR, I've been failing to import this PR and the error is following

Just rebased. Thanks for pointing out.

@cccclai
Copy link
Contributor

cccclai commented Nov 18, 2024

Thanks! There is still a small lintrunner error in the CI...

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

- Fix get_ptq_per_channel_quant_config not founded 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 e95f171 into pytorch:main Nov 18, 2024
39 of 41 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants