`register_initializer` should create an unique name to resolve name collisions

[titaiwangms]

The feature was initially proposed here: microsoft/onnxscript#2680 (comment), where we saw a silent name collision leading to problematic optimized model.

We should firstly refactor rewrite rules to use the official initializer registration API to avoid silent errors:
https://github.com/microsoft/onnxscript/blob/478acf741471a4764d507d52a4ca6b544a63fc86/onnxscript/rewriter/_rewrite_rule.py#L685-L691

Also, instead of throwing errors, register_initializer can solve this automatically:

ir-py/src/onnx_ir/_core.py

Lines 2727 to 2743 in 926397c

	def register_initializer(self, value: Value) -> None:
	"""Register an initializer to the graph.
	This is a convenience method to register an initializer to the graph with
	checks.
	Args:
	value: The :class:`Value` to register as an initializer of the graph.
	It must have its ``.const_value`` set.
	Raises:
	ValueError: If a value of the same name that is not this value
	is already registered.
	ValueError: If the value does not have a name.
	ValueError: If the initializer is produced by a node.
	ValueError: If the value does not have its ``.const_value`` set.
	"""

cc @justinchuby @xadupre @gramalingam more thoughts

[justinchuby]

Allowing an option to avoid conflicts seems doable. Creating a globally unique name (within the graph) however is O(n). Would be good to know what level of guarantee we can to provide

[gramalingam]

We hit one more similar bug, fixed in this PR: microsoft/onnxscript#2883 ... why not handle initializer values just like non-initializer values (wrt names)? That is, value-identity is based on object, and names are adjusted to avoid conflicts when serializing to proto. The main issue is the use of name-based dictionary for initializers. Do we really need it? I asked copilots/agents to investigate this, and they have a recommendation and an implementation of the recommendation. See below for their recommendation.

[gramalingam]

AI generated recommendation below. Longer version of this attached:

init-names.md

Root Cause: dict[str, Value]

The collision issue isn't just about register_initializer() — it's structural. GraphInitializers inherits from UserDict[str, Value], making names the identity mechanism for initializers. This has cascading consequences:

• Adding an initializer requires a unique name upfront (can't defer to serialization)
• Two initializers with the same name = silent dict overwrite → data loss (the exact bug in microsoft/onnxscript#2883)
• Tape.initializer() requires a name but has no graph context to check for conflicts — and this is the primary API rewrite rules use

Non-initializer values don't have this problem: their names are cosmetic, auto-generated by NameAuthority, and uniquified by NameFixPass before serialization. Initializers are the only values where names function as identity rather than hints.

Re: O(n) concern

@justinchuby — great question about the cost guarantee. Our approach uses NameAuthority's monotonic counter, not a linear scan:

def _generate_unique_value_name(self, prefix: str) -> str:
    counter = 0
    while True:
        candidate = f"{prefix}_{counter}"
        counter += 1
        if not self._name_authority.has_value_name(candidate):
            self._name_authority.register_value_name(candidate)
            return candidate

has_value_name() is an O(1) set lookup. In practice this is O(1) amortized — the counter advances monotonically, so collisions with existing names are rare and bounded. This gives per-graph uniqueness cheaply. Cross-scope uniqueness (the ONNX no-shadowing rule) is deferred to NameFixPass, which already does a single top-down model traversal with a scope stack — the architecturally correct place for it.

Proposed Solution (Two Phases)

Phase 1: Graph.create_initializer() factory (non-breaking)

A new factory method that generates unique names via NameAuthority and registers the initializer in one step:

value = graph.create_initializer(tensor, name_hint="fused_weight")
# → name "fused_weight" if available, else "fused_weight_0", "fused_weight_1", ...

Tape.initializer() is updated to route through this factory when attached to a graph, making name=None legal. Rewrite rules no longer need to worry about naming:

# Before (PR #2883 pattern — collision-prone)
op.initializer(fused_weights, name=inbound_node.inputs[1].name)

# After — safe, no naming concern
op.initializer(fused_weights, name_hint="fused_weight")

Phase 2: Identity-based GraphInitializers (internal refactor)

Replace UserDict[str, Value] with an identity-based collection:

class GraphInitializers:
    _values: list[Value]                  # ordered (deterministic serialization)
    _identity_set: set[Value]             # O(1) membership by identity
    _name_cache: dict[str, Value] | None  # lazy, rebuilt on name-based access

This eliminates the root cause — names become optional hints, not identity. The Value.name setter simplifies from 36 lines (with dict pop/reinsert surgery) to ~8 lines. Backward-compatible dict-like access (graph.initializers["name"], "name" in graph.initializers) is preserved via the lazy name cache during a deprecation period.

Key insight: NameFixPass already handles unnamed values — _process_value() checks if not value.name and generates one, with full scope-aware uniquification. The only blocker today is Value.name setter's prohibition on None for initializers (_core.py:2858-2861), which exists solely because of the dict constraint. Removing the dict means NameFixPass handles initializer naming out of the box.

Key Design Decisions

• Names as hints, not identity — aligns initializers with how all other values already work in the IR, and with every major compiler IR (LLVM, MLIR, PyTorch FX, JAX)
• Invariant separation — mutation-time invariants (Value ownership, const_value set, no producer) vs serialization-time invariants (unique names, no shadowing). The current dict conflates these.
• Backward compatibility — all read patterns (["name"], in, .values()) preserved via lazy cache. Only ~19 write-by-key sites need mechanical migration to .add()
• GraphProtocol contract (MutableMapping[str, ValueProtocol]) needs updating (the public interface that downstream consumers like onnxscript depend on)

Implementation Status

We have a working implementation on a branch with all 3128 existing tests passing. A PR will follow once we've aligned on the approach here. We've also done a full audit of ~100 graph.initializers access sites across both onnx_ir and onnxscript — ~80% work unchanged, the rest are mechanical migrations.

A detailed ~1000-line analysis document covering the spec constraints, implementation details, migration plan, risk assessment, and comparison with other IRs is available for review.

Discussion Points

1. Phase 1 vs Phase 2 ordering — Phase 1 (create_initializer()) is non-breaking and solves the immediate problem. Phase 2 (identity storage) addresses the root cause. Ship Phase 1 first, or combine?
2. Deprecation timeline for graph.initializers["name"] = value — how many minor versions?
3. Soft duplicate warning — should register_initializer() warn (not error) on duplicate names to catch mistakes early without blocking transformations?

This is a superset of the original issue — it solves not just the collision problem but the underlying architectural tension between name-as-identity storage and graph-agnostic transformation APIs.