[torchlib] Fix aten_conv3d generating a 2D bias when bias is missing#2935
Open
javierdejesusda wants to merge 1 commit into
Open
[torchlib] Fix aten_conv3d generating a 2D bias when bias is missing#2935javierdejesusda wants to merge 1 commit into
javierdejesusda wants to merge 1 commit into
Conversation
## Problem
The missing-bias branch of aten_conv3d was copy-pasted from
aten_conv3d_complex, so it built the bias shape with
Concat(weight_dim_0, [2]). That produces a rank-2 bias ([out_channels, 2]),
which violates the ONNX Conv spec (B must be 1D, shape [out_channels]) and
can cause downstream issues. aten_conv1d and aten_conv2d already use the
correct pattern.
## Fix
Use the same construction as aten_conv2d / aten_conv1d:
bias_shape = op.Expand(weight_dim_0, op.Constant(value_ints=[1]))
aten_conv3d_complex is left unchanged: its rank-2 bias is correct for the
real/imag pair of a complex tensor.
## Tests
Added test_conv3d_without_bias_produces_1d_bias in
tests/function_libs/torch_lib/e2e_ops_tests.py. It exports a bias-less
nn.Conv3d and asserts the synthesized Conv bias is 1D. assert_onnx_program
alone does not catch the bug because the rank-2 bias broadcasts numerically,
so the test checks the bias rank explicitly. It fails before this change and
passes after.
Fixes microsoft#2931
Author
|
@microsoft-github-policy-service agree |
justinchuby
reviewed
Jun 15, 2026
Comment on lines
+1022
to
+1032
| # The bias synthesized for a bias-less conv must be 1D ([out_channels]) to | ||
| # match the ONNX Conv spec. See https://github.com/microsoft/onnxscript/issues/2931. | ||
| inferred = onnx.shape_inference.infer_shapes(onnx_program.model_proto, data_prop=True) | ||
| shape_ranks = { | ||
| value_info.name: len(value_info.type.tensor_type.shape.dim) | ||
| for value_info in inferred.graph.value_info | ||
| if value_info.type.tensor_type.HasField("shape") | ||
| } | ||
| conv_nodes = [node for node in inferred.graph.node if node.op_type == "Conv"] | ||
| self.assertEqual(len(conv_nodes), 1) | ||
| self.assertEqual(shape_ranks[conv_nodes[0].input[2]], 1) |
Collaborator
There was a problem hiding this comment.
Ok to review this part as assert_onnx_program will already run the model and validate the shapes.
Author
There was a problem hiding this comment.
Before I drop it: the synthesized bias is all zeros, so assert_onnx_program passes even with the rank-2 bias. The conv output is numerically identical, so only the rank check fails (2 != 1) before the fix, which is the part that actually pins this regression. If you'd rather rely on assert_onnx_program for path coverage here, I'll remove the block and the onnx import to match the convention in the suite.
justinchuby
reviewed
Jun 15, 2026
| import math | ||
| import unittest | ||
|
|
||
| import onnx |
Collaborator
There was a problem hiding this comment.
Suggested change
| import onnx |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2935 +/- ##
=======================================
Coverage 72.66% 72.66%
=======================================
Files 259 259
Lines 31748 31748
Branches 3005 3005
=======================================
Hits 23069 23069
Misses 7660 7660
Partials 1019 1019 ☔ View full report in Codecov by Harness. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The
bias is Nonebranch ofaten_conv3dwas copy-pasted fromaten_conv3d_complex, so it buildsthe bias shape with
Concat(weight_dim_0, [2]), producing a rank-2 bias[out_channels, 2]. ONNXConvrequires a 1D bias[out_channels].aten_conv1dandaten_conv2dalready use the correctpattern.
aten_conv3d_complexis left unchanged - its rank-2 bias is correct for the real/imag pair.Adds
test_conv3d_without_bias_produces_1d_bias, which asserts the synthesized Conv bias is rank 1.It fails (
2 != 1) before the fix and passes after. (assert_onnx_programalone misses this becausethe rank-2 bias broadcasts numerically.)
Fixes #2931