Skip to content

Commit 6b97a45

Browse files
titaiwangmsCopilot
andcommitted
feat(graph-diff): recurse into subgraphs so control-flow/phase-split changes are visible
The Architecture-Diff tooling collapsed GRAPH-typed attributes (If then_branch / else_branch, Loop / Scan bodies) to a bare type string and never recursed into them. As a result the per-layer static-cache phase-split introduced by PR #328 -- an If(Greater(seq_len, 1)) selecting a prefill (masked) vs decode (Flash) attention path -- was completely invisible to the Architecture Diff CI: the top-level op sequence is unchanged (the If node is present on both sides), so the only signal lives inside the branch subgraphs that were being discarded. This recurses GRAPH and GRAPHS attributes into nested canonical forms so subgraph node structure participates in the comparison, reusing canonicalize_graph (inner node/value names are ignored the same way top-level ones are). diff_graphs gains a dedicated subgraph_structure_change record (MODERATE severity) for structurally significant subgraph deltas -- a node/branch added, removed, rewired, or a subgraph interface change -- while a pure inner-attribute tweak stays changed_attrs (MINOR). Structural significance propagates upward through nested subgraphs (e.g. an If inside an If). The nested diff detail is surfaced in the report (e.g. "then_branch: node[0] Concat: axis: 0 -> 1"). Additive and backward-compatible: non-GRAPH attributes are unchanged and the arch_diff.py consumer (which reads only op_sequence / node counts / the changes list) is unaffected. Adds 16 regression tests covering subgraph recursion, the structural-vs-minor severity boundary (incl. nested), GRAPHS-plural, op-swap no-double-count, and the readable fallback path. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
1 parent 8d492d9 commit 6b97a45

2 files changed

Lines changed: 511 additions & 13 deletions

File tree

src/mobius/_graph_diff.py

Lines changed: 136 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -54,11 +54,23 @@ def _dtype_str(value: ir.Value) -> str:
5454
return "UNKNOWN"
5555

5656

