Repository navigation
Address github issue 29071 - #29158
Conversation
InlineIfSubgraph was erasing name_to_initial_tensor_ before looking up the OrtValue via GetOrtValueInitializer, which always returned false (since GetOrtValueInitializer checks name_to_initial_tensor_ first). Fix: extract the OrtValue BEFORE erasing from name_to_initial_tensor_, then erase both name_to_initial_tensor_ and ortvalue_initializers_ from the source graph together, and insert into the destination graph under the (possibly renamed) name. This preserves the invariant that HasExternalDataInMemory implies a findable OrtValue at all times. Add regression test: GraphTest.InlineIfSubgraphTransfersOrtValues Relates to: #29071
Add the customer's repro model (gh_issue_29071_if_constant_folding.onnx) and its generating script (.py) to testdata. Add GH_Issue_29071_HasExternalDataInMemory test that loads the model through InferenceSession and verifies initialization succeeds.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
⚠️ Not ready to approve
The newly added Python testdata generator is very likely to fail lintrunner/ruff/ruff-format in CI unless it is formatted/linted per repo standards.
Pull request overview
Fixes a regression in Graph::InlineIfSubgraph where initializers backed by in-memory external data could lose their cached OrtValue during inlining (notably when constant-folding If nodes), and adds regression coverage to ensure models using this pattern load/initialize successfully.
Changes:
- Update
Graph::InlineIfSubgraphto extract any cached initializerOrtValuebefore erasing the source initializer mapping, and restore it into the destination graph under the (possibly renamed) initializer name. - Add a targeted unit test that simulates constant-folding/inlining and verifies that any initializer marked
HasExternalDataInMemorystill has a cached, allocatedOrtValueafterward. - Add an end-to-end test plus a repro model generator script under
onnxruntime/test/testdata/for GH issue 29071.
File summaries
| File | Description |
|---|---|
| onnxruntime/core/graph/graph.cc | Fixes initializer OrtValue transfer during If subgraph inlining to prevent broken in-memory external-data initializers. |
| onnxruntime/test/ir/graph_test.cc | Adds unit + E2E regression tests covering InlineIfSubgraph with in-memory external-data initializers. |
| onnxruntime/test/testdata/gh_issue_29071_if_constant_folding.py | Adds a script to regenerate the minimal ONNX repro model for GH issue 29071. |
Copilot's findings
- Files reviewed: 3/4 changed files
- Comments generated: 7
Note
Your feedback helps us improve the quality of this feature.
Please use 👍 or 👎 to tell us whether this assessment is correct.
- Fix docstring path to match actual file location - Fix default output filename to match checked-in model name - Fix unused variable warning (session -> _) - Fix misleading comment (else branch initializer is also large) - Ensure Python file passes ruff check and format
Add error checks in InlineFunction and MakeInitializerCopyIfNotExist to reject the case where a TensorProto has external-data-in-memory markers but no corresponding OrtValue. This state should never occur in normal operation and indicates a bug if reached.
Tianlei Wu (tianleiwu)
left a comment
Tianlei Wu (tianleiwu)
left a comment
There was a problem hiding this comment.
Review: Address github issue 29071
Verdict: Approve. Correct, well-scoped fix for the GH #29071 regression with strong regression coverage.
Core fix — Graph::InlineIfSubgraph
Root cause correctly identified: the old code called GetOrtValueInitializer(src_name, ...) after name_to_initial_tensor_.erase(src_name). Because GetOrtValueInitializer first requires GetInitializedTensor to succeed, the lookup always failed post-erase, so the OrtValue was never transferred and the inlined initializer ended up with HasExternalDataInMemory()==true but no cached OrtValue — crashing later in SaveInitializedTensors. The fix extracts the OrtValue before the erase, additionally clears the stale source ortvalue_initializers_ entry (a leak the old path left behind), and re-keys it under the possibly-renamed destination name. Since rename does not change the external_data address, re-keying is correct.
Invariant enforcement
The stricter checks in AddInitializedTensor / AddInitializedOrtValue / ReplaceInitializedTensorImpl are internally consistent. I verified that the ORT-format load path that sets the in-memory external-data marker also creates the backing OrtValue, and the flatbuffers loader only sets the memory-address tag under can_use_flatbuffer_for_initializers (the same condition that creates the OrtValue). So the removed "flatbuffers ort format (no OrtValue)" branch is genuinely unreachable, and the new ORT_THROW / ORT_RETURN_IF_NOT are valid backstops rather than behavior regressions.
I also audited external callers of AddInitializedTensor (OpenVINO qdq_stripping.cc / qdq_scales_fix.cc, training, vitisai). The OpenVINO paths materialize embedded data via GetTensorProtoWithDataIfInMemory before calling AddInitializedTensor, so the new hard throw does not fire on existing paths.
Tests
Comprehensive: InlineIfSubgraphTransfersOrtValues reproduces the exact regression, negative tests cover each tightened API, and GH_Issue_29071_HasExternalDataInMemory provides end-to-end session-init coverage against the checked-in repro model. The CreateIntializer→CreateInitializer typo cleanup is appreciated.
One optional readability nitpick inline. Nice fix overall.
This pull request addresses a regression in ONNX Runtime's graph inlining logic, specifically ensuring that initializers with in-memory external data (OrtValues) are correctly transferred when inlining subgraphs (such as during constant folding of If nodes). It also adds comprehensive regression tests and supporting test data to prevent similar issues in the future. The most important changes are: **Bug Fixes and Core Logic:** * Fixed a bug in `Graph::InlineIfSubgraph` to ensure that OrtValues for initializers with in-memory external data are extracted from the source graph before erasing their entries, and correctly restored in the destination graph after inlining. This maintains the invariant that an initializer marked as "HasExternalDataInMemory" always has a corresponding OrtValue. [[1]](diffhunk://#diff-e231a92b40d89409cc8e82436be0a15bc87ef95c93b303b9feaeab6e50c8835cR6067-R6077) [[2]](diffhunk://#diff-e231a92b40d89409cc8e82436be0a15bc87ef95c93b303b9feaeab6e50c8835cL6085-R6097) **Testing and Regression Coverage:** * Added a regression test `InlineIfSubgraphTransfersOrtValues` to `graph_test.cc` to verify that OrtValues are properly transferred when inlining If subgraphs with large initializers, preventing crashes during model saving. * Added an end-to-end regression test `GH_Issue_29071_HasExternalDataInMemory` to ensure that loading and initializing a model with this pattern works as expected. * Included the necessary import for `graph_utils.h` in the test file to support new test logic. **Test Data:** * Added a Python script (`gh_issue_29071_if_constant_folding.py`) to generate the minimal ONNX model that reproduces the original bug, supporting ongoing regression testing. --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> (cherry picked from commit 9ad4d58)
…29713) (#29792) ### Description Fixes #29713. On the CUDA EP, a device copy scheduled for a weight retained as an in-memory-external `OrtValue` initializer can have a source and destination that are the **same physical host buffer**. The host->host branch of `GPUDataTransfer::CopyTensor`/`CopyTensorAsync` did not handle this: - **Non-cuda-graph:** `ORT_ENFORCE(dst_data != src_data)` fired -> hard failure. - **CUDA-graph capture:** the `HOST_ACCESSIBLE` `cudaStreamSynchronize` is illegal during capture -> `CUDA 900` -> `cudaStreamEndCapture` `CUDA 901`, invalidating the capture. This became reachable once #29158 (initializer retention) landed. ### Fix Early-return the no-op self-copy at the top of the host->host branch in both methods, **before** the `HOST_ACCESSIBLE` stream sync (the key to keeping capture valid). This matches `CPUDataTransfer::CopyTensor` and the existing GPU->GPU guard. ### Testing Adds a CUDA EP internal regression test (`gpu_data_transfer_test.cc`) covering sync, async, and cuda-graph-capture self-copies. Verified failing before the fix (ORT_ENFORCE / CUDA 900->901) and passing after, on an RTX 3050 with a CUDA 12.8 `sm_86` build. --------- Co-authored-by: Trisha-sarvam <trishawadhwani@sarvam.ai> Co-authored-by: Cursor <cursoragent@cursor.com>
This pull request addresses a regression in ONNX Runtime's graph inlining logic, specifically ensuring that initializers with in-memory external data (OrtValues) are correctly transferred when inlining subgraphs (such as during constant folding of If nodes). It also adds comprehensive regression tests and supporting test data to prevent similar issues in the future.
The most important changes are:
Bug Fixes and Core Logic:
Graph::InlineIfSubgraphto ensure that OrtValues for initializers with in-memory external data are extracted from the source graph before erasing their entries, and correctly restored in the destination graph after inlining. This maintains the invariant that an initializer marked as "HasExternalDataInMemory" always has a corresponding OrtValue. [1] [2]Testing and Regression Coverage:
InlineIfSubgraphTransfersOrtValuestograph_test.ccto verify that OrtValues are properly transferred when inlining If subgraphs with large initializers, preventing crashes during model saving.GH_Issue_29071_HasExternalDataInMemoryto ensure that loading and initializing a model with this pattern works as expected.graph_utils.hin the test file to support new test logic.Test Data:
gh_issue_29071_if_constant_folding.py) to generate the minimal ONNX model that reproduces the original bug, supporting ongoing regression testing.