Architecture diff: recurse into subgraphs (make control-flow/phase-split changes visible)

[titaiwangms]

Motivation

Per the architecture review, the Architecture-Diff tooling should descend
into ONNX subgraphs so control-flow changes are visible in CI. This is the
separate, standalone PR for that work.

The Architecture Diff currently collapses every GRAPH-typed attribute
(an If's then_branch / else_branch, a Loop / Scan body) to a bare
type string and never recurses into it. Any change that lives inside a
subgraph is therefore invisible to the diff, even though the top-level op
sequence is unchanged.

Why it matters now

PR #328 (static-cache attention) introduces a per-layer phase split:
If(Greater(seq_len, 1)) selects a prefill (masked) path vs a decode path.
Because the If node itself is present on both the base and head graphs,
the top-level op sequence is identical — the only signal that anything
changed lives inside the branch subgraphs that the diff was discarding. So
a structural change of this kind would pass the Architecture Diff CI
silently.

The fix

• Recurse GRAPH and GRAPHS attributes into nested canonical forms so
  subgraph node structure participates in the comparison. This reuses the
  existing canonicalization (inner node/value names are ignored the same
  way top-level names already are), so it is deterministic and
  name-independent.
• Add a dedicated subgraph_structure_change record at MODERATE
  severity for structurally significant subgraph deltas — a node or branch
  added, removed, or rewired, or a subgraph interface change. A pure
  inner-attribute tweak (e.g. a Concat axis) stays changed_attrs
  (MINOR). Structural significance propagates upward through nested
  subgraphs (an If inside an If).
• Surface the nested detail in the rendered report
  (e.g. then_branch: node[0] Concat: axis: 0 -> 1).

The change is additive and backward-compatible: non-GRAPH attributes are
handled exactly as before, and the existing consumer (which reads only the
op sequence, node counts, and the change list) is unaffected.

Tests

Adds regression coverage for subgraph recursion, the
structural-vs-minor severity boundary (including nested cases),
GRAPHS-plural bodies, op-swap-not-double-counted, and the readable
fallback path. Full suite passes; lint and format are clean.

Review

Triple-reviewed (correctness/adversarial, readability, and code review)
prior to opening.

Known limitation / possible follow-up

diff_graphs does not compare node domain or num_outputs. As a result,
a subgraph delta that differs only by operator domain (e.g. a
standard-domain op swapped for a same-named contrib-domain op) is detected
but rated as a non-structural changed_attrs (MINOR) via the readable
subgraph changed fallback, rather than as a structural change. This is
pre-existing behavior at the top level too and is out of scope here; it can
be addressed in a follow-up if desired.

[github-actions]

🏗️ Architecture Diff

Comparing 8d492d9 → 6b97a45

Model	Sub-model	Changes	Status

No architecture changes detected. ✅

---

Legend: ⚪ No change · 🔵 Minor (attrs/inits) · 🟡 Moderate (nodes added/removed) · 🔴 Major (interface changed)

[github-actions]

Performance Comparison

Comparing 8d492d9 → 6b97a45

Model	Metric	Baseline	Current	Delta
bert (feature-extraction)	model_size_bytes	359 KB	359 KB	+0.0%	⚪
bert (feature-extraction)	num_nodes	60	60	+0.0%	⚪
falcon	model_size_bytes	364 KB	364 KB	+0.0%	⚪
falcon	num_nodes	66	66	+0.0%	⚪
gemma2	model_size_bytes	428 KB	428 KB	+0.0%	⚪
gemma2	num_nodes	107	107	+0.0%	⚪
gpt2	model_size_bytes	388 KB	388 KB	+0.0%	⚪
gpt2	num_nodes	53	53	+0.0%	⚪
llama	model_size_bytes	425 KB	425 KB	+0.0%	⚪
llama	num_nodes	61	61	+0.0%	⚪
llama (static-cache)	model_size_bytes	425 KB	425 KB	+0.0%	⚪
llama (static-cache)	num_nodes	58	58	+0.0%	⚪
mamba (ssm-text-generation)	model_size_bytes	296 KB	296 KB	+0.0%	⚪
mamba (ssm-text-generation)	num_nodes	98	98	+0.0%	⚪
phi3	model_size_bytes	421 KB	421 KB	+0.0%	⚪
phi3	num_nodes	59	59	+0.0%	⚪
phi3 (static-cache)	model_size_bytes	421 KB	421 KB	+0.0%	⚪
phi3 (static-cache)	num_nodes	56	56	+0.0%	⚪
qwen2	model_size_bytes	425 KB	425 KB	+0.0%	⚪
qwen2	num_nodes	61	61	+0.0%	⚪
qwen2 (static-cache)	model_size_bytes	425 KB	425 KB	+0.0%	⚪
qwen2 (static-cache)	num_nodes	58	58	+0.0%	⚪
qwen3_5_moe (hybrid-text-generation)	model_size_bytes	506 KB	506 KB	+0.0%	⚪
qwen3_5_moe (hybrid-text-generation)	num_nodes	275	275	+0.0%	⚪
qwen3_5_text (hybrid-text-generation)	model_size_bytes	458 KB	458 KB	+0.0%	⚪
qwen3_5_text (hybrid-text-generation)	num_nodes	129	129	+0.0%	⚪
qwen3_5_vl (hybrid-qwen-vl)	model_size_bytes	977 KB	977 KB	+0.0%	⚪
qwen3_5_vl (hybrid-qwen-vl)	num_nodes	413	413	+0.0%	⚪
t5 (seq2seq)	model_size_bytes	836 KB	836 KB	+0.0%	⚪
t5 (seq2seq)	num_nodes	166	166	+0.0%	⚪
whisper (speech-to-text)	model_size_bytes	1008 KB	1008 KB	+0.0%	⚪
whisper (speech-to-text)	num_nodes	128	128	+0.0%	⚪

No performance regressions.

[codecov]

The author of this PR, titaiwangms, is not an activated member of this organization on Codecov.
Please activate this user on Codecov to display this PR comment.
Coverage data is still being uploaded to Codecov.io for purposes of overall coverage calculations.
Please don't hesitate to email us at support@codecov.io with any questions.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[Copilot]

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

[justinchuby]

Thanks - could you run it on #328 to see what it produces?

[titaiwangms]

Review synthesis (4-reviewer fan-out: readability, code, critical, deep + empirical verification)

Overall: Clean, well-tested, well-motivated change — recursion is deterministic and name-independent as claimed, idempotence (diff_graphs(canon(g), canon(g)) == []) holds, and the structural-vs-MINOR propagation through nested Ifs is correctly transitive. Test suite passes (43/43). The two convergent findings below are about change-blindness gaps the recursion doesn't fully close — both are strictly better than the old <GRAPH> collapse (which hid everything), but they leave real change classes invisible despite the PR's stated goal of surfacing subgraph/control-flow changes. No correctness regressions.

Major

1. Captured (implicit) outer-value connectivity is severed at the subgraph boundary — a branch rewired to read a different outer tensor is invisible. src/mobius/_graph_diff.py (_attr_to_comparable recursion + canonicalize_graph value-ID strategy)
Flagged independently by code review and deep review, and I confirmed it empirically against the PR branch:

then-branch  Sub(x, z)  →  Sub(z, x)     (a real, order-sensitive data-flow change)
diff result: []   (NO diff)
base inner input_ids: [0, 1]   head inner input_ids: [0, 1]

If/Loop/Scan branches have empty formal graph.inputs and capture outer values implicitly. Each subgraph is canonicalized with a fresh value_id/counter, so captured outer values are assigned local IDs lazily by encounter order — disconnected from the parent ID-space. Any permutation of captured-value consumption that preserves encounter-order position is therefore invisible. Inner→inner edges are tracked; outer→inner edges are not. For a CI gate motivated by per-layer phase-split Ifs, a branch silently switching which KV/state tensor it consumes is exactly the break that should not pass.
→ Seed the subgraph's value_id from the parent graph's positional IDs for captured values (thread the outer map into a recursive helper), or at minimum document this as a named blind spot alongside the domain limitation.

2. num_outputs and top-level node domain are recorded by canonicalize_graph but never compared by diff_graphs — broader than the documented "Known limitation". src/mobius/_graph_diff.py:170 (recorded), diff never reads them
The "Known limitation" only calls out subgraph domain-only swaps. But: a node arity change (e.g. Dropout gaining its mask output, a Split emitting a different count) is fully invisible at top level (same op_type/attrs/input_ids → []) and under-rated as a contentless "subgraph changed" MINOR inside a subgraph. Likewise a top-level Add → com.microsoft::Add domain swap is fully invisible (op_sequence keys on op_type only), while the same swap inside a subgraph is MINOR — an inconsistency the note doesn't capture. num_outputs is arguably more structural than domain (it changes value-flow fan-out).
→ Either compare num_outputs (classify mismatch as structural) and fold domain into the op-sequence key (f"{domain}::{op_type}") so swaps are detected uniformly; or, if intentionally out of scope, stop recording these fields (so they can't perturb subgraph equality into a contentless MINOR) and expand the limitation note to name them.

Minor

• Sentinel-dict payload design ({"__subgraph__": ...} / {"__subgraphs__": ...}) threads an implicit contract through dict[str, Any] that requires three helpers (_attr_to_comparable, _is_subgraph_payload, _subgraph_list) and two module constants to decode. A small frozen dataclass (_CanonicalSubgraphs) at the call site would make isinstance(...) self-explanatory and delete the sentinels + helpers. (readability)
• Legend drift: _change_status now rates subgraph_structure_change as MODERATE, but the rendered legend text still describes Moderate as only "nodes added/removed". Update the legend. (code)
• Severity-asymmetry caveat (Question): subgraph interface change = MODERATE while top-level = MAJOR is defensible (external contract lives at top-level I/O). But canonicalize_graph reads stored Value.type annotations, not live inference. If a branch's output dtype changes but the enclosing If node's output Value.type wasn't refreshed, the top-level interface diff misses it and only the MODERATE subgraph signal fires — demoting a real MAJOR external break. Confirm the declarative builder always re-runs type inference on the enclosing value. (deep)

Resolved / confirmed fine

• Nested initializer_change staying MINOR (non-structural) is consistent with top-level treatment — both reviewers confirm no asymmetry is introduced. Fine as-is.
• test_top_level_op_swap_not_double_counted correctly locks preexisting behavior (op swap → structural only, never double-counted as changed_attrs).

Nits

Committed test docstrings carry PR-review artifact IDs (architect D12, Reviewer 6def2895 (a), Code-review NIT) — opaque post-merge; move rationale to commit messages and keep the behavioral sentence. _STRUCTURAL_SUB_TYPES "SUB" is ambiguous (reads as "sub-category" not "subgraph"); _subgraph_list returns canonical-form dicts, not subgraphs — rename. "subgraph changed" fallback string is asserted by a test but anonymous — extract a constant. it_out/ie_out abbreviations.

Praise

Structural significance propagates transitively through If-in-If, with tests pinning both the promote and the non-promote (stays-MINOR) cases — the easy-to-get-wrong boundary. Op-swap double-counting correctly avoided at top level and mirrored inside subgraphs. Name-independence genuinely holds across inner node names, captured-value names, and branch-output names. Strong adversarial test coverage including the honest "subgraph changed" fallback. The attr_details → plain_details rename is exactly right.

_{🤖 Generated by a multi-agent review fan-out (readability + code + critical + deep reviewers), with the implicit-capture finding empirically reproduced against the PR branch.}

[titaiwangms]

Addendum — adversarial review (3 additional findings, two empirically reproduced against the PR branch)

A fourth reviewer pass surfaced findings beyond the synthesis above. Findings #1 (captured-value connectivity) and #4 (subgraph interface MAJOR→MODERATE downgrade) reinforce the two Majors already posted. Three are new and material:

Major — diff_graphs inherits positional node matching in the recursive path → false-positive explanations on same-op insertions

src/mobius/_graph_diff.py (attr/connectivity diff after the difflib op-sequence diff)
Reproduced against the PR branch — inserting one Cast at the front of an If then-branch:

reported: subgraph_structure_change :: node[0] If: then_branch: + Cast; node[0] Cast: to: 1 → 6

The + Cast is correct, but Cast: to: 1 → 6 is bogus — it comes from positionally comparing the newly-inserted Cast(to=INT32) at index 0 against the original Cast(to=FLOAT). (Same-op insertion isn't skipped by the op_type guard because both are Cast.) The top-level op-swap test doesn't cover same-op insertions, and the flaw now also pollutes nested subgraph reporting.
→ Align nodes via difflib.SequenceMatcher "equal" blocks before attr/connectivity diff (or only diff within matched blocks).

Major — GRAPHS bodies paired by raw index → cascade of bogus mutations on a middle insertion

src/mobius/_graph_diff.py (_describe_subgraph_attr_change, range(count) pairing)
Reproduced — inserting Div at index 1 of a 3-body GRAPHS attr [Add, Sub, Mul] → [Add, Div, Sub, Mul]:

reported: bodies: subgraph[1]: - Sub; + Div; subgraph[2]: - Mul; + Sub; subgraph[3] added

Actual change: one Div body inserted at index 1. Index pairing only works for append/remove-at-end; a middle insertion shifts every following body and mis-reports it as a chain of mutations.
→ Align the GRAPHS lists with SequenceMatcher / canonical-subgraph hashes before recursing; report unmatched ranges as inserted/removed bodies.

Minor (defense-in-depth) — recursive canonicalization has no cycle/depth guard

src/mobius/_graph_diff.py (canonicalize_graph(attr.value) recursion)
The recursion tracks no visited-graph set or depth budget. A pathological/deeply-nested chain of If/Loop/Scan bodies can hit Python's recursion limit (and a genuinely cyclic graph-attr would loop forever). Since this runs in CI on PR-produced graphs, a visited-id guard + max-depth budget is cheap insurance. (Lower priority — real ONNX graphs are acyclic and shallow — but worth a guard.)

Overlapping (already in the synthesis)

• Captured/implicit outer-value connectivity severed at the subgraph boundary (Major Update security.md #1 above) — same root cause, same Sub(x,z)→Sub(z,x) repro.
• Subgraph interface change rated MODERATE while the same break is MAJOR top-level, and can be missed entirely if the enclosing node's Value.type is stale (Minor caveat above).

_{🤖 Adversarial pass; the two false-positive cascades reproduced by executing diff_graphs on the PR branch.}

[justinchuby]

Feel free to merge when you think it is ready, thanks

[titaiwangms]

ORT does not support control flow graph without graph break.

[titaiwangms]

Closing this PR for now.

The arch-diff subgraph-recursion tooling here was motivated by making the
per-layer If subgraphs in our static-cache export visible to the
Architecture-Diff CI. With #328's If phase-split now abandoned (it is
capture-hostile — see #328 / #340), that immediate consumer no longer exists:
the shipped static-cache attention (#340) is branchless, so there are no If
subgraphs for the diff to recurse into.

The subgraph-recursion tooling itself is general-purpose and isn't lost — it can
be re-proposed as its own standalone PR if/when there's a consumer for it.