From 548282fadca4db3a93a7b3883e6c55a11cb42569 Mon Sep 17 00:00:00 2001 From: Shay Date: Wed, 20 May 2026 20:37:21 -0700 Subject: [PATCH] =?UTF-8?q?perf(graph):=20PropositionGraph.topo=5Forder=20?= =?UTF-8?q?=E2=80=94=20Kahn's=20O(N+E)=20instead=20of=20O(N=C3=97E)=20(#92?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Comb pass 2026-05-21 (item 4). Pre-fix the topological-sort implementation in ``PropositionGraph.topo_order`` had two compounding inefficiencies: * ``queue.pop(0)`` on a list is O(N) per pop → O(N²) total * The inner ``for e in self.edges`` rescanned all edges on every iteration → O(N × E) overall This is invisible on today's 1–2 node production graphs but would become a real regression the moment compound-intent multi-node dispatch (ADR-0089 Phase C2) or the grounded realizer's multi-clause output (ADR-0088 Phase B follow-up) lands. Fix: standard Kahn's with a precomputed out-edge adjacency map and a ``deque`` for the work queue. O(N + E) overall. Deterministic output preserved — the queue is seeded with sorted zero-in-degree nodes (identical to the pre-fix list sort), and direct-successor order matches edge-iteration order (identical when edges retain insertion order). Pinned by 6 new tests in ``tests/test_graph_topo_order_perf.py``: * single-node graph (today's production shape) byte-identical to pre-fix output * empty graph returns empty tuple * chain (A→B→C→D) orders root → leaf * diamond (A→B, A→C, B→D, C→D) keeps A first, D last, B/C between * three disjoint roots emit in sorted order * 100-node chain returns correct full order (would have been visibly slow under the O(N²) pre-fix algorithm) Validation: * ``core eval cognition`` byte-identical (public 100/100/91.7/100) * ``core test --suite cognition`` 120/0/1 * ``core test --suite smoke`` 67/0 Comb-pass note: item 15 (GenerationResult.tokens typed tuple but assigned list) was investigated and turned out to be a Pyright false positive — ``GenerationResult.__post_init__`` already coerces to tuple via ``object.__setattr__``. Contract is enforced at runtime; only Pyright's static analyser misses the coercion site. No fix needed. --- generate/graph_planner.py | 42 +++++++++-- tests/test_graph_topo_order_perf.py | 105 ++++++++++++++++++++++++++++ 2 files changed, 140 insertions(+), 7 deletions(-) create mode 100644 tests/test_graph_topo_order_perf.py diff --git a/generate/graph_planner.py b/generate/graph_planner.py index aa692072..e3f6a057 100644 --- a/generate/graph_planner.py +++ b/generate/graph_planner.py @@ -8,6 +8,7 @@ move, and any constraints inherited from intent classification. from __future__ import annotations +from collections import defaultdict, deque from dataclasses import dataclass from enum import Enum, unique @@ -84,19 +85,46 @@ class PropositionGraph: return tuple(n.node_id for n in self.nodes if n.node_id not in targets) def topo_order(self) -> tuple[str, ...]: + """Kahn's topological sort over the graph's edges. + + Comb pass 2026-05-21 — pre-fix this implementation had two + compounding inefficiencies: + + * ``queue.pop(0)`` on a list is O(N) per pop ⇒ O(N²) total + * The inner ``for e in self.edges`` rescanned every edge on + every iteration ⇒ O(N × E) overall + + Properly implemented Kahn's is O(N + E) and produces the same + deterministic order for the same input (queue seeded with + sorted zero-in-degree nodes; ties on later iterations break + by insertion order, identical to the pre-fix list). + + Today's graphs are 1–2 nodes so cost is invisible — but + ADR-0089 Phase C2 (compound-intent multi-node dispatch) and + ADR-0088 Phase B (grounded realizer) both make multi-node + graphs realistic on the hot path. Fix lands before the + usage scales. + """ + # Build out-edge adjacency once: O(E). + out_edges: dict[str, list[str]] = defaultdict(list) in_degree: dict[str, int] = {n.node_id: 0 for n in self.nodes} for e in self.edges: + out_edges[e.source].append(e.target) in_degree[e.target] = in_degree.get(e.target, 0) + 1 - queue = sorted(nid for nid, deg in in_degree.items() if deg == 0) + # Seed with sorted zero-in-degree nodes (deterministic). + queue: deque[str] = deque( + sorted(nid for nid, deg in in_degree.items() if deg == 0) + ) order: list[str] = [] while queue: - nid = queue.pop(0) + nid = queue.popleft() # O(1) on a deque order.append(nid) - for e in self.edges: - if e.source == nid: - in_degree[e.target] -= 1 - if in_degree[e.target] == 0: - queue.append(e.target) + # Decrement in-degree of direct successors only: O(deg(nid)) + # amortised to O(E) total across the loop. + for target in out_edges[nid]: + in_degree[target] -= 1 + if in_degree[target] == 0: + queue.append(target) return tuple(order) def as_dict(self) -> dict[str, object]: diff --git a/tests/test_graph_topo_order_perf.py b/tests/test_graph_topo_order_perf.py new file mode 100644 index 00000000..d47926c1 --- /dev/null +++ b/tests/test_graph_topo_order_perf.py @@ -0,0 +1,105 @@ +"""``PropositionGraph.topo_order`` Kahn's correctness (comb pass 2026-05-21). + +Pre-fix the implementation was O(N²) on the outer loop (``queue.pop(0)`` +on a list) and O(N × E) overall because it rescanned all edges every +iteration. These tests pin Kahn's correctness on multi-node graphs +since today's production graphs are too small (1–2 nodes) to exercise +the algorithm — ADR-0089 Phase C2 (compound-intent multi-node +dispatch) is the consumer this fix prepares for. +""" + +from __future__ import annotations + +from generate.graph_planner import ( + GraphEdge, + GraphNode, + PropositionGraph, + Relation, +) +from generate.intent import IntentTag + + +def _node(node_id: str) -> GraphNode: + return GraphNode( + node_id=node_id, + subject=node_id, + predicate="is_defined_as", + obj="", + source_intent=IntentTag.DEFINITION, + ) + + +def test_topo_order_chain() -> None: + """A → B → C → D linearly orders root → leaf.""" + g = PropositionGraph( + nodes=(_node("A"), _node("B"), _node("C"), _node("D")), + edges=( + GraphEdge(source="A", target="B", relation=Relation.SEQUENCE), + GraphEdge(source="B", target="C", relation=Relation.SEQUENCE), + GraphEdge(source="C", target="D", relation=Relation.SEQUENCE), + ), + ) + assert g.topo_order() == ("A", "B", "C", "D") + + +def test_topo_order_diamond() -> None: + """A → B, A → C, B → D, C → D — both middle nodes precede D.""" + g = PropositionGraph( + nodes=(_node("A"), _node("B"), _node("C"), _node("D")), + edges=( + GraphEdge(source="A", target="B", relation=Relation.ELABORATION), + GraphEdge(source="A", target="C", relation=Relation.ELABORATION), + GraphEdge(source="B", target="D", relation=Relation.SEQUENCE), + GraphEdge(source="C", target="D", relation=Relation.SEQUENCE), + ), + ) + order = g.topo_order() + assert order[0] == "A" + assert order[-1] == "D" + assert set(order[1:3]) == {"B", "C"} + + +def test_topo_order_two_disjoint_roots_sorted() -> None: + """Two zero-in-degree roots → emitted in sorted order (deterministic).""" + g = PropositionGraph( + nodes=(_node("Z"), _node("A"), _node("M")), + edges=(), + ) + # All three are roots; sort_order pins determinism. + assert g.topo_order() == ("A", "M", "Z") + + +def test_topo_order_preserves_byte_identity_on_single_node() -> None: + """The current production graph shape: one node, no edges. + + Pre-fix output ``("p0",)`` is the post-fix output too — pins the + null-lift invariant on today's hot path. + """ + g = PropositionGraph(nodes=(_node("p0"),), edges=()) + assert g.topo_order() == ("p0",) + + +def test_topo_order_handles_empty_graph() -> None: + assert PropositionGraph().topo_order() == () + + +def test_topo_order_complexity_grows_linearly() -> None: + """Smoke test: a 100-node chain returns in linear time and order. + + Pre-fix this would have been O(N²) on the queue and O(N × E) + overall. We don't assert wall-clock; we assert the output is + correct on a size that would have been visibly slow. + """ + nodes = tuple(_node(f"n{i:03d}") for i in range(100)) + edges = tuple( + GraphEdge(source=f"n{i:03d}", target=f"n{i+1:03d}", relation=Relation.SEQUENCE) + for i in range(99) + ) + g = PropositionGraph(nodes=nodes, edges=edges) + order = g.topo_order() + assert len(order) == 100 + assert order[0] == "n000" + assert order[-1] == "n099" + # Every position must match the natural chain order. + for i, nid in enumerate(order): + assert nid == f"n{i:03d}"