[Relay][ONNX] fix #3134 converter where initializers were not registered as nodes #3143
Conversation
| @@ -898,7 +898,7 @@ def __init__(self, shape, dtype): | |||
| self._renames = {} | |||
| self._num_input = 0 | |||
| self._num_param = 0 | |||
| self._shape = shape | |||
| self._shape = shape if shape else {} | |||
There was a problem hiding this comment.
Is this for error reporting reasons?
There was a problem hiding this comment.
Yes, otherwise pulling value from None is ambiguous error.
There was a problem hiding this comment.
Cool, just ping me when CI is green and I will merge. Any thoughts on being robust to non-topo sorted models?
There was a problem hiding this comment.
Sure.
For non-topo onnx models, I guess it's doable. However I am not conviced so far to bring in a sorting function purely for what should supposed to be fixed by onnx/keras-onnx
There was a problem hiding this comment.
Yeah I agree, we shouldn't be fixing others bugs, is there some where that states it must be topo-sorted?
There was a problem hiding this comment.
here's the claim: https://github.com/onnx/onnx/blob/master/docs/IR.md#graphs
There was a problem hiding this comment.
Awesome, I think we should add a note in the docs as a warning, what do you think?
There was a problem hiding this comment.
I also saw there are some onnx utilities which check the graph, do you know if these check topo. ordering?
There was a problem hiding this comment.
actually onnx checker is a good idea
zhreshold
force-pushed
the
key-error-relay-onnx
branch
from
59601d3 to
b4deb62
Compare
|
the ci consistently fails at |
zhreshold
force-pushed
the
key-error-relay-onnx
branch
from
16b984e to
a248652
Compare
|
It looks like CI is broken, can you try to trigger a rebuild later today. I'll merge once CI is green. |
zhreshold
force-pushed
the
key-error-relay-onnx
branch
from
a248652 to
f6e9ef1
Compare
zhreshold
force-pushed
the
key-error-relay-onnx
branch
from
bc3cbea to
33b5089
Compare
|
@jroesch CI is green now. |
|
Thanks, @zhreshold @jroesch this is now merged |
…egistered as nodes (apache#3143)
…egistered as nodes (apache#3143)
Fix a keyerror when onnx initializers are not registered as nodes.