Skip to content

[Executorch][Quantization][BE] Refactor Choose Qparams #92592

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
Closed
wants to merge 1 commit into from
Closed

[Executorch][Quantization][BE] Refactor Choose Qparams #92592

wants to merge 1 commit into from

Conversation

JacobSzwejbka
Copy link
Contributor

Summary: Should hopefully be a little faster. Definitely cleaner to not create an observer inside the op

Test Plan: ci

Differential Revision: D42154677

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 18, 2023 •
edited
Loading

🔗 Helpful Links

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

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

❌ 1 Failures

As of commit 7a92886:

NEW FAILURES - The following jobs have failed:

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

@github-actions github-actions bot added the release notes: quantization release notes category label Jan 18, 2023
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D42154677

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D42154677

jerryzh168
jerryzh168 reviewed Jan 19, 2023
View reviewed changes
jerryzh168
jerryzh168 approved these changes Jan 19, 2023
View reviewed changes
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.

looks good, thanks!

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D42154677

@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D42154677

@JacobSzwejbka @facebook-github-bot
[Executorch][Quantization][BE] Refactor Choose Qparams (pytorch#92592)
7a92886 
Summary:
Pull Request resolved: pytorch#92592

Should hopefully be a little faster. Definitely cleaner to not create an observer inside the op

Test Plan: ci

Reviewed By: jerryzh168

Differential Revision: D42154677

fbshipit-source-id: 2ab0d6a554e2675b8c5feb7c9c9c4755554fa2b2
@facebook-github-bot
Copy link
Contributor

This pull request was exported from Phabricator. Differential Revision: D42154677

@JacobSzwejbka
Copy link
Contributor Author

@pytorchbot merge -f

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 20, 2023

❌ 🤖 pytorchbot command failed:

@pytorchbot merge: error: argument -f/--force: expected one argument

usage: @pytorchbot merge [-g | -f MESSAGE | -l] [-r [{viable/strict,master}]]

Try @pytorchbot --help for more info.

@JacobSzwejbka
Copy link
Contributor Author

@pytorchbot --help

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 20, 2023

PyTorchBot Help

usage: @pytorchbot [-h] {merge,revert,rebase,label,drci} ...

In order to invoke the bot on your PR, include a line that starts with
@pytorchbot anywhere in a comment. That line will form the command; no
multi-line commands are allowed. 

Example:
    Some extra context, blah blah, wow this PR looks awesome

    @pytorchbot merge

optional arguments:
  -h, --help            Show this help message and exit.

command:
  {merge,revert,rebase,label,drci}
    merge               Merge a PR
    revert              Revert a PR
    rebase              Rebase a PR
    label               Add label to a PR
    drci                Update Dr. CI

Merge

usage: @pytorchbot merge [-g | -f MESSAGE | -l] [-r [{viable/strict,master}]]

Merge an accepted PR, subject to the rules in .github/merge_rules.json.
By default, this will wait for all required checks (lint, pull) to succeed before merging.

optional arguments:
  -g, --green           Merge when all status checks running on the PR pass. To add status checks, use labels like `ciflow/trunk`.
  -f MESSAGE, --force MESSAGE
                        Merge without checking anything. This requires a reason for auditting purpose, for example:
                        @pytorchbot merge -f 'Minor update to fix lint. Expecting all PR tests to pass'
  -l, --land-checks     [Deprecated - your PR instead now gets the `ciflow/trunk` label on approval] Merge with land time checks. This will create a new branch with your changes rebased on viable/strict and run a majority of trunk tests _before_ landing to increase trunk reliability and decrease risk of revert. The tests added are: pull, Lint and trunk. Note that periodic is excluded.
  -r [{viable/strict,master}], --rebase [{viable/strict,master}]
                        Rebase the PR to re run checks before merging.  Accepts viable/strict or master as branch options and will default to viable/strict if not specified.

Revert

usage: @pytorchbot revert -m MESSAGE -c
                          {nosignal,ignoredsignal,landrace,weird,ghfirst}

Revert a merged PR. This requires that you are a Meta employee.

Example:
  @pytorchbot revert -m="This is breaking tests on trunk. hud.pytorch.org/" -c=nosignal

optional arguments:
  -m MESSAGE, --message MESSAGE
                        The reason you are reverting, will be put in the commit message. Must be longer than 3 words.
  -c {nosignal,ignoredsignal,landrace,weird,ghfirst}, --classification {nosignal,ignoredsignal,landrace,weird,ghfirst}
                        A machine-friendly classification of the revert reason.

Rebase

usage: @pytorchbot rebase [-s | -b BRANCH]

Rebase a PR. Rebasing defaults to the stable viable/strict branch of pytorch.
You must have write permissions to the repo to rebase a PR.

optional arguments:
  -s, --stable          [DEPRECATED] Rebase onto viable/strict
  -b BRANCH, --branch BRANCH
                        Branch you would like to rebase to

Label

usage: @pytorchbot label labels [labels ...]

Adds label to a PR

positional arguments:
  labels  Labels to add to given Pull Request

Dr CI

usage: @pytorchbot drci

Update Dr. CI. Updates the Dr. CI comment on the PR in case it's gotten out of sync with actual CI results.

@JacobSzwejbka
Copy link
Contributor Author

@pytorchbot merge -f the singular failing test failed to random networking issue in the statistics upload stage, the actual test passed.

@pytorch-bot
Copy link

pytorch-bot bot commented Jan 20, 2023

❌ 🤖 pytorchbot command failed:

@pytorchbot: error: unrecognized arguments: singular failing test failed to random networking issue in the statistics upload stage, the actual test passed.

usage: @pytorchbot [-h] {merge,revert,rebase,label,drci} ...

Try @pytorchbot --help for more info.

@JacobSzwejbka
Copy link
Contributor Author

@pytorchbot merge -f ' the singular failing test failed to random networking issue in the statistics upload stage, the actual test passed.'

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

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

jerryzh168
jerryzh168 reviewed Jan 20, 2023
View reviewed changes
torch/ao/quantization/utils.py
# setting zero_point to (-1 * min *inv_scale) we get
# Xq = Round((Xf - min) * inv_scale)
zero_point = -1 * min_val / scale
else:
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check qscheme here as well?

@atalman
Copy link
Contributor

atalman commented Jan 21, 2023

@pytorchbot revert -m "Breaks internal tests" -c ghfirst

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

Can't revert PR that was landed via phabricator as D42154677. Please revert by going to the internal diff and clicking Unland.

@atalman
Copy link
Contributor

atalman commented Jan 23, 2023

@pytorchbot revert -m "Breaks internal tests" -c ghfirst

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

Can't revert PR that was landed via phabricator as D42154677. Please revert by going to the internal diff and clicking Unland.

@atalman
Copy link
Contributor

atalman commented Jan 23, 2023

@pytorchbot revert -m "Breaks internal tests" -c ghfirst

@pytorchmergebot
Copy link
Collaborator

@pytorchbot successfully started a revert job. Check the current status here.
Questions? Feedback? Please reach out to the PyTorch DevX Team

@pytorchmergebot
Copy link
Collaborator

Can't revert PR that was landed via phabricator as D42154677. Please revert by going to the internal diff and clicking Unland.

@malfet
Copy link
Contributor

malfet commented Jan 23, 2023

@bigfootjon @clee2000 not sure why this test were able to merge via GHF, but could not be reverted via GHF. Had something changed with the signals? (As internal PR was abandoned after landing)

@atalman atalman mentioned this pull request Jan 23, 2023
Closed
@malfet
Copy link
Contributor

malfet commented Jan 23, 2023 •
edited
Loading

Test that failed to pass internally is this one:

pytorch/test/quantization/jit/test_ondevice_quantization.py

Line 527 in afe6ea8

def test_device_side_api(self): 

RuntimeError: isGenericDict() INTERNAL ASSERT FAILED at "fbcode/caffe2/aten/src/ATen/core/ivalue_inl.h":1969, please report a bug to PyTorch. Expected GenericDict but got Int ()
  File "/usr/local/fbcode/platform010/lib/python3.8/unittest/case.py", line 60, in testPartExecutor
    yield
  File "/usr/local/fbcode/platform010/lib/python3.8/unittest/case.py", line 676, in run
    self._callTestMethod(testMethod)
  File "/usr/local/fbcode/platform010/lib/python3.8/unittest/case.py", line 633, in _callTestMethod
    method()
  File "/data/sandcastle/boxes/eden-trunk-hg-fbcode-fbsource/buck-out/v2/gen/fbcode/544943cfd42425fb/caffe2/test/quantization/__test_quantization__/test_quantization#link-tree/caffe2/test/quantization/jit/test_ondevice_quantization.py", line 529, in test_device_side_api
    self._check_device_side_api(model)
  File "/data/sandcastle/boxes/eden-trunk-hg-fbcode-fbsource/buck-out/v2/gen/fbcode/544943cfd42425fb/caffe2/test/quantization/__test_quantization__/test_quantization#link-tree/caffe2/test/quantization/jit/test_ondevice_quantization.py", line 498, in _check_device_side_api
    self._check_serdes_and_device_side_api_helper(model, True)
  File "/data/sandcastle/boxes/eden-trunk-hg-fbcode-fbsource/buck-out/v2/gen/fbcode/544943cfd42425fb/caffe2/test/quantization/__test_quantization__/test_quantization#link-tree/caffe2/test/quantization/jit/test_ondevice_quantization.py", line 435, in _check_serdes_and_device_side_api_helper
    m = _load_for_lite_interpreter(buffer)  # Error here
  File "/data/sandcastle/boxes/eden-trunk-hg-fbcode-fbsource/buck-out/v2/gen/fbcode/544943cfd42425fb/caffe2/test/quantization/__test_quantization__/test_quantization#link-tree/torch/jit/mobile/__init__.py", line 50, in _load_for_lite_interpreter
    cpp_module = torch._C._load_for_lite_interpreter_from_buffer(f.read(), map_location)

Ok, I can confirm that this test is not executed in OSS, but only internally, see

pytorch/test/test_quantization.py

Lines 119 to 127 in afe6ea8

from quantization.jit.test_quantize_jit import TestQuantizeJit # noqa: F401
from quantization.jit.test_quantize_jit import TestQuantizeJitPasses # noqa: F401
from quantization.jit.test_quantize_jit import TestQuantizeJitOps # noqa: F401
from quantization.jit.test_quantize_jit import TestQuantizeDynamicJitPasses # noqa: F401
from quantization.jit.test_quantize_jit import TestQuantizeDynamicJitOps # noqa: F401
# Quantization specific fusion passes
from quantization.jit.test_fusion_passes import TestFusionPasses # noqa: F401
from quantization.jit.test_deprecated_jit_quant import TestDeprecatedJitQuantized # noqa: F401

@vkuzo : can you please either re-enable this test in OSS or disable it internally?

malfet added a commit that referenced this pull request Jan 23, 2023
@malfet
Revert "[Executorch][Quantization][BE] Refactor Choose Qparams (#92592)"
c0dd9b3 
This reverts commit 59071ab.

It breaks `quantization.jit.test_ondevice_quantization.TestOnDeviceDynamicPTQFinalize`, which is not run in OSS, but is mandatory for internal CI.
@bigfootjon
Copy link
Member

@bigfootjon @clee2000 not sure why this test were able to merge via GHF, but could not be reverted via GHF. Had something changed with the signals? (As internal PR was abandoned after landing)

I don't see why this failed to revert. What logic to mergebot use to determine if something was landed via a Diff?

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

@jerryzh168 jerryzh168 jerryzh168 approved these changes

Assignees
No one assigned
Labels
fb-exported Merged release notes: AO frontend release notes: quantization release notes category Reverted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@JacobSzwejbka @facebook-github-bot @pytorchmergebot @atalman @malfet @bigfootjon @jerryzh168