[OpenVINO]Session Options Appended After AppendExecutionProvider #23852
Conversation
Changes to make sure to honor SessionOptions API Contract
| config_options = &session_options->config_options; | ||
| } | ||
|
|
||
| std::array<const void*, 2> configs_array = {provider_options_map, config_options}; |
There was a problem hiding this comment.
how about declare a struct type?
struct OpenVINOProviderFactoryCreateParams {
const ProviderOptions* provider_options_map;
const ConfigOptions* config_options;
};This would be much easier to read
There was a problem hiding this comment.
Earlier we had a similar structure but looking at this code, https://github.com/microsoft/onnxruntime/blob/99c51a326e0ff54a56e7b194204d459932084408/onnxruntime/core/session/provider_bridge_ort.cc#L1992C2-L1992C3
I desired to have consistency.
onnxruntime/core/providers/openvino/openvino_provider_factory.cc
Outdated
Show resolved
Hide resolved
onnxruntime/core/providers/openvino/openvino_provider_factory.cc
Outdated
Show resolved
Hide resolved
sfatimar
changed the title
|
@fs-eire Please comment. |
|
/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline |
|
Azure Pipelines successfully started running 9 pipeline(s). |
| @@ -101,6 +101,7 @@ struct ProviderInfo { | |||
| bool so_context_embed_mode{false}; // ORT session option | |||
| bool so_share_ep_contexts{false}; // ORT session option | |||
| fs::path so_context_file_path{}; // ORT session option | |||
There was a problem hiding this comment.
Is it expected that so_context_file_path and cache_dir use different initializer values?
There was a problem hiding this comment.
@yihonglyu , Yes cache_dir refers to OV caching and so_context_file_path is used when ORT caching (so_context_enable) is set.
Though there is no benefit of enabling both simultaneously, it can be enabled and can have different values.
Cache_DIR points to a directory and so_context_file_path points to a EP Ctx file name.
|
/azp run Linux CPU CI Pipeline, Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline, Linux GPU TensorRT CI Pipeline, Linux OpenVINO CI Pipeline, MacOS CI Pipeline, ONNX Runtime Web CI Pipeline, onnxruntime-binary-size-checks-ci-pipeline, Linux QNN CI Pipeline |
|
Azure Pipelines successfully started running 9 pipeline(s). |
|
/azp run Linux CPU Minimal Build E2E CI Pipeline, Linux GPU CI Pipeline |
|
Azure Pipelines successfully started running 2 pipeline(s). |
| } | ||
|
|
||
| std::array<void*, 2> pointers_array = *reinterpret_cast<const std::array<void*, 2>*>(void_params); | ||
| const ProviderOptions provider_options = *reinterpret_cast<ProviderOptions*>(pointers_array[0]); |
There was a problem hiding this comment.
in latest commit, this line changed to make a copy of the ProviderOptions object. Is this done intentionally?
Description
To honor SessionOption API Contract the ordering of AddConfigOption and AppendExecutionProvider_OpenVINO should not matter. This PR is fixing that issue
Motivation and Context
This PR fixes a regression happened during last PR in ordering of SessionOptions.