Skip to content
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

[Quant] Respect non_leaf_module_list for activation modules #88498

Closed
wants to merge 3 commits into from

Conversation

andrewor14
Copy link
Contributor

@andrewor14 andrewor14 commented Nov 4, 2022

Stack from ghstack (oldest at bottom):

Summary: This commit fixes the bug where non_leaf_module_list
was not respected for activation modules like torch.nn.Sigmoid
and torch.nn.Tanh. Today, these modules default to
default_fixed_qparams_range_0to1_fake_quant, and there is no
way to configure them to use any other activation_post_process
(e.g. FixedQParamsObserver) (see this mapping).
non_leaf_module_list is a "list of non-leaf modules we want
to add observer" (see prepare docstring). If the user explicitly
specified to insert observers for these modules, we should respect
that instead of continuing to use the default.

Test Plan:
python test/test_quantization.py TestQuantizeEagerPTQStatic.test_activations_in_non_leaf_module_list

Reviewers: vkuzo, jerryzh168

Subscribers: vkuzo, jerryzh168

cc @jerryzh168 @jianyuh @raghuramank100 @jamesr66a @vkuzo @jgong5 @Xia-Weiwen @leslie-fang-intel

Summary: This commit fixes the bug where `non_leaf_module_list`
was not respected for activation modules like `torch.nn.Sigmoid`
and `torch.nn.Tanh`. Today, these modules default to
`default_fixed_qparams_range_0to1_fake_quant`, and there is no
way to configure them to use any other activation_post_process
(e.g. FixedQParamsObserver). (source)[https://github.com/pytorch/pytorch/blob/dc00bb51b8d370bf3891f0edb2c6e0c2914e329a/torch/ao/quantization/quantization_mappings.py#L188-L193]
`non_leaf_module_list` is a "list of non-leaf modules we want
to add observer" (see prepare docstring). If the user explicitly
specified to insert observers for these modules, we should respect
that instead of continuing to use the default.

Test Plan:
python test/test_quantization.py TestQuantizeEagerPTQStatic.test_activations_in_non_leaf_module_list

Reviewers: vkuzo, jerryzh168

Subscribers: vkuzo, jerryzh168

[ghstack-poisoned]
@pytorch-bot
Copy link

pytorch-bot bot commented Nov 4, 2022

🔗 Helpful Links

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

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

✅ No Failures

As of commit e366ed8:
💚 Looks good so far! There are no failures yet. 💚

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

@pytorch-bot pytorch-bot bot added the release notes: quantization release notes category label Nov 4, 2022
andrewor14 added a commit that referenced this pull request Nov 4, 2022
Summary: This commit fixes the bug where `non_leaf_module_list`
was not respected for activation modules like `torch.nn.Sigmoid`
and `torch.nn.Tanh`. Today, these modules default to
`default_fixed_qparams_range_0to1_fake_quant`, and there is no
way to configure them to use any other activation_post_process
(e.g. FixedQParamsObserver). (source)[https://github.com/pytorch/pytorch/blob/dc00bb51b8d370bf3891f0edb2c6e0c2914e329a/torch/ao/quantization/quantization_mappings.py#L188-L193]
`non_leaf_module_list` is a "list of non-leaf modules we want
to add observer" (see prepare docstring). If the user explicitly
specified to insert observers for these modules, we should respect
that instead of continuing to use the default.

Test Plan:
python test/test_quantization.py TestQuantizeEagerPTQStatic.test_activations_in_non_leaf_module_list

Reviewers: vkuzo, jerryzh168

Subscribers: vkuzo, jerryzh168

ghstack-source-id: 8466b870c5f643d53b54fbac35fafa5c2841e919
Pull Request resolved: #88498
@github-actions github-actions bot added the oncall: quantization Quantization support in PyTorch label Nov 4, 2022
@andrewor14 andrewor14 added the topic: bug fixes topic category label Nov 4, 2022
Summary: This commit fixes the bug where `non_leaf_module_list`
was not respected for activation modules like `torch.nn.Sigmoid`
and `torch.nn.Tanh`. Today, these modules default to
`default_fixed_qparams_range_0to1_fake_quant`, and there is no
way to configure them to use any other activation_post_process
(e.g. FixedQParamsObserver). [source](https://github.com/pytorch/pytorch/blob/dc00bb51b8d370bf3891f0edb2c6e0c2914e329a/torch/ao/quantization/quantization_mappings.py#L188-L193)
`non_leaf_module_list` is a "list of non-leaf modules we want
to add observer" (see prepare docstring). If the user explicitly
specified to insert observers for these modules, we should respect
that instead of continuing to use the default.

Test Plan:
python test/test_quantization.py TestQuantizeEagerPTQStatic.test_activations_in_non_leaf_module_list

Reviewers: vkuzo, jerryzh168

Subscribers: vkuzo, jerryzh168

[ghstack-poisoned]
andrewor14 added a commit that referenced this pull request Nov 4, 2022
Summary: This commit fixes the bug where `non_leaf_module_list`
was not respected for activation modules like `torch.nn.Sigmoid`
and `torch.nn.Tanh`. Today, these modules default to
`default_fixed_qparams_range_0to1_fake_quant`, and there is no
way to configure them to use any other activation_post_process
(e.g. FixedQParamsObserver). [source](https://github.com/pytorch/pytorch/blob/dc00bb51b8d370bf3891f0edb2c6e0c2914e329a/torch/ao/quantization/quantization_mappings.py#L188-L193)
`non_leaf_module_list` is a "list of non-leaf modules we want
to add observer" (see prepare docstring). If the user explicitly
specified to insert observers for these modules, we should respect
that instead of continuing to use the default.

Test Plan:
python test/test_quantization.py TestQuantizeEagerPTQStatic.test_activations_in_non_leaf_module_list

Reviewers: vkuzo, jerryzh168

Subscribers: vkuzo, jerryzh168

ghstack-source-id: 8466b870c5f643d53b54fbac35fafa5c2841e919
Pull Request resolved: #88498
Summary: This commit fixes the bug where `non_leaf_module_list`
was not respected for activation modules like `torch.nn.Sigmoid`
and `torch.nn.Tanh`. Today, these modules default to
`default_fixed_qparams_range_0to1_fake_quant`, and there is no
way to configure them to use any other activation_post_process
(e.g. FixedQParamsObserver) (see this [mapping](https://github.com/pytorch/pytorch/blob/dc00bb51b8d370bf3891f0edb2c6e0c2914e329a/torch/ao/quantization/quantization_mappings.py#L188-L193)).
`non_leaf_module_list` is a "list of non-leaf modules we want
to add observer" (see prepare docstring). If the user explicitly
specified to insert observers for these modules, we should respect
that instead of continuing to use the default.

Test Plan:
python test/test_quantization.py TestQuantizeEagerPTQStatic.test_activations_in_non_leaf_module_list

Reviewers: vkuzo, jerryzh168

Subscribers: vkuzo, jerryzh168

[ghstack-poisoned]
andrewor14 added a commit that referenced this pull request Nov 4, 2022
Summary: This commit fixes the bug where `non_leaf_module_list`
was not respected for activation modules like `torch.nn.Sigmoid`
and `torch.nn.Tanh`. Today, these modules default to
`default_fixed_qparams_range_0to1_fake_quant`, and there is no
way to configure them to use any other activation_post_process
(e.g. FixedQParamsObserver) (see this [mapping](https://github.com/pytorch/pytorch/blob/dc00bb51b8d370bf3891f0edb2c6e0c2914e329a/torch/ao/quantization/quantization_mappings.py#L188-L193)).
`non_leaf_module_list` is a "list of non-leaf modules we want
to add observer" (see prepare docstring). If the user explicitly
specified to insert observers for these modules, we should respect
that instead of continuing to use the default.

Test Plan:
python test/test_quantization.py TestQuantizeEagerPTQStatic.test_activations_in_non_leaf_module_list

Reviewers: vkuzo, jerryzh168

Subscribers: vkuzo, jerryzh168

ghstack-source-id: 8466b870c5f643d53b54fbac35fafa5c2841e919
Pull Request resolved: #88498
@andrewor14 andrewor14 requested a review from vkuzo November 4, 2022 16:02
elif non_leaf_module_list is not None and type_before_parametrizations(child) in non_leaf_module_list:
if needs_observation(child):
insert_activation_post_process(child)
elif _has_special_act_post_process(child):
Copy link
Contributor

@jerryzh168 jerryzh168 Nov 4, 2022

Choose a reason for hiding this comment

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

is this a hack so that eager mode api is bc? should we also considering removing this hack by asking users to explicitly set fixed qparam configs for these ops so that people don't need to use non_leaf_module api to workaround this limitation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think so. There's no validation logic for eager mode right now so we just override it here. Yes I think simply removing it will make things cleaner, though it'll be a broader scope change since it'll break a lot of existing use cases. I'll leave that for a future PR to unblock #88456.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Filed #88579

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.

LGTM, I think we should also consider removing the hack and break the BC for these ops (I think we already did that in fx graph mode quant)

@andrewor14
Copy link
Contributor Author

Thanks, I'm merging this first to unblock #88456. We can discuss further if we want to do the validation for eager mode separately.

@andrewor14
Copy link
Contributor Author

@pytorchbot merge

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 4, 2022
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks pass (ETA 0-4 Hours).

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

kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Nov 5, 2022
…88498)

Summary: This commit fixes the bug where `non_leaf_module_list`
was not respected for activation modules like `torch.nn.Sigmoid`
and `torch.nn.Tanh`. Today, these modules default to
`default_fixed_qparams_range_0to1_fake_quant`, and there is no
way to configure them to use any other activation_post_process
(e.g. FixedQParamsObserver) (see this [mapping](https://github.com/pytorch/pytorch/blob/dc00bb51b8d370bf3891f0edb2c6e0c2914e329a/torch/ao/quantization/quantization_mappings.py#L188-L193)).
`non_leaf_module_list` is a "list of non-leaf modules we want
to add observer" (see prepare docstring). If the user explicitly
specified to insert observers for these modules, we should respect
that instead of continuing to use the default.

Test Plan:
python test/test_quantization.py TestQuantizeEagerPTQStatic.test_activations_in_non_leaf_module_list

Reviewers: vkuzo, jerryzh168

Subscribers: vkuzo, jerryzh168

Pull Request resolved: pytorch#88498
Approved by: https://github.com/jerryzh168
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
…88498)

Summary: This commit fixes the bug where `non_leaf_module_list`
was not respected for activation modules like `torch.nn.Sigmoid`
and `torch.nn.Tanh`. Today, these modules default to
`default_fixed_qparams_range_0to1_fake_quant`, and there is no
way to configure them to use any other activation_post_process
(e.g. FixedQParamsObserver) (see this [mapping](https://github.com/pytorch/pytorch/blob/dc00bb51b8d370bf3891f0edb2c6e0c2914e329a/torch/ao/quantization/quantization_mappings.py#L188-L193)).
`non_leaf_module_list` is a "list of non-leaf modules we want
to add observer" (see prepare docstring). If the user explicitly
specified to insert observers for these modules, we should respect
that instead of continuing to use the default.

Test Plan:
python test/test_quantization.py TestQuantizeEagerPTQStatic.test_activations_in_non_leaf_module_list

Reviewers: vkuzo, jerryzh168

Subscribers: vkuzo, jerryzh168

Pull Request resolved: pytorch#88498
Approved by: https://github.com/jerryzh168
@facebook-github-bot facebook-github-bot deleted the gh/andrewor14/32/head branch June 8, 2023 15:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged oncall: quantization Quantization support in PyTorch release notes: quantization release notes category topic: bug fixes topic category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants