ONNX Runtime PR #32303: Why a Successful Compile Produced Duplicate Graph Definitions


ONNX Runtime PR #32303: Why a Successful Compile Produced Duplicate Graph Definitions

ONNX Runtime PR #32303 fixes a small object-selection error with a much larger consequence.

The Compile API could report success after writing an ONNX model that contained duplicate graph definitions. Loading that output then failed with an error such as:

Duplicate definition-site for (X).

The graph was not duplicated because the compiler deliberately serialized it twice. It was duplicated because the serializer cleared one GraphProto and appended into another.

The destination already contained a graph

The affected path used a custom callback to determine how initializers should be stored.

At the model level, serialization began by copying the model’s existing protobuf state into the caller’s output object:

model_proto = model_proto_;

That assignment included the existing graph.

The destination graph therefore already contained its inputs, outputs, nodes, value information, and initializers.

The function then asked the in-memory Graph object to serialize its current state into that destination:

graph.ToGraphProtoWithCustomInitializerHandling(
    handle_initializer_func,
    state,
    *model_proto.mutable_graph());

For that operation to replace the existing graph fields, the graph-level serializer first needed to clear the fields it was responsible for regenerating.

The old code cleared the backing proto

Graph::ToGraphProtoInternal() received the destination as a reference named graph_proto.

The Graph object also held a pointer named graph_proto_ to its own backing protobuf.

Those objects can refer to the same graph in an ordinary synchronization path, but they were different objects in this custom output path.

The old implementation cleared the member pointer:

graph_proto_->clear_node();
graph_proto_->clear_input();
graph_proto_->clear_output();
graph_proto_->clear_value_info();

It then populated the function parameter:

graph_proto.add_input();
graph_proto.add_output();
graph_proto.add_node();

The clear and the append were operating on different objects.

A minimal example looks like this:

Backing graph:
  input X
  output Z
  node Add

Destination graph after model copy:
  input X
  output Z
  node Add

The old serializer then performed:

Clear the backing graph

Append current graph state
to the destination graph

The destination became:

inputs:
  X
  X

outputs:
  Z
  Z

nodes:
  Add
  Add

The backing graph, meanwhile, had its serialized fields removed even though the caller had asked the function to populate a separate output object.

Why writing the model still succeeded

A serialized protobuf message can contain repeated entries in repeated fields.

The write operation therefore did not have to fail merely because two input entries used the same name or two nodes defined the same values.

The semantic conflict surfaced later, when ONNX Runtime loaded and validated the emitted model.

At that point, two definitions of X were no longer harmless repeated protobuf entries. They violated the graph’s definition rules.

This explains the apparently contradictory result:

CompileModel:
  success

Model serialization:
  success

Reloading the emitted graph:
  failure

Serialization success proved that bytes were produced. It did not prove that those bytes represented a valid ONNX graph.

The fix clears the destination

The merged change removes the underscore from the four clear targets:

graph_proto.clear_node();
graph_proto.clear_input();
graph_proto.clear_output();
graph_proto.clear_value_info();

The function now clears the same object it later repopulates.

That preserves both relevant use cases.

Ordinary graph synchronization

When the caller passes the graph’s own backing proto, the destination and backing object are the same object.

The function clears and regenerates that graph as before.

Serialization into a separate output

When the caller passes a copied or independently created destination, only that destination is cleared.

The backing graph remains untouched, and the destination receives one regenerated set of graph fields.

The fix does not move initializer handling into ToGraphProtoInternal().

That separation is intentional. The custom initializer path clears existing dense and sparse initializers separately, then re-adds each initializer according to the user callback’s decision.

The resulting ownership is clear:

ToGraphProtoInternal
  owns regeneration of:
    inputs
    outputs
    nodes
    value information

Custom initializer handling
  owns regeneration of:
    dense initializers
    sparse initializers

Why the custom initializer path exposed the mistake

Other serialization paths commonly build a fresh GraphProto and assign the completed result into the model.

The custom initializer path behaved differently.

It first copied the complete ModelProto, preserving model-level metadata and other fields. It then modified the graph already inside that copy.

That made the correctness of in-place graph replacement important.

The reported Compile API case also used an output write callback. After constructing the output ModelProto, ONNX Runtime serialized it through the supplied stream callback.

The write callback did not cause the duplicated definitions. It delivered the already malformed model produced by the custom graph serialization path.

The underlying defect was therefore below the output transport boundary:

Compile API options
        ↓
Build optimized ModelProto
        ↓
Custom initializer serialization
        ↓
Wrong GraphProto cleared
        ↓
Graph fields appended to populated destination
        ↓
Output callback writes duplicated graph

The tests cover the observable failure

The added Compile API tests construct a small Add model and exercise the affected output configuration.

One test uses no initializer. It verifies that:

