add type annotations to torch.nn.quantized.modules.conv

[guilhermeleobas]

closes gh-49700

No mypy issues were found in the first three entries deleted from mypy.ini:

[mypy-torch.nn.qat.modules.activations]
ignore_errors = True

[mypy-torch.nn.qat.modules.conv]
ignore_errors = True

[mypy-torch.nn.quantized.dynamic.modules.linear]
ignore_errors = True

[facebook-github-bot]

💊 CI failures summary and remediations

As of commit 2f41a83 (more details on the Dr. CI page):

---

• 1/2 failures possibly* introduced in this PR
  • 1/1 non-CircleCI failure(s)
• 1/2 broken upstream at merge base 321b988 from Jan 06 until Jan 07

---

1 job timed out:

• pytorch_linux_bionic_py3_8_gcc9_coverage_test1

---

🚧 1 fixed upstream failure:

These were probably caused by upstream breakages that were already fixed.

Please rebase on the viable/strict branch (expand for instructions)

If your commit is older than viable/strict, run these commands:

git fetch https://github.com/pytorch/pytorch viable/strict
git rebase FETCH_HEAD

Check out the recency history of this "viable master" tracking branch.

• pytorch_linux_bionic_py3_8_gcc9_coverage_test1 from Jan 06 until Jan 07 (11cdb91 - 321b988)
  • 🔁 rerun

---

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 to the (internal) Dr. CI Users group.

This comment has been revised 27 times.

[codecov]

Codecov Report

Merging #49702 (a93a1d6) into master (963f762) will increase coverage by 5.54%.
The diff coverage is 76.92%.

@@            Coverage Diff             @@
##           master   #49702      +/-   ##
==========================================
+ Coverage   75.14%   80.69%   +5.54%     
==========================================
  Files        1891     1895       +4     
  Lines      205325   205464     +139     
==========================================
+ Hits       154291   165795   +11504     
+ Misses      51034    39669   -11365     

[rgommers]

Thanks @guilhermeleobas. This looks good. I made a couple of comments on how the ignores could be avoided, but don't make them (yet) because I'm not sure they're desirable. Let's see what @ezyang and @malfet think.

The PR could also be merged as is, that'd be perfectly fine in practice.

[rgommers]

So the issue is that the subclass __init__ methods have two fewer parameters (no transposed, output_padding) than _ConvNd. The current fix works, but technically the current way the class hierarchy is organized is not correct and the better way to do it would be:

    def __init__(self, in_channels, out_channels, kernel_size, stride=1,
                 padding=0, dilation=1, groups=1, bias=True,
                 padding_mode='zeros'):
        # All subclasses have this signature
        raise NotImplementedError

    def _init(self, in_channels, out_channels, kernel_size, stride,
              padding, dilation,
              transposed, output_padding,
              groups, bias,
              padding_mode='zeros'):
        super(_ConvNd, self).__init__()

and then subclasses should call super(Conv1d, self)._init(

I think that would be a good change, but it is only possible if there are no subclasses of _ConvNd floating around (it's private, but you never know).

Let's see what other reviewers think.

[guilhermeleobas]

Ok, I'll wait the other reviewers before changing this part of the code

[ezyang]

Soooo it is a little tricky. torch.nn._ConvNd is sadly de facto public API; see https://github.com/pytorch/fairseq/blob/master/fairseq/modules/quantization/scalar/modules/qconv.py#L14 for an example. But this is quantized ConvNd, and this code is relatively new and there ought to be less uses. cc @jerryzh168 for more thoughts.

I'm trying to decide if I agree that the alternate version is better.

[ezyang]

I guess the high level question I have is why the subclass init methods have fewer parameters. This seems like a blatant oversight to me.

[rgommers]

I guess the high level question I have is why the subclass init methods have fewer parameters

Because those two parameters are implicitly defined by the dimensionality of subclass - e.g. Conv1d hardcodes transpose, output_padding to False, (0,). It would not make sense for the user to define those when initializing Conv1d.

transpose is always False, so that really doesn't make much sense - probably code just copied from the non-quantized version or something like that.

[ezyang]

OK fair enough. The suggested alternative seems like a reasonable way to shut up the type checker then.

[guilhermeleobas]

I've updated the code to use _init.

[jerryzh168]

I see, are we going to have similar changes in https://github.com/pytorch/pytorch/blob/master/torch/nn/modules/conv.py as well?

[facebook-github-bot]

@walterddr has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[ezyang]

@walterddr I accidentally commandeered the diff, feel free to commandeer it back

[walterddr]

no problem at all @ezyang please go ahead and merge it. :-)

[facebook-github-bot]

@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

[facebook-github-bot]

@ezyang merged this pull request in 55919a4.