@fmassa I initially did it this way as it seems that Soumith implied that it should be separate from the functional interface: #3867 (comment)

I continued doing it this way because it seemed like there already were differences between the functional and module interfaces. For example, the module interface supports an argument padding_mode that can be circular or zeros. I assumed that things were done this way as circular padding also requires a separate F.pad before the conv is called.

Putting it in the functional interface also requires delving substantially deeper into the darker sides of Pytorch, and it wasn't clear to me where the appropriate location was, nor whether it would break something to have one op actually become 2 ops.

Uhhh are we sure we want this? I remember @Yangqing strongly arguing against adding this option as it's often quite surprising.

@apaszke Do you remember any of the reasons he gave for strongly warning against this?

@soumith mentioned that dynamically calculating padding is bad for a "variety of efficiency/serialization reasons", but these concerns don't seem relevant for research users.

On the other hand, for research users this is an unnecessary pain point. I've had multiple people complain that this isn't in Pytorch. I'm not convinced that the performance/serialization issues are worth the cost here.

yes, we want this. YQ's concerns were around exported models not being static, and the ONNX export pass needs to error out if the padding info cannot be statically determined (@Chillee you might have to add a test for the ONNX export).

At this point, the user need and benefits outweigh not wanting it, and our JIT / production export now can comfortably handle dynamic models (it wasn't true when we last thought about this)

@Chillee I wasn't aware that circular padding was added only in the nn.Module interface.

I'm less sure we want to diverge the functionalities of both the module and functional interfaces, because some things are only possible to be done with the functional interface (for example, when we want to have the weights of a model to be the output of a network).

But I'll let @soumith and @apaszke chime in on their views

my main thinking around matching everything in the functional interface was around how much state we can carry to reuse previous calculations. We can carry state across forward calls in the Module, but not in the Functional interface.

I didn't explore the line of thought around how much state (such as pre-calculating padding values etc.) can be carried across iterations and how much benefits it brings.

@fmassa One issue with putting it in the functional interface is that the semantics around auto-calculating "static padding" become fairly muddled. Currently, we can auto-calculate static padding and have it for free if certain rules are followed (stride=1, (filter-1)*dilation is divisible by 2).

It's not clear how we would do this in the functional interface.

I think the right place to add this in the functional interface would be in the F.pad function or something along those lines.

Added tests and fixed JIT compatibility. Removed the WIP tag - I think this is ready for review.

@Chillee

One issue with putting it in the functional interface is that the semantics around auto-calculating "static padding" become fairly muddled. Currently, we can auto-calculate static padding and have it for free if certain rules are followed (stride=1, (filter-1)*dilation is divisible by 2).

I don't quite get this. The overhead of calculation of if the padding is static or not is negligible wrt the convolution time, so doing this in the functional codepath would not be a problem at all.

It's not clear how we would do this in the functional interface.

Here is an example

def _conv2d(...):
     # same as conv2d now

def conv2d(...):
    is_static, padding = _compute_if_padding_is_static(...)
    if is_static:
        input = pad(input, padding, etc)
        return _conv2d(input, ...)
    return _conv2d(input, ...)

I think the right place to add this in the functional interface would be in the F.pad function or something along those lines.

I actually think we should instead add support for different paddings on left / right / top / bottom. This would be a generalization of what we currently have.
But this is a bit more involved to do.

@fmassa I guess it wasn't clear to me what "static padding" actually meant. If that's still considered static padding, then I'm fine with doing it in the functional interface. Would you be able to provide a pointer to what I should be changing?

Also, as a side note, the documentation for F.conv2d isn't quite correct: https://pytorch.org/docs/stable/nn.html#id22

It says it supports padding_mode but it doesn't.

yes, we want this. YQ's concerns were around exported models not being static, and the ONNX export pass needs to error out if the padding info cannot be statically determined (@Chillee you might have to add a test for the ONNX export).

At this point, the user need and benefits outweigh not wanting it, and our JIT / production export now can comfortably handle dynamic models (it wasn't true when we last thought about this)

I agree with @soumith 's comment in that ONNX Conv op will support similar semantics as same being here, with the auto_pad option SAME_UPPER. This option was previously on path to deprecation, but due to clear use cases has since been retained in the design. Given that I feel that this can be handled for ONNX export.

@Chillee do you want someone else to take on finishing this ? It would be nice to get this in for 1.3

@eellison I've been busy (and will be busy) until the 23rd, but I should be able to finish it up in the week after that (which should be at least a week before 1.3?).

I think the cut is the 24th actually... although potentially we could cherry-pick this in

Ah alright I'll take a crack tonight at fixing it then.

@ZolotukhinM @jamesr66a @raghuramank100 this PR breaks convolution tests. Do you think it's doable to land this PR before the release and have quant parity with it ?

To clarify a bit more, the tests that break are the ones that look like (the last line specifically).

        FileCheck().check('ClassType<Observer> = prim::GetAttr[name="observer_for_') \
                   .check_next('prim::CallMethod[name="forward"](%observer_for_') \
                   .check('ClassType<Observer> = prim::GetAttr[name="observer_for_') \
                   .check_next('prim::CallMethod[name="forward"](%observer_for_') \
                   .check('Tensor = aten::conv2d') \

The tests pass as long as I comment that out - it's not clear to me if that's the right thing to do.

@Chillee quantization has a mirror'd conv implementation so to maintain parity they have to do more than fix the tests. Since the branch cut is today/tomorrow quant people (cc @jamesr66a) think it would be better to wait until after the cut to land rather than rush in a quant fix.

I forgot that quantization needed to maintain parity when I commented above about getting it into the release, that's my bad.

Not that it matters, but I have a python notebook detailing the design choices:

https://gist.github.com/Yangqing/47772de7eb3d5dbbff50ffb0d7a98964

As an architect, I still believe that the SAME padding is not something that we should introduce back into the common usage patterns. This being said, I was the person that included SAME paddings in ONNX for compatibility reasons, so I can understand why one might want it.

I somehow missed the notification 16 days ago, sorry.

To clarify a bit more, the tests that break are the ones that look like (the last line specifically).

        FileCheck().check('ClassType<Observer> = prim::GetAttr[name="observer_for_') \
                   .check_next('prim::CallMethod[name="forward"](%observer_for_') \
                   .check('ClassType<Observer> = prim::GetAttr[name="observer_for_') \
                   .check_next('prim::CallMethod[name="forward"](%observer_for_') \
                   .check('Tensor = aten::conv2d') \

The tests pass as long as I comment that out - it's not clear to me if that's the right thing to do.

Could you please paste what graph do we actually have there? To print it, you could add

print(str(m._c._get_module("conv")._get_method('conv2d_forward').graph))

before the FileCheck invocation.

I'm pretty sure this test can be adjusted to the changes you're doing - it basically just checks that after the transformation we have instrumentation calls to "observers" before the conv call that we're instrumenting. It essentially looks for specific strings and my guess is that conv now looks slightly differently.

@Chillee , @ZolotukhinM @jamesr66a what can we do to land this

Sorry I've been busy recently - will get back to this within a week or two.

Somebody else pushed it across the finish line when I couldn't :) #45667