Skip to content
This repository has been archived by the owner on Nov 17, 2023. It is now read-only.

[MXNET-34] Onnx Module to import onnx models into mxnet #9963

Merged
merged 36 commits into from
Mar 14, 2018
Merged

[MXNET-34] Onnx Module to import onnx models into mxnet #9963

merged 36 commits into from
Mar 14, 2018

Conversation

anirudhacharya
Copy link
Member

@anirudhacharya anirudhacharya commented Mar 2, 2018 •
edited
Loading

Description

Module to import onnx models into mxnet.

Checklist

Essentials

  • Passed code style checking (make lint)
  • Changes are complete (i.e. I finished coding on this PR)
  • All changes have test coverage.
  • Unit tests are added for small changes to verify correctness (e.g. adding a new operator)
  • Code is well-documented:
  • For user-facing API changes, API doc string has been updated.
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • API to import onnx models.

Comments

@Roshrini @spidydev @lupesko @reminisce @piiswrong @szha
@nswamy @sandeep-krishnamurthy
@awslabs/aws-deep-learning-sdk

reminisce
reminisce reviewed Mar 2, 2018
View reviewed changes
Copy link
Contributor

@reminisce reminisce left a comment

Choose a reason for hiding this comment

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

The underscore prefix in the name of the folder _import makes it look like an internal module. Any good reason of naming it in this way?

python/mxnet/contrib/onnx/_import/__init__.py Outdated Show resolved Hide resolved
@anirudhacharya
Copy link
Member Author

For all the test cases to pass for this module, we will need changes in the CI process. It needs Onnx and protobuf in the CI process for the tests to run. Currently the onnx-mxnet project fetches these packages in the CI process -

[Container] 2018/03/02 00:14:07 Running command wget https://github.com/google/protobuf/releases/download/v2.6.1/protobuf-2.6.1.tar.gz
Downloading onnx-1.0.1.tar.gz (763kB)

@anirudhacharya anirudhacharya mentioned this pull request Mar 2, 2018
Closed
2 tasks
@KellenSunderland
Copy link
Contributor

