Fix ONNX operator_export_type on the new registry by thiagocrepaldi · Pull Request #87735 · pytorch/pytorch

thiagocrepaldi · 2022-10-25T22:10:05Z

Pytorch's ONNX pipelines do not run with BUILD_CAFFE2=0, so tests for operator_export_type==ONNX_ATEN or ONNX_ATEN_FALLBACK are not fully tested, which caused this regression to the core feature of ONNX exporter: ONNX ATen fallback.

This PR fixes this regression, but we discuss with Meta on how to use only BUILD_CAFFE2=0 builds instead of ``BUILD_CAFFE2=1`

pytorch-bot · 2022-10-25T22:10:09Z

🔗 Helpful Links

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

📄 Preview Python docs built from this PR
📄 Preview C++ docs built from this PR
❓ Need help or want to give feedback on the CI? Visit our office hours

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

❌ 1 Failures

As of commit 14ec6aa:

The following jobs have failed:

macos-12-py3-x86-64 / test (default, 1, 2, macos-12)

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

BowenBao · 2022-10-28T17:08:19Z

+    # Direct ATen export requested
+    # For BUILD_CAFFE2=0 builds, if domain=="aten" and operator_export_type==ONNX_ATEN,
+    #   an aten::ATen operator is created regardless of symbolics existence
+    # For BUILD_CAFFE2=1, the same applies only if there is no symbolic available


Related to the CI failure on test_py_diagnose_emits_error, where it expects a operator_supported_in_newer_opset_version error for aten::diagonal when exporting in opset 9. After this change, the error was not thrown, and aten::diagonal is exported as aten::ATen(...). The operator_export_type of export is the default _C_onnx.OperatorExportTypes.ONNX.

Is this behavior expected, for BUILD_CAFFE2=1?

Nope, there was a failure in the logic. Thanks for catching this

BowenBao · 2022-10-28T17:08:58Z

Some test failures

2022-10-26T02:05:53.1811889Z =========================== short test summary info ============================
2022-10-26T02:05:53.1812306Z FAILED test/onnx/test_pytorch_onnx_caffe2_quantized.py::TestQuantizedOps::test_cat
2022-10-26T02:05:53.1815203Z FAILED test/onnx/test_pytorch_onnx_no_runtime.py::TestONNXExport::test_export_dict
2022-10-26T02:05:53.1816998Z FAILED test/onnx/test_pytorch_onnx_caffe2_quantized.py::TestQuantizedOps::test_qconv_model
2022-10-26T02:05:53.1818106Z FAILED test/onnx/test_pytorch_onnx_caffe2_quantized.py::TestQuantizedOps::test_qlinear_model
2022-10-26T02:05:53.1818678Z FAILED test/onnx/test_pytorch_onnx_caffe2_quantized.py::TestQuantizedOps::test_quantized_add
2022-10-26T02:05:53.1821321Z FAILED test/onnx/internal/test_diagnostics.py::TestOnnxDiagnostics::test_py_diagnose_emits_error
2022-10-26T02:05:53.1822149Z FAILED test/onnx/test_pytorch_onnx_caffe2_quantized.py::TestQuantizedOps::test_sequential
2022-10-26T02:05:53.1832674Z FAILED test/onnx/test_pytorch_onnx_caffe2_quantized.py::TestQuantizedOps::test_small_model

thiagocrepaldi · 2022-11-01T14:18:03Z

@BowenBao all tests are passing now

BowenBao

LGTM 🚢

Signed-off-by: Thiago Crepaldi <[email protected]>

thiagocrepaldi · 2022-11-01T17:39:29Z

@pytorchbot merge -g

pytorchmergebot · 2022-11-01T17:41:41Z

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 · 2022-11-01T18:57:23Z

Merge failed

Reason: The following mandatory check(s) failed (Rule ONNX exporter):

pull

Dig deeper by viewing the failures on hud

Details for Dev Infra team

Raised by workflow job

thiagocrepaldi · 2022-11-02T15:52:47Z

@pytorchbot merge -f "macos pipeline failed due to aborted artifact upload"

pytorchmergebot · 2022-11-02T15:54:37Z

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

Fixes pytorch#87313 Our ONNX pipelines do not run with BUILD_CAFFE2=0, so tests for operator_export_type ONNX_ATEN and ONNX_ATEN_FALLBACK will not be fully tested, allowing regressions to happen again. We need to run the same set of tests for both BUILD_CAFFE2=0 and 1 Pull Request resolved: pytorch#87735 Approved by: https://github.com/AllenTiTaiWang, https://github.com/BowenBao

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

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

thiagocrepaldi · 2022-11-29T19:01:32Z

#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

Fixes pytorch#87313 Our ONNX pipelines do not run with BUILD_CAFFE2=0, so tests for operator_export_type ONNX_ATEN and ONNX_ATEN_FALLBACK will not be fully tested, allowing regressions to happen again. We need to run the same set of tests for both BUILD_CAFFE2=0 and 1 Pull Request resolved: pytorch#87735 Approved by: https://github.com/AllenTiTaiWang, https://github.com/BowenBao (cherry picked from commit 2aed670)

Fixes #87313 Our ONNX pipelines do not run with BUILD_CAFFE2=0, so tests for operator_export_type ONNX_ATEN and ONNX_ATEN_FALLBACK will not be fully tested, allowing regressions to happen again. We need to run the same set of tests for both BUILD_CAFFE2=0 and 1 Pull Request resolved: #87735 Approved by: https://github.com/AllenTiTaiWang, https://github.com/BowenBao (cherry picked from commit 2aed670) Co-authored-by: Thiago Crepaldi <[email protected]>

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)

* 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 <[email protected]> * 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 <[email protected]> * fix lint warnings Co-authored-by: Thiago Crepaldi <[email protected]> Co-authored-by: Andrey Talman <[email protected]>

Fixes pytorch#87313 Our ONNX pipelines do not run with BUILD_CAFFE2=0, so tests for operator_export_type ONNX_ATEN and ONNX_ATEN_FALLBACK will not be fully tested, allowing regressions to happen again. We need to run the same set of tests for both BUILD_CAFFE2=0 and 1 Pull Request resolved: pytorch#87735 Approved by: https://github.com/AllenTiTaiWang, https://github.com/BowenBao

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

Fixes pytorch#87313 Our ONNX pipelines do not run with BUILD_CAFFE2=0, so tests for operator_export_type ONNX_ATEN and ONNX_ATEN_FALLBACK will not be fully tested, allowing regressions to happen again. We need to run the same set of tests for both BUILD_CAFFE2=0 and 1 Pull Request resolved: pytorch#87735 Approved by: https://github.com/AllenTiTaiWang, https://github.com/BowenBao

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 requested review from BowenBao and abock as code owners October 25, 2022 22:10

pytorch-bot Bot added the release notes: onnx torch.onnx related changes that should show up in the release notes label Oct 25, 2022

thiagocrepaldi added module: onnx Related to torch.onnx topic: bug fixes topic category labels Oct 25, 2022

pytorchbot added the open source label Oct 25, 2022

thiagocrepaldi mentioned this pull request Oct 25, 2022

Improper model conversion from PyTorch to ONNX with torch.onnx.OperatorExportTypes.ONNX_ATEN flag #87313

Closed

justinchuby reviewed Oct 25, 2022

View reviewed changes

Comment thread torch/onnx/utils.py Outdated

Comment thread torch/onnx/utils.py Outdated

Comment thread torch/onnx/utils.py Outdated

thiagocrepaldi force-pushed the thiagofc/fix-onnx-registry-fallback branch from 41e5234 to 06dc424 Compare October 26, 2022 01:31

bdhirsh added the triaged This issue has been looked at a team member, and triaged and prioritized into an appropriate module label Oct 27, 2022

titaiwangms approved these changes Oct 28, 2022

View reviewed changes

pytorch-bot Bot added the ciflow/trunk Trigger trunk jobs on your pull request label Oct 28, 2022

BowenBao reviewed Oct 28, 2022

View reviewed changes

thiagocrepaldi force-pushed the thiagofc/fix-onnx-registry-fallback branch 4 times, most recently from 4379d65 to ea89404 Compare October 31, 2022 22:42

thiagocrepaldi requested a review from BowenBao November 1, 2022 14:18

Fix ONNX operator_export_type on the new registry

77af8e1

thiagocrepaldi force-pushed the thiagofc/fix-onnx-registry-fallback branch from ea89404 to ae7b749 Compare November 1, 2022 16:54

BowenBao approved these changes Nov 1, 2022

View reviewed changes

Comment thread test/onnx/test_pytorch_onnx_no_runtime.py Outdated

Fix regression for onnx export with caffe2 builds

14ec6aa

Signed-off-by: Thiago Crepaldi <[email protected]>

thiagocrepaldi force-pushed the thiagofc/fix-onnx-registry-fallback branch from ae7b749 to 14ec6aa Compare November 1, 2022 17:39

pytorchmergebot added the Merged label Nov 2, 2022

pytorchmergebot closed this in 2aed670 Nov 2, 2022

thiagocrepaldi mentioned this pull request Nov 4, 2022

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

Closed

thiagocrepaldi added this to the 1.13.1 milestone Nov 16, 2022

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

This was referenced Dec 2, 2022

Fix ONNX operator_export_type on the new registry #87735 #90044

Merged

[v.1.13.1] Release Tracker #89855

Closed

izaitsevfb mentioned this pull request Dec 3, 2022

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

Merged

thiagocrepaldi deleted the thiagofc/fix-onnx-registry-fallback branch May 4, 2023 17:17

Uh oh!

Conversation

thiagocrepaldi commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pytorch-bot Bot commented Oct 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

🔗 Helpful Links

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

❌ 1 Failures

Uh oh!

Uh oh!

Uh oh!

Uh oh!

BowenBao Oct 28, 2022

Choose a reason for hiding this comment

Uh oh!

thiagocrepaldi Oct 31, 2022

Choose a reason for hiding this comment

Uh oh!

BowenBao commented Oct 28, 2022

Uh oh!

thiagocrepaldi commented Nov 1, 2022

Uh oh!

BowenBao left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thiagocrepaldi commented Nov 1, 2022

Uh oh!

pytorchmergebot commented Nov 1, 2022

Merge started

Uh oh!

pytorchmergebot commented Nov 1, 2022

Merge failed

Uh oh!

thiagocrepaldi commented Nov 2, 2022

Uh oh!

pytorchmergebot commented Nov 2, 2022

Merge started

Uh oh!

thiagocrepaldi commented Nov 29, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

thiagocrepaldi commented Oct 25, 2022 •

edited

Loading

pytorch-bot Bot commented Oct 25, 2022 •

edited

Loading