Skip to content

[Frontend][ONNX] Replace : in input name with _ #13011

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

Closed
mehrdadh wants to merge 1 commit into from
Closed

[Frontend][ONNX] Replace : in input name with _ #13011

Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
23 changes: 18 additions & 5 deletions python/tvm/relay/frontend/onnx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5629,6 +5629,7 @@ def _parse_graph_initializers(self, graph):

def _parse_graph_input(self, graph):
for i in graph.input:
i_name_compatible = None
Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo Oct 7, 2022

Choose a reason for hiding this comment

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

this seems a bit messy as now i_name_compatible can be of types [None, str].

This is actually an issue since the python str of "" is not truthy (it is false) which may be able to be an onnx name (correct me if im wrong). If this is the intention it is a bit unclear and deserves a comment.

Why can't we just do something like:

new_name = i.name.replace(":", "_") 
self._renames[i.name] = new_name 
i.name = new_name

# from onnx v0.2, GraphProto.input has type ValueInfoProto,
# and the name is 'i.name'
i_name, i_shape, d_type, i_shape_name = get_info(i)
Expand All @@ -5642,9 +5643,11 @@ def _parse_graph_input(self, graph):
continue
else:
Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo Oct 7, 2022

Choose a reason for hiding this comment

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

I notice you only handle the else branch here. Will we not have the same parsing error from the other branches?

self._num_input += 1
self._input_names.append(i_name)
if i_name in self._shape:
i_shape = self._shape[i_name]
# cleanup input name by replacing `:` in name with `_`
i_name_compatible = i_name.replace(":", "_")
self._input_names.append(i_name_compatible)
if i_name_compatible in self._shape:
i_shape = self._shape[i_name_compatible]
else:
if "?" in str(i_shape):
warning_msg = (
Expand All @@ -5654,10 +5657,20 @@ def _parse_graph_input(self, graph):
)
warnings.warn(warning_msg)
if isinstance(self._dtype, dict):
dtype = self._dtype[i_name] if i_name in self._dtype else d_type
dtype = (
self._dtype[i_name_compatible]
if i_name_compatible in self._dtype
else d_type
)
else:
dtype = d_type
self._nodes[i_name] = new_var(i_name, shape=i_shape, dtype=dtype)
self._nodes[i_name_compatible] = new_var(
i_name_compatible, shape=i_shape, dtype=dtype
)

if i_name_compatible:
Copy link
Contributor

@AndrewZhaoLuo AndrewZhaoLuo Oct 7, 2022

Choose a reason for hiding this comment

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

In it's current state it seems the intention this branch will always be evaluated in the else branch on line 5643.

self._renames[i_name] = i_name_compatible
i_name = i_name_compatible
self._inputs[i_name] = self._nodes[i_name]

def _check_user_inputs_in_outermost_graph_scope(self):
Expand Down