Hi @anirudhacharya, are you blocked by the CI change? You are able to change the CI process along with your PR, and installing two dependencies shouldn't be hard. Just let us know if you'd like help doing this. (To get started you can see how other python deps are installed here: https://github.com/apache/incubator-mxnet/blob/master/tests/ci_build/Dockerfile.cpu and here: https://github.com/apache/incubator-mxnet/blob/master/tests/ci_build/install/install_testdeps.sh for example.)

sandeep-krishnamurthy
sandeep-krishnamurthy reviewed Mar 5, 2018
View reviewed changes
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution.
Few minor comments.

python/mxnet/contrib/onnx/_import/import_helper.py Outdated Show resolved Hide resolved
python/mxnet/contrib/onnx/_import/import_onnx.py Show resolved Hide resolved
python/mxnet/contrib/onnx/_import/import_onnx.py Outdated Show resolved Hide resolved
python/mxnet/contrib/onnx/_import/import_onnx.py Outdated Show resolved Hide resolved
python/mxnet/contrib/onnx/_import/translation_utils/__init__.py Outdated Show resolved Hide resolved
python/mxnet/contrib/onnx/_import/op_translations/reduce_max.py Outdated Show resolved Hide resolved
@anirudhacharya
Copy link
Member Author

@piiswrong can you please take a look at the code. We will keep adding more operator support, but can you review the general design of the module. I think I have addressed all your concerns.

sandeep-krishnamurthy
sandeep-krishnamurthy reviewed Mar 6, 2018
View reviewed changes
Copy link
Contributor

@sandeep-krishnamurthy sandeep-krishnamurthy left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM.
Can you please check PR build failures.

@CodingCat
Copy link
Contributor

Hi, the community has passed to vote about associating the code changes with JIRA (https://lists.apache.org/thread.html/ab22cf0e35f1bce2c3bf3bec2bc5b85a9583a3fe7fd56ba1bbade55f@%3Cdev.mxnet.apache.org%3E)

We have updated the guidelines for contributors in https://cwiki.apache.org/confluence/display/MXNET/Development+Process, please ensure that you have created a JIRA at https://issues.apache.org/jira/projects/MXNET/issues/ to describe your work in this pull request and include the JIRA title in your PR as [MXNET-xxxx] your title where MXNET-xxxx is the JIRA id

Thanks!

marcoabreu
marcoabreu suggested changes Mar 7, 2018
View reviewed changes
tests/python/unittest/test_layers.py Outdated Show resolved Hide resolved
@anirudhacharya anirudhacharya mentioned this pull request Mar 8, 2018
Closed
@anirudhacharya anirudhacharya changed the title Onnx Module to import onnx models into mxnet [MXNET-34] Onnx Module to import onnx models into mxnet Mar 8, 2018
marcoabreu
marcoabreu suggested changes Mar 8, 2018
View reviewed changes
Jenkinsfile Outdated Show resolved Hide resolved
Jenkinsfile Outdated Show resolved Hide resolved
example/onnx/test_super_resolution.py Outdated Show resolved Hide resolved
Jenkinsfile Outdated Show resolved Hide resolved
example/onnx/test_super_resolution.py Outdated Show resolved Hide resolved
python/mxnet/contrib/__init__.py Show resolved Hide resolved
tests/python/integrationtest/test_layers.py Outdated Show resolved Hide resolved
tests/python/integrationtest/test_layers.py Outdated Show resolved Hide resolved
tests/python/integrationtest/test_layers.py Outdated Show resolved Hide resolved
marcoabreu
marcoabreu reviewed Mar 8, 2018
View reviewed changes
Jenkinsfile Outdated Show resolved Hide resolved
marcoabreu
marcoabreu reviewed Mar 9, 2018
View reviewed changes
Jenkinsfile Outdated Show resolved Hide resolved
marcoabreu
marcoabreu reviewed Mar 9, 2018
View reviewed changes
Jenkinsfile Outdated Show resolved Hide resolved
marcoabreu
marcoabreu reviewed Mar 9, 2018
View reviewed changes
Jenkinsfile Outdated Show resolved Hide resolved
@anirudhacharya anirudhacharya mentioned this pull request Mar 9, 2018
Closed
Acharya added 2 commits March 9, 2018 15:20
Onnx Module to import onnx models into mxnet
d80f95a 
* Change package name to onnx from serde.
* Remove onnx install time dependency
* Remove Renamer class
* Add apache license to files.
* Refactor test files to tests/python folder.
* Removed export folder.
* Refactor Attribute COnverter logic

Signed-off-by: Acharya <[email protected]>
Changing the translation and utils file.
23c409a 
Signed-off-by: Acharya <[email protected]>
marcoabreu
marcoabreu suggested changes Mar 13, 2018
View reviewed changes
Copy link
Contributor

@marcoabreu marcoabreu left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Two minor changes

tests/python-pytest/onnx/onnx_test.py Outdated Show resolved Hide resolved
tests/python-pytest/onnx/onnx_test.py Outdated Show resolved Hide resolved
marcoabreu
marcoabreu suggested changes Mar 13, 2018
View reviewed changes
Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

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

Please add a correctness test

tests/python-pytest/onnx/onnx_test.py Outdated Show resolved Hide resolved
marcoabreu
marcoabreu approved these changes Mar 13, 2018
View reviewed changes
Copy link
Contributor

@marcoabreu marcoabreu left a comment

Choose a reason for hiding this comment

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

Approved in terms of CI

Roshrini
Roshrini reviewed Mar 13, 2018
View reviewed changes
Copy link
Member

@Roshrini Roshrini left a comment

Choose a reason for hiding this comment

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

Few comments

python/mxnet/contrib/onnx/_import/import_helper.py Show resolved Hide resolved
python/mxnet/contrib/onnx/_import/import_model.py Show resolved Hide resolved
python/mxnet/contrib/onnx/_import/import_model.py Outdated Show resolved Hide resolved
python/mxnet/contrib/onnx/_import/import_helper.py Show resolved Hide resolved
nswamy
nswamy suggested changes Mar 14, 2018
View reviewed changes
Copy link
Member

@nswamy nswamy left a comment

Choose a reason for hiding this comment

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

LGTM, except for a few comments

python/mxnet/test_utils.py Outdated Show resolved Hide resolved
python/mxnet/test_utils.py Outdated Show resolved Hide resolved
python/mxnet/test_utils.py Outdated Show resolved Hide resolved
nswamy
nswamy approved these changes Mar 14, 2018
View reviewed changes
Copy link
Member

@nswamy nswamy left a comment

Choose a reason for hiding this comment

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

LGTM, lets merge after the CI tests are run.

@reminisce
Copy link
Contributor

Is there a plan to add a doc page with these changes? Normally we don't expect users to find out APIs by reading the code.

Remove fetching test files by ETag.
b24baba 
Will add it as a separate PR as per review comments.

Signed-off-by: Acharya <[email protected]>
@anirudhacharya
Copy link
Member Author

anirudhacharya commented Mar 14, 2018 •
edited
Loading

@reminisce there is a follow up PR with the doc page changes. Please refer - #10140

@nswamy nswamy merged commit 93e0ceb into apache:master Mar 14, 2018
cjolivier01 pushed a commit to cjolivier01/mxnet that referenced this pull request Mar 14, 2018
@anirudhacharya
[MXNET-34] Onnx Module to import onnx models into mxnet (apache#9963)
3611d57 
* Onnx Module to import onnx models into mxnet

* Change package name to onnx from serde.
* Remove onnx install time dependency
* Remove Renamer class
* Add apache license to files.
* Refactor test files to tests/python folder.
* Removed export folder.
* Refactor Attribute COnverter logic

Signed-off-by: Acharya <[email protected]>

* Changing the translation and utils file.


Signed-off-by: Acharya <[email protected]>

* - Fixed Pylint issues
- Added Sigmoid operator.
- Add onnx, protobuf as CI pipeline dependencies.

Signed-off-by: Acharya <[email protected]>

* Add UTs for reduce ops.

Signed-off-by: Acharya <[email protected]>

* pylint - newline, whitespace.

Signed-off-by: Acharya <[email protected]>

*     Added operators:
    - AvgPool
    - ArgMax
    - ArgMin
    - Abs

    Minor changes in logic for import_onnx

* Added operators:
- Ceil
- Cast
- Constant

* - Added Pad operator support.
- Minor changes for comments

* RandomUniform,Normal,Sub,Mul,Div,Tanh,Relu,Reciprocal,Sqrt operators
added.

Signed-off-by: Acharya <[email protected]>

* lint fix

* Add protobuf-compile to CI bash script. Add MatMul and Pow operator.

Signed-off-by: Acharya <[email protected]>

* Max,Min,Sum,Reduce operators.

Signed-off-by: Acharya <[email protected]>

* BatchNorm,SpatialBN, Split

Signed-off-by: Acharya <[email protected]>

* Slice,Transpose and Squeeze Operators.

Signed-off-by: Acharya <[email protected]>

* Onnx tests in CI integration tests.

Signed-off-by: Acharya <[email protected]>

* Addressing Marco's comments

Signed-off-by: Acharya <[email protected]>

* Floor, LeakyRelu, Elu, PRelu, Softmax, Exp, Log operator.

* Added operators:
- Convolution
- Deconvolution

Refactored convert_operator

* lint fix

* Rebase fix

* Added Maxpool operator

* Adding FullyConnected operator

* Adding operator- GlobalPooling - max and avg
Minor lint fixes.

* Adding operator - Gemm

* Change test Path, LRN and Dropout operator.

* Add asserts for the super_res example.

Signed-off-by: Acharya <[email protected]>

* Fixing conv test failures.
Removed redundant code

* Update Jenkins job.

Signed-off-by: Acharya <[email protected]>

* Nits: Removing commented out code

* Rebase after Docker PR

* Fetch test files by version number. Verify the high resolution example.

Signed-off-by: Acharya <[email protected]>

* Fix method arguments for Python3.5+

Signed-off-by: Acharya <[email protected]>

* Remove logging configuration from test files.

Signed-off-by: Acharya <[email protected]>

* Verify result image in example by hash

Signed-off-by: Acharya <[email protected]>

* Remove fetching test files by ETag.
Will add it as a separate PR as per review comments.

Signed-off-by: Acharya <[email protected]>
jinhuang415 pushed a commit to jinhuang415/incubator-mxnet that referenced this pull request Mar 30, 2018
@anirudhacharya
[MXNET-34] Onnx Module to import onnx models into mxnet (apache#9963)
be617fe 
* Onnx Module to import onnx models into mxnet

* Change package name to onnx from serde.
* Remove onnx install time dependency
* Remove Renamer class
* Add apache license to files.
* Refactor test files to tests/python folder.
* Removed export folder.
* Refactor Attribute COnverter logic

Signed-off-by: Acharya <[email protected]>

* Changing the translation and utils file.


Signed-off-by: Acharya <[email protected]>

* - Fixed Pylint issues
- Added Sigmoid operator.
- Add onnx, protobuf as CI pipeline dependencies.

Signed-off-by: Acharya <[email protected]>

* Add UTs for reduce ops.

Signed-off-by: Acharya <[email protected]>

* pylint - newline, whitespace.

Signed-off-by: Acharya <[email protected]>

*     Added operators:
    - AvgPool
    - ArgMax
    - ArgMin
    - Abs

    Minor changes in logic for import_onnx

* Added operators:
- Ceil
- Cast
- Constant

* - Added Pad operator support.
- Minor changes for comments

* RandomUniform,Normal,Sub,Mul,Div,Tanh,Relu,Reciprocal,Sqrt operators
added.

Signed-off-by: Acharya <[email protected]>

* lint fix

* Add protobuf-compile to CI bash script. Add MatMul and Pow operator.

Signed-off-by: Acharya <[email protected]>

* Max,Min,Sum,Reduce operators.

Signed-off-by: Acharya <[email protected]>

* BatchNorm,SpatialBN, Split

Signed-off-by: Acharya <[email protected]>

* Slice,Transpose and Squeeze Operators.

Signed-off-by: Acharya <[email protected]>

* Onnx tests in CI integration tests.

Signed-off-by: Acharya <[email protected]>

* Addressing Marco's comments

Signed-off-by: Acharya <[email protected]>

* Floor, LeakyRelu, Elu, PRelu, Softmax, Exp, Log operator.

* Added operators:
- Convolution
- Deconvolution

Refactored convert_operator

* lint fix

* Rebase fix

* Added Maxpool operator

* Adding FullyConnected operator

* Adding operator- GlobalPooling - max and avg
Minor lint fixes.

* Adding operator - Gemm

* Change test Path, LRN and Dropout operator.

* Add asserts for the super_res example.

Signed-off-by: Acharya <[email protected]>

* Fixing conv test failures.
Removed redundant code

* Update Jenkins job.

Signed-off-by: Acharya <[email protected]>

* Nits: Removing commented out code

* Rebase after Docker PR

* Fetch test files by version number. Verify the high resolution example.

Signed-off-by: Acharya <[email protected]>

* Fix method arguments for Python3.5+

Signed-off-by: Acharya <[email protected]>

* Remove logging configuration from test files.

Signed-off-by: Acharya <[email protected]>

* Verify result image in example by hash

Signed-off-by: Acharya <[email protected]>

* Remove fetching test files by ETag.
Will add it as a separate PR as per review comments.

Signed-off-by: Acharya <[email protected]>
@anirudhacharya anirudhacharya deleted the onnx1 branch March 31, 2018 20:10
rahul003 pushed a commit to rahul003/mxnet that referenced this pull request Jun 4, 2018
@anirudhacharya @nswamy
[MXNET-34] Onnx Module to import onnx models into mxnet (apache#9963)
37e8947 
* Onnx Module to import onnx models into mxnet

* Change package name to onnx from serde.
* Remove onnx install time dependency
* Remove Renamer class
* Add apache license to files.
* Refactor test files to tests/python folder.
* Removed export folder.
* Refactor Attribute COnverter logic

Signed-off-by: Acharya <[email protected]>

* Changing the translation and utils file.


Signed-off-by: Acharya <[email protected]>

* - Fixed Pylint issues
- Added Sigmoid operator.
- Add onnx, protobuf as CI pipeline dependencies.

Signed-off-by: Acharya <[email protected]>

* Add UTs for reduce ops.

Signed-off-by: Acharya <[email protected]>

* pylint - newline, whitespace.

Signed-off-by: Acharya <[email protected]>

*     Added operators:
    - AvgPool
    - ArgMax
    - ArgMin
    - Abs

    Minor changes in logic for import_onnx

* Added operators:
- Ceil
- Cast
- Constant

* - Added Pad operator support.
- Minor changes for comments

* RandomUniform,Normal,Sub,Mul,Div,Tanh,Relu,Reciprocal,Sqrt operators
added.

Signed-off-by: Acharya <[email protected]>

* lint fix

* Add protobuf-compile to CI bash script. Add MatMul and Pow operator.

Signed-off-by: Acharya <[email protected]>

* Max,Min,Sum,Reduce operators.

Signed-off-by: Acharya <[email protected]>

* BatchNorm,SpatialBN, Split

Signed-off-by: Acharya <[email protected]>

* Slice,Transpose and Squeeze Operators.

Signed-off-by: Acharya <[email protected]>

* Onnx tests in CI integration tests.

Signed-off-by: Acharya <[email protected]>

* Addressing Marco's comments

Signed-off-by: Acharya <[email protected]>

* Floor, LeakyRelu, Elu, PRelu, Softmax, Exp, Log operator.

* Added operators:
- Convolution
- Deconvolution

Refactored convert_operator

* lint fix

* Rebase fix

* Added Maxpool operator

* Adding FullyConnected operator

* Adding operator- GlobalPooling - max and avg
Minor lint fixes.

* Adding operator - Gemm

* Change test Path, LRN and Dropout operator.

* Add asserts for the super_res example.

Signed-off-by: Acharya <[email protected]>

* Fixing conv test failures.
Removed redundant code

* Update Jenkins job.

Signed-off-by: Acharya <[email protected]>

* Nits: Removing commented out code

* Rebase after Docker PR

* Fetch test files by version number. Verify the high resolution example.

Signed-off-by: Acharya <[email protected]>

* Fix method arguments for Python3.5+

Signed-off-by: Acharya <[email protected]>

* Remove logging configuration from test files.

Signed-off-by: Acharya <[email protected]>

* Verify result image in example by hash

Signed-off-by: Acharya <[email protected]>

* Remove fetching test files by ETag.
Will add it as a separate PR as per review comments.

Signed-off-by: Acharya <[email protected]>
zheng-da pushed a commit to zheng-da/incubator-mxnet that referenced this pull request Jun 28, 2018
@anirudhacharya @zheng-da
[MXNET-34] Onnx Module to import onnx models into mxnet (apache#9963)
c4b0b97 
* Onnx Module to import onnx models into mxnet

* Change package name to onnx from serde.
* Remove onnx install time dependency
* Remove Renamer class
* Add apache license to files.
* Refactor test files to tests/python folder.
* Removed export folder.
* Refactor Attribute COnverter logic

Signed-off-by: Acharya <[email protected]>

* Changing the translation and utils file.


Signed-off-by: Acharya <[email protected]>

* - Fixed Pylint issues
- Added Sigmoid operator.
- Add onnx, protobuf as CI pipeline dependencies.

Signed-off-by: Acharya <[email protected]>

* Add UTs for reduce ops.

Signed-off-by: Acharya <[email protected]>

* pylint - newline, whitespace.

Signed-off-by: Acharya <[email protected]>

*     Added operators:
    - AvgPool
    - ArgMax
    - ArgMin
    - Abs

    Minor changes in logic for import_onnx

* Added operators:
- Ceil
- Cast
- Constant

* - Added Pad operator support.
- Minor changes for comments

* RandomUniform,Normal,Sub,Mul,Div,Tanh,Relu,Reciprocal,Sqrt operators
added.

Signed-off-by: Acharya <[email protected]>

* lint fix

* Add protobuf-compile to CI bash script. Add MatMul and Pow operator.

Signed-off-by: Acharya <[email protected]>

* Max,Min,Sum,Reduce operators.

Signed-off-by: Acharya <[email protected]>

* BatchNorm,SpatialBN, Split

Signed-off-by: Acharya <[email protected]>

* Slice,Transpose and Squeeze Operators.

Signed-off-by: Acharya <[email protected]>

* Onnx tests in CI integration tests.

Signed-off-by: Acharya <[email protected]>

* Addressing Marco's comments

Signed-off-by: Acharya <[email protected]>

* Floor, LeakyRelu, Elu, PRelu, Softmax, Exp, Log operator.

* Added operators:
- Convolution
- Deconvolution

Refactored convert_operator

* lint fix

* Rebase fix

* Added Maxpool operator

* Adding FullyConnected operator

* Adding operator- GlobalPooling - max and avg
Minor lint fixes.

* Adding operator - Gemm

* Change test Path, LRN and Dropout operator.

* Add asserts for the super_res example.

Signed-off-by: Acharya <[email protected]>

* Fixing conv test failures.
Removed redundant code

* Update Jenkins job.

Signed-off-by: Acharya <[email protected]>

* Nits: Removing commented out code

* Rebase after Docker PR

* Fetch test files by version number. Verify the high resolution example.

Signed-off-by: Acharya <[email protected]>

* Fix method arguments for Python3.5+

Signed-off-by: Acharya <[email protected]>

* Remove logging configuration from test files.

Signed-off-by: Acharya <[email protected]>

* Verify result image in example by hash

Signed-off-by: Acharya <[email protected]>

* Remove fetching test files by ETag.
Will add it as a separate PR as per review comments.

Signed-off-by: Acharya <[email protected]>
@anirudhacharya anirudhacharya mentioned this pull request Jun 28, 2018
Merged
6 tasks
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@piiswrong piiswrong piiswrong left review comments

@sandeep-krishnamurthy sandeep-krishnamurthy sandeep-krishnamurthy left review comments

@reminisce reminisce reminisce left review comments

@Roshrini Roshrini Roshrini left review comments

@marcoabreu marcoabreu marcoabreu approved these changes

@nswamy nswamy nswamy approved these changes

@szha szha Awaiting requested review from szha szha is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
10 participants
@anirudhacharya @KellenSunderland @CodingCat @reminisce @piiswrong @nswamy @sandeep-krishnamurthy @Roshrini @marcoabreu @rajanksin