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

Update ONNX submodule to support opset 18 #12879

Closed
wants to merge 1 commit into from

Conversation

thiagocrepaldi
Copy link
Contributor

@thiagocrepaldi thiagocrepaldi commented Sep 7, 2022

This PR has the goal of updating ONNX to a particular commit and allow ORT to execute ONNX graphs with opset 18

PyTorch is about to merge the same change. However, because PyTorch depends on ORT for unit testing, ORT also must update its ONNX submodule for their pipelines to succeed. Otherwise, ORT will not recognize the new ops PyTorch ONNX converter adds, such as Col2Im

@thiagocrepaldi thiagocrepaldi added the converter related to ONNX converters label Sep 7, 2022
@thiagocrepaldi thiagocrepaldi changed the title Allow graphs with ONNX opseet 18 to be executed by ORT Allow graphs with ONNX opset 18 to be executed by ORT Sep 20, 2022
@thiagocrepaldi thiagocrepaldi added training issues related to ONNX Runtime training; typically submitted using template core runtime issues related to core runtime labels Sep 20, 2022
@thiagocrepaldi
Copy link
Contributor Author

@skottmckay could you look into this one?

@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-onnx-opset-18-support branch from c213a27 to 2a7c984 Compare September 29, 2022 13:50
@thiagocrepaldi thiagocrepaldi requested a review from a team as a code owner September 29, 2022 13:50
@thiagocrepaldi thiagocrepaldi changed the title Allow graphs with ONNX opset 18 to be executed by ORT Update ONNX submodule to support opset 18 Sep 29, 2022
@thiagocrepaldi thiagocrepaldi requested a review from snnn September 30, 2022 18:44
@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-onnx-opset-18-support branch from 2a7c984 to f8ea18e Compare October 3, 2022 17:00
@thiagocrepaldi
Copy link
Contributor Author

@snnn This PR changes ONNX from the latest stable release to a commit. No code/dataset change, but several tests consistently failed with

[E:onnxruntime:Default, provider_test_utils.cc:1045 Run] Resolve failed with status: Node (node1) Op (Resize) [ShapeInferenceError] Either `sizes` or `scales` must be provided, but not both of them
2022-10-03T18:03:46.8850993Z /onnxruntime_src/onnxruntime/test/providers/provider_test_utils.cc:1047: Failure
2022-10-03T18:03:46.8851477Z Value of: status.IsOK()
2022-10-03T18:03:46.8851741Z   Actual: false
2022-10-03T18:03:46.8851984Z Expected: true
2022-10-03T18:03:46.8852340Z Node (node1) Op (Resize) [ShapeInferenceError] Either `sizes` or `scales` must be provided, but not both of them

I wonder if the reason is because of the step 4 at onnxruntime/How_To_Update_ONNX_Dev_Notes.md, which states:

If you are updating ONNX from a released tag to a new commit, please ask Changming (@snnn) to deploy the new test data along with other test models to our CI build machines. This is to ensure that our tests cover every ONNX opset.

If that is the case, could you help updating the test data?

@snnn
Copy link
Member

snnn commented Oct 3, 2022

If you are updating ONNX from a released tag to a new commit, please ask Changming (@snnn) to deploy the new test data along with other test models to our CI build machines. This is to ensure that our tests cover every ONNX opset.

If that is the case, could you help updating the test data?

It needs something like this: #13062

And your PR needs an approval from the admins of this repo. I'm not one of the admins.

@skottmckay
Copy link
Contributor

Looks like the ONNX type/shape inferencing started enforcing the Resize spec more strictly for opset 13 which is causing some of the test failures. Tests should be updated to only provide either 'scales' or 'sizes'

onnx/onnx#4448

[  FAILED  ] ResizeOpTest.ResizeOpLinearDownSampleTest_tf_crop_and_resize
[  FAILED  ] ResizeOpTest.ResizeOpLinearDownSampleTest_4DBilinear1_WithSizes
[  FAILED  ] ResizeOpTest.ResizeOpLinearDownSampleTest_2DBilinear_pytorch_half_pixel
[  FAILED  ] ResizeOpTest.NhwcResizeOpLinearDownSampleTest_4DBilinear_pytorch_half_pixel_uint8
[  FAILED  ] ResizeOpTest.NhwcResizeOpLinearDownSampleTest_4DBilinear_pytorch_half_pixel_int8
[  FAILED  ] ResizeOpTest.ResizeOpLinearDownSampleTest_3DTrilinear_pytorch_half_pixel
[  FAILED  ] ResizeOpTest.ResizeOpNearestDownSampleTest_WithSizes
[  FAILED  ] ResizeOpTest.ResizeOpNearestUpSampleTest_WithSizes_CeilMode
[  FAILED  ] ResizeOpTest.ResizeOpNearestUpSample5dTest_WithSizes_CeilMode
[  FAILED  ] ResizeOpTest.ResizeOpNearestUpSample_Nearest2xOptimization_Sizes
[  FAILED  ] ResizeOpTest.ResizeOpCubicUpSampleTest_MultiChannel

@snnn
Copy link
Member

snnn commented Oct 25, 2022

It needs an approval from @onnxruntime-admin

@snnn snnn removed their request for review October 25, 2022 14:39
@thiagocrepaldi
Copy link
Contributor Author

thiagocrepaldi commented Oct 25, 2022

It needs an approval from @onnxruntime-admin

Yes, @faxu mentioned it was ok to merge this post 1.13 release.

@thiagocrepaldi thiagocrepaldi requested a review from faxu November 1, 2022 18:02
@thiagocrepaldi thiagocrepaldi force-pushed the thiagofc/add-onnx-opset-18-support branch from f8ea18e to db56371 Compare November 1, 2022 18:03
Copy link
Contributor

@faxu faxu left a comment

Choose a reason for hiding this comment

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

This should be updated to the latest released version of onnx (1.13) once it's available.

@thiagocrepaldi
Copy link
Contributor Author

Duplicate of #14279

@thiagocrepaldi thiagocrepaldi marked this as a duplicate of #14279 Feb 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
converter related to ONNX converters core runtime issues related to core runtime training issues related to ONNX Runtime training; typically submitted using template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants