ONNX export: Add Flatten before Gemm by vandanavk · Pull Request #13356 · apache/mxnet

vandanavk · 2018-11-21T18:35:28Z

Description

ONNX's spec specifies that Gemm input data should be 2D, but this wasn't the case for VGG and ResNet models trained in MXNet and exported to ONNX. This PR adds a flatten node before Gemm to make the input data 2D.

Related issues: onnx/models#90, onnx/onnx#1101, onnx/models#91 (comment)

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

The PR title starts with [MXNET-$JIRA_ID], where $JIRA_ID refers to the relevant JIRA issue created (except PRs with tiny changes)
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)
Nightly tests are added for complicated/long-running ones (e.g. changing distributed kvstore)
Build tests will be added for build configuration changes (e.g. adding a new build option with NCCL)
Code is well-documented:
For user-facing API changes, API doc string has been updated.
For new C++ functions in header files, their functionalities and arguments are documented.
For new examples, README.md is added to explain the what the example does, the source of the dataset, expected performance on test set and reference to the original paper if applicable
Check the API doc at http://mxnet-ci-doc.s3-accelerate.dualstack.amazonaws.com/PR-$PR_ID/$BUILD_ID/index.html
To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

Add a flatten node before Gemm

Comments

Testing done:

Exported all VGG models (https://github.com/onnx/models/tree/master/models/image_classification/vgg) and ResNetv1 models (https://github.com/onnx/models/tree/master/models/image_classification/resnet) and executed the validation notebook.
A test in mxnet_export_test for fully connected

anirudhacharya

how are we testing this change?

vandanavk · 2018-11-21T19:17:09Z

@anirudhacharya the test is WIP (added a note in the PR description). Adding a test for fully connected in mxnet_export_test.

vandanavk · 2018-11-21T19:17:27Z

@mxnet-label-bot add [ONNX, pr-work-in-progress]

python/mxnet/contrib/onnx/mx2onnx/export_onnx.py

vandanavk · 2018-11-29T23:15:16Z

@mxnet-label-bot update [ONNX, pr-awaiting-review]

nswamy · 2018-12-03T04:32:41Z

@Roshrini, @anirudhacharya for review?

Roshrini

Tested this fix on Arcface model in model zoo, LGTM

nswamy · 2018-12-08T03:50:51Z

@vandanavk
Please review the CI failure

  _, out_shapes, _ = sym.infer_shape(**inputs)

Traceback (most recent call last):

  File "tests/python-pytest/onnx/export/mxnet_export_test.py", line 486, in <module>

    test_models("bvlc_googlenet", (1, 3, 224, 224), (1, 1000))

  File "tests/python-pytest/onnx/export/mxnet_export_test.py", line 142, in test_models

    converted_model_path = onnx_mxnet.export_model(sym, params, [input_shape], np.float32, onnx_file)

  File "/work/mxnet/python/mxnet/contrib/onnx/mx2onnx/export_model.py", line 87, in export_model

    verbose=verbose)

  File "/work/mxnet/python/mxnet/contrib/onnx/mx2onnx/export_onnx.py", line 211, in create_onnx_graph_proto

    graph_outputs = MXNetGraph.get_outputs(sym, params, in_shape, output_label)

  File "/work/mxnet/python/mxnet/contrib/onnx/mx2onnx/export_onnx.py", line 152, in get_outputs

    assert len(out_shapes) == len(out_names)

codecov-io · 2018-12-10T23:35:06Z

Codecov Report

Merging #13356 into master will increase coverage by 0.53%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master   #13356      +/-   ##
==========================================
+ Coverage   82.73%   83.26%   +0.53%     
==========================================
  Files         721      746      +25     
  Lines       79494    83497    +4003     
  Branches     3193     3193              
==========================================
+ Hits        65770    69526    +3756     
- Misses      12864    13113     +249     
+ Partials      860      858       -2

Impacted Files	Coverage Δ
...hon/mxnet/contrib/onnx/mx2onnx/_op_translations.py	`18.55% <0%> (-0.14%)`	⬇️
src/operator/nn/depthwise_convolution_tf.cuh	`83.52% <0%> (-2.36%)`	⬇️
src/operator/contrib/quadratic_op-inl.h	`96.84% <0%> (-2.11%)`	⬇️
src/operator/softmax_output-inl.h	`59.19% <0%> (-0.9%)`	⬇️
python/mxnet/visualization.py	`44.94% <0%> (ø)`	⬆️
tests/cpp/kvstore/gpu_topology_test.cc	`97.28% <0%> (ø)`
tests/cpp/operator/batchnorm_test.cc	`87.07% <0%> (ø)`
tests/cpp/operator/runner/core_op_runner_test.cc	`95.86% <0%> (ø)`
tests/cpp/operator/fully_conn_perf.cc	`94.73% <0%> (ø)`
tests/cpp/include/test_core_op.h	`95.23% <0%> (ø)`
... and 23 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ba02bf2...739f181. Read the comment docs.

vandanavk · 2018-12-11T23:31:45Z

@zhreshold for review

