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

[Relay][ONNX] fix #3134 converter where initializers were not registered as nodes #3143

Merged
merged 6 commits into from
May 20, 2019
Merged

[Relay][ONNX] fix #3134 converter where initializers were not registered as nodes #3143

Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
72bdca9
fix initalizers not registered as nodes
zhreshold May 7, 2019
a34e020
add onnx checker
zhreshold May 8, 2019
ad898a0
fix lint
zhreshold May 10, 2019
33b5089
catch onnx checker error
zhreshold May 15, 2019
c23ee2a
fix import
zhreshold May 17, 2019
0a72077
fix error catching
zhreshold May 17, 2019
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
17 changes: 16 additions & 1 deletion python/tvm/relay/frontend/onnx.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -934,7 +934,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 {}
Copy link
Member

Choose a reason for hiding this comment

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

Is this for error reporting reasons?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes, otherwise pulling value from None is ambiguous error.

Copy link
Member

Choose a reason for hiding this comment

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

Cool, just ping me when CI is green and I will merge. Any thoughts on being robust to non-topo sorted models?

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Yeah I agree, we shouldn't be fixing others bugs, is there some where that states it must be topo-sorted?

Copy link
Member Author

Choose a reason for hiding this comment

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

here's the claim: https://github.com/onnx/onnx/blob/master/docs/IR.md#graphs

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, I think we should add a note in the docs as a warning, what do you think?

Copy link
Member

Choose a reason for hiding this comment

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

I also saw there are some onnx utilities which check the graph, do you know if these check topo. ordering?

Copy link
Member Author

Choose a reason for hiding this comment

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

actually onnx checker is a good idea

self._dtype = dtype

def from_onnx(self, graph, opset):
Expand Down Expand Up @@ -966,6 +966,9 @@ def from_onnx(self, graph, opset):
if not init_tensor.name.strip():
raise ValueError("Tensor's name is required.")
self._params[init_tensor.name] = self._parse_array(init_tensor)
self._nodes[init_tensor.name] = new_var(init_tensor.name,
shape=self._params[init_tensor.name].shape,
dtype=self._params[init_tensor.name].dtype)
for i in graph.input:
# from onnx v0.2, GraphProto.input has type ValueInfoProto,
# and the name is 'i.name'
Expand Down Expand Up @@ -1179,6 +1182,18 @@ def from_onnx(model,
params : dict of str to tvm.NDArray
The parameter dict to be used by relay
"""
try:
import onnx
if hasattr(onnx.checker, 'check_model'):
# try use onnx's own model checker before converting any model
try:
onnx.checker.check_model(model)
except onnx.onnx_cpp2py_export.checker.ValidationError as e:
import warnings
# the checker is a bit violent about errors, so simply print warnings here
warnings.warn(str(e))
except ImportError:
pass
g = GraphProto(shape, dtype)
graph = model.graph
try:
Expand Down