-
Notifications
You must be signed in to change notification settings - Fork 49
Report import failure error code #715
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: ovep-develop
Are you sure you want to change the base?
Conversation
7dbe8e9
to
faf1a6e
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@javier-intel .. DO we have a way to trigger compatibility failure and ensure exception triggered by elf loader and UMD is caught here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM !
c75e346
to
64bcdad
Compare
@javier-intel can you please rebase this branch so I can merge it in. Mayuresh has already approved. |
Why has this not been merged yet ? @javier-intel ? |
@MayureshV1 please merge PR if you are approving ..You must have permissions |
@sfatimar I think it flew under the radar. It's a simple change but will probably need careful rebase as it's been some time since it was implemented. This week as I'll be busy with traveling so either I do it next week or someone from your team picks it up. |
a718244
to
c3dc39d
Compare
c3dc39d
to
93a6e38
Compare
@MayureshV1 can you please review again? I just built, haven't tested :( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR introduces proper error handling for OpenVINO model import failures by creating typed exceptions that can communicate specific error codes back to the ONNX Runtime core, particularly for invalid binary errors during context model imports.
- Enhanced exception handling with a new typed exception system that preserves error codes
- Modified OvExceptionBoundary to support typed exceptions for model import operations
- Added fallback logic for NPU compilation failures with better error reporting
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.
File | Description |
---|---|
exceptions.h | New exception class with error code parsing and Status conversion |
ov_interface.cc | Updated exception boundary template and ImportModel to use typed exceptions |
openvino_execution_provider.cc | Added exception handling wrapper in Compile method |
backend_manager.cc | Enhanced NPU fallback logic with typed exception handling |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
// Newer drivers | ||
if ((type_ == type::import_model) && | ||
(error_code_ == 0x7800000f /* ZE_RESULT_ERROR_INVALID_NATIVE_BINARY */)) { | ||
std::string message{error_name_ + ", code 0x" + std::to_string(error_code_) + "\nModel needs to be recompiled\n"}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Using std::to_string() to format a hexadecimal value will produce decimal output. Use std::format or create a proper hex string with std::hex to maintain the '0x' prefix meaning.
std::string message{error_name_ + ", code 0x" + std::to_string(error_code_) + "\nModel needs to be recompiled\n"}; | |
std::ostringstream oss; | |
oss << error_name_ << ", code 0x" << std::hex << std::nouppercase << error_code_ << "\nModel needs to be recompiled\n"; | |
std::string message = oss.str(); |
Copilot uses AI. Check for mistakes.
} catch (...) { | ||
ORT_THROW(log_tag + std::vformat(fmt.get(), std::make_format_args(args...))); | ||
const auto message = log_tag + (args + ...); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The fold expression '(args + ...)' assumes all arguments are string-like and support concatenation. This will fail if any argument is not a string type. Consider using std::format or string conversion for each argument.
Copilot uses AI. Check for mistakes.
} catch (...) { | ||
ORT_THROW(log_tag + std::vformat(fmt.get(), std::make_format_args(args...))); | ||
const auto message = log_tag + (args + ...); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same issue as above - the fold expression '(args + ...)' assumes all arguments support string concatenation, which may not be the case for all argument types.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Backend Manager.cc has CPU fallback flow for NPU compile failure scenario. This logic was moved to basic_backedn.cc and needs to be adapted.
Description
Report invalid binary when context model can't be imported by OV
Motivation and Context
Correctly communicate to ORT core the failure to import EpCtx
https://jira.devtools.intel.com/browse/CVS-167480