Skip to content

Adding support for exporting models with variable length input/output to ONNX #20034

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
pk-g wants to merge 2 commits into from
Closed

Adding support for exporting models with variable length input/output to ONNX #20034

pk-g wants to merge 2 commits into from

Conversation

@pk-g
Copy link
Contributor

@pk-g pk-g commented May 1, 2019 •
edited by houseroad
Loading

@pytorchbot pytorchbot added oncall: jit Add this issue/PR to JIT oncall triage queue module: onnx Related to torch.onnx module: pybind Related to our Python bindings / interactions with other Python libraries labels May 1, 2019
@pk-g pk-g force-pushed the peykash/var_length_export branch 2 times, most recently from cc22369 to 0ff3c48 Compare May 3, 2019 22:00
@ezyang ezyang requested a review from houseroad May 6, 2019 14:39
houseroad
houseroad reviewed May 8, 2019
View reviewed changes
test/onnx/test_operators.py Outdated
Copy link
Member

@houseroad houseroad May 8, 2019

Choose a reason for hiding this comment

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

Do you mean when we use dynamic_axes, we must set input_names and output_names as well?

Copy link
Contributor Author

@pk-g pk-g May 8, 2019

Choose a reason for hiding this comment

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

Not exactly. User does NOT have to to provide input_names/ output_names on export API, if they already know what names are being used in the graph, they shall simply provide such names as keys to dynamic_axes dictionary and NOT specify input_names / output_names . however, if they do not know the names OR intend to use different names on exported model, they also have the option. They can do so by providing those names in input_names / output_names and using them in dynamic_axes.

@pk-g
Copy link
Contributor Author

pk-g commented May 13, 2019

@houseroad gentle reminder in case there are more feedback on this. thanks!

houseroad
houseroad reviewed May 20, 2019
View reviewed changes
houseroad
houseroad reviewed May 20, 2019
View reviewed changes
houseroad
houseroad reviewed May 20, 2019
View reviewed changes
houseroad
houseroad reviewed May 20, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

I think we should ask users to specify the input_names/output_names if they use dynamic_axes. Otherwise, they have to guess the name of the inputs/outputs, because they don't have the control of them.

@pk-g pk-g force-pushed the peykash/var_length_export branch from 0ff3c48 to 67eeec3 Compare May 31, 2019 02:02
@pk-g
Copy link
Contributor Author

pk-g commented May 31, 2019 •
edited
Loading

I think we should ask users to specify the input_names/output_names if they use dynamic_axes. Otherwise, they have to guess the name of the inputs/outputs, because they don't have the control of them.

@houseroad that's a good point. we are already letting user know by generating a warning during _validate_dynamic_axes method. The warning is essentially generated if names are not matching or they are not provided as input/output names.

@pk-g pk-g force-pushed the peykash/var_length_export branch 3 times, most recently from 7299d67 to f814e2d Compare June 3, 2019 18:07
@pk-g
Copy link
Contributor Author

pk-g commented Jun 3, 2019

@houseroad gentle reminder ... thanks!

facebook-github-bot
facebook-github-bot reviewed Jun 3, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

houseroad
houseroad approved these changes Jun 3, 2019
View reviewed changes
Copy link
Member

@houseroad houseroad left a comment

Choose a reason for hiding this comment

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

LGTM

@pk-g pk-g force-pushed the peykash/var_length_export branch from f814e2d to 05ae9d4 Compare June 4, 2019 17:21
facebook-github-bot
facebook-github-bot reviewed Jun 5, 2019
View reviewed changes
Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

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

@houseroad has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@facebook-github-bot
Copy link
Contributor

facebook-github-bot commented Jun 5, 2019

@houseroad merged this pull request in 98e3aae.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@facebook-github-bot facebook-github-bot facebook-github-bot left review comments

@houseroad houseroad houseroad approved these changes

Assignees

No one assigned

Labels

Merged module: onnx Related to torch.onnx module: pybind Related to our Python bindings / interactions with other Python libraries oncall: jit Add this issue/PR to JIT oncall triage queue open source

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@pk-g @facebook-github-bot @houseroad @ezyang @pytorchbot @mruberry