57+
# Sentinel keys marking a recursively-canonicalised subgraph payload inside an
58+
# attribute's comparable value. diff_graphs uses these to render subgraph
59+
# deltas readably instead of dumping a raw nested canonical dict.
60+
_SUBGRAPH_KEY = "__subgraph__"
61+
_SUBGRAPHS_KEY = "__subgraphs__"
62+
63+
5764
def _attr_to_comparable(attr: ir.Attr) -> Any:
5865
"""Convert an attribute to a JSON-serialisable comparable value.
5966
60-
Graph and tensor attributes are reduced to their type string so
61-
that canonicalisation stays lightweight.
67+
Most attributes reduce to their scalar / list value. GRAPH-typed
68+
attributes (``If``'s ``then_branch`` / ``else_branch``, ``Loop`` /
69+
``Scan`` bodies) are *recursively canonicalised* so subgraph structure
70+
participates in the diff — without this, per-layer phase-split ``If``
71+
subgraphs are invisible to the architecture diff. Remaining opaque
72+
types (TENSOR, SPARSE_TENSOR, TYPE_PROTO, …) are recorded as their type
73+
string to keep canonicalisation lightweight.
6274
"""
6375
simple_types = {
6476
ir.AttributeType.FLOAT,
@@ -74,7 +86,14 @@ def _attr_to_comparable(attr: ir.Attr) -> Any:
7486
if isinstance(v, tuple):
7587
return list(v)
7688
return v
77-
# For TENSOR, GRAPH etc. just record the type
89+
# Recurse into subgraphs so their node structure is compared, not collapsed.
90+
# Subgraphs reuse canonicalize_graph, so inner node/value names are ignored
91+
# the same way top-level ones are (see canonicalize_graph's name-independence).
92+
if attr.type == ir.AttributeType.GRAPH:
93+
return {_SUBGRAPH_KEY: canonicalize_graph(attr.value)}
94+
if attr.type == ir.AttributeType.GRAPHS:
95+
return {_SUBGRAPHS_KEY: [canonicalize_graph(g) for g in attr.value]}
96+
# For TENSOR, SPARSE_TENSOR, TYPE_PROTO, … just record the type.
7897
return f"<{attr.type.name}>"
7998

8099

@@ -203,13 +222,92 @@ def _describe_port_diff(base_port: dict, head_port: dict) -> str:
203222
return "; ".join(parts) or "changed"
204223

205224

225+
def _is_subgraph_payload(value: Any) -> bool:
226+
"""True if *value* is a recursively-canonicalised subgraph payload."""
227+
return isinstance(value, dict) and (_SUBGRAPH_KEY in value or _SUBGRAPHS_KEY in value)
228+
229+
230+
def _subgraph_list(value: Any) -> list[dict]:
231+
"""Extract the list of subgraph canonical forms from an attr payload."""
232+
if isinstance(value, dict):
233+
if _SUBGRAPH_KEY in value:
234+
return [value[_SUBGRAPH_KEY]]
235+
if _SUBGRAPHS_KEY in value:
236+
return list(value[_SUBGRAPHS_KEY])
237+
return []
238+
239+
240+
# Nested sub-change types that make a subgraph delta *structurally* significant
241+
# (a node/branch was added/removed, rewired, or the subgraph interface moved),
242+
# as opposed to a mere inner-attribute tweak. Any of these promotes the
243+
# containing attribute to a subgraph_structure_change → MODERATE. Note this is
244+
# uniformly MODERATE: unlike a *top-level* interface_change (MAJOR, an external
245+
# model-contract break), a subgraph's interface is internal control-flow plumbing,
246+
# so it stays MODERATE here — a deliberate asymmetry.
247+
# ``subgraph_structure_change`` is included so structural significance
248+
# *propagates* upward through nested subgraphs (e.g. an If inside an If).
249+
_STRUCTURAL_SUB_TYPES = frozenset(
250+
{
251+
"added_node",
252+
"removed_node",
253+
"changed_connectivity",
254+
"interface_change",
255+
"subgraph_structure_change",
256+
}
257+
)
258+
259+
260+
def _describe_subgraph_attr_change(key: str, base_val: Any, head_val: Any) -> tuple[str, bool]:
261+
"""Describe a GRAPH-typed attribute change, recursing into the subgraph(s).
262+
263+
Returns ``(detail, structural)`` where *detail* is a readable summary
264+
that surfaces the nested diff (e.g. ``"then_branch: node[0] Concat:
265+
axis: 0 → 1"`` or ``"then_branch: + Mul; - Add"``) and *structural* is
266+
True when the nested delta adds/removes a node or branch, rewires
267+
connectivity, or changes the subgraph interface — i.e. a change that
268+
should outrank a pure inner-attribute tweak.
269+
"""
270+
base_subs = _subgraph_list(base_val)
271+
head_subs = _subgraph_list(head_val)
272+
count = max(len(base_subs), len(head_subs))
273+
structural = False
274+
parts: list[str] = []
275+
for idx in range(count):
276+
bs = base_subs[idx] if idx < len(base_subs) else None
277+
hs = head_subs[idx] if idx < len(head_subs) else None
278+
# The attribute *key* already names a single subgraph (then_branch,
279+
# body, …); only disambiguate by index when there are several (GRAPHS).
280+
label = "" if count == 1 else f"subgraph[{idx}]"
281+
if bs is None or hs is None:
282+
# A whole branch/body was added or removed.
283+
structural = True
284+
verb = "added" if bs is None else "removed"
285+
parts.append(f"{label} {verb}".strip())
286+
continue
287+
sub_changes = diff_graphs(bs, hs)
288+
if not sub_changes:
289+
continue
290+
if {c["type"] for c in sub_changes} & _STRUCTURAL_SUB_TYPES:
291+
structural = True
292+
inner = "; ".join(c["details"] for c in sub_changes)
293+
parts.append(f"{label}: {inner}" if label else inner)
294+
detail = f"{key}: " + ("; ".join(parts) if parts else "subgraph changed")
295+
return detail, structural
296+
297+
206298
def diff_graphs(base: dict, head: dict) -> list[dict[str, Any]]:
207299
"""Compare two canonical graph representations.
208300
209301
Returns a list of change dicts. Each dict has a ``"type"`` key with
210302
one of: ``"added_node"``, ``"removed_node"``, ``"changed_attrs"``,
303+
``"subgraph_structure_change"``, ``"changed_connectivity"``,
211304
``"interface_change"``, ``"initializer_change"``. A ``"details"``
212305
key carries human-readable information about the change.
306+
307+
``subgraph_structure_change`` is emitted for a GRAPH-typed attribute
308+
(``If`` branches, ``Loop`` / ``Scan`` bodies) whose nested graph gains
309+
or loses a node/branch, is rewired, or changes interface; a subgraph
310+
delta that only tweaks an inner attribute stays ``changed_attrs``.
213311
"""
214312
changes: list[dict[str, Any]] = []
215313

@@ -304,19 +402,32 @@ def diff_graphs(base: dict, head: dict) -> list[dict[str, Any]]:
304402
if bn["attributes"] != hn["attributes"]:
305403
ba = bn["attributes"]
306404
ha = hn["attributes"]
307-
attr_details: list[str] = []
405+
plain_details: list[str] = []
308406
all_keys = sorted(set(ba) | set(ha))
309407
for k in all_keys:
310408
bv = ba.get(k)
311409
hv = ha.get(k)
312-
if bv != hv:
313-
attr_details.append(f"{k}: {bv!r}{hv!r}")
314-
changes.append(
315-
{
316-
"type": "changed_attrs",
317-
"details": (f"node[{i}] {bn['op_type']}: " + ", ".join(attr_details)),
318-
}
319-
)
410+
if bv == hv:
411+
continue
412+
if _is_subgraph_payload(bv) or _is_subgraph_payload(hv):
413+
detail, structural = _describe_subgraph_attr_change(k, bv, hv)
414+
changes.append(
415+
{
416+
"type": (
417+
"subgraph_structure_change" if structural else "changed_attrs"
418+
),
419+
"details": f"node[{i}] {bn['op_type']}: {detail}",
420+
}
421+
)
422+
else:
423+
plain_details.append(f"{k}: {bv!r}{hv!r}")
424+
if plain_details:
425+
changes.append(
426+
{
427+
"type": "changed_attrs",
428+
"details": (f"node[{i}] {bn['op_type']}: " + ", ".join(plain_details)),
429+
}
430+
)
320431
if bn["input_ids"] != hn["input_ids"]:
321432
changes.append(
322433
{
@@ -343,7 +454,12 @@ def _change_status(change_list: list[dict[str, Any]]) -> str:
343454
types = {c["type"] for c in change_list}
344455
if types & {"interface_change"}:
345456
return _STATUS_MAJOR
346-
if types & {"added_node", "removed_node", "changed_connectivity"}:
457+
if types & {
458+
"added_node",
459+
"removed_node",
460+
"changed_connectivity",
461+
"subgraph_structure_change",
462+
}:
347463
return _STATUS_MODERATE
348464
if types & {"changed_attrs", "initializer_change"}:
349465
return _STATUS_MINOR
@@ -459,6 +575,7 @@ def _sha_link(sha: str) -> str:
459575
removed = [c for c in change_list if c["type"] == "removed_node"]
460576
attrs = [c for c in change_list if c["type"] == "changed_attrs"]
461577
connectivity = [c for c in change_list if c["type"] == "changed_connectivity"]
578+
subgraph = [c for c in change_list if c["type"] == "subgraph_structure_change"]
462579
iface = [c for c in change_list if c["type"] == "interface_change"]
463580
inits = [c for c in change_list if c["type"] == "initializer_change"]
464581

@@ -474,6 +591,12 @@ def _sha_link(sha: str) -> str:
474591
lines.append(f"- `{c['details']}`")
475592
lines.append("")
476593

594+
if subgraph:
595+
lines.append("**Subgraph structure changes:**")
596+
for c in subgraph:
597+
lines.append(f"- `{c['details']}`")
598+
lines.append("")
599+
477600
if attrs:
478601
lines.append("**Modified attributes:**")
479602
for c in attrs:

0 commit comments

Comments
 (0)