[VitisAI] external_ep_library typo fix#2027
[VitisAI] external_ep_library typo fix#2027akholodnamdcom wants to merge 1 commit intomicrosoft:mainfrom
Conversation
|
akholodnamdcom is this pull-request needed for the next release? |
There was a problem hiding this comment.
Pull request overview
Fixes a misspelled VitisAI provider option key so the Windows-specific logic can locate the configured external EP library when the correct option name is used.
Changes:
- Update the VitisAI provider option lookup key from
external_ep_libraytoexternal_ep_library.
src/models/model.cpp
Outdated
| if (provider_options.name == "VitisAI") { | ||
| if (const auto opt_it = std::find_if(provider_options.options.begin(), provider_options.options.end(), | ||
| [](const auto& pair) { return pair.first == "external_ep_libray"; }); | ||
| [](const auto& pair) { return pair.first == "external_ep_library"; }); | ||
| opt_it != provider_options.options.end()) { |
There was a problem hiding this comment.
Changing the VitisAI provider option key from "external_ep_libray" to "external_ep_library" is a user-facing behavior change: any existing configs/scripts using the old misspelling will no longer trigger this branch. Consider accepting both keys (and optionally emitting a deprecation warning when the misspelling is used) to preserve backwards compatibility while fixing the typo.
src/models/model.cpp
Outdated
| if (provider_options.name == "VitisAI") { | ||
| if (const auto opt_it = std::find_if(provider_options.options.begin(), provider_options.options.end(), | ||
| [](const auto& pair) { return pair.first == "external_ep_libray"; }); | ||
| [](const auto& pair) { return pair.first == "external_ep_library"; }); |
There was a problem hiding this comment.
Please add backwards compatibility support. Since we do not know the version of the runtime library, we should support this failing and then falling back to the old name with the typo.
There was a problem hiding this comment.
This thing appears in configs of models, not runtime library. Not sure we should care about backward compatibility. Will double check.
src/models/model.cpp
Outdated
| [](const auto& pair) { return pair.first == "external_ep_library"; }); | ||
| opt_it != provider_options.options.end()) { | ||
| auto lib_name = opt_it->second; | ||
| auto lib = LoadLibrary(lib_name.c_str()); |
There was a problem hiding this comment.
This loadlibrary call seems buggy. It loads the library, does not check for errors when loading the library, and does not close the library (leaking the resource).
Please use proper RAII. See existing support here for loading libraries.
Also, let's not guard this on windows if that is possible.
There was a problem hiding this comment.
Let's ask original author (@BoarQing) of that thing to take a look. I do not have an env to try this out.
src/models/model.cpp
Outdated
| [](const auto& pair) { return pair.first == "external_ep_library"; }); | ||
| opt_it != provider_options.options.end()) { | ||
| auto lib_name = opt_it->second; | ||
| auto lib = LoadLibrary(lib_name.c_str()); |
There was a problem hiding this comment.
On a separate note, why does the library need to be loaded inside ort-genai? It seems the library will be loaded inside ort anyway since we are registering it as a custom opes library. What value does it add to load it here?
Will CreateEpFactories not be invoked when the plug-in library is loaded inside ort?
There was a problem hiding this comment.
@BoarQing could you please answer the question.
akholodnamdcom
force-pushed
the
akholodn/external_ep_library_typo_fix
branch
from
d1714bf to
ca26d11
Compare
Depends on microsoft/onnxruntime#27647