[reland][quant] Remove nn.quantized.ReLU module and nn.quantized.functional.relu (#47415) #48038
Conversation
…tional.relu (#47415) Summary: nn.ReLU works for both float and quantized input, we don't want to define an nn.quantized.ReLU that does the same thing as nn.ReLU, similarly for nn.quantized.functional.relu this also removes the numerical inconsistency for models quantizes nn.ReLU independently in qat mode Test Plan: Imported from OSS Reviewed By: z-a-f [ghstack-poisoned]
…ntized.functional.relu (#47415)" Summary: nn.ReLU works for both float and quantized input, we don't want to define an nn.quantized.ReLU that does the same thing as nn.ReLU, similarly for nn.quantized.functional.relu this also removes the numerical inconsistency for models quantizes nn.ReLU independently in qat mode Test Plan: Imported from OSS Reviewed By: z-a-f Differential Revision: [D25000462](https://our.internmc.facebook.com/intern/diff/D25000462) [ghstack-poisoned]
| if type(mod) == torch.nn.intrinsic.qat.ConvBnReLU1d: | ||
| mod.weight, mod.bias = fuse_conv_bn_weights( | ||
| mod.weight, mod.bias, mod.bn.running_mean, mod.bn.running_var, | ||
| mod.bn.eps, mod.bn.weight, mod.bn.bias) |
There was a problem hiding this comment.
thoughts about moving this to a separate PR to minimize risk?
| @@ -21,11 +21,9 @@ def _get_name(self): | |||
|
|
|||
| @classmethod | |||
| def from_float(cls, mod): | |||
| activation_post_process = mod.activation_post_process | |||
There was a problem hiding this comment.
any chance you could expand on the reasoning for this change?
There was a problem hiding this comment.
sure, previous code is relying on the fact the we attach activation_post_process to ReLU inside the sequential, but this PR removes activation_post_process for ReLU, so we changed the insert observer logic to add activation_post_process to sequential directly so we don't need to get activation_post_process from relu any more
activation_post_process = mod[1].activation_post_process
this is getting activation_post_proceess from relu.
There was a problem hiding this comment.
makes sense, thanks! maybe we can add a quick comment to the code?
There was a problem hiding this comment.
will add a comment to the place we attach activation_post_process for _FusedModule (in quantization/quantize.py)
| @@ -137,14 +136,18 @@ def insert_activation_post_process(m, special_act_post_process=None): | |||
| m._forward_hooks.move_to_end(handle.id, last=False) | |||
|
|
|||
| for name, child in module.named_children(): | |||
There was a problem hiding this comment.
would it make sense to add a stronger test for this section to help catch future issues? For example, something like a model which has modules going through all of the branches in these if statements, passing some data through it, and verifying that every observer has seen values?
There was a problem hiding this comment.
I think we do have tests for each of the branches, although it's probably spread out in different op tests.
There was a problem hiding this comment.
if we make an artificial model, not sure it would be runnable or not actually.
There was a problem hiding this comment.
it doesn't have to be this PR (since we are adding an fb-only test), but seems like just increasing integration model coverage for matching numerics between eager and FX would be valuable. It seems like TestQuantizeFxModels has various models commented out, perhaps we can invest in increasing coverage a bit.
There was a problem hiding this comment.
yeah I think we should add more e2e tests as well
…ntized.functional.relu (#47415)" Summary: nn.ReLU works for both float and quantized input, we don't want to define an nn.quantized.ReLU that does the same thing as nn.ReLU, similarly for nn.quantized.functional.relu this also removes the numerical inconsistency for models quantizes nn.ReLU independently in qat mode Test Plan: Imported from OSS Reviewed By: z-a-f Differential Revision: [D25000462](https://our.internmc.facebook.com/intern/diff/D25000462) [ghstack-poisoned]
…ntized.functional.relu (#47415)" Summary: nn.ReLU works for both float and quantized input, we don't want to define an nn.quantized.ReLU that does the same thing as nn.ReLU, similarly for nn.quantized.functional.relu this also removes the numerical inconsistency for models quantizes nn.ReLU independently in qat mode Test Plan: Imported from OSS Reviewed By: z-a-f Differential Revision: [D25000462](https://our.internmc.facebook.com/intern/diff/D25000462) [ghstack-poisoned]
💊 CI failures summary and remediationsAs of commit be090d7 (more details on the Dr. CI page):
🚧 6 ongoing upstream failures:These were probably caused by upstream breakages that are not fixed yet:
ci.pytorch.org: 2 failed
This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.Please report bugs/suggestions on the GitHub issue tracker or post in the (internal) Dr. CI Users group. This comment has been revised 9 times. |
…ntized.functional.relu (#47415)" Summary: nn.ReLU works for both float and quantized input, we don't want to define an nn.quantized.ReLU that does the same thing as nn.ReLU, similarly for nn.quantized.functional.relu this also removes the numerical inconsistency for models quantizes nn.ReLU independently in qat mode Test Plan: Imported from OSS Reviewed By: z-a-f Differential Revision: [D25000462](https://our.internmc.facebook.com/intern/diff/D25000462) [ghstack-poisoned]
|
This pull request has been merged in 8aaca4b. |
Stack from ghstack:
Summary:
nn.ReLU works for both float and quantized input, we don't want to define an nn.quantized.ReLU
that does the same thing as nn.ReLU, similarly for nn.quantized.functional.relu
this also removes the numerical inconsistency for models quantizes nn.ReLU independently in qat mode
Test Plan: Imported from OSS
Reviewed By: z-a-f
Differential Revision: D25000462