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.

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

[Frontend][ONNX] Support ONNX Scan operator #9438

Merged
merged 8 commits into from
Dec 9, 2021
Merged

Conversation

shengxinhu
Copy link
Contributor

support onnx scan operator

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.

LGTM

dtype = output_node.checked_type.dtype
scan_output_vars.append(_expr.var(name, shape=shape, dtype=dtype))
scan_output_init.append(_op.zeros(shape, dtype))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems has a more elegant method(similar as operator Loop existing implementation):
scan_output_vars.append(_expr.var(name, shape=_ty.Any() * len(shape), dtype=dtype))
scan_output_init.append(_op.rehape(_expr.const(np.array[]).astype(dtype)), [0] + [1]*(len(shape)-1))
so the following _op.strided_slice could be removed, as the scan_output_init is empty.
But I did not make it work, could you take a look?

@mbrookhart
Copy link
Contributor

mbrookhart commented Dec 2, 2021

@shengxinhu My apologies, I missed this one, I should have reviewed it three weeks ago, it seems. Any chance you can rebase to resolve the conflicts?

Edit: NVM, it was a simple "two prs added lines to the end of the test list" issue, I could fix that in the GUI

@masahi
Copy link
Member

masahi commented Dec 7, 2021

The change seems to have a lot of duplication with Loop converter. Can we clean things up?

@shengxinhu
Copy link
Contributor Author

Hi @masahi , I didn't get your point of duplication with loop converter, could you point it out sepecifically?

@masahi
Copy link
Member

masahi commented Dec 9, 2021

I mean, just looking at the two implementations, it's obvious that you started by copy-pasting Loop converter (even comments are identical). Can we refactor common code into a helper function, or is it difficult to cleanly extract common bits?

This is not just for usual code-hygiene reason. I believe there are some issues in Loop converter in preserving static shape information. I've never seen a practical ONNX model with Loop op that can be successfully compiled by TVM - I've seen / heard that ONNX models converted from TF ssd mobilenet v1, MaskRCNN, and mlperf DLRM has Loop op, all of them failed to compile with TVM. So I don't want to see the Loop converter implementation in question duplicated for other ops.

See for example this issue regarding ssd mobilenet v1 #8284 (comment) Note that the original tf ssd mobilenet v1 model compiles and runs fine when imported directly by TF frontend, so I can only assume that this is a ONNX converter issue.

@shengxinhu
Copy link
Contributor Author

Hi @masahi , thanks for your explanations. I indeed copy some code in Loop converter implementation. I think only the get_var function could be extracted. do you think is it necessary? If Loop is questionable, Scan is likely to be the same. I also have not seen an ONNX model with scan operator, I realized it just for a task assigned to me.

@masahi
Copy link
Member

masahi commented Dec 9, 2021

ok, no problem.

@masahi masahi merged commit ee629b1 into apache:main Dec 9, 2021
mikepapadim pushed a commit to mikepapadim/tvm that referenced this pull request Dec 9, 2021
* [Frontend][ONNX] Support ONNX Scan operator

* fix lint

* remove test_scan_sum in unsupported_onnx_tests

* support scan opset 8

* fix lint

* fix negative axes bug

* fix lint

Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 7, 2022
* [Frontend][ONNX] Support ONNX Scan operator

* fix lint

* remove test_scan_sum in unsupported_onnx_tests

* support scan opset 8

* fix lint

* fix negative axes bug

* fix lint

Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 11, 2022
* [Frontend][ONNX] Support ONNX Scan operator

* fix lint

* remove test_scan_sum in unsupported_onnx_tests

* support scan opset 8

* fix lint

* fix negative axes bug

* fix lint

Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
yangulei pushed a commit to yangulei/tvm that referenced this pull request Jan 12, 2022
* [Frontend][ONNX] Support ONNX Scan operator

* fix lint

* remove test_scan_sum in unsupported_onnx_tests

* support scan opset 8

* fix lint

* fix negative axes bug

* fix lint

Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
ylc pushed a commit to ylc/tvm that referenced this pull request Jan 13, 2022
* [Frontend][ONNX] Support ONNX Scan operator

* fix lint

* remove test_scan_sum in unsupported_onnx_tests

* support scan opset 8

* fix lint

* fix negative axes bug

* fix lint

Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
qsqqsqqsq-intellif pushed a commit to qsqqsqqsq-intellif/tvm that referenced this pull request Apr 29, 2022
* [Frontend][ONNX] Support ONNX Scan operator

* fix lint

* remove test_scan_sum in unsupported_onnx_tests

* support scan opset 8

* fix lint

* fix negative axes bug

* fix lint

Co-authored-by: Matthew Brookhart <mbrookhart@octoml.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants