Skip to content

[OpenVINO EP] Use OpenVINO cmake - #19184

Closed
Ilya Lavrenov (ilya-lavrenov) wants to merge 1 commit into
microsoft:mainfrom
ilya-lavrenov:use-openvino-cmake
Closed

[OpenVINO EP] Use OpenVINO cmake#19184
Ilya Lavrenov (ilya-lavrenov) wants to merge 1 commit into
microsoft:mainfrom
ilya-lavrenov:use-openvino-cmake

Conversation

@ilya-lavrenov

Copy link
Copy Markdown
Contributor

Description

Current OpenVINO master removes InferenceEngine and ngraph as deprecated components (replaced by merged OpenVINO component). So, cmake scripts need to be adjusted to be ready for this change

@ilya-lavrenov Ilya Lavrenov (ilya-lavrenov) changed the title Use OpenVINO cmake [OpenVINO EP] Use OpenVINO cmake Jan 17, 2024
@ilya-lavrenov
Ilya Lavrenov (ilya-lavrenov) force-pushed the use-openvino-cmake branch 4 times, most recently from 429da9f to 313db2f Compare January 18, 2024 21:10
@ilya-lavrenov

Copy link
Copy Markdown
Contributor Author

/azp run Linux OpenVINO CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Commenter does not have sufficient privileges for PR 19184 in repo microsoft/onnxruntime

@ilya-lavrenov

Copy link
Copy Markdown
Contributor Author

@jywu-msft Preetha Veeramalai (@preetha-intel) could you please help with review?

@jywu-mysoft

Copy link
Copy Markdown
Contributor

+sfatimar

@jywu-mysoft

Copy link
Copy Markdown
Contributor

/azp run Linux OpenVINO CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ilya-lavrenov

Copy link
Copy Markdown
Contributor Author

Fixed code style issue
Please, help to run once again

Comment thread cmake/CMakeLists.txt
message(FATAL_ERROR "[Couldn't locate OpenVINO] OpenVINO may not have been initialized")
endif()

# Check OpenVINO version for support

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

The openvino Version is critical in get_capability of OVEP. It will break the execution flow. Its not advisable to remove the OPENVINO_VERSION

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

See changes in onnxruntime/core/providers/openvino/ov_versions/capability.cc where OpenVINO version is extracted from OpenVINO headers. No needs to weird cmake logic to pass OpenVINO version via cmake

Comment thread cmake/CMakeLists.txt
file(READ $ENV{INTEL_OPENVINO_DIR}/deployment_tools/inference_engine/version.txt VER)
endif()

if (NOT DEFINED ENV{INTEL_OPENVINO_DIR})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This ensures whether OPENVINO is sourced into the env. Not advisable to remove as it might end up with crash

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

INTEL_OPENVINO_DIR is not defined in many distribution types like conda-forge, brew etc.
It's defined only by OpenVINO archive.

In order to check OpenVINO version, you must either use OpenVINO_VERSION vars from OpenVINOConfig.cmake or OpenVINO headers (openvino/core/version.hpp).

Please, see other changes where I've added version check in C++ files.

@ilya-lavrenov

Copy link
Copy Markdown
Contributor Author

@jywu-msft sfatimar Preetha Veeramalai (@preetha-intel) please, review and help to merge.

@jywu-mysoft

Copy link
Copy Markdown
Contributor

@jywu-msft sfatimar Preetha Veeramalai (@preetha-intel) please, review and help to merge.

I can help merge. sfatimar, Preetha Veeramalai (@preetha-intel) can you review and let me know? thanks.

@jywu-mysoft

Copy link
Copy Markdown
Contributor

/azp run Linux OpenVINO CI Pipeline

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@ilya-lavrenov

Copy link
Copy Markdown
Contributor Author

Replaced by #19966, which also contains this change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ep:OpenVINO issues related to OpenVINO execution provider

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants