Feature request: reverse & bidirectional LSTM

[robinvanemden]

Currently, only forward LSTM has been implemented. Reverse and, particularly, bidirectional LSTM would be useful for OCR type applications.

[robinvanemden]

A link to a first version of Python code that generates LSTM ONNX models with testable input/output, based on ONNXruntime's source code:

https://www.dropbox.com/s/n86jjm7iax9vgq6/lstm_test_generator.zip?dl=1

[kraiskil]

Thanks for the zip, looks like a nice test generating script.

When implementing the LSTM I left out all but the plain version since I could not find a description on what the alternative versions compute (so I thought they were somewhat academic and not widely used). Would you have a link to recommend that describes these variants?

[robinvanemden]

In practice, the bidirectional version of LSTM actually seems to be used most often. It appears, for instance, in most OCR related NN's, such as the popular EasyOCR and PaddleOCR toolkits.

To further illustrate the use of bidirectional LSTM in these toolkits, I created an ONNX version of a PaddlePaddle OCR model which can be downloaded here. See the bidirectional LSTM operator near the end of the graph.

[robinvanemden]

Also, some links with more information on bidirectional LSTM: https://paperswithcode.com/method/bilstm https://www.dropbox.com/s/axlhbxv04rgmqt0/bidirect.pdf?dl=1 https://machinelearningmastery.com/develop-bidirectional-lstm-sequence-classification-python-keras/ https://stackoverflow.com/questions/44858147/merge-a-forward-lstm-and-a-backward-lstm-in-keras And a nice implementation in NCNN: https://github.com/Tencent/ncnn/blob/13d12da245e515031d35455a68701b254870a760/src/layer/lstm.cpp

[kraiskil]

Thanks for the links. Reading the part "In problems where all timesteps of the input sequence are available" unclogged my thinking :) The one project I did with LSTMs didn't have this future data available, and I guess this got me confused...

So, please correct me if I am wrong: from an onnx2c LSTM-node implementation point-of-view a bidirectional LSTM works like two independent LSTMs in parallel, working on (momentarily) different data. The reversing and de-reversing of the reverse-lane input to the LSTM is done by the surrounding network, and the bidirectional LSTM node itself computes values just like a forward-only node?

Would the reverse (a.k.a. backwards)-only LSTM node then be implemented as a forward-only node with the input data reversed? Or is there something I miss, and computing this would need to be a bidirectional LSTM where the forward-lane data results are ignored?

[robinvanemden]

Would the reverse (a.k.a. backwards)-only LSTM node then be implemented as a forward-only node with the input data reversed?

Exactly! For bidirectional LSTM, you then merge the forward and reverse LSTM's, as illustrated in the anotated Keras example here:

https://stackoverflow.com/a/62994263 ... also see the subsequent answer on that page: https://stackoverflow.com/a/62991158

[kraiskil]

I took a closer look at the zip, and it looks like that is a forward-only lstm. Did I misunderstand something, or is that work-in-progress?

[robinvanemden]

If I am not mistaken, you should be able to generate a bidirectional LSTM graph by setting the "bidirectional" argument of the "generate_model" call in the Python script to True. I am currently outdoors, but will double check first thing tomorrow! I can also pregenerate some tests, if that would be useful.

[kraiskil]

Ah, right. Thanks, and no hurry :D Onnxruntime didn't install out-of-the-box for me - I haven't used it before - so I'll have a look at generating models later.

BTW, I had a quick look, and seems onnx doesn't have any bidirectional LSTM backend tests. It would be nice to have some from upstream, but I don't know how much of a learning curve this requires...

[robinvanemden]

I found a relevant pull request at the official onnx/onnx repo, which ought to have been merged some time ago. I have made the LSTM tests available here: https://www.dropbox.com/sh/ibg0w5jscuouas6/AADINF8EOf67iGHPImwpsn7ya?dl=0

[kraiskil]

Indeed, that commit does mention a file onnx/backend/test/data/node/test_lstm_bidirectional/model.onnx as an added one, but hat pull request is still 'open'. There is no such file in current onnx master yet. Oh well, local tests will do. Thanks for the extensive set :)

[kraiskil]

Please note: the branch reference above is work-in-progress, and its history will be re-written when merged into master. Also it does not yet implement bidirectional LSTM.

Turns out there were quite a few corners of LSTM that were not implemented...

[kraiskil]

@robinvanemden bidirectional LSTM is implemented in branch bidirectional_lstm. It passes with the single bidirectional test from the onnx pull request you linked to. Please test if you have other code easily available. I'll have a look at that python script that generates tests cases with onnxruntime.

I also suspect the reverse-only LSTM code is broken, and that the reverse-test might be broken too. Reverse is now calculated in the LSTM forward lane, but the test passes...

[robinvanemden]

@robinvanemden bidirectional LSTM is implemented in branch bidirectional_lstm

Super! I will take a look later today. I will also check the reverse test, and fix it if it's broken. And if that's of use to you, I can generate additional tests with the Python script and share them with you.

[kraiskil]

Thanks. And more tests are of course welcome. I'd really like to see more "official" tests from onnx too, but so far they have not been that comprehensive.

There was still one problem with the number sequence lengths and batch sizes. I had to disable the lstm_peephole test because it failed. This could well be a bug in onnx2c and not the test - beware of chasing red herrings :)

[robinvanemden]

Thanks again for your LSTM implementation, and my apologies that I have not yet created the additional tests. I will generate them this weekend!

[kraiskil]

Don't worry about that :) There is a test for the feature now, and unless you have a test or use case that does not work, I'm ready to merge in the bidirectional_lstm branch into master.

[robinvanemden]

I see you just added the "clip" functionality plus its test as well, super! Would it be of use for me to take a closer look at the three commented out tests? Could they be implemented as well? Aside from those three tests (that might not be relevant anyway), this branch seems more than merge-worthy 😄

[kraiskil]

Yes, please, have a look. I commented them out since they don't compile and - to me - it seems they are against the ONNX specification. My intention was to remove those tests... since they are from a pull request to ONNX that was not accepted and for which tests didn't pass. So perhaps those tests failed because of the same problems I was seeing?

lstm_with_initial_c and lstm_with_initial_h seem to have the B tensor of rank 3 - specification requires a rank of 2. lstm_with_initial_c_and_h fails in codegen already with the sequence_lens not being of rank 1. I didn't look into this since I'm not quite sure what the required functionality when giving sequence_lens is anyway. This is maybe one test & implementation that should be added. I'm not sure how common variable length sequences within a batch are, though...

[kraiskil]

I squash-merged the bidirectional_lstm branch with some cleanups into master. I'm letting the branch hang around for a while still "just in case". Thanks for your help @robinvanemden

[robinvanemden]

I squash-merged the bidirectional_lstm branch with some cleanups into master. I'm letting the branch hang around for a while still "just in case". Thanks for your help @robinvanemden

Thanks again for your Bidirectional LSTM implementation! I have been busier than expected the last few weeks. But I hope to find some time in the near future to delve a little further into the remaining tests.