-
Notifications
You must be signed in to change notification settings - Fork 57
CVS-175504: Additional single bin simplifications + fixes for bi-directional compatibility #851
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
CVS-175504: Additional single bin simplifications + fixes for bi-directional compatibility #851
Conversation
b699a31 to
eef7dba
Compare
…n_upstream_continued
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 simplifies shared context management and fixes EP context importing in the OpenVINO execution provider. The changes remove the Clear() method from shared context components and modify the stop logic to keep the existing shared context alive rather than clearing it after serialization. Additionally, it consolidates shared context handling by making EP context handler responsible for creating or retrieving the shared context during initialization.
Key Changes:
- Removed
Clear()methods fromSharedContextandBinManagerclasses, replacing context clearing with context removal from the manager - Refactored
EPCtxHandler::Initialize()to return aSharedContextand moved shared context creation logic fromBackendManagerto the EP context handler - Updated
BackendManagerto acceptSharedContext&reference instead of managing it viaSharedContextManager&
Reviewed Changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| ov_shared_context.h | Removed Clear() method declaration; added active_context_path_ member and logic to erase context from map on clear |
| ov_shared_context.cc | Removed Clear() method implementation |
| ov_bin_manager.h | Removed Clear() and ShouldSerialize() method declarations; added DeserializeImpl() |
| ov_bin_manager.cc | Removed Clear() and ShouldSerialize() implementations; simplified Serialize() logic; added error handling wrapper in Deserialize() |
| openvino_execution_provider.h | Added shared_context_ member variable |
| openvino_execution_provider.cc | Modified to initialize and store shared context from EPCtxHandler::Initialize(); removed Clear() call on stop |
| onnx_ctx_model_helper.h | Changed Initialize() to return SharedContext; removed GetSharedContextForEpContextSubgraph() declaration; updated includes |
| onnx_ctx_model_helper.cc | Implemented shared context creation/retrieval in Initialize(); removed GetSharedContextForEpContextSubgraph() |
| contexts.h | Added GetOutputModelPath() helper; refactored GetOutputBinPath() to use it |
| backend_manager.h | Changed to accept SharedContext& reference instead of SharedContextManager& |
| backend_manager.cc | Removed shared context creation logic; changed member from pointer to reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
MayureshV1
left a comment
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 !
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
Copilot reviewed 11 out of 11 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| DeserializeImpl(stream, shared_context); | ||
| } catch (const std::exception& e) { | ||
| ORT_THROW(e.what(), "\nCould not deserialize binary data. This could mean the bin is corrupted or incompatible. Try re-generating ep context cache."); |
Copilot
AI
Nov 17, 2025
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 error message construction is incorrect. ORT_THROW typically takes a single string message, but this call passes two arguments separated by a comma. This should likely be a string concatenation using + or formatting the message as a single string literal.
| ORT_THROW(e.what(), "\nCould not deserialize binary data. This could mean the bin is corrupted or incompatible. Try re-generating ep context cache."); | |
| ORT_THROW(std::string(e.what()) + "\nCould not deserialize binary data. This could mean the bin is corrupted or incompatible. Try re-generating ep context cache."); |
Description
Fixes plugin -> bridge ep importing. Modifies stop logic to keep existing shared context alive instead of clearing it (a new shared context will be created after stop).