Fixed group attribute in convolution op

[javidcf]

Also minor change reading shape dimensions so it works for different dimensionalities.
Fixes #2084

[fatcat-z]

The code LGTM, thanks for your contributions.

Could you please add a test (https://github.com/onnx/tensorflow-onnx/blob/main/tests/test_backend.py) to cover this?

[javidcf]

I wrote this test case for this:

@check_opset_min_version(11, "Pad")
def test_conv_unknown_kernel_channels(self):
    x_shape = [2, 10, 3]
    x_val = make_xval(x_shape)
    kernel_shape = [4, 3, 5]
    kernel_val = make_xval(kernel_shape)
    pad_val = np.array([[0, 0], [0, 0], [0, 0]], np.int64)
    def func(x, kernel, pad):
        # Make kernel dimensions unknown
        kernel = tf.pad(kernel, pad)
        conv = tf.nn.conv1d(x, kernel, stride=[1], padding='VALID')
        return tf.identity(conv, name='output')
    self._run_test_case(func, [_OUTPUT], {_INPUT: x_val, _INPUT1: kernel_val, _INPUT2: pad_val})

Unfortunately, in the process of doing so I found another bug (already present before the fix) with convolution kernels with unknown shape. My fix solves the issue of the wrong value in the op group attribute, but attempting to run the above test function with ONNX (after my fix) results in this error:

[ONNXRuntimeError] : 1 : FAIL : Non-zero status code returned while running Conv node. Name:'conv1d' Status Message: kernel_shape is not compatible with W shape. kernel_shape: {1,-1} W: {5,3,1,4}

The error can also be reproduced before my fix by simply adding the following line before the call to tf.nn.conv1d:

x = tf.pad(x, pad)

The problem is I think this is actually an onnxruntime issue. This line from onnxruntime seems to suggest the code is not properly checking that the dimensions are known (not -1), or maybe kernel_shape_specified should be false in that case...

Either way, I'm not sure if a different test should be added which somehow just checks the attribute value with a graph_validator without trying to run the operation (which I'm not sure if it's possible, since ops not used by the result get pruned) or leave the test as it is and keep this "on hold" until the other issue is resolved.

EDIT: Another possibility is that the kernel_shape attribute of the Conv operation should always be fully defined. The operator spec does not explicitly say whether partially defined shapes are allowed or not.
EDIT 2: Submitted issue microsoft/onnxruntime#14869 about it.
EDIT 3: Seems that -1 is indeed not allowed in kernel_shape, so tf2onnx should not generate the attribute (which is optional for convolution operations anyway) if any of its values is not known. I'll add that fix.

[javidcf]

Apologies, I had deleted this branch by mistake.

[fatcat-z]

Could you please resolve the conflict in test_backend.py file?

[fatcat-z]

Thanks for your contributions!