[ONNX] LSTM op conversion

This is in regards to the PR that adds LSTM support for ONNX parser - https://github.com/apache/incubator-tvm/pull/4825/files

Currently, the code assumes that the input names of the ONNX LSTM ops are same as listed in at https://github.com/onnx/onnx/blob/master/docs/Operators.md#inputs-3---8

The code in Relay parser is

However, I have a model whose input names are not exactly these

image

image

So, IIUC, we might have misread the ONNX spec. Basically, we should use onnx_input length to understand how many attributes are optional. And then we should use inputs[index_number] directly to parse instead of inputs[“initial_c”] etc.

@jwfromm I have 1 day ONNX experience, not sure if I understood correctly. Let me know what you think here.

cc @adb

The reason we did this with names instead of argument position is that it seems like onnx is not consistent with missing inputs. For example, a layer that has both initial_h and initial_c defined might have them as inputs[5] and inputs[6] respectively. However if only initial_c is defined it would take the spot of initial_h as inputs[5]. As far as I could tell there was no way to tell which optional input a specific input position has without the name. Please correct me if im wrong.

Yes, my understanding is similar. That is, I believe that argument position and attributes can make it confusing to parse.

However, with my limited ONNX experience, input names do not seem to be the solution either because input names might not be necessarily initial_h and initial_c (we might be able to check this quickly by running unit test case in ONNX with different input names).

I am not sure of the ONNX Proto python object. Instead of input names (like inital_h_reshape in my case), is there a way to read the attribute(?) initial_h and make the necessary connections?

So I think you’re right that if the onnx graph generator is following the spec for optional inputs, they need to use empty strings for those not provided unless they are trailing. In that case, we should be able to check input length and get arguments by position and check if they’re empty strings.

Specifically, I’m reading the ONNX guidelines which quote:

Optional Inputs and Outputs
Some operators have inputs that are marked as optional, which means that a referring node MAY forgo providing values for such inputs.

Some operators have outputs that are optional. When an actual output parameter of an operator is not specified, the operator implementation MAY forgo computing values for such outputs.

There are two ways to leave an optional input or output unspecified: the first, available only for trailing inputs and outputs, is to simply not provide that input; the second method is to use an empty string in place of an input or output name.

Each node referring to an operator with optional outputs MUST provide a name for each output that is computed and MUST NOT provide names for outputs that are not computed.

External Tensor Data

We would have to trust that everyone follows this guideline but I guess that’s more likely then them naming the inputs correctly. I’ll update my recent GRU PR to include this fix.

1 Like

Thanks Josh. I will follow the PR and I will also check on my end after your update.

Hey @anijain2305 the PR has been merged, can you check and make sure your import now works?