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

[v1.x] Onnx Support for Dropout #19837

Merged
merged 6 commits into from
Feb 8, 2021
Merged

Conversation

Zha0q1
Copy link
Contributor

@Zha0q1 Zha0q1 commented Feb 4, 2021

For inference we can just use Identity for Dropout

@Zha0q1 Zha0q1 requested a review from szha as a code owner February 4, 2021 02:50
@mxnet-bot
Copy link

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-cpu, unix-cpu, unix-gpu, website, windows-gpu, centos-gpu, sanity, centos-cpu, miscellaneous, edge, 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 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 Feb 4, 2021
return nodes
else:
return [make_node("Dropout", input_nodes, [name], ratio=probability, name=name)]
if axes != [None]:
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you need to check for string value 'None'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

convert_string_to_list() will return [None] if input is 'None'

@@ -1125,16 +1125,19 @@ def convert_dropout(node, **kwargs):
opset_version = kwargs["opset_version"]

probability = float(attrs.get("p", 0.5))
axes = convert_string_to_list((attrs.get("axes", "None")))
Copy link
Contributor

Choose a reason for hiding this comment

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

Double parenthesis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch!

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

@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 Feb 4, 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 Feb 5, 2021
@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 Feb 5, 2021
return nodes
else:
return [make_node("Dropout", input_nodes, [name], ratio=probability, name=name)]
if mode != 'training':
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want to check and preserve the Dropout nodes if mode is training? Otherwise if inference mode, return identity op.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think dropout is almost never used in inference because the output values will be randomly dropped and theoretically it's designed for training only. I think not supporting 'both' mode, at least for now, can confirm this while we export the models in the model zoo.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, just trying to understand the mode parameter and how it applies to these training-only ops. Is mode set to 'training' in the models or are you just depending on the default from attrs.get() above?

Just wondering for future plans to export models for training (if needed.) Do we need to specify whether we are exporting for training or for inference in the export call, or can we make it operator dependent?

Copy link
Contributor

Choose a reason for hiding this comment

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

Feel free to merge, we should just continue this conversation for future uses.

Copy link
Contributor Author

@Zha0q1 Zha0q1 Feb 6, 2021

Choose a reason for hiding this comment

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

Sorry, just trying to understand the mode parameter and how it applies to these training-only ops. Is mode set to 'training' in the models or are you just depending on the default from attrs.get() above?

Just wondering for future plans to export models for training (if needed.) Do we need to specify whether we are exporting for training or for inference in the export call, or can we make it operator dependent?

mode is a parameter to MXNet operator Dropout only; it's now a parameter for the model :). I think since we targeting inference we might be able to set it to Identity for now

@lanking520 lanking520 added pr-awaiting-review PR is waiting for code review and removed pr-awaiting-testing PR is reviewed and waiting CI build and test labels Feb 6, 2021
Copy link
Contributor

@josephevans josephevans left a comment

Choose a reason for hiding this comment

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

LGTM!

@Zha0q1 Zha0q1 merged commit 0066ccb into apache:v1.x Feb 8, 2021
@access2rohit access2rohit mentioned this pull request Feb 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
pr-awaiting-review PR is waiting for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants