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

Onnx Dynamic Shapes Support#20001

Merged
Zha0q1 merged 12 commits intoapache:v1.xfrom
Zha0q1:onnx_dynamic_shapes
Mar 12, 2021
Merged

Onnx Dynamic Shapes Support#20001
Zha0q1 merged 12 commits intoapache:v1.xfrom
Zha0q1:onnx_dynamic_shapes

Conversation

@Zha0q1
Copy link
Contributor

@Zha0q1 Zha0q1 commented Mar 10, 2021

RFC #20000

@Zha0q1 Zha0q1 requested a review from szha as a code owner March 10, 2021 05:23
@mxnet-bot
Copy link

mxnet-bot commented Mar 10, 2021

Hey @Zha0q1 , Thanks for submitting the PR
All tests are already queued to run once. If tests fail, you can trigger one or more tests again with the following commands:

  • To trigger all jobs: @mxnet-bot run ci [all]
  • To trigger specific jobs: @mxnet-bot run ci [job1, job2]

CI supported jobs: [windows-gpu, unix-cpu, sanity, centos-cpu, website, edge, windows-cpu, miscellaneous, centos-gpu, unix-gpu, clang]

Note:
Only following 3 categories can trigger CI :PR Author, MXNet Committer, Jenkins Admin.
All CI tests must pass before the PR can be merged.

@lanking520 lanking520 added the pr-work-in-progress PR is still work in progress label Mar 10, 2021
@Zha0q1 Zha0q1 changed the title [WIP] Onnx Dynamic Shapes Support Onnx Dynamic Shapes Support Mar 10, 2021
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Mar 11, 2021
@Zha0q1
Copy link
Contributor Author

Zha0q1 commented Mar 11, 2021

@mxnet-bot run ci [all]

@mxnet-bot
Copy link

mxnet-bot commented Mar 11, 2021

Jenkins CI successfully triggered : [centos-gpu, centos-cpu, website, clang, unix-gpu, miscellaneous, unix-cpu, windows-cpu, edge, windows-gpu, sanity]

@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test and removed pr-work-in-progress PR is still work in progress labels Mar 11, 2021
@lanking520 lanking520 added pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Mar 11, 2021
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Mar 11, 2021
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-work-in-progress PR is still work in progress and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Mar 11, 2021
@lanking520 lanking520 added pr-awaiting-testing PR is reviewed and waiting CI build and test pr-awaiting-review PR is waiting for code review and removed pr-work-in-progress PR is still work in progress pr-awaiting-testing PR is reviewed and waiting CI build and test labels Mar 12, 2021
waytrue17
waytrue17 reviewed Mar 12, 2021
View reviewed changes
python/mxnet/contrib/onnx/mx2onnx/export_model.py
# Create the model (ModelProto)
onnx_model = helper.make_model(onnx_graph)

# Run shape inference on the model. Due to ONNX bug/incompatibility this may or may not crash
Copy link
Contributor

@waytrue17 waytrue17 Mar 12, 2021

Choose a reason for hiding this comment

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

Why do we run shape inference here? Seems previously we don't have it

Copy link
Contributor Author

@Zha0q1 Zha0q1 Mar 12, 2021

Choose a reason for hiding this comment

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

This is an optional step. Doing shape inference may help with some runtime optimization and we can visualize the graph better, since the noes will have input and output shapes labeled

Copy link
Contributor Author

@Zha0q1 Zha0q1 Mar 12, 2021

Choose a reason for hiding this comment

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

shape inference is default to off

Copy link
Contributor

@waytrue17 waytrue17 Mar 12, 2021

Choose a reason for hiding this comment

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

Makes sense. We can keep it here, and turn it off if it crashes on some cases later on.

waytrue17
waytrue17 approved these changes Mar 12, 2021
View reviewed changes
Copy link
Contributor

@waytrue17 waytrue17 left a comment

Choose a reason for hiding this comment

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

LGTM, thanks

@Zha0q1 Zha0q1 merged commit 601e3b9 into apache:v1.x Mar 12, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

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

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

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

1 more reviewer

@waytrue17 waytrue17 waytrue17 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

pr-awaiting-review PR is waiting for code review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Zha0q1 @mxnet-bot @waytrue17 @lanking520