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

Fix ATen Fallback for BUILD_CAFFE2=0 for ONNX-only ops #88504

Closed

Conversation

thiagocrepaldi
Copy link
Collaborator

@thiagocrepaldi thiagocrepaldi commented Nov 4, 2022

Follow-up for #87735

A new corner case was identified and fixed.
A new unit test has been added to guard it when BUILD_CAFFE2=0 is executed on CI again

Fixes #87313

@pytorch-bot
Copy link

pytorch-bot bot commented Nov 4, 2022

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/88504

Note: Links to docs will display an error until the docs builds have been completed.

❌ 2 Failures

As of commit 10bf121:

The following jobs have failed:

This comment was automatically generated by Dr. CI and updates every 15 minutes.

@pytorch-bot pytorch-bot bot added the release notes: onnx torch.onnx related changes that should show up in the release notes label Nov 4, 2022
@thiagocrepaldi thiagocrepaldi added module: onnx Related to torch.onnx topic: bug fixes topic category labels Nov 4, 2022
Copy link
Collaborator

@justinchuby justinchuby left a comment

Choose a reason for hiding this comment

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

LGTM🚀

@BowenBao
Copy link
Collaborator

BowenBao commented Nov 9, 2022

@pytorchbot merge -g

@pytorch-bot pytorch-bot bot added the ciflow/trunk Trigger trunk jobs on your pull request label Nov 9, 2022
@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (ETA: 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase by leaving the following comment on this PR:
@pytorchbot rebase

Details for Dev Infra team Raised by workflow job

@thiagocrepaldi
Copy link
Collaborator Author

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (ETA: 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: This PR is too stale; the last push date was more than 3 days ago. Please rebase and try again. You can rebase by leaving the following comment on this PR:
@pytorchbot rebase

Details for Dev Infra team Raised by workflow job

Follow-up for pytorch#87735

Once again, because BUILD_CAFFE2=0 is not tested for ONNX exporter, one
scenario slipped through. A use case where the model can be exported
without aten fallback when operator_export_type=ONNX_ATEN_FALLBACK and
BUILD_CAFFE2=0

A new unit test has been added, but it won't prevent regressions if
BUILD_CAFFE2=0 is not executed on CI again
@thiagocrepaldi
Copy link
Collaborator Author

@pytorchbot merge -g

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged once all checks on your PR pass since you used the green (-g) flag (ETA: 0-4 Hours).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@pytorchmergebot
Copy link
Collaborator

Merge failed

Reason: 2 additional jobs have failed, first few of them are: trunk ,trunk / macos-12-py3-x86-64 / test (default, 2, 2, macos-12)

Details for Dev Infra team Raised by workflow job

@thiagocrepaldi
Copy link
Collaborator Author

@pytorchbot merge -f "2 failures Unrelated to ONNX"

@pytorchmergebot
Copy link
Collaborator

Merge started

Your change will be merged immediately since you used the force (-f) flag, bypassing any CI checks (ETA: 1-5 minutes).

Learn more about merging in the wiki.

Questions? Feedback? Please reach out to the PyTorch DevX Team

Advanced Debugging
Check the merge workflow status
here

@thiagocrepaldi thiagocrepaldi added module: regression It used to work, and now it doesn't triage review labels Nov 16, 2022
@malfet
Copy link
Contributor

malfet commented Nov 28, 2022

It is not a regression in a sense that it behaved the same in 1.12.1 isn't it?

@thiagocrepaldi
Copy link
Collaborator Author

@malfet This is actually a regression. The bug reported on 1.12.1 was similar and triggered a full review of the feature after a buggy feature reimplementation

@thiagocrepaldi
Copy link
Collaborator Author

#84382 was the base PR that replaced the "old" implementation for onnx symbolic registry, leading to a regression on 1.13 that was fixed by this PR

izaitsevfb pushed a commit to izaitsevfb/pytorch that referenced this pull request Dec 3, 2022
Follow-up for pytorch#87735

Once again, because BUILD_CAFFE2=0 is not tested for ONNX exporter, one scenario slipped through. A use case where the model can be exported without aten fallback when operator_export_type=ONNX_ATEN_FALLBACK and BUILD_CAFFE2=0

A new unit test has been added, but it won't prevent regressions if BUILD_CAFFE2=0 is not executed on CI again

Fixes pytorch#87313

Pull Request resolved: pytorch#88504
Approved by: https://github.com/justinchuby, https://github.com/BowenBao

(cherry picked from commit 5f0783b)
izaitsevfb pushed a commit to izaitsevfb/pytorch that referenced this pull request Dec 3, 2022
Follow-up for pytorch#87735

Once again, because BUILD_CAFFE2=0 is not tested for ONNX exporter, one scenario slipped through. A use case where the model can be exported without aten fallback when operator_export_type=ONNX_ATEN_FALLBACK and BUILD_CAFFE2=0

A new unit test has been added, but it won't prevent regressions if BUILD_CAFFE2=0 is not executed on CI again

Fixes pytorch#87313

Pull Request resolved: pytorch#88504
Approved by: https://github.com/justinchuby, https://github.com/BowenBao

(cherry picked from commit 5f0783b)
izaitsevfb pushed a commit to izaitsevfb/pytorch that referenced this pull request Dec 6, 2022
Follow-up for pytorch#87735

Once again, because BUILD_CAFFE2=0 is not tested for ONNX exporter, one scenario slipped through. A use case where the model can be exported without aten fallback when operator_export_type=ONNX_ATEN_FALLBACK and BUILD_CAFFE2=0

A new unit test has been added, but it won't prevent regressions if BUILD_CAFFE2=0 is not executed on CI again

Fixes pytorch#87313

Pull Request resolved: pytorch#88504
Approved by: https://github.com/justinchuby, https://github.com/BowenBao

(cherry picked from commit 5f0783b)
atalman added a commit that referenced this pull request Dec 7, 2022
* Fix ATen Fallback for BUILD_CAFFE2=0 for ONNX-only ops (#88504)

Follow-up for #87735

Once again, because BUILD_CAFFE2=0 is not tested for ONNX exporter, one scenario slipped through. A use case where the model can be exported without aten fallback when operator_export_type=ONNX_ATEN_FALLBACK and BUILD_CAFFE2=0

A new unit test has been added, but it won't prevent regressions if BUILD_CAFFE2=0 is not executed on CI again

Fixes #87313

Pull Request resolved: #88504
Approved by: https://github.com/justinchuby, https://github.com/BowenBao

(cherry picked from commit 5f0783b)

* Update test/onnx/test_pytorch_onnx_no_runtime.py

Co-authored-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>

* Update test/onnx/test_pytorch_onnx_no_runtime.py

Fix linter

* Update test/onnx/test_pytorch_onnx_no_runtime.py

Co-authored-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>

* fix lint warnings

Co-authored-by: Thiago Crepaldi <thiago.crepaldi@microsoft.com>
Co-authored-by: Andrey Talman <atalman@fb.com>
kulinseth pushed a commit to kulinseth/pytorch that referenced this pull request Dec 10, 2022
Follow-up for pytorch#87735

Once again, because BUILD_CAFFE2=0 is not tested for ONNX exporter, one scenario slipped through. A use case where the model can be exported without aten fallback when operator_export_type=ONNX_ATEN_FALLBACK and BUILD_CAFFE2=0

A new unit test has been added, but it won't prevent regressions if BUILD_CAFFE2=0 is not executed on CI again

Fixes pytorch#87313

Pull Request resolved: pytorch#88504
Approved by: https://github.com/justinchuby, https://github.com/BowenBao
@thiagocrepaldi thiagocrepaldi deleted the thiagofc/followup-87735 branch October 31, 2023 19:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ciflow/trunk Trigger trunk jobs on your pull request Merged module: onnx Related to torch.onnx module: regression It used to work, and now it doesn't open source release notes: onnx torch.onnx related changes that should show up in the release notes topic: bug fixes topic category triage review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Improper model conversion from PyTorch to ONNX with torch.onnx.OperatorExportTypes.ONNX_ATEN flag
6 participants