[Frontend, pytorch] Vc/pytorch lstm #8447

vvchernov · 2021-07-12T08:30:31Z

LSTM layer was supported for pytorch frontend (see [Torch] Support aten::lstm #6474)
Tests for different types of LSTM (unidirectional, bidirectional, stacked, with projection and their combinations) were implemented. Tests are performed for pytorch model and onnx model converted from pytorch

jwfromm · 2021-07-12T16:10:19Z

@masahi can you help review and shepherd this PR?

vvchernov · 2021-07-12T17:58:28Z

Hello @masahi, I have finished my work and waiting for your review and response

emijiayw · 2021-07-12T19:24:53Z

tests/python/frontend/pytorch/test_lstms.py

+        self.device = device
+        self.batch_first = batch_first
+
+        # Network defition


Network definition

yeah let's fix this @vvchernov

masahi · 2021-07-13T00:52:57Z

thanks @vvchernov this is great. Please fix the lint issue.

vvchernov · 2021-07-13T19:05:41Z

@masahi @jwfromm Hello guys, I have fixed all issues related to lint and resolved merge conflicts, but tvm-ci failed. After that I chose main with success status in jenkins (this one https://ci.tlcpack.ai/blue/organizations/jenkins/tvm/detail/main/1292/pipeline) and merged it to my branch. but tvm-ci failed again. In both cases it seems that it is not related to my changings in the code. First Build:GPU failed, currently python3: i386 failed. What do you think about it? May be you can give me a hash with stable commit for main branch

masahi · 2021-07-13T20:20:24Z

@vvchernov Both failures look new to me, they can be flaky tests. Can you try running CI again (rebase and push)?

…upported

…ated by new combination of LSTM types

…orch frontend for case without biases. exception in the test for conversion LSTM with projection from pytorch to ONNX

vvchernov · 2021-07-14T18:57:18Z

@masahi I followed your recommendation and tvm-ci passed successfully. But I observed that my test did not check case where LSTM layer without bias weights. I updated test and checked these cases with combination of other LSTM modifications. Observed bugs were fixed (please review last commit). It seems that all modification of LSTM supported by pytorch are supported on tvm side.

tests/python/frontend/pytorch/test_lstms.py

masahi · 2021-07-17T06:23:48Z

tests/python/frontend/pytorch/test_lstms.py

+    # if err < 1e-6:
+    #     print("SUCCESS: RESULTS ARE THE SAME WITH MAX ERROR {} AND EPSILON {}".format(err, 1e-6))
+    # else:
+    #     print("WARNING: RESULTS ARE NOT THE SAME WITH ERROR {}".format(err))


Remove these comments

masahi · 2021-07-17T06:24:05Z

tests/python/frontend/pytorch/test_lstms.py

+                        dev = tvm.cpu(0)
+                        with tvm.transform.PassContext(opt_level=3):
+                            lib = relay.build(mod, target=target, params=params)
+                    elif format == "onnx":


Remove onnx test

My idea to check modifications of LSTM layer converted from pytorch to onnx is related to that pytorch frontend supports less ML layers than onnx one. It means that complicated neural network contained LSTM can be not supported from pytorch model on tvm side, but onnx model converted from pytorch can be. Moreover there is specific of stacked LSTM support, I do not think that it is tested on onnx frontend side

Ok. Then please address my other comments. Also you can avoid creating a temp directory by writing onnx file to an in-memory buffer, see https://github.com/facebookresearch/detr/blob/master/test_all.py#L136-L140

It does not work for onnx model loading, but I found correct way with io.BytesIO using. done

masahi · 2021-07-17T06:24:39Z

tests/python/frontend/pytorch/test_lstms.py

+    # Create outdir directory to keep temporal files
+    out_dir = Path.cwd().joinpath("output")
+    out_dir.mkdir(exist_ok=True, parents=True)
+    has_proj = "p" in lstm_type


No need to create temp directory if you remove onnx test.

masahi · 2021-07-17T06:24:56Z

tests/python/frontend/pytorch/test_lstms.py

+                    compare(tvm_output, golden_output_batch)
+
+    # Remove output directory with tmp files
+    shutil.rmtree(out_dir)


Remove it with onnx test

masahi · 2021-07-17T06:25:20Z

tests/python/frontend/pytorch/test_lstms.py

+                        # Model compilation by tvm
+                        target = tvm.target.Target("llvm", host="llvm")
+                        dev = tvm.cpu(0)
+                        with tvm.transform.PassContext(opt_level=3):


Please test on cuda too.

masahi · 2021-07-17T07:21:34Z

@vvchernov Don't forget #8447 (comment)

…ng tmp dir was removed. remove unneccessary comments

masahi · 2021-07-20T12:37:09Z

Thanks @vvchernov !

masahi · 2021-07-29T00:36:28Z

@vvchernov I was informed that the test added in this PR is taking more than 20 min of CI time. Can you reduce the number of test cases?

See https://ci.tlcpack.ai/job/tvm/job/main/1356/testReport/cython.tests.python.frontend.pytorch/

cheimu · 2021-09-16T09:55:56Z

Hi there. I found this error still exist. I just clone this repo yesterday

vvchernov · 2021-09-19T10:14:29Z

Hello @cheimu! Dropout is not supported on TVM side. Therefore LSTM with dropout does not work. In general it is so because some people think that dropout is used for training only, but I know that for some TTS model it can be used for inference. Unfortunately the current state is that dropout op is dummy, it has attributes but does not have compute implementation. I do not know when dropout will be supported on TVM side.

* lstm layer conversion to relay from pytorch model (TorchScript) was supported * bidirectional LSTM layer was supported for pytorch API * lstm tests were implemented. fixes in pytorch lstm * fix pytorch bidirectional lstm. update test comment * black format and some small fixes * LSTM with projection was supported for pytorch frontend. test was updated by new combination of LSTM types * lint fixes * add bias switcher for LSTM types test. fix LSTM implementation in pytorch frontend for case without biases. exception in the test for conversion LSTM with projection from pytorch to ONNX * transfer test_lstms to pytest format * onnx model saving was implemented through io.BytesIO. creating/removing tmp dir was removed. remove unneccessary comments * gpu target was added to the test Co-authored-by: Valery Chernov <[email protected]>

jwfromm requested a review from masahi July 12, 2021 16:10

vvchernov changed the title ~~WIP: Vc/pytorch lstm~~ Vc/pytorch lstm Jul 12, 2021

emijiayw reviewed Jul 12, 2021

View reviewed changes

masahi self-assigned this Jul 12, 2021

vvchernov force-pushed the vc/pytorch_lstm branch 3 times, most recently from e7c5ea2 to 883c261 Compare July 13, 2021 07:47

vvchernov added 7 commits July 13, 2021 23:41

lstm layer conversion to relay from pytorch model (TorchScript) was s…

e119ef3

…upported

bidirectional LSTM layer was supported for pytorch API

306be52

lstm tests were implemented. fixes in pytorch lstm

f598bce

fix pytorch bidirectional lstm. update test comment

367c5fe

black format and some small fixes

57f7473

LSTM with projection was supported for pytorch frontend. test was upd…

c73b80d

…ated by new combination of LSTM types

lint fixes

c976a8e

vvchernov force-pushed the vc/pytorch_lstm branch from a272930 to c976a8e Compare July 13, 2021 20:43

add bias switcher for LSTM types test. fix LSTM implementation in pyt…

ee14d93

…orch frontend for case without biases. exception in the test for conversion LSTM with projection from pytorch to ONNX

masahi reviewed Jul 15, 2021

View reviewed changes

tests/python/frontend/pytorch/test_lstms.py Outdated Show resolved Hide resolved

vvchernov force-pushed the vc/pytorch_lstm branch 2 times, most recently from 65a8607 to fc4ac84 Compare July 16, 2021 11:22

transfer test_lstms to pytest format

67d380e

vvchernov force-pushed the vc/pytorch_lstm branch from fc4ac84 to 67d380e Compare July 16, 2021 17:10

masahi reviewed Jul 17, 2021

View reviewed changes

vvchernov changed the title ~~Vc/pytorch lstm~~ [Frontend, pytorch] Vc/pytorch lstm Jul 18, 2021

onnx model saving was implemented through io.BytesIO. creating/removi…

16964e5

…ng tmp dir was removed. remove unneccessary comments

vvchernov force-pushed the vc/pytorch_lstm branch from a944157 to 16964e5 Compare July 19, 2021 11:02

gpu target was added to the test

a5396a4

masahi approved these changes Jul 20, 2021

View reviewed changes

masahi merged commit d5818b4 into apache:main Jul 20, 2021

masahi mentioned this pull request Jul 20, 2021

[Torch] Support aten::lstm #6474

Closed

denise-k mentioned this pull request Jul 30, 2021

[TEST] test_lstms taking 20+ mins on CI runs #8608

Closed

vvchernov deleted the vc/pytorch_lstm branch August 18, 2021 18:36

junrushao mentioned this pull request Nov 1, 2021

Apache TVM v0.8 Release Note Candidate #9416

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Frontend, pytorch] Vc/pytorch lstm #8447

[Frontend, pytorch] Vc/pytorch lstm #8447

vvchernov commented Jul 12, 2021 •

edited

Loading

jwfromm commented Jul 12, 2021

vvchernov commented Jul 12, 2021

emijiayw Jul 12, 2021

masahi Jul 15, 2021

vvchernov Jul 19, 2021

masahi commented Jul 13, 2021

vvchernov commented Jul 13, 2021

masahi commented Jul 13, 2021

vvchernov commented Jul 14, 2021

masahi Jul 17, 2021

vvchernov Jul 19, 2021

masahi Jul 17, 2021

vvchernov Jul 17, 2021

masahi Jul 17, 2021

vvchernov Jul 19, 2021 •

edited

Loading

masahi Jul 17, 2021

vvchernov Jul 19, 2021

masahi Jul 17, 2021

vvchernov Jul 19, 2021

masahi Jul 17, 2021

vvchernov Jul 19, 2021

masahi commented Jul 17, 2021

masahi commented Jul 20, 2021

masahi commented Jul 29, 2021 •

edited

Loading

cheimu commented Sep 16, 2021

vvchernov commented Sep 19, 2021

[Frontend, pytorch] Vc/pytorch lstm #8447

[Frontend, pytorch] Vc/pytorch lstm #8447

Conversation

vvchernov commented Jul 12, 2021 • edited Loading

jwfromm commented Jul 12, 2021

vvchernov commented Jul 12, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masahi commented Jul 13, 2021

vvchernov commented Jul 13, 2021

masahi commented Jul 13, 2021

vvchernov commented Jul 14, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vvchernov Jul 19, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

masahi commented Jul 17, 2021

masahi commented Jul 20, 2021

masahi commented Jul 29, 2021 • edited Loading

cheimu commented Sep 16, 2021

vvchernov commented Sep 19, 2021

vvchernov commented Jul 12, 2021 •

edited

Loading

vvchernov Jul 19, 2021 •

edited

Loading

masahi commented Jul 29, 2021 •

edited

Loading