Roshrini · 2018-12-19T01:04:44Z

@zhreshold Can you please review/merge this PR?

vandanavk · 2018-12-20T21:04:38Z

@mxnet-label-bot update [ONNX, pr-awaiting-merge]

* Add Flatten before Gemm * ONNX export test: Allow multiple inputs in forward pass * ONNX export: Test for fully connected

vandanavk requested a review from szha as a code owner November 21, 2018 18:35

anirudhacharya reviewed Nov 21, 2018

View reviewed changes

marcoabreu added ONNX pr-work-in-progress PR is still work in progress labels Nov 21, 2018

vandanavk changed the title ~~ONNX export: Add Flatten before Gemm~~ [WIP] ONNX export: Add Flatten before Gemm Nov 21, 2018

vandanavk commented Nov 21, 2018

View reviewed changes

python/mxnet/contrib/onnx/mx2onnx/export_onnx.py Outdated Show resolved Hide resolved

vandanavk force-pushed the onnx_gemm branch 2 times, most recently from 372cb0a to 8cd959d Compare November 26, 2018 17:19

vandanavk force-pushed the onnx_gemm branch 3 times, most recently from d70ff8a to bcc947d Compare November 29, 2018 22:58

vandanavk changed the title ~~[WIP] ONNX export: Add Flatten before Gemm~~ ONNX export: Add Flatten before Gemm Nov 29, 2018

marcoabreu added pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress labels Nov 29, 2018

vandanavk force-pushed the onnx_gemm branch 2 times, most recently from 2421bd0 to 2975ab8 Compare December 7, 2018 18:24

Roshrini approved these changes Dec 7, 2018

View reviewed changes

vandanavk force-pushed the onnx_gemm branch 2 times, most recently from b323418 to 739f181 Compare December 10, 2018 21:46

vandanavk force-pushed the onnx_gemm branch 4 times, most recently from dd22dc1 to 24f47c2 Compare December 17, 2018 19:53

vandanavk added 3 commits December 18, 2018 09:51

Add Flatten before Gemm

647f30e

ONNX export test: Allow multiple inputs in forward pass

c93fbc5

ONNX export: Test for fully connected

5e632a0

vandanavk force-pushed the onnx_gemm branch from 24f47c2 to 5e632a0 Compare December 18, 2018 17:51

zhreshold approved these changes Dec 19, 2018

View reviewed changes

marcoabreu added pr-awaiting-merge Review and CI is complete. Ready to Merge and removed pr-awaiting-review PR is waiting for code review labels Dec 20, 2018

ThomasDelteil merged commit a4f2ed5 into apache:master Dec 20, 2018

rondogency pushed a commit to rondogency/incubator-mxnet that referenced this pull request Jan 9, 2019

ONNX export: Add Flatten before Gemm (apache#13356)

0de8760

* Add Flatten before Gemm * ONNX export test: Allow multiple inputs in forward pass * ONNX export: Test for fully connected

This was referenced Jan 9, 2019

Problems with ResNet18 onnx/models#87

Closed

Duplicate VGG19 models onnx/models#93

Closed

haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019

ONNX export: Add Flatten before Gemm (apache#13356)

aac961f

* Add Flatten before Gemm * ONNX export test: Allow multiple inputs in forward pass * ONNX export: Test for fully connected

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ONNX export: Add Flatten before Gemm#13356

ONNX export: Add Flatten before Gemm#13356
ThomasDelteil merged 3 commits intoapache:masterfrom
vandanavk:onnx_gemm

vandanavk commented Nov 21, 2018 •

edited

Loading

Uh oh!

anirudhacharya left a comment

Uh oh!

vandanavk commented Nov 21, 2018

Uh oh!

vandanavk commented Nov 21, 2018

Uh oh!

Uh oh!

vandanavk commented Nov 29, 2018

Uh oh!

nswamy commented Dec 3, 2018

Uh oh!

Roshrini left a comment

Uh oh!

nswamy commented Dec 8, 2018

Uh oh!

codecov-io commented Dec 10, 2018

Uh oh!

vandanavk commented Dec 11, 2018

Uh oh!

Roshrini commented Dec 19, 2018

Uh oh!

vandanavk commented Dec 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

Conversation

vandanavk commented Nov 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Checklist

Essentials

Changes

Comments

Uh oh!

anirudhacharya left a comment

Choose a reason for hiding this comment

Uh oh!

vandanavk commented Nov 21, 2018

Uh oh!

vandanavk commented Nov 21, 2018

Uh oh!

Uh oh!

vandanavk commented Nov 29, 2018

Uh oh!

nswamy commented Dec 3, 2018

Uh oh!

Roshrini left a comment

Choose a reason for hiding this comment

Uh oh!

nswamy commented Dec 8, 2018

Uh oh!

codecov-io commented Dec 10, 2018

Codecov Report

Uh oh!

vandanavk commented Dec 11, 2018

Uh oh!

Roshrini commented Dec 19, 2018

Uh oh!

vandanavk commented Dec 20, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

vandanavk commented Nov 21, 2018 •

edited

Loading