onnx / tensorflow-onnx

Convert TensorFlow, Keras, Tensorflow.js and Tflite models to ONNX
Apache License 2.0
2.3k stars 432 forks source link

The kernel_shape of keras.layers.Conv1D may be wrong #1807

Open maybeLee opened 2 years ago

maybeLee commented 2 years ago

Describe the bug Not sure if it is the intended behavior. I was using keras to build the model and then convert to ONNX through tf2onnx.convert.from_keras However, I notice the kernel_shape in ONNX is INTS instead of INT when I was trying to convert Conv1D.

System information

To Reproduce

import tensorflow as tf
import tf2onnx

x = tf.keras.layers.Input((32,3))
y = tf.keras.layers.Conv1D(3,3)(x)
model = tf.keras.Model(x,y)
model.summary()
input_shape = model.layers[0].input_shape[0]
spec = (tf.TensorSpec(input_shape, tf.float32, name="input"),)
model, _ = tf2onnx.convert.from_keras(model, input_signature=spec, \
        opset=15, output_path="temp")
print(model.graph.node[2])

You may access the code here: https://colab.research.google.com/drive/19hoAWV79v7eXiAsQ3yL5Zbfg8oNSn4H6?usp=sharing

Screenshots image

It seems that the attribute of Conv1D should be INT with only one int 3, instead of INTS with two ints. This situation also happens in the stride and dilations.

fatcat-z commented 2 years ago

According to the definition of Conv here, kernel_shape should be a list of ints so using INTS is expected.

maybeLee commented 2 years ago

Hi @fatcat-z, thanks for your reply! I now understand that the kernel_shape should be a list of ints. However, I think the problem of the output kernel_shape is that it should be

attribute {
  name: "kernel_shape"
  ints: 3
  type: INTS
}

instead of

attribute {
  name: "kernel_shape"
  ints: 1
  ints: 3
  type: INTS
}

In fact, the reason I post this issue is that I was using a library: onnx2pytorch to convert the ONNX outputted by tf2onnx to PyTorch. However, this library uses the shape of kernel_shape attribute to determine whether the graph node is a Conv1D node or Conv2D node and thereby incorrectly infers the node type to Conv2D node in this scenario: https://github.com/ToriML/onnx2pytorch/blob/89a3fe7b208952760138c550c499fe9c57c35d45/onnx2pytorch/convert/layer.py#L46

For your reference, I notice that PyTorch's onnx.export can convert Conv1D's kernel_shape to:

attribute {
  name: "kernel_shape"
  ints: 3
  type: INTS
}

by running the following scripts:

from torch import nn
import torch
class NeuralNetwork(nn.Module):
    def __init__(self):
        super(NeuralNetwork, self).__init__()
        self.conv1d_stack = nn.Sequential(
            nn.Conv1d(32, 3, 3),
        )
    def forward(self, x):
        out = self.conv1d_stack(x)
        return out

device = "cuda" if torch.cuda.is_available() else "cpu"
model = NeuralNetwork().to(device)
dummy_input = torch.randn(10, 32, 3, device=device)
torch.onnx.export(model, dummy_input, "pytorch.onnx", verbose=True)
import onnx
onnx_model = onnx.load("pytorch.onnx")
print(onnx_model.graph.node)
fatcat-z commented 2 years ago

Good catch, sounds like there are something we can do.

BTW, your contributions are also welcome to resolve this issue!

jtattermusch commented 10 hours ago

I've looked into this a bit and found out that in the tensorflow model graph, the Conv1D is internally represented as Conv2D op, therefore spatial will be 2 in the snippet below (as the node.type will be Conv2D and node.type[-2] simply gets the character before the last)

https://github.com/onnx/tensorflow-onnx/blob/f85e88eecd62949c89b1c455dcd86689b9a05c1c/tf2onnx/onnx_opset/nn.py#L358

So in a sense, generating kernel_shape with 2 dimensions is expected, as that's how many dimensions a Conv2D kernel has. Whether tf2onnx should attempt converting the Conv2D operation to a proper 1D conv is something I don't know.