[Frontend][ONNX] Replace : in input name with _
#13011
Conversation
AndrewZhaoLuo
left a comment
AndrewZhaoLuo
left a comment
There was a problem hiding this comment.
Hmm, I think the current code is a bit confusing, can you try to clean it up?
Regardless, I suspect this might not be the best approach. This PR is designed to fix a problem in the parser, why not fix the problem in the parser? Or is the grammar of relay fundamentally incompatible with ":"?
I only say this because you can make a relay variable with ":". If this is allowed in the relay language it should also be supported by the parser.
| @@ -5642,9 +5643,11 @@ def _parse_graph_input(self, graph): | |||
| continue | |||
| else: | |||
There was a problem hiding this comment.
I notice you only handle the else branch here. Will we not have the same parsing error from the other branches?
|
|
||
| def _parse_graph_input(self, graph): | ||
| for i in graph.input: | ||
| i_name_compatible = None |
There was a problem hiding this comment.
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
| i_name_compatible, shape=i_shape, dtype=dtype | ||
| ) | ||
|
|
||
| if i_name_compatible: |
There was a problem hiding this comment.
In it's current state it seems the intention this branch will always be evaluated in the else branch on line 5643.
|
The issue is fixed with this PR: |
2 participants
This PR changes onnx frontend to rename input with
:character. This will fix this issue:#12362