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

Advance upstreams to 25.02 #298

Merged
merged 7 commits into from
Mar 6, 2025
Merged

Conversation

nv-kmcgill53
Copy link
Contributor

No description provided.

Copy link
Contributor

@mc-nv mc-nv left a comment

Choose a reason for hiding this comment

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

Why do we need to install OpenVINO for ONNXRuntime?

@nv-kmcgill53
Copy link
Contributor Author

Why do we need to install OpenVINO for ONNXRuntime?

Onnxruntime has a concept of execution providers (EPs) and there are two we have historically supported: OpenVINO Execution Provider and the TensorRT Execution Provider. Since we install the OpenVINO backend in our container, we thought it would be a good idea not to limit our users when using the OpenVINO EP with ONNXRuntime.

@mc-nv
Copy link
Contributor

mc-nv commented Mar 4, 2025

Why do we need to install OpenVINO for ONNXRuntime?

Onnxruntime has a concept of execution providers (EPs) and there are two we have historically supported: OpenVINO Execution Provider and the TensorRT Execution Provider. Since we install the OpenVINO backend in our container, we thought it would be a good idea not to limit our users when using the OpenVINO EP with ONNXRuntime.

My concern here is that we may making a double work.
OpenVINO contributes into ONNX Runtime same as to Triton Inference Server.
Scenario that ONNX Runtime uses for OpenVINO are related to headers it will obtain during the compilation.
https://github.com/microsoft/onnxruntime/blob/main/cmake/onnxruntime_providers_openvino.cmake#L6-L11

Unless we start pulling openvino library alongside with onnxruntime library it could be a redundant work.
Note I didn't have time to make a full review and ananlizes and not going to block this PR.

@nv-kmcgill53
Copy link
Contributor Author

My concern here is that we may making a double work.
OpenVINO contributes into ONNX Runtime same as to Triton Inference Server.
Scenario that ONNX Runtime uses for OpenVINO are related to headers it will obtain during the compilation.
https://github.com/microsoft/onnxruntime/blob/main/cmake/onnxruntime_providers_openvino.cmake#L6-L11

This issue has been around for as long as I can remember. Maybe @tanmayv25 can give more context on this, but the way I understand this part of the code is: We ran into an issue where the versions of openvino needed to match between the ORT openvino EP and the openvino_backend itself, otherwise we would have conflicting installs. We also have the difference where we build openvino from source in the openvino backend, and we use the prebuilt openvino libs for the ORT openvino EP. IIRC this is because we invoked some flags on the openvino build to enable certain compiler optimizations we were taking advantage of. All of this I am recalling is from 2+ years ago, so the ecosystem may have changed a lot since I looked deep into this.

@nv-kmcgill53
Copy link
Contributor Author

nv-kmcgill53 commented Mar 4, 2025

Note I didn't have time to make a full review and ananlizes and not going to block this PR.

Please change your status from "Requesting Changes" to "Approved."

@mc-nv mc-nv self-requested a review March 5, 2025 00:29
@mc-nv
Copy link
Contributor

mc-nv commented Mar 5, 2025

Note I didn't have time to make a full review and ananlizes and not going to block this PR.

Please change your status from "Requesting Changes" to "Approved."

It's approved but I think we need a follow up ticket in JIRA to review it usage, looks like it could be a redundant step.

@nv-kmcgill53
Copy link
Contributor Author

I think we need a follow up ticket in JIRA to review it usage

Created ticket TPRD-1214

@nv-kmcgill53 nv-kmcgill53 merged commit f58118c into main Mar 6, 2025
3 checks passed
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.

2 participants