-
Notifications
You must be signed in to change notification settings - Fork 6.8k
[ONNX] Foward port new mx2onnx into master #20355
Conversation
Hey @Zha0q1 , Thanks for submitting the PR
CI supported jobs: [centos-gpu, windows-gpu, website, windows-cpu, clang, centos-cpu, miscellaneous, unix-gpu, unix-cpu, sanity, edge] Note: |
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.
LGTM, thanks!
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.
Thank you! Few small questions: Can python/mxnet/onnx/mx2onnx/LICENSE
be deleted, as mx2onnx is part of mxnet and not a separate project?
python/mxnet/onnx/README.md
Outdated
### [GluonNLP](https://nlp.gluon.ai/model_zoo/catalog.html) Pretrained Model Support Matrix] | ||
GluonNLP is a popular NLP toolkit built on top of MXNet. Below is the model support matrix for GluonNLP (v0.10.0) models. |
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.
GluonNLP v0.10 does not support MXNet 2. Have you tried with MXNet 2 / should the GluonNLP version be updated here?
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 think "forward port gluoncv/gluonnlp model tests once they are compatible with MXNet 2.0" from the PR description addresses my question. So mx2onnx currently supports exporting a symbol obtained via guonnlp 0.10 to onnx, it's just that we need mxnet 1 to export that symbol from gluonnlp 0.10
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.
Yes that's correct, let me tweak the doc to note that explicitly thanks!
tools/license_header.py
Outdated
# Dual-Licensed under Apache 2.0 and Nvidia BSD-3 | ||
'python/mxnet/onnx/mx2onnx/_export_onnx.py', | ||
'python/mxnet/onnx/mx2onnx/_op_translations/_op_translations_opset12.py', | ||
'python/mxnet/onnx/mx2onnx/_op_translations/_op_translations_opset13.py', |
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.
Dual licensed files should also be mentioned in the root LICENSE file. Further, as these files start with AL2 License text, why do you need to include them in license_header.py
? If the license test fails, would it succeed if Line 17 (the line after end of AL2 text would be empty instead of #
?
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.
the op_translation
files were originally developed by Nvidia I believe so they were dual licensed https://github.com/apache/incubator-mxnet/blob/97a2b609042e8594c6b1ad655a74f7eef585cfd0/python/mxnet/onnx/mx2onnx/_op_translations/_op_translations_opset12.py. Let me also mention them in root LICENSE
* initial: forward port mx2onnx and remove onnx2mx * fix sanity * add onnx operator unit tests * add test file * add model test * fix license & doc * fix * marching toward 2.0 * fix typo * add more ops * more ops * more ops * more ops * fix softmax and sanity * more ops * more ops * more ops * naming * more ops * more ops * more ops and bug fix * more ops and skip unvisited tests * fix sanity * fix for onnx18 * more ops * fix * fix onnx 18 * more ops * skip model test * update read me * more ops * more ops * more ops * more ops * more ops * Update test_models.py
* initial: forward port mx2onnx and remove onnx2mx * fix sanity * add onnx operator unit tests * add test file * add model test * fix license & doc * fix * marching toward 2.0 * fix typo * add more ops * more ops * more ops * more ops * fix softmax and sanity * more ops * more ops * more ops * naming * more ops * more ops * more ops and bug fix * more ops and skip unvisited tests * fix sanity * fix for onnx18 * more ops * fix * fix onnx 18 * more ops * skip model test * update read me * more ops * more ops * more ops * more ops * more ops * Update test_models.py
* initial: forward port mx2onnx and remove onnx2mx * fix sanity * add onnx operator unit tests * add test file * add model test * fix license & doc * fix * marching toward 2.0 * fix typo * add more ops * more ops * more ops * more ops * fix softmax and sanity * more ops * more ops * more ops * naming * more ops * more ops * more ops and bug fix * more ops and skip unvisited tests * fix sanity * fix for onnx18 * more ops * fix * fix onnx 18 * more ops * skip model test * update read me * more ops * more ops * more ops * more ops * more ops * Update test_models.py
This pr forward ports all the new mx2onnx changes in v1.9.x (or v1.x as of 06/15/2020) and deprecates onnx2mx as discussed in #20063.
The expectation is that models generated with and exportable by MXNet v1.x should be continue to be able to export fine with MXNet 2.0.
Done:
TODO in future prs:
update on 06/30/2021
Since this pr 7152685 MXNet 2.0 is steering toward the new Gluon 2.0 interface and np and npx operators. This PR will also try to adapt the existing conversion functions to the new operators.
Supported Ops (WIP):