Skip to content

[ONNX] Handle multiple imports #13065

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

Merged
jwfromm merged 2 commits into from
Oct 17, 2022
Merged

[ONNX] Handle multiple imports #13065

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
3ffc368
onnx get right import
AndrewZhaoLuo Oct 13, 2022
e4a0c87
fixins
AndrewZhaoLuo Oct 14, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion python/tvm/relay/frontend/onnx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5906,7 +5906,16 @@ def from_onnx(
graph = model.graph

try:
opset_in_model = model.opset_import[0].version if model.opset_import else 1
opset_in_model = 1
if model.opset_import:
# TODO: for now we only really support ai.onnx op set
# TODO: handle other namespaces well see https://github.com/apache/tvm/issues/10950
for opset_identifier in model.opset_import:
# As per https://github.com/onnx/onnx/blob/main/docs/IR.md
# All operator sets except the default one must specify the operator version
if str(opset_identifier.domain) in ["ai.onnx", ""]:

@jwfromm jwfromm Oct 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think this will still run into some issues. When there is only one opset_import, it doesnt have a domain name, just a version. We'll need to add a special case when the length of model.opset_import is 1 and just pull out the version directly.

@AndrewZhaoLuo AndrewZhaoLuo Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Do you mean opset_identifier won't have the attribute "domain", or that it will return the empty string?

@AndrewZhaoLuo AndrewZhaoLuo Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It could be an earlier ONNX spec which we do not support.

@jwfromm jwfromm Oct 14, 2022

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It returns an empty string despite having a version that needs to be respected. If we dont handle this case we'll fail the import_model test.

@AndrewZhaoLuo AndrewZhaoLuo Oct 14, 2022 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm in the code, it should be fine with the "" (we check if the string is "" or ai.onnx), I'll investigate this test.

@AndrewZhaoLuo AndrewZhaoLuo Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The test fail was due to known error #13067.

opset_in_model = opset_identifier.version
break
except AttributeError:
opset_in_model = 1

Expand Down