[OV EP] Followup: External initializers in memory - remove the 32MB threshold #919
Conversation
Remove the 32MB threshold for embedding external initializers in the ONNX model proto. Now, if external weights are present and there is more than one external initializer, data will not be embedded regardless of size. CVS-174613
Refactored the OVEP_ExtInit_Tests to generate and load the ONNX model and large external initializer data only once per test suite, rather than per test case. Model and weights are now stored as static members and cleaned up after all tests. This reduces redundant file I/O and memory usage. Test logic and device probing remain unchanged.
There was a problem hiding this comment.
Pull request overview
This PR removes the artificial 32MB threshold for external initializers in the OpenVINO execution provider, allowing the feature to be used more broadly without size limitations.
Changes:
- Removed the 32MB size threshold check in backend_manager.cc
- Refactored the test code to use static test suite setup/teardown for better resource management
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| onnxruntime/core/providers/openvino/backend_manager.cc | Removed the MAX_EMBEDDED_INITIALIZER_SIZE constant and simplified the condition for using external initializers |
| onnxruntime/test/providers/openvino/openvino_ep_ext_init.cc | Refactored test to use static class members and suite-level setup/teardown, eliminating per-test file operations |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (!model_proto.SerializeToOstream(&model_file)) { | ||
| throw std::runtime_error("Failed to serialize model"); | ||
| } |
There was a problem hiding this comment.
The error handling pattern is inconsistent - using exceptions in static setup while using ASSERT macros in test body. Consider using ASSERT_TRUE here for consistency with the test framework, or document why exceptions are preferred in SetUpTestSuite.
| if (!weights_file.is_open()) { | ||
| throw std::runtime_error("Failed to open weights file"); | ||
| } |
There was a problem hiding this comment.
The error handling pattern is inconsistent - using exceptions in static setup while using ASSERT macros in test body. Consider using ASSERT_TRUE here for consistency with the test framework, or document why exceptions are preferred in SetUpTestSuite.
| if (!model_data_.has_value() || !weights_data_.has_value()) { | ||
| throw std::runtime_error("Failed to load model or weights into memory"); | ||
| } |
There was a problem hiding this comment.
The error handling pattern is inconsistent - using exceptions in static setup while using ASSERT macros in test body. Consider using ASSERT_TRUE here for consistency with the test framework, or document why exceptions are preferred in SetUpTestSuite.
There was a problem hiding this comment.
what's the common practice in ovep? should I just fail the test when I have some io error?
MayureshV1
requested review from
Copilot and
ericcraw
and removed request for
ericcraw
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
2 participants
Description
To stay on the safer side, the ext initializer feature was limited with a 32MB threshold for all ext initializers. We can now remove the artificial condition.
Related to CVS-174613