[ONNX] Handle multiple imports #13065
Conversation
|
Thanks for contributing to TVM! Please refer to the contributing guidelines https://tvm.apache.org/docs/contribute/ for useful information and tips. Please request code reviews from Reviewers by @-ing them in a comment.
Generated by tvm-bot |
|
cc @sfvaroglu |
python/tvm/relay/frontend/onnx.py
Outdated
| 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.name) in ["ai.onnx", ""]: |
There was a problem hiding this comment.
I'm not sure if its inconsistent but the models I'm looking at use opset_identifer.domain rather than name. Also, it seems like if there is only one import there is no domain name, just the version and its assumed the domain is ai.onnx. This is at least the case for resnet50-v1-7 which is used in the tests.
There was a problem hiding this comment.
It should be domain yeah, fixed.
| 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", ""]: |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Do you mean opset_identifier won't have the attribute "domain", or that it will return the empty string?
There was a problem hiding this comment.
It could be an earlier ONNX spec which we do not support.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hmm in the code, it should be fine with the "" (we check if the string is "" or ai.onnx), I'll investigate this test.
There was a problem hiding this comment.
The test fail was due to known error #13067.
|
@tvm-bot rerun |
|
PTAL @jwfromm |
AndrewZhaoLuo
force-pushed
the
aluo/onnx-fix-namespaces
branch
from
1e29caa to
e4a0c87
Compare
|
LGTM, thank you @AndrewZhaoLuo |
* onnx get right import * fixins
* onnx get right import * fixins
Ref: https://github.com/onnx/onnx/blob/main/docs/IR.md
Right now we take the first imported op set as the operator version for rest of conversion. This assumes the first imported op set is the default (ai.onnx), however this is not necessarily the case.
In the future, we need to support multiple operator sets well (see #10950). For now, we just try to find the default (ai.onnx) namespace and use that for the opset version.