Fix duplicated subgraph initializers when saving a model with external data

[Yoonseok Kim (yoonseok-kim)]

Description

Graph::AddExternalInitializersToGraphProtoImpl() recurses into subgraphs and re-adds every initializer, but never clears the entries that Node::ToProto() already placed in the subgraph proto. Each subgraph initializer therefore appears twice in the saved model, and the stale entry keeps the source model's data location:

If.then_branch:
  Wa -> source.onnx.data   offset 0        (stale entry, not removed)
  Wa -> saved.onnx.data    offset 0        (re-added by the recursion)

The saved model violates the ONNX spec and fails to load:

Data of TensorProto ( tensor name: Wa ) should be stored in source.onnx.data, but it is not regular file.

Fix by clearing the subgraph proto's initializers before the recursion, the same way the sibling path ToGraphProtoWithCustomInitializerHandlingImpl() already does. The recursion re-adds every initializer (dense, string, and sparse), so the clear is lossless.

Added two tests (subgraph initializer embedded vs. externalized); both fail without the fix.

Motivation and Context

Saving an optimized model with external initializers silently produces an invalid model whenever a subgraph owns an initializer, regardless of data location or size — a 256-byte tensor in an If branch reproduces it. This is easy to hit: since onnx 1.21, save_as_external_data=True externalizes subgraph initializers by default. Saving succeeds and the failure only surfaces when the saved model is loaded.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[Copilot]

🟡 Changes recommended

The new sparse-initializer clearing path lacks regression coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Fixes duplicate subgraph initializers when saving ONNX models with external data.

Changes:

• Clears dense and sparse subgraph initializers before regeneration.
• Adds embedded and externalized initializer regression tests.

File summaries

File	Description
onnxruntime/core/graph/graph.cc	Prevents duplicate subgraph initializers.
onnxruntime/test/framework/save_model_with_external_initializers.cc	Adds save-and-reload regression coverage.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 1
• Review effort level: Balanced

---

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Chris Thompson (christopherthompson81)]

Independent confirmation - I diagnosed this today from a different direction and arrived at the identical patch (same two clear_* calls in the same loop, same DISABLE_SPARSE_TENSORS guard), so consider this a second data point rather than a competing change. Some details from the other trigger path that may be worth folding into the description, since they make the bug considerably easier to find:

A second way in, with a different error. Rather than onnx.save(..., save_as_external_data=True), this also fires through SessionOptions.optimized_model_filepath combined with the two session.optimized_model_external_initializers_* entries. There the stale entry does not retain a source data location, so the symptom is not the data-location message but:

INVALID_GRAPH : ... In Node, ("the_loop", Loop, "", -1) : ... ,
Error <name> initializer name is not unique

Searching that string currently returns nothing, which is most of why this took a while to pin down at our end.

It predates the onnx 1.21 default. The description attributes exposure to save_as_external_data=True externalizing subgraph initializers by default since onnx 1.21. That is true for that path, but the optimized_model_filepath path is older: reproduced identically on 1.23.2, 1.24.4 and 1.29.0, CPU and CUDA alike, at every graph optimization level including ORT_DISABLE_ALL.

If branches with no author-written initializers are also affected. One of our two affected graphs is a NeMo log-mel preprocessor whose If branches ship with empty initializer lists. It still fails, because the duplicated entry is a constant ORT itself folds into the branch before serialization - which is also why ORT_DISABLE_ALL is no defense. Worth knowing that "my subgraphs have no initializers" does not rule this out.

Dropping either session-config entry makes the same graphs round-trip cleanly, which is the workaround we shipped in the interim.

Verification. Built from source at bb331b7 (Release, Linux, CPU) with the identical change and ran --gtest_filter=SaveWithExternalInitializers.*. Without the graph.cc change my regression test fails both on a raw-ModelProto duplicate-name check and on the subsequent load; with it, 4/4 pass. Your two tests are more thorough than mine, so nothing to add there - though if you want a fixture that needs no programmatic construction, the existing testdata/loop_sub_one.onnx has a Loop body owning an initializer and reproduces directly.

Happy to see this land as-is.