Repository navigation
Feature/openvino model package - #2717
Preetha Veeramalai (preetha-intel) wants to merge 13 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Unresolved path-safety, registration-lifecycle, and package-completeness issues can cause unsafe or unusable output.
Review effort: Balanced
Findings: 2
Open (8)
What changed in this PR
Adds OpenVINO model packaging for ONNX Runtime’s OrtModelPackageApi, with per-device variants and shared weights.
Changes:
- Supports compilation and packaging of precompiled models.
- Extends EPContext generation with OpenVINO plugin support.
- Adds pass registration, configuration examples, documentation, and tests.
| File | Description |
|---|---|
test/passes/openvino/test_openvino_model_package.py |
Tests packaging and registration behavior. |
olive/passes/openvino/model_package.py |
Implements compilation, asset staging, and manifests. |
olive/passes/openvino/examples/model_package_config_template.json |
Provides an example workflow. |
olive/passes/onnx/context_binary.py |
Adds OpenVINO compilation and plugin registration handling. |
olive/olive_config.json |
Registers the pass and supported providers. |
docs/README_OpenVINO.md |
Documents configuration, usage, and package format. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Mayuresh Varerkar (MayureshV1)
left a comment
Mayuresh Varerkar (MayureshV1)
left a comment
There was a problem hiding this comment.
Preetha Veeramalai (@preetha-intel) . Can you please fix the below suggestions.
Also, overall guidance was to hold off merging this PR till Model Packaging support becomes a stable ABI, which is expected to be soon.
| } | ||
|
|
||
|
|
||
| class OpenVINOModelPackage(Pass): |
There was a problem hiding this comment.
i'm a little bit confused about this pass. we have a CLI generate-model-package to generate a model package. I'd like to split this into 2 steps: EPContextBinaryGenerator and generate model package. the generating model package logic should be generic and all other ep can use it as well.
There was a problem hiding this comment.
does this pass only create npu package? if we want to add support for cpu, gpu as well to ort-package. will this pass work? that's why i'm thinking having a generic packaging logic will be better here.
There was a problem hiding this comment.
The CLI generate-model-package is a GenAI specific model packaging which manually creates the manifest.json and component.json. But the OpenVINOModelPackage builds the package through ORT's ort.ModelPackage API. ORT owns the manifest, component and variant metadata, and the shared-asset hashing and URI.
I agree the packaging should be generic, and it already is in the pass. The ort.ModelPackage calls don't depend on OpenVINO, and only discovery and compile are OpenVINO-specific. Splitting it by moving the ORT-API packaging out of this pass can be done as the follow up once other EP's also want to accomodate the Olive but the CLI's GenAI packaging and the pass's ORT packaging are two different formats, so it can't be one generic CLI path without making it a different mode.
|
I've moved the context binary logic into #2732. can you check this pr? |
9bc198e to
a68d4b7
Compare
a68d4b7 to
b67a720
Compare
I agree on the design where we have the EpContextBinary seperate from model packaging. Initially when this pass was added only OpenVINO use case was there hence this got swayed towards OV support. I have now modified to have a seperate generic EPContextBinary and ORTModelPackage which uses the ORT modelPackage authoring API's which is very different from the CLI generate-model-package (Made specifically for GenAI where genai_config.json is a neccessity). Xiaoyu Z (@xiaoyu-work) Can you review the updated code as it handles the end-to-end flow |
Mayuresh Varerkar (MayureshV1)
left a comment
Mayuresh Varerkar (MayureshV1)
left a comment
There was a problem hiding this comment.
Thanks for the restructuring into a generic EPContextBinaryGenerator + ORTModelPackage on top of ORT's authoring API, and for addressing my earlier comments (NVL, load_config removal, failing on missing devices). A few small doc and cleanup items are inline. I'll sync with you offline on sequencing with #2732 and the ORT 1.32 dependency (#33640) before the next round.
Could you also refresh the PR description? It still mentions package_only mode, allow_partial_package and docs/README_OpenVINO.md, which no longer match the code.
|
Preetha Veeramalai (@preetha-intel) thanks for the quick turnaround on Fixed in this update ✅
Could you resolve the two doc/comment threads that are now fixed? Still to address
|
ac1c3d9 to
c1f3b46
Compare
Mayuresh Varerkar (MayureshV1)
left a comment
Mayuresh Varerkar (MayureshV1)
left a comment
There was a problem hiding this comment.
Preetha Veeramalai (@preetha-intel) thanks for the updates in aaecfaa/c1f3b46. The registration fallback warning and the runtime session_options cleanup look good, and the refreshed description matches the code now.
Still open:
embed_context/weight_sharing/disable_cpu_fallbackon the OpenVINO path. See my follow-up in that thread.- Mocked
ORTModelPackagetests. They will likely fail on ORT < 1.32 because of the version check (inline). create_shared_asset=Falseweights check. It is skipped when the source model comes fromsource_model_path(inline).- Overlap with #2732. Both PRs still change the OpenVINO compile path in
context_binary.py. Let's settle which PR owns it before the next round. - ORT 1.32 / microsoft/onnxruntime#33640. Thanks for moving the PR to draft. Please post local native-test results once a build with the bindings is available.
Xiaoyu Z (@xiaoyu-work) could you take a look at your two open threads? The split into EPContextBinaryGenerator + ORTModelPackage landed in 578fcbc, and the <=1.23.2 question has an answer.
|
|
||
|
|
||
| @pytest.fixture | ||
| def mock_model_package(): |
There was a problem hiding this comment.
This fixture says it exercises the pass "even with ORT builds that lack the authoring bindings", but _run_for_config calls _check_ort_version(ort.__version__) before it reaches ModelPackage. On any released ORT (< 1.32) every test using this fixture will fail with the version RuntimeError instead of testing the orchestration. This looks like it came in with 24c66ac. Could you patch onnxruntime.__version__ (e.g. to "1.32.0") inside the fixture? CI hasn't run on this PR yet, so this hasn't shown up.
| @staticmethod | ||
| def _get_asset_model_path(model, config) -> Optional[Path]: | ||
| if not config.create_shared_asset: | ||
| return Path(config.shared_asset_model_path) if config.shared_asset_model_path else None |
There was a problem hiding this comment.
With create_shared_asset=False, this returns None unless shared_asset_model_path is set explicitly, so the source_model_path attribute set by EPContextBinaryGenerator is ignored. The weights check in _run_for_config is then skipped. A weightless composite from EPContextBinaryGenerator with create_shared_asset: false and no external-initializer folder in session_options produces a package whose variants can't find their weights. Could this fall back to model_attributes["source_model_path"] for the check, as the create_shared_asset=True path does? A test for that case would help.
| ort.unregister_execution_provider_library.assert_not_called() | ||
|
|
||
|
|
||
| def test_registered_ep_library_propagates_real_error(): |
There was a problem hiding this comment.
Nit: there's no test for the new fallback in registered_ep_library, where registration fails with an unrelated error but ep_name is already exposed by get_ep_devices(). Could you add one that checks the warning is logged and unregister isn't called? A .virtual registration name in the same situation should still raise.
Mayuresh Varerkar (MayureshV1)
left a comment
Mayuresh Varerkar (MayureshV1)
left a comment
There was a problem hiding this comment.
Preetha Veeramalai (@preetha-intel) thanks for a9d4da1. It covers the version-patched fixture, the source_model_path weights check, the registration-fallback tests and the enum ValueError. Three items are still open, inline:
- The new EPContext option mapping overrides values the user sets explicitly.
_read_compatnow calls a function thatonnxruntimedoesn't export.- The doc and docstring still say options are passed verbatim.
…on handling - Validate package_name and package_name + package_suffix as single path components - Set source_model_path for single-model OpenVINO EPContext output so ORTModelPackage defaults work - Reject update_genai_config/genai_config_role for OpenVINO instead of ignoring them - Share typed session option handling (log_severity_level, log_verbosity_level) with the plugin EP path
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
lintrunner found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
Mayuresh Varerkar (MayureshV1)
left a comment
Mayuresh Varerkar (MayureshV1)
left a comment
There was a problem hiding this comment.
Preetha Veeramalai (@preetha-intel) thanks for e5af08c/579e7e1. The embed/CPU-fallback precedence, the _pybind_state restore and the README mapping table all look good, and the tests cover them.
None of the items below depend on #2732, so they can go in now:
-
CI:
Python formatfails (job). Everything it reports is in the new tests, apart from one advice-level item inmodel_package.py:test_context_binary.py:150:from pathlib import Pathinside the test reimports and shadows the module-level import (W0404/W0621). Please drop it.- RUF043 (
match=with unescaped metacharacters). Use raw strings orre.escape:test_context_binary.py:593, 600test_ort_model_package.py:204, 295
- SIM117 (nested
with):test_context_binary.py:637, 650. Combine them into a singlewith. test_context_binary.py:487, 490: RUF012 (mutable class attribute__members__) and B008 (FakeEnum()as a default argument).- W0212 (protected access) throughout both files, and W0621 for the
mock_model_package/native_model_packagefixtures. Add a file-level# pylint: disable=protected-access,redefined-outer-namelike other Olive tests, or use@pytest.mark.usefixtureswhere the fixture value isn't used. - Advice R0916 (too many boolean expressions) at
model_package.py:399in_safe_relative_location. Optional: split the check into named conditions.
Please run
lintrunner -alocally to confirm it's clean. -
PR description. It still says "ORT options are passed through verbatim; Olive sets no EPContext keys." After
a9d4da1, Olive setsep.context_enable,ep.context_embed_modeandsession.disable_cpu_ep_fallback(plusembed_compiled_data_into_modelforaot) fromembed_context/disable_cpu_fallback. Please update it to match the README. -
Nit:
test_read_compat_reads_from_ort_pybind_modulepatches withcreate=True, so it still passes ifget_compatibility_info_from_modelis missing from_pybind_state. Droppingcreate=True(and skipping when the attribute is absent) would catch a rename.
Still open, tracked separately: the overlap with #2732, and ORT 1.32 / microsoft/onnxruntime#33640. Please post the local native-test results once a build with the bindings is available.
Adds support for building ONNX Runtime
.ortpackagemodel packages.This takes two passes:
EPContextBinaryGenerator(extended for OpenVINO)compile_flow:aotusesModelCompiler,jituses anInferenceSession.target_devices: compiles one EPContext model per device ID. The pass returns a composite model with one<device_id>component per device and fails if a requested device is not discovered or fails to compile.ep_device_filters,ep_registration_name,output_model_name,model_compiler_options.registered_ep_librarycontext manager handles EP library registration:ORTModelPackage(new, EP-agnostic).ortpackagethrough ORT'sonnxruntime.ModelPackageauthoring API (ORT >= 1.32). Each input model (a composite component or a single model) becomes a variant of one component..binfiles and EPContext-referenced files, preserving their relative locations...and files that resolve outside the model directory.CompositeModel, with no compile step.