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.

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

Jump to bottom

Get list of unsupported ONNX operators #2995

Merged
merged 2 commits into from
May 16, 2019
Merged

Get list of unsupported ONNX operators #2995

merged 2 commits into from
May 16, 2019

Conversation

markrogersjr
Copy link
Contributor

Improve user experience by reporting the entire list of ops that are not supported in ONNX. @JennyChou5 @yidawang @zhreshold @srkreddy1238 @kazum

JennyChou5
JennyChou5 reviewed Apr 10, 2019
View reviewed changes
Copy link

@JennyChou5 JennyChou5 left a comment

Choose a reason for hiding this comment

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

traversing graph while reporting missing operator lists is good for users.
suggest checking op_name in identity_list or op_name in convert_map or op_name = 'Constant' instead of creating another supported_op set.

python/tvm/relay/frontend/onnx.py Outdated
@@ -879,6 +879,79 @@ def _get_convert_map(opset):
'Shape': Shape.get_converter(opset),
}

supported_ops = set([
Copy link

Choose a reason for hiding this comment

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

I am thinking defining the supported_ops set is redundant.
checking op_name in identity_list or op_name in convert_map or op_name = 'Constant' should be enough

python/tvm/relay/frontend/onnx.py Outdated
unsupported_ops = []
for node in graph.node:
op_name = node.op_type
if op_name not in supported_ops:
Copy link

Choose a reason for hiding this comment

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

checking op_name in identity_list or op_name in convert_map or op_name = 'Constant' should be enough

nnvm/python/nnvm/frontend/onnx.py Outdated
@@ -775,6 +775,80 @@ def _get_convert_map(opset):
}


supported_ops = set([
Copy link

Choose a reason for hiding this comment

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

I am thinking defining the supported_ops set is redundant.
checking op_name in identity_list or op_name in convert_map or op_name = 'Constant' should be enough

nnvm/python/nnvm/frontend/onnx.py Outdated
unsupported_ops = []
for node in graph.node:
op_name = node.op_type
if op_name not in supported_ops:
Copy link

Choose a reason for hiding this comment

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

checking op_name in identity_list or op_name in convert_map or op_name = 'Constant' should be enough

@icemelon icemelon added the status: need update need update based on feedbacks label Apr 15, 2019
@tqchen
Copy link
Member

tqchen commented Apr 20, 2019

@KoinFlyp please act on @JennyChou5 's comment

@markrogersjr markrogersjr force-pushed the onnx_missing_ops branch 2 times, most recently from 5c6a913 to 3d5f193 Compare April 20, 2019 19:21
@tqchen
Copy link
Member

tqchen commented Apr 20, 2019

@JennyChou5 please check again and https://docs.tvm.ai/contribute/code_review.html#approve-and-request-changes-explicitly

JennyChou5
JennyChou5 reviewed Apr 22, 2019
View reviewed changes
Copy link

@JennyChou5 JennyChou5 left a comment •
edited
Loading

Choose a reason for hiding this comment

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

even though currently _identity_list is empty but it would be good to make logic complete.

nnvm/python/nnvm/frontend/onnx.py Outdated
unsupported_ops = set()
for node in graph.node:
op_name = node.op_type
if op_name not in convert_map and op_name != 'Constant':
Copy link

Choose a reason for hiding this comment

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

should we check the _identity_list? even though currently it is empty but it would be good to make logic complete.

python/tvm/relay/frontend/onnx.py Outdated
unsupported_ops = set()
for node in graph.node:
op_name = node.op_type
if op_name not in convert_map and op_name != 'Constant':
Copy link

Choose a reason for hiding this comment

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

should we check the _identity_list? even though currently it is empty but it would be good to make logic complete.

@markrogersjr markrogersjr force-pushed the onnx_missing_ops branch 2 times, most recently from 8b4be42 to 6761072 Compare April 22, 2019 19:36
JennyChou5
JennyChou5 approved these changes Apr 22, 2019
View reviewed changes
Copy link

@JennyChou5 JennyChou5 left a comment

Choose a reason for hiding this comment

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

logical good and correct.

yongwww
yongwww reviewed Apr 22, 2019
View reviewed changes
nnvm/python/nnvm/frontend/onnx.py Outdated
unsupported_ops.add(op_name)
if unsupported_ops:
msg = ['The following operators are not supported for frontend ONNX: ']
for i, op_name in enumerate(unsupported_ops):
Copy link
Member

Choose a reason for hiding this comment

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

how about ops = ', '.join(str(op) for op in unsupported_ops)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We’d still have to concatenate the ‘The following ops...’ string and I generally avoid concatenating strings with +. Your way is neater, though, so I’ll change it.

@tqchen tqchen merged commit 76ae2dc into apache:master May 16, 2019
@tqchen tqchen added status: accepted and removed status: need update need update based on feedbacks labels May 16, 2019
@tqchen
Copy link
Member

tqchen commented May 16, 2019

Thanks, @yongwww @JennyChou5 @KoinFlyp this is now merged

wweic pushed a commit to wweic/tvm that referenced this pull request Jun 26, 2019
@markrogersjr
Get list of unsupported ONNX operators (apache#2995)
30d026f
wweic pushed a commit to neo-ai/tvm that referenced this pull request Jun 27, 2019
@markrogersjr @wweic
Get list of unsupported ONNX operators (apache#2995)
010639e
@tqchen tqchen mentioned this pull request Nov 8, 2019
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yongwww yongwww yongwww left review comments

@JennyChou5 JennyChou5 JennyChou5 approved these changes

@tqchen tqchen tqchen approved these changes

Assignees
No one assigned
Labels
status: accepted
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@markrogersjr @tqchen @yongwww @JennyChou5 @icemelon