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

Onnx multi output #13390

Merged
ThomasDelteil merged 4 commits into from
Nov 26, 2018
Merged

Onnx multi output #13390

ThomasDelteil merged 4 commits into from
Nov 26, 2018

Conversation

@safrooze
Copy link
Contributor

@safrooze safrooze commented Nov 24, 2018

Description

ONNX export does not create a correct graph when there are multiple outputs. This PR fixes this bug.

Checklist

Essentials

Please feel free to remove inapplicable items for your PR.

  • 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:
  • To the my best knowledge, examples are either not affected by this change, or have been fixed to be compatible with this change

Changes

  • Fixed ONNX export to handle graphs with multiple outputs.
  • Added unit-test for ONNX export and output to test both single output and multi-output graphs

Comments

Before this change, export_onnx would treat the graph as a sequential graph with output as the last node in the sequence. This change fixes this behavior by using sym.list_outputs() to record all output nodes.

@safrooze safrooze requested a review from szha as a code owner November 24, 2018 05:32
@safrooze
Copy link
Contributor Author

safrooze commented Nov 24, 2018

@Roshrini Please review.

anirudhacharya
anirudhacharya reviewed Nov 24, 2018
View reviewed changes
@anirudhacharya
Copy link
Member

anirudhacharya commented Nov 24, 2018

Thanks for doing this.

marcoabreu
marcoabreu reviewed Nov 24, 2018
View reviewed changes
@stu1130
Copy link
Contributor

stu1130 commented Nov 25, 2018

@mxnet-label-bot add [pr-awaiting-response]
Thanks for your contribution @safrooze

@marcoabreu marcoabreu added the pr-awaiting-response PR is reviewed and waiting for contributor to respond label Nov 25, 2018
@safrooze
Add ONNX unit-test
36f1441
@safrooze
Added multi-output shape inference.
11e7012 
- Removed unnecessary forward_pass() call
- Modified infer_output_shape to return multiple shapes for multiple outputs as well as output names.
@safrooze
Copy link
Contributor Author

safrooze commented Nov 26, 2018

@anirudhacharya all issues addressed. Please review asap. I'd like this to be part of tonight's nightly if possible.

anirudhacharya
anirudhacharya reviewed Nov 26, 2018
View reviewed changes
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.

minor comments/questions. mostly LGTM.

@safrooze
Copy link
Contributor Author

safrooze commented Nov 26, 2018

@mxnet-label-bot remove [pr-awaiting-response]

@marcoabreu marcoabreu removed the pr-awaiting-response PR is reviewed and waiting for contributor to respond label Nov 26, 2018
ThomasDelteil
ThomasDelteil approved these changes Nov 26, 2018
View reviewed changes
Copy link
Contributor

@ThomasDelteil ThomasDelteil left a comment

Choose a reason for hiding this comment

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

Spoke with Sina offline, let's make sure that the infer shape solution works with network that have loss layers, otherwise LGTM

@ThomasDelteil ThomasDelteil merged commit cb627bc into apache:master Nov 26, 2018
@safrooze
Copy link
Contributor Author

safrooze commented Nov 26, 2018

The current implementation of handling existence of training labels as part of the symbolic graph very broken. There are multiple issues with it:

  1. It assumes that all layers that depend on label are 'Output' layers and hence don't need label for forward. This isn't necessarily true if someone implements an actual loss as part of the forward pass.
  2. It assumes there is a single label and that label is named in a very specific way, which can easily be incorrect.

My PR does not change this fragile implementation behavior, but this needs to be fixed. I created #13407 to address this issue.

This was referenced Nov 27, 2018
Closed
Merged
@safrooze safrooze deleted the onnx-multi-output branch November 27, 2018 20:57
@safrooze safrooze mentioned this pull request Nov 27, 2018
Merged
4 tasks
@vandanavk vandanavk mentioned this pull request Dec 12, 2018
Merged
5 tasks
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

3 more reviewers

@marcoabreu marcoabreu marcoabreu left review comments

@anirudhacharya anirudhacharya anirudhacharya approved these changes

@ThomasDelteil ThomasDelteil ThomasDelteil approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@safrooze @anirudhacharya @stu1130 @ThomasDelteil @marcoabreu