Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add unidirectional sequence lstm #11183

Merged
merged 16 commits into from
May 27, 2022
Merged

Add unidirectional sequence lstm #11183

merged 16 commits into from
May 27, 2022

Conversation

SebastianBoblest
Copy link
Contributor

This work has mostly been done by @vdkhoi Khoi Duy Vo from ETAS Gmbh.
We add parser support for UnidirectionalSequenceLSTM layers in tflite.

A question regarding the test:
At the moment it uses a toy model that I store in a repo in my github account.
Should we copy this to the TVM repo or what is the best way to do this?

@vdkhoi
Copy link

vdkhoi commented May 4, 2022

All tests passed

huajsj
huajsj reviewed May 4, 2022
View reviewed changes
tests/python/frontend/tflite/test_forward.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/tflite.py Outdated Show resolved Hide resolved
@SebastianBoblest
Copy link
Contributor Author

Now this fails on windows. I restarted the CI pipeline three times. Does not seem to be related to our changes. Should or could we do anything to resolve this?

@Mousius
Copy link
Member

Mousius commented May 5, 2022

@SebastianBoblestETAS I think this is effecting more than just this PR, I've raised #11220 to track it, please stand by 😸

huajsj
huajsj reviewed May 7, 2022
View reviewed changes
python/tvm/relay/frontend/tflite.py Outdated Show resolved Hide resolved
python/tvm/relay/frontend/tflite.py Outdated Show resolved Hide resolved
huajsj
huajsj approved these changes May 10, 2022
View reviewed changes
Copy link
Contributor

@huajsj huajsj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

code LGTM, thanks @SebastianBoblestETAS, @vdkhoi.

@SebastianBoblest
Copy link
Contributor Author

@Mousius Hi, sorry this is again failing but in a totally different place now.
Should we simply wait or trigger the CI/CD again?

@SebastianBoblest
Copy link
Contributor Author

There is still the issue of the model we use in the tests.
Where should we put it?
@huajsj Is your approval not enough for this to get merged?

@Mousius
Copy link
Member

Mousius commented May 11, 2022

@SebastianBoblestETAS I re-ran the CI yesterday and it looks green, though I don't really know that much about this PR other than the CI issues - maybe @leandron can help get this merged? 😸

@SebastianBoblest
Copy link
Contributor Author

@leandron Hi, could you maybe help with getting this merged?
The issue that still should get resolved however is that our test uses a model that I have stored in a repo in my own github profile.
Other tests seem to use models from public model zoos. But for our case we would prefer a sample model that only has a UnidirectionalSequenceLSTM layer. Should we put it in a data folder in TVM or what would you suggest?

@vdkhoi
Copy link

vdkhoi commented May 12, 2022

@junrushao1994 Hi, could you also have a look on our PR and help to make it merged? Thanks.

@SebastianBoblest
Copy link
Contributor Author

@mbrookhart @jwfromm @Huyuwei @hlu1 @AndrewZhaoLuo @kazum @siju-samuel @srkreddy1238 @FrozenGene
Hi all,
could someone of you help us with this PR?
Sorry to bother all of you, I just looked in CONTRIBUTORS.md for all committers that are familiar with frontends.
Thanks in advance!

@AndrewZhaoLuo AndrewZhaoLuo self-assigned this May 18, 2022
@AndrewZhaoLuo
Copy link
Contributor

Ill take a look tomorrow

@vdkhoi
Copy link

vdkhoi commented May 23, 2022

Ill take a look tomorrow

@AndrewZhaoLuo did you review our PR? We are ready to answer your question.

AndrewZhaoLuo
AndrewZhaoLuo approved these changes May 23, 2022
View reviewed changes
Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for getting back to you late. LGTM!

python/tvm/relay/frontend/tflite.py Outdated Show resolved Hide resolved
@vdkhoi
Copy link

vdkhoi commented May 24, 2022

@Mousius We just add some comments as request from @AndrewZhaoLuo, but the tvm-ci failed again. Could you please support us to restart it?

@AndrewZhaoLuo
Copy link
Contributor

AndrewZhaoLuo commented May 24, 2022 •
edited
Loading

@vdkhoi you can restart CI by pushing an empty commit.

e.g.
git commit --allow-empty "jostle-ci" and git push

@SebastianBoblest
Copy link
Contributor Author

@AndrewZhaoLuo I updated the comment in the function and made it more precise.
The most important difference of the two "unbind" versions is actually that the onnx-Version takes a exp.Call object and the one in tflite a tvm.relay.frontend.tflite.TensorWrapper. So inferring the shape also works differently.

@AndrewZhaoLuo
Copy link
Contributor

AndrewZhaoLuo commented May 25, 2022 •
edited
Loading

Sometimes you also need to rebase on main to solve flaky CI issues :/ . Sorry about that

@SebastianBoblest
Copy link
Contributor Author

@AndrewZhaoLuo I just rebased. Let's hope for the best 😄

@AndrewZhaoLuo
Copy link
Contributor

Thanks! Sorry this took a long time to get merged.

@AndrewZhaoLuo AndrewZhaoLuo merged commit 7766ab2 into apache:main May 27, 2022
@vdkhoi
Copy link

vdkhoi commented May 27, 2022

Thanks! Sorry this took a long time to get merged.

Thank you. Finally :) it done

driazati pushed a commit to driazati/tvm that referenced this pull request May 27, 2022
@driazati
Add unidirectional sequence lstm (apache#11183)
40bde2f 
* UnidirectionalLSTM added

* fixed missing import

* fixed pylint warnings

* black formatted tflite.py

* corrections according to reviewer comments

* fixed black formatting

* just to trigger the CI again

* assertion now tests that there are exactly 24 input tensors.

* black formatted tflite.py

* added explanatory comment regarding unused imports

* removed unused import

* nothing

* nothing

* added some details in a comment about the differences in unbind regarding to the version in common.py

* improved comment on unbind

* fix of black issue
@driazati driazati mentioned this pull request Jul 14, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@vdkhoi vdkhoi vdkhoi left review comments

@AndrewZhaoLuo AndrewZhaoLuo AndrewZhaoLuo approved these changes

@anonymous-me-prog anonymous-me-prog anonymous-me-prog approved these changes

@huajsj huajsj huajsj approved these changes

Assignees

@AndrewZhaoLuo AndrewZhaoLuo

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@SebastianBoblest @vdkhoi @Mousius @AndrewZhaoLuo @huajsj @anonymous-me-prog