[ao][ns] Replacing List[QConfigMapping] in PNP by HDCharles · Pull Request #86922 · pytorch/pytorch

HDCharles · 2022-10-13T18:00:58Z

Stack from ghstack (oldest at bottom):

-> [ao][ns] Replacing List[QConfigMapping] in PNP #86922

Summary: Added QConfigMultiMapping which is essentially a
List[QConfigMapping] with set methods and dedicated handling to
avoid unwanted matches and improve UX.

note: the from future import annotations line caused weird errors when the
QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved.

Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows

Reviewers:

Subscribers:

Tasks:

Tags:

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

pytorch-bot · 2022-10-13T18:01:03Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

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

✅ No Failures

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

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

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 93cf9ae Pull Request resolved: #86922

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 92353bd Pull Request resolved: #86922

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: fdaa02f Pull Request resolved: #86922

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 532dae4 Pull Request resolved: #86922

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 4dc008b Pull Request resolved: #86922

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 6fd6a77 Pull Request resolved: #86922

vkuzo · 2022-10-24T17:49:01Z

torch/ao/ns/_numeric_suite_fx.py

    model: torch.nn.Module,
    example_inputs: Any,
-    qconfig_mappings: List[QConfigMapping],
+    qconfig_mappings: Union[List[QConfigMapping], QConfigMultiMapping],


can we just take the new object instead of old object or new object?

I can do that, just didn't want to break BC unnecessarily when its easy enough to maintain

No one is using it yet, so there is no expectation of BC. We can change API as needed until we get some customers, in fact this is the time when it's easiest to change the API.

vkuzo · 2022-10-24T17:51:04Z

torch/ao/ns/fx/qconfig_multi_mapping.py

+            while len(qconfig_list) < len(self.qconfig_mappings_list):
+                qconfig_list.append(None)
+
+    def _remove_duplicates(self, qconfig_list) -> None:


maybe move this outside the class since it's not changing any class variables?

vkuzo · 2022-10-24T19:22:07Z

torch/ao/ns/fx/qconfig_multi_mapping.py

+}
+
+
+class QConfigMultiMapping:


any thoughts on how the user should think about the order of the results? For example, if the user does

QConfigMultiMapping().\ set_global([a, b]).\ set_object_type(nn.Conv2d, [c, d])

how would the results be ordered in the resulting loggers? I think any reasonable order is fine, I'm just asking what the actual order would be. We should probably document it.

i'd probably rather split this consideration into a seperate PR because if we're going to have guarantees about ordering we'd probably want a more robust test suite for it. There's no explicit ordering considerations at the moment, thoguh I assume it'd be ordered as one would expected, i.e. fifo for qconfigs.

makes sense, it seems that for anything other than testing one qconfig an order would be required in order to interpret the results correctly

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 5c36a38 Pull Request resolved: #86922

vkuzo · 2022-10-24T22:26:31Z

torch/ao/ns/_numeric_suite_fx.py

    model: torch.nn.Module,
    example_inputs: Any,
-    qconfig_mappings: List[QConfigMapping],
+    qconfig_mappings: QConfigMultiMapping,


nit: update var name?

vkuzo

thanks for working on this!

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: a161400 Pull Request resolved: #86922

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: a145d06 Pull Request resolved: #86922

HDCharles · 2022-10-26T18:52:11Z

@pytorchmergebot merge

pytorchmergebot · 2022-10-26T18:56:49Z

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

github-actions · 2022-10-26T18:57:31Z

Hey @HDCharles.
You've committed this PR, but it does not have both a 'release notes: ...' and 'topics: ...' label. Please add one of each to the PR. The 'release notes: ...' label should represent the part of PyTorch that this PR changes (fx, autograd, distributed, etc) and the 'topics: ...' label should represent the kind of PR it is (not user facing, new feature, bug fix, perf improvement, etc). The list of valid labels can be found here for the 'release notes: ...' and here for the 'topics: ...'.
For changes that are 'topic: not user facing' there is no need for a release notes label.

Summary: Added QConfigMultiMapping which is essentially a List[QConfigMapping] with set methods and dedicated handling to avoid unwanted matches and improve UX. note: the from __future__ import annotations line caused weird errors when the QConfigMultiMapping class was put in _numeric_suite_fx.py so it was moved. Test Plan: python test/test_quantization.py TestFxNumericSuiteNShadows Reviewers: Subscribers: Tasks: Tags: Pull Request resolved: pytorch#86922 Approved by: https://github.com/vkuzo

[wip][ao][ns] Replacing List[QConfigMapping] in PNP

60a5ce2

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

HDCharles requested review from jerryzh168 and z-a-f as code owners October 13, 2022 18:00

pytorch-bot bot added the release notes: quantization release notes category label Oct 13, 2022

HDCharles added a commit that referenced this pull request Oct 13, 2022

[wip][ao][ns] Replacing List[QConfigMapping] in PNP

1f8d803

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 93cf9ae Pull Request resolved: #86922

Update on "[wip][ao][ns] Replacing List[QConfigMapping] in PNP"

1e5fcbb

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

Update on "[wip][ao][ns] Replacing List[QConfigMapping] in PNP"

33fe567

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

HDCharles added a commit that referenced this pull request Oct 17, 2022

[wip][ao][ns] Replacing List[QConfigMapping] in PNP

4403854

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: ghstack-source-id: 92353bd Pull Request resolved: #86922

Update on "[wip][ao][ns] Replacing List[QConfigMapping] in PNP"

a0433cd

Summary: Test Plan: Reviewers: Subscribers: Tasks: Tags: [ghstack-poisoned]

HDCharles changed the title ~~[wip][ao][ns] Replacing List[QConfigMapping] in PNP~~ [ao][ns] Replacing List[QConfigMapping] in PNP Oct 18, 2022

HDCharles requested a review from vkuzo October 24, 2022 17:10

vkuzo reviewed Oct 24, 2022

View reviewed changes

vkuzo approved these changes Oct 24, 2022

View reviewed changes

pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 24, 2022

pytorchmergebot added the Merged label Oct 26, 2022

pytorchmergebot closed this in f7a04f3 Oct 26, 2022

HDCharles added the topic: improvements topic category label Oct 26, 2022

facebook-github-bot deleted the gh/HDCharles/119/head branch June 8, 2023 14:36

Conversation

HDCharles commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot bot commented Oct 13, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

✅ No Failures

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

vkuzo left a comment

Choose a reason for hiding this comment

Uh oh!

HDCharles commented Oct 26, 2022

Uh oh!

pytorchmergebot commented Oct 26, 2022

Merge started

Uh oh!

github-actions bot commented Oct 26, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HDCharles commented Oct 13, 2022 •

edited

Loading

pytorch-bot bot commented Oct 13, 2022 •

edited

Loading