[ONNX] Handle multiple imports by AndrewZhaoLuo · Pull Request #13065 · apache/tvm

AndrewZhaoLuo · 2022-10-13T18:18:01Z

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.

tvm-bot · 2022-10-13T18:18:05Z

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.

No users to tag found in teams: onnx_{See #10317 for details}
Built docs for commit e4a0c87 can be found here.

_{Generated by tvm-bot}

AndrewZhaoLuo · 2022-10-13T18:18:13Z

cc @sfvaroglu

AndrewZhaoLuo · 2022-10-14T04:12:24Z

@jwfromm

jwfromm

This is a good starting point, thanks @AndrewZhaoLuo!

jwfromm · 2022-10-14T18:02:56Z

+ 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", ""]:


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.

It should be domain yeah, fixed.

jwfromm · 2022-10-14T20:19:26Z

+ 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", ""]:


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.

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

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

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.

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

The test fail was due to known error #13067.

AndrewZhaoLuo · 2022-10-14T22:13:10Z

@tvm-bot rerun

AndrewZhaoLuo · 2022-10-14T22:20:35Z

PTAL @jwfromm

jwfromm · 2022-10-17T19:57:21Z

LGTM, thank you @AndrewZhaoLuo

* onnx get right import * fixins

AndrewZhaoLuo · 2022-10-13T18:18:01Z

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.

tvm-bot · 2022-10-13T18:18:05Z

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.

No users to tag found in teams: onnx_{See #10317 for details}
Built docs for commit e4a0c87 can be found here.

_{Generated by tvm-bot}

AndrewZhaoLuo · 2022-10-13T18:18:13Z

cc @sfvaroglu

AndrewZhaoLuo · 2022-10-14T04:12:24Z

@jwfromm

jwfromm

This is a good starting point, thanks @AndrewZhaoLuo!

jwfromm · 2022-10-14T18:02:56Z

+ 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", ""]:


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.

It should be domain yeah, fixed.

jwfromm · 2022-10-14T20:19:26Z

+ 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", ""]:


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.

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

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

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.

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

The test fail was due to known error #13067.

AndrewZhaoLuo · 2022-10-14T22:13:10Z

@tvm-bot rerun

AndrewZhaoLuo · 2022-10-14T22:20:35Z

PTAL @jwfromm

jwfromm · 2022-10-17T19:57:21Z

LGTM, thank you @AndrewZhaoLuo

* onnx get right import * fixins

AndrewZhaoLuo · 2022-10-13T18:18:01Z

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.

tvm-bot · 2022-10-13T18:18:05Z

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.

No users to tag found in teams: onnx_{See #10317 for details}
Built docs for commit e4a0c87 can be found here.

_{Generated by tvm-bot}

AndrewZhaoLuo · 2022-10-13T18:18:13Z

cc @sfvaroglu

AndrewZhaoLuo · 2022-10-14T04:12:24Z

@jwfromm

jwfromm

This is a good starting point, thanks @AndrewZhaoLuo!

jwfromm · 2022-10-14T18:02:56Z

+ 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", ""]:


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.

It should be domain yeah, fixed.

jwfromm · 2022-10-14T20:19:26Z

+ 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", ""]:


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.

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

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

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.

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

The test fail was due to known error #13067.

AndrewZhaoLuo · 2022-10-14T22:13:10Z

@tvm-bot rerun

AndrewZhaoLuo · 2022-10-14T22:20:35Z

PTAL @jwfromm

jwfromm · 2022-10-17T19:57:21Z

LGTM, thank you @AndrewZhaoLuo

* onnx get right import * fixins

AndrewZhaoLuo · 2022-10-13T18:18:01Z

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.

tvm-bot · 2022-10-13T18:18:05Z

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.

No users to tag found in teams: onnx_{See #10317 for details}
Built docs for commit e4a0c87 can be found here.

_{Generated by tvm-bot}

AndrewZhaoLuo · 2022-10-13T18:18:13Z

cc @sfvaroglu

AndrewZhaoLuo · 2022-10-14T04:12:24Z

@jwfromm

jwfromm

This is a good starting point, thanks @AndrewZhaoLuo!

jwfromm · 2022-10-14T18:02:56Z

+ 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", ""]:


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.

It should be domain yeah, fixed.

jwfromm · 2022-10-14T20:19:26Z

+ 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", ""]:


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.

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

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

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.

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

The test fail was due to known error #13067.

AndrewZhaoLuo · 2022-10-14T22:13:10Z

@tvm-bot rerun

AndrewZhaoLuo · 2022-10-14T22:20:35Z

PTAL @jwfromm

jwfromm · 2022-10-17T19:57:21Z

LGTM, thank you @AndrewZhaoLuo

* onnx get right import * fixins

AndrewZhaoLuo · 2022-10-13T18:18:01Z

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.

tvm-bot · 2022-10-13T18:18:05Z

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.

No users to tag found in teams: onnx_{See #10317 for details}
Built docs for commit e4a0c87 can be found here.

_{Generated by tvm-bot}

AndrewZhaoLuo · 2022-10-13T18:18:13Z

cc @sfvaroglu

AndrewZhaoLuo · 2022-10-14T04:12:24Z

@jwfromm

jwfromm

This is a good starting point, thanks @AndrewZhaoLuo!

jwfromm · 2022-10-14T18:02:56Z

+ 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", ""]:


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.

It should be domain yeah, fixed.

jwfromm · 2022-10-14T20:19:26Z

+ 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", ""]:


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.

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

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

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.

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

The test fail was due to known error #13067.

AndrewZhaoLuo · 2022-10-14T22:13:10Z

@tvm-bot rerun

AndrewZhaoLuo · 2022-10-14T22:20:35Z

PTAL @jwfromm

jwfromm · 2022-10-17T19:57:21Z

LGTM, thank you @AndrewZhaoLuo

* onnx get right import * fixins

AndrewZhaoLuo · 2022-10-13T18:18:01Z

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.

tvm-bot · 2022-10-13T18:18:05Z

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.

No users to tag found in teams: onnx_{See #10317 for details}
Built docs for commit e4a0c87 can be found here.

_{Generated by tvm-bot}

AndrewZhaoLuo · 2022-10-13T18:18:13Z

cc @sfvaroglu

AndrewZhaoLuo · 2022-10-14T04:12:24Z

@jwfromm

jwfromm

This is a good starting point, thanks @AndrewZhaoLuo!

jwfromm · 2022-10-14T18:02:56Z

+ 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", ""]:


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.

It should be domain yeah, fixed.

jwfromm · 2022-10-14T20:19:26Z

+ 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", ""]:


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.

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

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

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.

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

The test fail was due to known error #13067.

AndrewZhaoLuo · 2022-10-14T22:13:10Z

@tvm-bot rerun

AndrewZhaoLuo · 2022-10-14T22:20:35Z

PTAL @jwfromm

jwfromm · 2022-10-17T19:57:21Z

LGTM, thank you @AndrewZhaoLuo

* onnx get right import * fixins

AndrewZhaoLuo · 2022-10-13T18:18:01Z

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.

tvm-bot · 2022-10-13T18:18:05Z

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.

No users to tag found in teams: onnx_{See #10317 for details}
Built docs for commit e4a0c87 can be found here.

_{Generated by tvm-bot}

AndrewZhaoLuo · 2022-10-13T18:18:13Z

cc @sfvaroglu

AndrewZhaoLuo · 2022-10-14T04:12:24Z

@jwfromm

jwfromm

This is a good starting point, thanks @AndrewZhaoLuo!

jwfromm · 2022-10-14T18:02:56Z

+ 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", ""]:


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.

It should be domain yeah, fixed.

jwfromm · 2022-10-14T20:19:26Z

+ 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", ""]:


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.

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

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

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.

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

The test fail was due to known error #13067.

AndrewZhaoLuo · 2022-10-14T22:13:10Z

@tvm-bot rerun

AndrewZhaoLuo · 2022-10-14T22:20:35Z

PTAL @jwfromm

jwfromm · 2022-10-17T19:57:21Z

LGTM, thank you @AndrewZhaoLuo

* onnx get right import * fixins

AndrewZhaoLuo · 2022-10-13T18:18:01Z

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.

tvm-bot · 2022-10-13T18:18:05Z

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.

No users to tag found in teams: onnx_{See #10317 for details}
Built docs for commit e4a0c87 can be found here.

_{Generated by tvm-bot}

AndrewZhaoLuo · 2022-10-13T18:18:13Z

cc @sfvaroglu

AndrewZhaoLuo · 2022-10-14T04:12:24Z

@jwfromm

jwfromm

This is a good starting point, thanks @AndrewZhaoLuo!

jwfromm · 2022-10-14T18:02:56Z

+ 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", ""]:


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.

It should be domain yeah, fixed.

jwfromm · 2022-10-14T20:19:26Z

+ 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", ""]:


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.

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

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

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.

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

The test fail was due to known error #13067.

AndrewZhaoLuo · 2022-10-14T22:13:10Z

@tvm-bot rerun

AndrewZhaoLuo · 2022-10-14T22:20:35Z

PTAL @jwfromm

jwfromm · 2022-10-17T19:57:21Z

LGTM, thank you @AndrewZhaoLuo

* onnx get right import * fixins

jwfromm approved these changes Oct 14, 2022
View reviewed changes

jwfromm requested changes Oct 14, 2022
View reviewed changes

jwfromm reviewed Oct 14, 2022
View reviewed changes

AndrewZhaoLuo added 2 commits October 14, 2022 15:21

onnx get right import
3ffc368

fixins
e4a0c87

AndrewZhaoLuo force-pushed the aluo/onnx-fix-namespaces branch from 1e29caa to e4a0c87Compare October 14, 2022 22:21

jwfromm approved these changes Oct 17, 2022
View reviewed changes

jwfromm merged commit c14f5e1 into apache:mainOct 17, 2022

xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 10, 2022
[ONNX] Handle multiple imports (apache#13065)
a0e3582
* onnx get right import * fixins

xinetzone pushed a commit to daobook/tvm that referenced this pull request Nov 25, 2022
[ONNX] Handle multiple imports (apache#13065)
679ccdd
* onnx get right import * fixins

gigiblender mentioned this pull request Feb 9, 2023
[TUZ-6] Add a direct Onnx to Relax Importer octoml/relax#14
Merged