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

onnx_proto should be build as .so when onnxruntime_BUILD_SHARED_LIB is set. #5236

Closed
wants to merge 1 commit into from

Conversation

xkszltl
Copy link
Contributor

@xkszltl xkszltl commented Sep 21, 2020

When tensorrt provider is moved to its own shared lib, static onnx-ml.pb.cc will causes duplicated descriptor_table_onnx_2fonnx_2dml_2eproto in both (main/provider) libs with re-registration.

… is set.

When tensorrt provider is moved to its own shared lib, static onnx-ml.pb.cc will causes duplicated `descriptor_table_onnx_2fonnx_2dml_2eproto` in both (main/provider) libs with re-registration.
@xkszltl xkszltl requested a review from a team as a code owner September 21, 2020 15:31
@xkszltl xkszltl mentioned this pull request Sep 21, 2020
@snnn
Copy link
Member

snnn commented Sep 22, 2020

We would rather take the change for ONNX upstream instead of patching it here. The official ONNX python package should be built in the same way , otherwise this PR will cause conflict between ONNX Runtime python package and ONNX python package, which we don't want to see.

@xkszltl
Copy link
Contributor Author

xkszltl commented Sep 22, 2020

What do you mean by "build in the same way"?
BUILD_SHARED_LIBS is configurable in onnx repo, so it really up to the final user to decide how to set it.

Regarding "official package", I recommend dual build to ship both .so and .a together, so the decision can be made a link time of downstream.
Regarding "official python package", usually python package should not have .a right?

@xkszltl
Copy link
Contributor Author

xkszltl commented Sep 22, 2020

BTW if you plan to do something in onnx repo, I recommend to merge this first before you have solution ready.

@snnn
Copy link
Member

snnn commented Sep 22, 2020

What do you mean by "build in the same way"?

If you think onnx_proto should be build as .so , then please tell ONNX team to do so in their official builds, not us. ONNX is community driven, they have weekly meeting that opens to public. You may raise this issue to the ONNX community and ask their opinions. You can join their zoom session and directly talk to them, they would be happy to listen to you. They absolutely want more people get involved.

"build in the same way" means by your suggestion every one should dynamic link to onnx_proto instead of having a private copy inside of their own package. If ONNX community decides to take the move, I'm happy to follow them.

@xkszltl
Copy link
Contributor Author

xkszltl commented Sep 22, 2020

If you think onnx_proto should be build as .so , then please tell ONNX team to do so in their official builds, not us.

Aha good to get some clarity on that.
I used to think onnx and ort are handled by different teams, but keep hearing people refer to you as onnx team...

@tianleiwu
Copy link
Contributor

close this for now according to comments.

@tianleiwu tianleiwu closed this Sep 28, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants