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
Add type annotations to torch.onnx.* modules #45258
Conversation
💊 CI failures summary and remediationsAs of commit 1b52305 (more details on the Dr. CI page):
Extra GitHub checks: 1 failed
codecov.io: 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 or post in the (internal) Dr. CI Users group. This comment has been revised 63 times. |
@@ -76,7 +77,7 @@ def export(model, args, f, export_params=True, verbose=False, training=None, | |||
if aten or export_raw_ir: | |||
assert operator_export_type is None | |||
assert aten ^ export_raw_ir | |||
operator_export_type = OperatorExportTypes.ATEN if aten else OperatorExportTypes.RAW | |||
operator_export_type = OperatorExportTypes.ONNX_ATEN if aten else OperatorExportTypes.RAW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is no mention to OperatorExportTypes.ATEN
in the source code. So, I think this value was supposed to be OperatorExportTypes.ONNX_ATEN
.
@@ -466,7 +468,7 @@ def export_to_pretty_string(model, args, f, export_params=True, verbose=False, t | |||
if aten or export_raw_ir: | |||
assert operator_export_type is None | |||
assert aten ^ export_raw_ir | |||
operator_export_type = OperatorExportTypes.ATEN if aten else OperatorExportTypes.RAW | |||
operator_export_type = OperatorExportTypes.ONNX_ATEN if aten else OperatorExportTypes.RAW |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same here
@@ -432,7 +434,7 @@ def _model_to_graph(model, args, verbose=False, | |||
param_names = input_and_param_names[len(input_and_param_names) - len(params):] | |||
params_dict = dict(zip(param_names, params)) | |||
|
|||
if training is None or training == TrainingMode.EVAL or (training == TrainingMode.PRESERVE and not is_originally_training): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is_originally_training
is not defined anywhere. So, I guess this part of the conditional isn't executed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks @guilhermeleobas
This has a merge conflict, can you rebase @guilhermeleobas? |
Failures are real
Edit: Fixed! CI should be all green now |
@guilhermeleobas there's still CI failures here that are real:
Can you update the PR? |
@rgommers those issues should now be fixed. The current mypy failures are not related to any changes introduced in this PR and there are fixed (ignored) in a different PR
|
Yes. Would be nice to get those fixes landed first and then re-run the CI on this PR, because the |
Failure is not related to this PR:
|
LGTM now, thanks @guilhermeleobas. @malfet would you be able to land this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ezyang has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.
Unlanding. This appears to have broken multiple builds (see the Github CI, too). Sample snippet:
|
@rgommers done! |
Codecov Report
@@ Coverage Diff @@
## master #45258 +/- ##
=======================================
Coverage 80.83% 80.83%
=======================================
Files 1859 1859
Lines 200633 200639 +6
=======================================
+ Hits 162177 162182 +5
- Misses 38456 38457 +1 |
@guilhermeleobas could you please reopen a new PR? I cna't import this one anymore |
@ezyang, sure! |
Fixes #45215
Still need to resolve a few mypy issues before a review. In special, there is an error which I don't know how to solve, see:
is_originally_training
is used but never defined/imported ontorch/onnx/utils.py
,