fx quant: do not observe bias on F.conv #49623
Conversation
Summary:
Ensures that conv bias is not observed in a `F.conv{n}d` call.
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
[ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit 9a9304c (more details on the Dr. CI page):
❄️ 1 failure tentatively classified as flakybut reruns have not yet been triggered to confirm:
|
Summary:
Ensures that conv bias is not observed in a `F.conv{n}d` call.
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
[ghstack-poisoned]
Summary:
(not ready for review)
Ensures that conv bias is not observed in a `F.conv{n}d` call.
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
[ghstack-poisoned]
Summary:
Ensures that conv bias is not observed in a `F.conv{n}d` call.
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
ghstack-source-id: ecaed70854cde1df3edf0e1f0c309b40f4763cd5
Pull Request resolved: #49623
Summary:
(not ready for review)
Ensures that conv bias is not observed in a `F.conv{n}d` call.
Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: [D25652856](https://our.internmc.facebook.com/intern/diff/D25652856)
[ghstack-poisoned]
Summary:
Ensures that conv bias is not observed in a `F.conv{n}d` call. This should be a small speedup in PTQ, and will change numerics (in a good way) for QAT if someone is using `F.conv`.
Test Plan:
```
python test/test_quantization.py TestQuantizeFxOps.test_conv_functional_bias_not_observed
```
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: [D25652856](https://our.internmc.facebook.com/intern/diff/D25652856)
[ghstack-poisoned]
| torch.nn.functional.conv3d : [2], | ||
| } | ||
|
|
||
| def node_arg_is_bias(node: Node, arg: Any) -> bool: |
There was a problem hiding this comment.
nit: shall we share the code of this function with node_arg_is_weight as well
There was a problem hiding this comment.
I don't feel too strongly. The definition diverges in the next PR though so not sure if worth it.
There was a problem hiding this comment.
lmk if a change is needed here given the context from the next PR, I'm flexible
There was a problem hiding this comment.
sounds good, we can keep the current approach
| quantized_nodes[dim]) | ||
|
|
||
| @skipIfNoFBGEMM | ||
| def test_conv_functional_bias_not_observed(self): |
There was a problem hiding this comment.
optional: can we merge this test with previous test and call it "test_conv"? we have "test_linear" that checks all combinations of is_module/is_not_module and has_rleu/doesn't have relu and relu module/functional. I think that would be a more exhaustive testing.
There was a problem hiding this comment.
hmm, that was intentional, there are benefits when each test case is separate. This allows for faster debugging when chasing down an individual issue (so there is no need to comment out the unrelated test cases in the large loop). Perhaps we can align on a way to both share code and also test one thing at a time, lmk your preference.
There was a problem hiding this comment.
I think we should do exhaustive tests on what we want to support first. in this particular case, the problem is that we did not test functional conv before. Testing for special cases that we encounter in model might be too ad hoc and it will be hard to know what is covered and what is not in the future.
I think we can first make sure our unit test cover all possible modules/ops etc. we want to support, and then if there is any corner case we can cover with these ad hoc tests (typically should be some interaction between ops + qconfig instead of one op). one such example is here: https://github.com/pytorch/pytorch/blob/master/test/quantization/test_quantize_fx.py#L538
There was a problem hiding this comment.
regarding separating test cases I think as long as there is no code duplication it should be OK.
There was a problem hiding this comment.
sure, I made the functional test more comprehensive.
Regarding no code duplication, I think it's good to optimize for in general, but code readability and ease of use is something to consider as well. Some of the current test utils seem to bias on the "minimize code reuse" side while sacrificing readability, at least for people who were not the code authors. Maybe in the future we can consider on ways to do both.
Summary:
Ensures that conv bias is not observed in a `F.conv{n}d` call. This should be a small speedup in PTQ, and will change numerics (in a good way) for QAT if someone is using `F.conv`.
Test Plan:
```
python test/test_quantization.py TestQuantizeFxOps.test_conv_functional_bias_not_observed
```
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: [D25652856](https://our.internmc.facebook.com/intern/diff/D25652856)
[ghstack-poisoned]
Summary:
Ensures that conv bias is not observed in a `F.conv{n}d` call. This should be a small speedup in PTQ, and will change numerics (in a good way) for QAT if someone is using `F.conv`.
Test Plan:
```
python test/test_quantization.py TestQuantizeFxOps.test_conv_functional_bias_not_observed
```
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: [D25652856](https://our.internmc.facebook.com/intern/diff/D25652856)
[ghstack-poisoned]
|
This pull request has been merged in c3a7591. |
Stack from ghstack:
Summary:
Ensures that conv bias is not observed in a
F.conv{n}dcall. This should be a small speedup in PTQ, and will change numerics (in a good way) for QAT if someone is usingF.conv.Test Plan:
Reviewers:
Subscribers:
Tasks:
Tags:
Differential Revision: D25652856