Bidirectional GRU and SimpleRNN support

[cjermain]

This PR introduces support for Bidirectional GRU and SimpleRNN layers, and resolves #175. It implements a major refactor of the RNN code to simplify the compos-ability of the pieces, so that code can be effectively reused. The bidirectional converter now leverages the code within the lstm, gru, and simplernn modules. This code uses a bidirectional flag to determine support for this feature.

This diff is probably best viewed in split mode. :) Happy to try to split it up if that will help with the review.

• Add Bidirectional tests for GRU and SimpleRNN

[lgtm-com]

This pull request introduces 11 alerts and fixes 1 when merging 7c9aecb into ee2945c - view on LGTM.com

new alerts:

• 6 for Unused import
• 5 for Unused local variable

fixed alerts:

• 1 for Unused local variable

[cjermain]

All of the Bidirectional tests now cover SimpleRNN and GRU, along with LSTM.

@wenbingl, can you review this PR?

[cjermain]

Looks like there is a bug in Python 3.5. I'll circle back on this later.

[wenbingl]

Thanks a lot for the PR.
Python 3.5 failure is related to some code branch of opset < 9. please have a fixing and I'll merge the PR.

[cjermain]

Thanks @wenbingl. In 82035fc I left the setting of initial states for Bidirectional layers as a future task. There is currently no explicit test, but I can add the test and functionality in a separate PR.

The test was failing because of the same issue that we discussed in #400. When I updated the test_Bidirectional_with_bias code, I introduced a batch size > 1 -- so the failure occurred when it tried to use initial hidden states with a fixed batch size of 1. The current code only supports a fixed batch size for opset < 9, which you mentioned was for supporting a specific onnxruntime version. Since we don't have that version in the tests, I'm not sure how to test against it. I would suggest that if it is critical to keep, we add a CI stage of that version of onnxruntime. I can also revisit that in a separate PR when implementing the full support for the hidden initial states.

[wenbingl]

Thanks @wenbingl. In 82035fc I left the setting of initial states for Bidirectional layers as a future task. There is currently no explicit test, but I can add the test and functionality in a separate PR.

The test was failing because of the same issue that we discussed in #400. When I updated the test_Bidirectional_with_bias code, I introduced a batch size > 1 -- so the failure occurred when it tried to use initial hidden states with a fixed batch size of 1. The current code only supports a fixed batch size for opset < 9, which you mentioned was for supporting a specific onnxruntime version. Since we don't have that version in the tests, I'm not sure how to test against it. I would suggest that if it is critical to keep, we add a CI stage of that version of onnxruntime. I can also revisit that in a separate PR when implementing the full support for the hidden initial states.

Yes, I noticed it as well. I remember the test was that opset < 9 would only run on batch_size == 1 case, Can we do it as well?
I know sometimes the backward compatibility is a pain point for us, but we have to do that to avoid to spoil other existing app which built on ours.

[cjermain]

I'm happy to keep backward compatibility. Is there a way that we can simulate the environment (e.g. a specific version of onnxruntime that we can tests against)? Should that be addressed in the follow up PR?

[BarzelS]

Hi @cjermain,
I've tried to convert Bidirectional GRU, and I'm getting this error, it wasn't suppose to work with this PR?
Do I need to set any flag?
I'm using this in order to convert:

onnx_model = keras2onnx.convert_keras(model, model.name, target_opset=9)

Tried higher target_opset but I'm getting:

RuntimeError: The opset 10 conversion not support yet, the current maximum opset version supported is 9.

ValueError: Unsupported class for Bidirectional layer: <class 'tensorflow.python.keras.layers.recurrent_v2.GRU'>

[cjermain]

@DronistB, I started a new issue (#607) to track this -- please see my comments there.