Skip to content
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.

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

Set onnx opset version before model select #37466

Closed
wants to merge 2 commits into from
Closed

Set onnx opset version before model select #37466

wants to merge 2 commits into from

Conversation

@suffiank
Copy link
Contributor

@suffiank suffiank commented Apr 28, 2020

Set opset version before model select call - which is used to trigger warnings.

@dr-ci
Copy link

@dr-ci dr-ci bot commented Apr 29, 2020

💊 Build failures summary and remediations

As of commit 66295e5 (more details on the Dr. CI page):


  • 1/1 failures possibly* introduced in this PR
    • 1/1 non-CircleCI failure(s)

ci.pytorch.org: 1 failed


This comment was automatically generated by Dr. CI (expand for details).Follow this link to opt-out of these comments for your Pull Requests.

Please report bugs/suggestions on the GitHub issue tracker.

See how this bot performed.

This comment has been revised 1 time.

Loading

@spandantiwari
Copy link
Contributor

@spandantiwari spandantiwari commented Apr 29, 2020

@suffiank - can you elaborate in the description what issue are you trying to solve?

Loading

@@ -498,9 +498,9 @@ def _export(model, args, f, export_params=True, verbose=False, training=None,
# If you really know what you're doing, you can turn
# training=TrainingMode.TRAINING or training=TrainingMode.PRESERVE,
# (to preserve whatever the original training mode was.)
_set_opset_version(opset_version)
_set_operator_export_type(operator_export_type)
Copy link
Contributor

@spandantiwari spandantiwari Apr 29, 2020

Choose a reason for hiding this comment

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

Will this set the original mode of the model (eval vs train) after the export in all scenarios? Just trying to understand if there's any effect of taking these two steps outside the with block. @lara-hdr

Loading

Copy link
Contributor Author

@suffiank suffiank Apr 29, 2020

Choose a reason for hiding this comment

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

So I noticed the message print even when I set opset 12. Initially I thought the parameter wasn't being respected and an edit was required. Turns out it's just a bad message. I then matched the call sequence as in the pretty export function.

Loading

@@ -43,8 +43,8 @@ def select_model_mode_for_export(model, mode):
if is_originally_training:
warnings.warn("You are exporting the model to ONNX while in training mode with "
"'train' parameter not specified. The model will default to inference mode export. "
"If you wish to export a training amenable ONNX model, specify train=TrainingMode.TRAIN or "
"train=TrainingMode.PRESERVE (to preserve the original model state) in torch.onnx.export().")
"If you wish to export a training amenable ONNX model, specify training=TrainingMode.TRAINING or "
Copy link
Contributor

@spandantiwari spandantiwari Apr 29, 2020

Choose a reason for hiding this comment

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

This looks good.

Loading

@spandantiwari
Copy link
Contributor

@spandantiwari spandantiwari commented Apr 29, 2020

@suffiank - can you please add a test to cover this?

Loading

@spandantiwari spandantiwari requested a review from lara-hdr Apr 29, 2020
@suffiank
Copy link
Contributor Author

@suffiank suffiank commented Apr 29, 2020

@suffiank - can you please add a test to cover this?

So I already see tests to cover expected opset version. And also model state is preserved after export (eval vs. training). Since this only fixes printing of warning message, it's better without?

Loading

@lara-hdr
Copy link
Contributor

@lara-hdr lara-hdr commented Apr 29, 2020

Thanks @suffiank. I had also created a similar PR #37397.
@houseroad could you please review/merge either PR?

Loading

Copy link
Contributor

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

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

Loading

@facebook-github-bot
Copy link
Contributor

@facebook-github-bot facebook-github-bot commented Apr 30, 2020

@houseroad merged this pull request in d5363e6.

Loading

@suffiank suffiank deleted the sukha/set-opset-version-before-select-model branch Apr 30, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants