-
Notifications
You must be signed in to change notification settings - Fork 3.2k
[ort-build] Pass ORT_EXTRA_INTERFACE_FLAGS to onnxruntime_session #24368
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
base: main
Are you sure you want to change the base?
Conversation
These flags, USE_CUDA etc, were added before we split the EPs into shared libraries. Now they should not exist. For example, now if a onnxruntime.dll was not built with --use_cuda, it could not load CUDA EP at all. If we want a generic-interface, then all such flags should be always set to true in all ORT code. Therefore I suggest removing the if condition:
|
For example, onnxruntime/core/optimizer/layout_transformation/layout_transformation.cc also use USE_CUDA. The code is part of onnxruntime_optimizer.lib library. Your change does not impact that that lib. I think, the problem you are trying to fix is more than a build problem. We need to understand why layout_transformation use that macro and if there is any downside of always turning the code path on. |
Hi Changming, The real issue we are facing with this cmake logic is the utility fn for compiler settings are generic and called for all targets but it does not accept list of compiler settings as argument per target. There are global arrays used for multiple targets (aliasing and some cases set redundantly for some target) so it's not easy to determine which target uses what unless we grep the whole code base and build a usage matrix. Here in this case, we wanted to set ORT_EXTRA_INTERFACE_FLAGS for onnxruntime.dll but we later found out there are other static targets which also requires these pre-processors def, as they get finally linked to dll. |
That's what I was saying: all onnxruntime_*.lib need to apply the pre-processors defs. However, even the code can compile, the logic might not be correct. |
One problem with PR 23530, win-qnn-arm64-ci-pipeline.yml uses option qnn_build_args: '--use_qnn --use_generic_interface' which is wrong, should be enable_generic_interface. and the strategy SHARED_LIB_GENERIC_INTERFACE is same with SHARED_LIB because qnn_build_args never get use by build command. |
Thanks for the feedback. How would you like to proceed further? Plan to remove all the pre-processor in the code base that links to ORT ? |
Good catch, I can fix this. |
any updates on this? is this needed for ORT 1.22 ? |
I created a PR that properly uses the D:\a\_work\1\s\onnxruntime\core\session\provider_bridge_ort.cc:1766 onnxruntime::ProviderLibrary::Get [ONNXRuntimeError] : 1 : FAIL : Error loading "D:\a\_work\1\b\RelWithDebInfo\RelWithDebInfo\onnxruntime_providers_openvino.dll" which is missing. (Error 126: "The specified module could not be found.")
1:
1: Provider:OpenVINOExecutionProvider
1: unknown file: error: C++ exception with description "D:\a\_work\1\s\onnxruntime\core\session\provider_bridge_ort.cc:1766 onnxruntime::ProviderLibrary::Get [ONNXRuntimeError] : 1 : FAIL : Error loading "D:\a\_work\1\b\RelWithDebInfo\RelWithDebInfo\onnxruntime_providers_openvino.dll" which is missing. (Error 126: "The specified module could not be found.")
1: " thrown in the test body.
1:
Here's the PR: #24552 |
Description
The onnxruntime v1.21.0 merge broke the 'generic-interface' code. PR #23530 cleaned-up the implementation but made a change: it only adds the ORT_EXTRA_INTERFACE_FLAGS preprocessor definitions to the onnxruntime target. But the important part of registration - onnxruntime/core/session/provider_registration.cc - isn't compiled by the onnxruntime target, it's compiled by the onnxruntime_session target, which is linked into onnxruntime. This means that the registration doesn't get passed the flags to enable EP 'awareness' and registration fails.
This PR is the most pragmatic fix I can make; I'm not very familiar with the OnnxRuntime build - please let me know if there's a better way to fix this. This is a local fix to unblock Windows development - this should probably be fixed in main, too.
Motivation and Context
QNN EP registration fails in onnxruntime.dll binaries compiled with the --enable_generic_interface parameter.