Set kernel_shape attribute on Conv/ConvTranspose in torchlib#2972
Set kernel_shape attribute on Conv/ConvTranspose in torchlib#2972titaiwangms with Copilot wants to merge 4 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR aligns torchlib’s aten::convolution ONNX lowering with the ONNX Conv/ConvTranspose specification by explicitly setting the kernel_shape attribute when it can be derived reliably from the (static) convolution weight tensor shape, and omitting it when the spatial kernel shape is not fully static.
Changes:
- Added
_conv_kernel_shapehelper to derivekernel_shapefromweight.shape[2:]when all spatial dims are statically known. - Updated
_aten_convolution_onnxto passkernel_shapethrough toop.Convandop.ConvTranspose. - Updated
_aten_convolution_complex_onnxto computekernel_shapefor complex weights (excluding the trailing real/imag dimension) and pass it to all Conv/ConvTranspose calls.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2972 +/- ##
==========================================
- Coverage 72.64% 72.63% -0.01%
==========================================
Files 265 265
Lines 32192 32204 +12
Branches 3038 3041 +3
==========================================
+ Hits 23385 23391 +6
- Misses 7776 7779 +3
- Partials 1031 1034 +3 ☔ View full report in Codecov by Harness. |
Review summary (spun up review team: code-reviewer, critical-reviewer, integration-reviewer, qa-tester)Verdict: fix is correct and verified to resolve pytorch/pytorch#190890. QA verificationRan the exact repro from pytorch/pytorch#190890 against this branch:
Code review findingsNo Critical/Major bugs found. Confirmed:
Minor (raised independently by two reviewers): No regression test was added asserting Minor: The complex-conv path ( No blocking issues; the missing test coverage is worth addressing before/after merge but doesn't affect correctness of the fix as verified above. |
@copilot address these |
…kernel_shape tests
Addressed both minor points:
Existing convolution numerical opinfo tests still pass. |
| _testing.assert_onnx_program(onnx_program) | ||
|
|
||
| conv_nodes = [ | ||
| n for n in onnx_program.model_proto.graph.node if n.op_type == "Conv" |
There was a problem hiding this comment.
| n for n in onnx_program.model_proto.graph.node if n.op_type == "Conv" | |
| n for n in onnx_program.model.graph if n.op_type == "Conv" |
Use the IR object
|
|
||
| conv_nodes = [ | ||
| n | ||
| for n in onnx_program.model_proto.graph.node |
Per the ONNX Conv spec,
kernel_shapedescribes the spatial shape of the convolution kernel, but torchlib omitted it, leaving the attribute absent from exported models.Changes
_conv_kernel_shapehelper: deriveskernel_shapefrom the weight's spatial dims (all dims except the leading output/input channel dims). ReturnsNonewhen the spatial shape is not fully static, so the attribute is omitted and left to ONNX inference rather than emitting an incorrect value._aten_convolution_onnx: passeskernel_shapetoop.Convandop.ConvTranspose(coversaten_conv1d/conv2d/conv3d/convolutionand the transpose path)._aten_convolution_complex_onnx: computeskernel_shapefrom the complex weight, excluding the trailing size-2 real/imag dimension, and applies it to all Conv/ConvTranspose calls.Result