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.

Sign up for GitHub

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

Jump to bottom

Bump ONNX version #16325

Merged
merged 3 commits into from
Aug 10, 2023
Merged

Bump ONNX version #16325

merged 3 commits into from
Aug 10, 2023

Conversation

BowenBao
Copy link
Contributor

@BowenBao BowenBao commented Jun 12, 2023 •
edited
Loading

Description

Bump ONNX version to https://github.com/onnx/onnx/tree/rel-1.14.1 to include a fix for segfault when shape inferencing nested onnx functions.

Motivation and Context

Resolves #16170

snnn
snnn previously requested changes Jun 12, 2023
View reviewed changes
Copy link
Member

@snnn snnn left a comment

Choose a reason for hiding this comment

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

ORT needs to support backward compatibility. Any model works with version X should also work with versions >= X. Therefore every ORT release should only be built only against a release version of ONNX . We cannot take this change unless we are sure ONNX will release a new version before ORT does so.

@BowenBao
Copy link
Contributor Author

On another note, there are a few opschema related build errors after bumping ONNX

E.g., https://dev.azure.com/onnxruntime/onnxruntime/_build/results?buildId=1030340&view=logs&j=c2b47fc6-1299-5221-0391-3bae1c8274d3&t=cf1fa3d9-bd35-57ce-6b0a-a56e54a856fa

