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

ONNX export: Add Flatten before Gemm #13356

Merged
merged 3 commits into from
Dec 20, 2018
Merged

Conversation

vandanavk
Copy link
Contributor

@vandanavk vandanavk commented Nov 21, 2018

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:https://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:
  1. 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.
  2. A test in mxnet_export_test for fully connected

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

@anirudhacharya anirudhacharya left a comment

Choose a reason for hiding this comment

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

how are we testing this change?

@vandanavk
Copy link
Contributor Author

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

@vandanavk
Copy link
Contributor Author

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

@marcoabreu marcoabreu added ONNX pr-work-in-progress PR is still work in progress labels Nov 21, 2018
@vandanavk vandanavk changed the title ONNX export: Add Flatten before Gemm [WIP] ONNX export: Add Flatten before Gemm Nov 21, 2018
@vandanavk vandanavk force-pushed the onnx_gemm branch 2 times, most recently from 372cb0a to 8cd959d Compare November 26, 2018 17:19
@vandanavk vandanavk force-pushed the onnx_gemm branch 3 times, most recently from d70ff8a to bcc947d Compare November 29, 2018 22:58
@vandanavk vandanavk changed the title [WIP] ONNX export: Add Flatten before Gemm ONNX export: Add Flatten before Gemm Nov 29, 2018
@vandanavk
Copy link
Contributor Author

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

@marcoabreu 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
@nswamy
Copy link
Member

nswamy commented Dec 3, 2018

@Roshrini, @anirudhacharya for review?

@vandanavk vandanavk force-pushed the onnx_gemm branch 2 times, most recently from 2421bd0 to 2975ab8 Compare December 7, 2018 18:24
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.

Tested this fix on Arcface model in model zoo, LGTM

@nswamy
Copy link
Member

nswamy commented Dec 8, 2018

@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)

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

Codecov Report

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

Impacted file tree graph

@@            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
Copy link
Contributor Author

@zhreshold for review

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

@zhreshold Can you please review/merge this PR?

@vandanavk
Copy link
Contributor Author

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

@marcoabreu 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 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
* Add Flatten before Gemm

* ONNX export test: Allow multiple inputs in forward pass

* ONNX export: Test for fully connected
haohuanw pushed a commit to haohuanw/incubator-mxnet that referenced this pull request Jun 23, 2019
* Add Flatten before Gemm

* ONNX export test: Allow multiple inputs in forward pass

* ONNX export: Test for fully connected
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
ONNX pr-awaiting-merge Review and CI is complete. Ready to Merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants