-
Notifications
You must be signed in to change notification settings - Fork 430
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
Allow FX graph mode post-training dynamic quantisation of BlurConv2d operations. #1995
Conversation
…vided (i.e., for BlurConv2d). This allows FX graph mode post-training quantisation of models with 2d convolutions replaced by BlurConv2d (issue mosaicml#1985).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Stamping to unblock but will rely on @dblalock's review for correctness.
Let's please wait for Davis' review. I would like that before merging
@BrettRyland We will take a look at it and get back to you soon. Also will check the other quantization error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM; basically just suggested a few local changes to polish it.
The one subtlety I found is that this would change the semantics when the filter spatial size is larger than the input + padding, depending on whether channels
is supplied or inferred. So I suggested just removing the checks and having it always throw at the conv2d in this case.
The "suggestions" aren't super legible since Github won't let them span deletions, but the result is just this:
if channels < 1: # Use dynamic control flow
_, channels, h, w = input.shape
if (filter.shape[0] == 1) and (channels > 1):
# assume filt is already a rank 4 tensor
filter = filter.repeat((channels, 1, 1, 1))
return F.conv2d(input, filter, stride=stride, padding=padding, groups=channels, bias=None)
@BrettRyland Your issue regarding Instead of Feel free to modify PR as suggested above. |
Thanks for this. I've updated the PR with all of the above suggestions other than the one I commented on above. |
@BrettRyland It is indeed a PyTorch quantization issue. I created an issue for PyTorch with a minimal repro (pytorch/pytorch#95492)
|
@dblalock will handle the suggestions in a followup PR. Merging this for the release. |
@dblalock : Just checked all your comments in detail and I don't think anything else is needed for this PR. Please let me know if you think otherwise. |
What does this PR do?
This PR extracts the dynamic control flow operations in
blur_2d
into static control flow operations in theBlurConv2d
class while leaving the dynamic control flow operations in place forBlurMaxPool2d
. This allows FX graph mode post-training dynamic quantisation to be applied to models where blurpool has been applied to convolutions (though not max pooling).There still seems to be an issue with static quantisation however, which I'm unsure of how to fix:
What issue(s) does this change relate to?
This PR partially fixes issue #1985 in that blurpool applied to convolutions uses static control flow and no longer prevents FX graph mode post-training dynamic quantisation. Blurpool applied to max pool continues to use dynamic control flow as before.
Before submitting
pre-commit
on your change? (see thepre-commit
section of prerequisites)