In file included from /onnxruntime_src/onnxruntime/core/graph/contrib_ops/contrib_defs.cc:3:
/onnxruntime_src/onnxruntime/core/graph/contrib_ops/contrib_defs.cc: In function ‘void onnxruntime::contrib::RegisterContribSchemas()’:
/onnxruntime_src/onnxruntime/core/graph/contrib_ops/contrib_defs.h:50:18: error: conversion from ‘onnx::OpSchema’ to non-scalar type ‘onnx::OpSchemaRegistry::OpSchemaRegisterOnce’ requested
   50 |       schema_func(ONNX_NAMESPACE::OpSchema(#name, __FILE__, __LINE__))

@jcwchen do you have context?

@jcwchen
Copy link
Contributor

jcwchen commented Jun 12, 2023

@jcwchen do you have context?

Thanks for catching this issue. I might find the root cause: onnx/onnx#5221 (comment). Will discuss it with the author and update ONNX if needed.

This was referenced Jun 12, 2023
Merged
Merged
@BowenBao BowenBao reopened this Jun 21, 2023
@jcwchen
Copy link
Contributor

jcwchen commented Jun 21, 2023

Sorry for accidentally automatic close. The fix in ONNX has been merged. Please try the latest one. Thank you!

@jcwchen jcwchen reopened this Jun 21, 2023
@snnn snnn dismissed their stale review June 22, 2023 22:34

I'm on vacation. Cannot help review changes in this PR.

thiagocrepaldi
thiagocrepaldi reviewed Jun 24, 2023
View reviewed changes
Copy link
Contributor

@thiagocrepaldi thiagocrepaldi left a comment

Choose a reason for hiding this comment

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

Maybe @faxu can help reviewing this PR while @snnn is on vacation

adityagoel4512 pushed a commit to adityagoel4512/onnx that referenced this pull request Jul 28, 2023
@jcwchen @adityagoel4512
Remove added explicit for OpSchemaRegisterOnce (onnx#5322)
a2d1ba3 
### Description
<!-- - Describe your changes. -->
Remove added explicit for OpSchemaRegisterOnce.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->
Try to fix microsoft/onnxruntime#16325. explicit
was added for OpSchemaRegisterOnce recently.

---------

Signed-off-by: Chun-Wei Chen <[email protected]>
Signed-off-by: Aditya Goel <[email protected]>
adityagoel4512 pushed a commit to adityagoel4512/onnx that referenced this pull request Jul 28, 2023
@jcwchen @adityagoel4512
Remove added explicit for OpSchemaRegisterOnce (onnx#5322)
b7623f5 
### Description
<!-- - Describe your changes. -->
Remove added explicit for OpSchemaRegisterOnce.

### Motivation and Context
<!-- - Why is this change required? What problem does it solve? -->
<!-- - If it fixes an open issue, please link to the issue here. -->
Try to fix microsoft/onnxruntime#16325. explicit
was added for OpSchemaRegisterOnce recently.

---------

Signed-off-by: Chun-Wei Chen <[email protected]>
Signed-off-by: Aditya Goel <[email protected]>
@BowenBao BowenBao marked this pull request as ready for review August 9, 2023 20:34
@BowenBao BowenBao requested review from a team as code owners August 9, 2023 20:34
@BowenBao BowenBao requested review from abock and jcwchen August 9, 2023 20:35
@BowenBao
Copy link
Contributor Author

BowenBao commented Aug 9, 2023

Saw CI error

CMake Error at abseil_cpp-subbuild/abseil_cpp-populate-prefix/src/abseil_cpp-populate-stamp/verify-abseil_cpp-populate.cmake:11 (message):
File not found:
/build/deps/github.com/abseil/abseil-cpp/archive/refs/tags/20220623.1.zip

In 1.0.68 deps where I created 1.0.69 from, the file is under path github.com/abseil/abseil-cpp/archive/refs/tags/20230125.3.zip.
I did notice from file onnxruntime/tools/ci_build/github/azure-pipelines/templates/download-deps.yml, the deps version number is set to 1.0.63 instead of latest 1.0.68.

@snnn What should I do?

thiagocrepaldi
thiagocrepaldi previously approved these changes Aug 9, 2023
View reviewed changes
@BowenBao
Copy link
Contributor Author

BowenBao commented Aug 9, 2023

Guess I should create from 1.0.63

@snnn
Copy link
Member

snnn commented Aug 9, 2023

Guess I should create from 1.0.63

Nothing to do with the history versions. You don't need to care what 1.0.63 has. Just download all the files from deps.txt and upload them.

@BowenBao
Copy link
Contributor Author

BowenBao commented Aug 9, 2023

Unsure about this CI failure Windows_SCA / Onnxruntime-SCA-win32-WINML-x64 (pull_request)

Error: -09 22:05:16,861 build [ERROR] - Python bindings use typeid so you can't disable RTTI

Looks unrelated.

snnn
snnn approved these changes Aug 9, 2023
View reviewed changes
Copy link
Member

@snnn snnn left a comment

Choose a reason for hiding this comment

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

Thank you!
Please also get an approval from Pranav.

@BowenBao BowenBao requested a review from pranavsharma August 10, 2023 00:15
@BowenBao BowenBao merged commit 6986981 into microsoft:main Aug 10, 2023
@jcwchen jcwchen mentioned this pull request Aug 11, 2023
Merged
@snnn snnn mentioned this pull request Aug 11, 2023
Merged
@jeyblu jeyblu mentioned this pull request Aug 11, 2023
Closed
snnn added a commit that referenced this pull request Aug 11, 2023
@snnn
Fix CI build (#17118)
4728f20 
### Description
Some pipelines are failing. It is because PR #16325 set ONNX version to
`rel-1.14.1` . It is a branch name, not a commit or tag name. It means
whenever the branch got a new commit, we will auto pick it and use it.
jchen351 pushed a commit that referenced this pull request Aug 12, 2023
@BowenBao @jchen351
Bump ONNX version (#16325)
dacf31b 
### Description
Bump ONNX version to https://github.com/onnx/onnx/tree/rel-1.14.1 to
include a fix for segfault when shape inferencing nested onnx functions.



### Motivation and Context
Resolves #16170
jchen351 pushed a commit that referenced this pull request Aug 12, 2023
@snnn @jchen351
Fix CI build (#17118)
e918e57 
### Description
Some pipelines are failing. It is because PR #16325 set ONNX version to
`rel-1.14.1` . It is a branch name, not a commit or tag name. It means
whenever the branch got a new commit, we will auto pick it and use it.
@PeixuanZuo PeixuanZuo mentioned this pull request Aug 14, 2023
Merged
snnn pushed a commit that referenced this pull request Aug 14, 2023
@PeixuanZuo
[ROCm] fix python package pipeline (#17136)
be2200c 
ROCm python package pipeline failed because this
PR(#16325) changed onnx
version to a commit and we need to build onnx from source. Low protobuf
version will cause build errors.
This PR remove `cmake ` and `protobuf ` from Dockerfile, these two will
install by `install_os_deps.sh`.
kleiti pushed a commit to kleiti/onnxruntime that referenced this pull request Mar 22, 2024
@BowenBao @kleiti
Bump ONNX version (microsoft#16325)
c9c71ae 
### Description
Bump ONNX version to https://github.com/onnx/onnx/tree/rel-1.14.1 to
include a fix for segfault when shape inferencing nested onnx functions.



### Motivation and Context
Resolves microsoft#16170
kleiti pushed a commit to kleiti/onnxruntime that referenced this pull request Mar 22, 2024
@snnn @kleiti
Fix CI build (microsoft#17118)
d0638f6 
### Description
Some pipelines are failing. It is because PR microsoft#16325 set ONNX version to
`rel-1.14.1` . It is a branch name, not a commit or tag name. It means
whenever the branch got a new commit, we will auto pick it and use it.
kleiti pushed a commit to kleiti/onnxruntime that referenced this pull request Mar 22, 2024
@PeixuanZuo @kleiti
[ROCm] fix python package pipeline (microsoft#17136)
311b825 
ROCm python package pipeline failed because this
PR(microsoft#16325) changed onnx
version to a commit and we need to build onnx from source. Low protobuf
version will cause build errors.
This PR remove `cmake ` and `protobuf ` from Dockerfile, these two will
install by `install_os_deps.sh`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@snnn snnn snnn approved these changes

@pranavsharma pranavsharma pranavsharma approved these changes

@thiagocrepaldi thiagocrepaldi thiagocrepaldi left review comments

@abock abock Awaiting requested review from abock

@jcwchen jcwchen Awaiting requested review from jcwchen

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Segfaults when loading model with local functions, works fine if model is inlined by ONNX
5 participants
@BowenBao @jcwchen @snnn @pranavsharma @thiagocrepaldi