Repository navigation
Retain external-data file handles and original model directories - #33062
Gopalakrishnan Nallasamy (GopalakrishnanN) wants to merge 15 commits into
Conversation
Parse native models and external tensor data through retained file objects. Carry captured directories through graph processing and initializer loading. Add file-lifetime, directory-context, range, and loading regressions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
It changes security-sensitive, cross-platform file handling across 62 files without local POSIX or CUDA validation.
Review effort: Balanced
Findings: None
What changed in this PR
Retains opened model/external-data files and preserves model-directory context throughout loading and optimization.
Changes:
- Introduces
ModelPath, canonical file opening, retained mappings, and checked ranges. - Propagates directory context through graphs, sessions, optimizers, providers, and CUDA loading.
- Adds extensive lifetime, path, range, and loader-handoff tests and documentation.
No actionable defects were identified in the reviewed diff.
| File | Description |
|---|---|
onnxruntime/test/platform/file_io_test.cc |
Updates mapping error assertions. |
onnxruntime/test/platform/env_test.cc |
Tests canonical opening, mappings, and model paths. |
onnxruntime/test/framework/tensorutils_test.cc |
Adds external-file regression coverage. |
onnxruntime/test/framework/external_data_loader_test.cc |
Tests retained-file loader behavior. |
onnxruntime/test/ep_graph/test_ep_graph.cc |
Adapts graph path validation. |
onnxruntime/core/session/provider_bridge_ort.cc |
Bridges ModelPath across providers. |
onnxruntime/core/session/inference_session.h |
Retains parsed model context. |
onnxruntime/core/session/inference_session.cc |
Preserves context through session loading. |
onnxruntime/core/providers/webnn/builders/model_builder.cc |
Uses the plain path where required. |
onnxruntime/core/providers/vsinpu/vsinpu_util.h |
Accepts ModelPath. |
onnxruntime/core/providers/vsinpu/vsinpu_util.cc |
Implements updated signature. |
onnxruntime/core/providers/vitisai/imp/global_api.cc |
Exposes the filesystem path. |
onnxruntime/core/providers/tensorrt/onnx_ctx_model_helper.cc |
Returns the underlying path. |
onnxruntime/core/providers/shared_library/provider_wrappedtypes.h |
Updates shared-provider wrappers. |
onnxruntime/core/providers/shared_library/provider_interfaces.h |
Updates provider interfaces. |
onnxruntime/core/providers/shared_library/provider_bridge_provider.cc |
Forwards retained model context. |
onnxruntime/core/providers/shared_library/provider_api.h |
Updates path-validation API. |
onnxruntime/core/providers/openvino/onnx_ctx_model_helper.cc |
Adapts context-path handling. |
onnxruntime/core/providers/openvino/backend_manager.cc |
Uses the underlying model path. |
onnxruntime/core/providers/nv_tensorrt_rtx/onnx_ctx_model_helper.cc |
Adapts model-path access. |
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/op_builder_helpers.h |
Accepts ModelPath. |
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/op_builder_helpers.cc |
Implements updated signature. |
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/helper.h |
Adds model-context support. |
onnxruntime/core/providers/nnapi/nnapi_builtin/builders/helper.cc |
Propagates model context. |
onnxruntime/core/providers/migraphx/migraphx_execution_provider.cc |
Stores the plain model path. |
onnxruntime/core/providers/migraphx/migraphx_execution_provider_utils.h |
Adapts path-based graph IDs. |
onnxruntime/core/providers/cuda/cuda_external_data_loader.h |
Adds open-file loading API. |
onnxruntime/core/providers/cuda/cuda_external_data_loader.cc |
Loads CUDA data from retained files. |
onnxruntime/core/platform/windows/env.h |
Declares canonical file opening. |
onnxruntime/core/platform/windows/env.cc |
Implements canonical opening and mapping. |
onnxruntime/core/platform/posix/env.cc |
Implements canonical opening and mapping. |
onnxruntime/core/platform/env.h |
Defines retained-file and range APIs. |
onnxruntime/core/platform/env.cc |
Captures and opens model paths. |
onnxruntime/core/optimizer/transpose_optimization/ort_optimizer_api_impl.cc |
Retains context during tensor access. |
onnxruntime/core/optimizer/qdq_transformer/qdq_util.h |
Accepts ModelPath. |
onnxruntime/core/optimizer/qdq_transformer/qdq_util.cc |
Propagates model context. |
onnxruntime/core/optimizer/optimizer_execution_frame.h |
Updates execution-frame paths. |
onnxruntime/core/optimizer/optimizer_execution_frame.cc |
Implements updated signatures. |
onnxruntime/core/optimizer/matmul_scale_fusion.cc |
Preserves context while unpacking. |
onnxruntime/core/optimizer/initializer.h |
Updates initializer constructors. |
onnxruntime/core/optimizer/initializer.cc |
Uses retained model context. |
onnxruntime/core/graph/model.h |
Extends model-loading interfaces. |
onnxruntime/core/graph/model.cc |
Parses models from retained files. |
onnxruntime/core/graph/graph.cc |
Propagates context through graphs. |
onnxruntime/core/graph/graph_flatbuffers_utils.h |
Updates serialization interfaces. |
onnxruntime/core/graph/graph_flatbuffers_utils.cc |
Preserves context during serialization. |
onnxruntime/core/framework/tensorprotoutils.h |
Declares retained external-data APIs. |
onnxruntime/core/framework/tensorprotoutils.cc |
Resolves, opens, maps, and reads retained files. |
onnxruntime/core/framework/session_state.h |
Accepts retained graph context. |
onnxruntime/core/framework/session_state.cc |
Propagates context during finalization. |
onnxruntime/core/framework/session_state_utils.h |
Updates initializer-loading interface. |
onnxruntime/core/framework/session_state_utils.cc |
Uses context-aware tensor loading. |
onnxruntime/core/framework/node_unit.h |
Returns ModelPath. |
onnxruntime/core/framework/node_unit.cc |
Implements updated accessor. |
onnxruntime/core/framework/max_shape_override.cc |
Preserves context in shadow models. |
onnxruntime/core/framework/graph_partitioner.cc |
Extracts plain paths where needed. |
onnxruntime/core/framework/external_data_loader.h |
Adds retained-file loader entry point. |
onnxruntime/core/framework/ep_context_utils.cc |
Adapts model-path access. |
include/onnxruntime/core/graph/graph.h |
Exposes context-aware graph paths. |
include/onnxruntime/core/graph/graph_viewer.h |
Returns ModelPath. |
include/onnxruntime/core/common/model_path.h |
Introduces the ModelPath abstraction. |
docs/Model_Loading_Performance.md |
Documents external-file lifetime contracts. |
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>
|
I suggest revisiting this PR after #33063 is merged. |
Tianlei Wu (tianleiwu)
left a comment
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Reviewed all 69 changed files at d8e91d3, including POSIX/Windows positional I/O and mapping ownership, native fallback/prepacked reads, model-directory propagation, CUDA staging/reader lifetimes, shared-provider interfaces, and regression tests. The retained-file and immutable-context design is a substantial improvement; one correctness regression is identified inline.
Current-head CI: wasm_Debug / build-wasm fails PathValidationTest.ValidateExternalDataPathSymlinkedModelAndData_HuggingFaceLayout in onnxruntime_test_all; onnxruntime_provider_test passes (run). Analyze (java) and Suggest fixes are cancelled. Of 100 checks, 95 are successful, 1 skipped, 1 neutral, 1 failed, and 2 cancelled; no named checks are queued/in progress.
Mergeability: GitHub reports CONFLICTING / DIRTY. A nonmutating merge-tree comparison against current origin/main (0ea42b2) confirms conflicts in onnxruntime/core/framework/graph_partitioner.cc and onnxruntime/core/session/inference_session.cc.
No local builds, tests, linters, or executable verification were performed, as requested. Please address the inline regression, resolve the conflicts, and obtain successful relevant CI before merging.
Resolve native loader, mapping, model parsing, telemetry, and graph-partitioning conflicts while preserving opened-file validation and EP context API updates. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Check Emscripten virtual-filesystem availability without invoking disabled syscall stubs. Reuse the check during path validation and cover captured directory state and retained HuggingFace symlink context. Update loading contracts for native opened-file validation. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Preserve the environment mapping interface while transferring ownership from the opened-file mapping type introduced by main. Leave outputs unchanged if mapping or deleter construction fails. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Use the native opened-file LoadTensor signature in the loader-handoff regression and preserve the Windows unmap helper for error reporting and warning-clean builds. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refresh the preserved PR ancestry to main 397657d. The incoming change only restores group-query attention is_int8 detection; existing native host/core validation scope is unchanged. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Delegate retained-file model loading through validated ModelProto loading while retaining its captured ModelPath. Publish model outputs only after successful construction and resolution; clear outputs on parsing/open/validation failures without invalidating aliased input data. Strengthen the five malformed-function regressions across const/move/bytes/path/explicit-context/fd sources and existing invalid-content coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Refresh PR microsoft#33062 to main 9f6baac, retaining the external-data context and malformed-function fixes. Preserve optional OpenVINO no-exception library probing and materialized-tensor loader correction with their tests. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The iOS cancellation was a90-minute timeout while Detox was stuck in simulator bootstatus/shutdown, before any Onnxruntime application/test lifecycle began. Build and dependency steps passed. This is distinct from the native application crash tracked33185. Queued one job-only retry of113052903466 on unchanged6abdb2f; no successful jobs were restarted. Will verify its actual result rather than call the retry a pass. |
|
The single iOS simulator-setup retry reached actual app execution and exposed the same AsyncWorker::toPromise SIGSEGV during EPContext data read tracked in #33185, matching the unchanged-native-source #29563 crash. Added exact job/stack to that issue. This is not treated as infrastructure anymore; no repeated retry or waiver. The native check remains an external mobile-owner blocker. |
|
Suggest fixes job113040994530/run37694016727 remains genuinely queued for over8hours on1ES.Pool=onnxruntime-github-Ubuntu2204-AMD-CPU with no runner assigned. No code/lint execution result exists yet. Requesting CI/pool provisioning attention rather than duplicate re-runs. The separate native iOS crash is explicitly tracked in #33185; all other completed current-head checks passed. |
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Resolve the model-path telemetry conflict using main bounded filename handling while retaining external-data file ownership. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Resolved the new conflict with current main be3b8e8 using a normal merge (65fdb94), preserving bounded session-filename telemetry and the retained external-data model path. Fresh Windows RelWithDebInfo MSVC build of onnxruntime_test_all passed. Targeted retained-file/model-path/loader/function/telemetry regressions: 119 passed, 15 explicitly reported Windows symlink-privilege skips, zero failures, exit 0. The React Native lifetime repair from #33204 is preserved. Push starts fresh CI; stale failing iOS runs were not retriggered. |
Replace the dangling heap-address validity oracle with DLL-local state observed after CPUIDInfo destruction. Exercise an intentionally retained consumer on a second load/unload and verify its final cleanup. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> (cherry picked from commit a350391)
|
The fresh native iOS E2E and Android gates both passed after the lifetime repair: https://github.com/microsoft/onnxruntime/actions/runs/37814696214/job/113452259910 and https://github.com/microsoft/onnxruntime/actions/runs/37814696214/job/113453500749 . Separately, Windows XNNPACK reproduced the unchanged CPUinfo DLL-unload address-oracle failure tracked in #33144 (17/18 CTest executables passed). I am applying the already published focused #33205 ownership-state repair, not rerunning the unchanged assertion or weakening the test. |
|
Published the same focused #33205 CPUinfo ownership-oracle repair as 3035011. Fresh exact-branch native standalone build passed; unload and refcount CTest executables each passed 1,000 repetitions, including the retained-consumer negative control. No production external-data or React Native behavior changed in this commit. Fresh CI will verify vcpkg/XNNPACK; the old failing test was not rerun on stale code. |
|
The newly published current head 3035011 has now passed both originally failing configurations: Windows vcpkg/XNNPACK https://github.com/microsoft/onnxruntime/actions/runs/37820641716/job/113460478372 and native iOS E2E https://github.com/microsoft/onnxruntime/actions/runs/37820641710/job/113471157589 . A separate WebGPU Debug job failed downloading Gradle with HTTP 500, with no compiler/test error; its fail-fast siblings were cancelled. That diagnosed infrastructure failure is being retried only after its containing workflow settles; successful jobs are not restarted. |
|
Final verification: current head 3035011 passed both originally failing native configurations (iOS E2E and Windows vcpkg/XNNPACK). The separate Gradle-distribution HTTP 500/cancelled WebGPU siblings have also recovered in workflow 37820642197 attempt 2. Only failed/cancelled jobs were retried; successful matrix jobs were not restarted. No test assertions or required checks were waived. |
|
Updated CI diagnosis after deeper job/artifact inspection: the CUDA plugin C++ step is explicitly marked SUCCESS, completed at 2026-10-08 20:07:42 UTC. The Python step started then and remained in progress until the 3h30 job timeout. Therefore the last visible passing GridSample line is incomplete C++ logging, not evidence that GridSample or the provider suite hung. The workflow runs the two C++ executables sequentially, then uses a separate Docker container to install the ORT wheel, numpy/onnx and CPU torch before running test_cuda_plugin_ep.py. No Python-step output, process listing or stack is available in the partial job log; the downloadable run log archive contains only the build job. The sole build-output artifact predates runtime tests, so it cannot contain their JUnit/CTest results. We cannot currently distinguish package-install/import/runtime stalls. Need unbuffered verbose Python-step output, child-process identification, timed faulthandler dumps and native/CUDA stacks if a test stalls; no demonstrable file-handle root cause justifies a code change or blind rerun. Job: https://github.com/microsoft/onnxruntime/actions/runs/37820642139/job/113488857392 . The separate Extended Minimal 24h runner-shortage retry was verified attempt 2; reviewer reapproval is still required despite resolved inline comments. |
Keep dependency commands, full unittest selection, failure exit status, C++ tests and the 210-minute job timeout unchanged. Enable unbuffered output and a passive 300-second faulthandler watchdog before imports to distinguish installation, import and test stalls. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Diagnostic-only follow-up pushed as 1ca0e49; the timeout is NOT claimed fixed. Only the existing Linux CUDA plugin Python step changed: unbuffered Python/pip output, explicit ORT-wheel/numpy-onnx/CPU-torch/test phase markers, verbose complete unittest selection, and passive all-thread Python faulthandler dumps every 300 seconds starting before imports. The watchdog is canceled on process exit; no forced exit, new skips, timeout changes, C++ changes or production file-handle changes. Local YAML/Bash extraction tests verified actual nested quoting, identical dependency commands and 210-minute timeout, pass/fail/import-error exit equivalence, unchanged fixture test selection/skips, and repeated import-time dumps using an accelerated validation-only interval. Fresh standard Linux GPU CI started automatically: https://github.com/microsoft/onnxruntime/actions/runs/37983430010 . In its Test Linux CUDA Plugin EP x64 Release job, expand Run CUDA Plugin EP Python Tests: the last phase/test name distinguishes installation/import/runtime stalls, and Timeout (0:05:00)! blocks contain Python thread stacks. Native/CUDA stack capture may still be needed after identifying a stalled test. No stale-head retry was issued. |
…on test The CUDA plugin and independent OpenVINO merge builds both fail because upstream f289d7a added an allocator argument. Dispatch the test call at compile time so the branch five-argument API and merged six-argument API both remain valid without changing production behavior or disabling coverage. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
Fresh diagnostic run37983430010 failed before the GPU/Python job could start: GCC rejects the added NativeLoaderReceivesTheOpenedFile test because main commit f289d7a (#33055) added an AllocatorPtr argument. Independent OpenVINO run37983430011/job113999369112 reports the identical tensorutils_test.cc:2925 error, so this is a shared native-build regression, not GPU infrastructure or Python watchdog failure. Fixed in b509304 with compile-time test-call dispatch supporting the branch five-argument signature and actual PR-merge six-argument signature; no production semantics, tests, timeout or diagnostics removed. The entire changed tensorutils_test.cc translation unit compiled successfully under MSVC /W4 /WX against both actual branch and failed-merge headers. An unmodified-test negative control fails against the merged header with the same missing allocator argument. Format/whitespace/editor checks pass. No stale-head retry issued; fresh CI starts automatically. The original Python-stage timeout remains unknown because the diagnostic GPU job was skipped behind this compile failure. |
…gistration Both WebGPU compile variants reject ModelPath at the D3D12 candidate-registration call added by upstream microsoft#33055. Integrate the exact already-tested upstream base 0066c38, align the helper declaration and implementation with ModelPath, and explicitly project Path() only for existing pathname parsing. Retain the ModelPath object for file validation rather than introducing an implicit conversion that loses its captured-directory context. Add a feature-guarded candidate-registration regression. Full production caller/helper and regression translation units compile with ENABLE_D3D12_FILE_LOADING and MSVC /W4 /WX. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
|
The b509304 GPU run37989760823 is green, including build114020619506 and test114041062586. Actual Python logs show 93 tests in2.502s, OK(skipped=2), and the full step finished in24s with no watchdog dump. The old Python timeout did not recur; this does not prove its original root cause. Fresh failures were diagnosed separately: Optional Lint C++ failed before linting when Docker Hub repeatedly returned HTTP429 for python:3.14-alpine; both WebGPU compile variants report C2664 at session_state_utils.cc:442 because new D3D12 candidate registration accepts filesystem::path while this PR uses ModelPath. The static novcpkg red check is a summary of that compile failure, not a separate test failure. Fixed in13770d4a4187081e9140b150cd142aae488afbed: integrate exact CI upstream base0066c380d0, retain ModelPath through candidate registration and validation, explicitly project Path() only for pathname parsing, and add a feature-guarded registration regression. Entire actual session_state_utils.cc, tensorprotoutils.cc and tensorutils_test.cc compile successfully under MSVC /W4 /WX with ENABLE_D3D12_FILE_LOADING enabled. Format/whitespace/editor checks pass; no local linked or GPU execution is claimed for this latest change. Production native retained-file behavior and Python diagnostics are unchanged. New-head CI starts automatically; no stale-head retries were issued, including the infrared lint failure or still-active WebGPU workflow. |
Description
Retains opened-file objects and original model-directory context through model parsing, external initializer sizing/mapping/reads, prepacked blobs, loader handoff, graph/subgraph optimization, sessions, and provider interfaces.
Refreshed on main
397657de778d05b17c0249ba37c0b4290e1ab3b5, preserving #33063's opened-file range validation,uint64_tlengths and mapping ownership, and the merged stable EPContext foundation.WASM now checks filesystem capability without invoking abort-capable syscall probes. Filesystem-enabled builds retain apparent and resolved model directories, restoring the HuggingFace snapshot/blob symlink fallback;
FILESYSTEM=0preserves its mounted-data/deferral contract. Explicit directory overrides do not gain a virtual-model-target escape path.Fresh CI exposed a separate model-loading reconciliation bug: retained-file pathname loading bypassed main's preconstruction function-proto validation. It now delegates through the validated loader while keeping captured
ModelPath. Proto loaders publish their candidate only after successful validation/resolution; file/bytes/fd failures clear outputs while retaining previous models long enough to protect aliased inputs.Expanded the five existing malformed-function regressions across const/move proto, bytes, pathname, captured-context and fd entry points, including non-null output arguments. Error assertions were not weakened.
Motivation and Context
File sizing, mapping and reads must refer to the same opened object rather than repeatedly resolving a mutable pathname. Directory context must survive cwd/model-path changes without weakening containment or explicit override policy. This remains distinct from the already-merged opened-file validation in #33063.
Fresh native validation
Own Windows RelWithDebInfo MSVC build passed with verified
protoc 33.6:Before the loader correction: all 5 failed. Final rebuild: 5 passed, no skips.
Final result: 76 passed, 15 symbolic-link privilege skips, zero failures, 91 total. Actual suites cover loader lifetime, tensor parsing, path validation, retained files, capture, random-access files and file I/O. EPGraph, the selected session patterns and OrtEpLibrary matched zero cases in this executable and are not public/plugin API runtime evidence.
Eight changed production translation units passed syntax compilation, including shared-provider bridge; changed-hunk clang-format and final delta checks passed.
Actual compiled WASM verification
Repository setup first failed on missing
emsdk.bat. Restored the pinned SDK in the owned worktree: Emscripten 4.0.23, owned Node 22.16.0.Build and both runtime commands passed:
Both executables link actual production common/framework methods, with
FILESYSTEM=0/1,ASSERTIONS=2,EXIT_RUNTIME=1,ENVIRONMENT=node,ALLOW_MEMORY_GROWTH=1, andSTACK_SIZE=5242880. This is compiled WASM production-path execution, not the earlier extracted-JavaScript probe. It does not establish full browser inference or the complete WASM gtest executable.Remaining merge gates
Final-head native provider/POSIX execution now passed:
Windows-native symlink skips are not counted as passes; the native POSIX and compiled filesystem-enabled WASM paths do execute the targeted layout.
Remaining: Suggest fixes is genuinely queued without an assigned runner for more than eight hours; iOS EPContext E2E hits the intermittent native AsyncWorker crash tracked in #33185. After a simulator setup timeout, one infrastructure retry reached the app and reproduced that crash; it is not being retried away. Formal request-changes review still requires explicit reconsideration.
Main's independent materialized dtype failure was fixed by #33168, included here, and #33183 closed only after the complete 1,650-model gate passed.