inputs appear once
output appears once
node appears once
initializer callback is not called
the emitted model parses
the emitted model runs

Another test includes an initializer and exercises two callback decisions:

keep initializer embedded

or

place initializer in an external file

For both cases, the test confirms that the initializer callback runs once, the graph fields appear once, the initializer appears once, and the resulting model can be loaded and executed.

A lower-level graph test also checks exact input, output, and node counts in the custom initializer serialization path.

These tests protect the user-visible contract that failed: compilation must emit a model that survives its next load and produces the expected result.

What the tests do not directly assert

The production fix clears four graph field groups:

node
input
output
value_info

The added assertions explicitly count nodes, inputs, outputs, and initializers. They do not include an exact-count assertion for a non-empty value_info collection.

The tests also do not serialize the same model repeatedly and compare the results, or separately assert that the source model’s backing proto remains unchanged.

Those are narrower coverage gaps, not evidence that the patch is incorrect.

The changed expression applies the same destination rule to all four regenerated graph fields. The principal regression is also exercised through both direct graph serialization and the public Compile API path.

The contract restored by the patch

An output parameter identifies the object a function has been asked to populate.

If that object is already populated, the function must replace the fields it owns before adding their current values.

Clearing a related source or backing object does not make the destination empty.

The repaired contract is:

Receive destination GraphProto
        ↓
clear destination-owned graph fields
        ↓
serialize current in-memory graph
        ↓
rebuild initializers through their selected policy
        ↓
emit one valid graph

A serializer must clear the destination it will populate, not a different object that happens to represent the same graph.

That is the four-line boundary repaired by ONNX Runtime PR #32303.

Sources

https://github.com/microsoft/onnxruntime/pull/32303

https://github.com/microsoft/onnxruntime/commit/cf5737297dd8c77a3b5499607b9fe2480e171748

https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/core/graph/graph.cc

https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/core/graph/model.cc

https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/core/framework/ep_context_utils.cc

https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/test/framework/inference_session_test.cc

https://github.com/microsoft/onnxruntime/blob/main/onnxruntime/test/ir/graph_test.cc

Popular posts from this blog

AMD Is Trying to Turn Kernel Diversity Into One Software Asset

The first broad integration was rolled back, but the direction of the software stack is difficult to ignore The GPU software race is often described as a contest over who can write the fastest kernel. That is only part of the problem. A company may have several teams capable of producing high-performance matrix-multiplication kernels. Some may work through template libraries. Others may use compiler-generated code, handwritten assembly, or model-specific optimization stacks. The existence of those kernels does not automatically create one coherent platform. They still need to be: discovered validated matched to the correct problem given the correct arguments launched through a stable runtime and delivered through a reliable software package Recent public code changes inside AMD’s software ecosystem showed an attempt to connect these layers more directly. The central idea was to allow hipBLASLt’s GEMM selection system to choose kernels created by multiple internal kernel technologi...
Read more

PyTorch #188031 — Why Did Forward Use 64-Bit Indexing While Backward Still Used uint32_t?

PyTorch Spatial Softmax, Part 1 of 3 — An asymmetric indexing contract and the patch that aligned it A recent PyTorch change fixed a 64-bit indexing problem in the spatial softmax backward path. The bug was not in the softmax formula itself. It was in the width of the integer used to calculate memory locations. For the same large tensor, the forward path already selected either 32-bit or 64-bit indexing based on the input size. One backward path, however, continued to calculate sizes, strides, and offsets using uint32_t . That created a possible split in behavior: Forward → 64-bit address calculation → accesses the intended elements Backward → 32-bit address calculation → offset wraparound → gradients read from or written to different locations A tensor could therefore complete the forward pass correctly while the corresponding backward kernel could no longer represent every address in that tensor. The changes described in PyTorch #188031 were later landed in PyTorch’s main branch. S...
Read more

NVIDIA CUTLASS #3017 — Why load() and store() Mapped the Same Tile Coordinate to Different Addresses

CUTLASS #3017, Part 1 of 3 — An asymmetry in address calculation and the patch that fixed it In February 2026, while reading through NVIDIA CUTLASS’s RegularTileIterator<layout::PitchLinear> implementation, I noticed an odd asymmetry. load() and store() accepted the same tile coordinate, yet they calculated the corresponding memory address differently. The difference was a single line. load() divided the contiguous coordinate by kElementsPerAccess . store() did not. When each memory access handled only one element, the difference remained invisible. But in vectorized paths, where a single access handles multiple elements, the same logical tile coordinate could resolve to two different physical addresses. I reported the issue as CUTLASS Issue #3017. The missing division was later added to store() in PR #3049. CUTLASS and tile iterators CUTLASS is NVIDIA’s open-source library for high-performance CUDA linear algebra. It provides C++ template abstractions for composing GPU ope...
Read more