-
Notifications
You must be signed in to change notification settings - Fork 3.8k
[RELAY] Port from_nnvm to NNVM as to_relay #2144
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.
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
Conversation
|
Very good idea, lets us get lots of Relay test cases for free |
f1f8f69 to
2a66ea2
Compare
27b2b18 to
d8d6371
Compare
|
@ZihengJiang @tqchen review would be good now, after rebasing I fixed a few bugs, tests are green, just need to fix linting and clean up a few things. |
241425e to
c3489fa
Compare
|
@siju-samuel, @eqy could you review this @tqchen said it would be good to have you do review. |
nnvm/python/nnvm/to_relay.py
Outdated
|
|
||
| if op_name == "null": | ||
| v = var(node_name, shape=oshape, dtype=odtype) | ||
| params.append(v) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This need to be updated to support the multiple output cases, consider directly reuse logic(we can refactor the function to take convert_map as argument) https://github.com/dmlc/tvm/blob/master/python/tvm/relay/frontend/mxnet.py#L469
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is code below to handle multiple outputs, it will generate a tuple of all the outputs unless I'm missing something.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that now, the two code handled it differently. In the mxnet converter code relay_map is asked to always record a list, tuple, or TuppleWrapper, and when we get the child we always fetch the corresponding index. This is going to be helpful for cases when we need post proc for multiple outputs(which can directly return a list).
The reason that I thought it only works with single output is due to the fact that when you get oshape, you only get output 0(instead of a list of all of them), so for functions like split, you only get the first output
nnvm/python/nnvm/to_relay.py
Outdated
| return op.divide(left, right) | ||
|
|
||
|
|
||
| def _to_rshift(children, attrs, odtype): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
consider summarizing function as _rbinop_scalar(new_op) as in https://github.com/dmlc/tvm/blob/master/python/tvm/relay/frontend/mxnet.py#L130
nnvm/python/nnvm/to_relay.py
Outdated
| return dense | ||
|
|
||
|
|
||
| def _to_relu(children, attrs, odtype): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Likely we can use _rename in https://github.com/dmlc/tvm/blob/master/python/tvm/relay/frontend/mxnet.py#L28
|
This is a little random but I noticed that we are using fancy python3.6 string features here, did we already declare that we are dropping support for python3.5? |
|
We do need to keep it compatible by not using the python3.6 features here |
| def check_model(sym, shapes, dtypes): | ||
| net = nnvm.graph.create(sym) | ||
| graph_json, mod, params = nnvm.compiler.build(net, 'llvm', shape=shapes, dtype=dtypes) | ||
| relay_model = to_relay.from_nnvm(net, shapes, dtypes) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Am I missing an import here? When I try to run this it can't find from_nnvm in to_relay.
36b75b5 to
9fe2e8f
Compare
Repair imports First test passes Fix bug in axes passing Add implementations for upsampling and pad Add support for bias with conv ops Rebase fixup Borrow the reshape changes while waiting on apache#2159 Debugging tests Repair after rebase Refactor to use common functionality Fix helper func Remove final eval Remove some debuggin Remove unecessary ws change Fix multiple outputs Tests pass Enable all tests Rebase repair Fix last test case Fix last test case All tests but RNNs pass Fix tests Remove final eval Reformat Fix bugs in MxNet Fix 3.5 support in Relay, and rename Factor common functionality into one file Apply most of code review Fix linting Fix NNVM linting Fix test error Fix MLP test Fix linting error One more linting issue Clean up diff and fix bugs Retrigger disable MLP test Retrigger Retrigger Roll back NNVM change Retrigger Retrigger Retrigger
9fe2e8f to
f94a529
Compare
Add code which converts and NNVM model to a Relay program.