perf(graph): PropositionGraph.topo_order — Kahn's O(N+E) instead of O(N×E) (#92)
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.
This commit is contained in:
parent
fd48931838
commit
548282fadc
2 changed files with 140 additions and 7 deletions
|
|
@ -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]:
|
||||
|
|
|
|||
105
tests/test_graph_topo_order_perf.py
Normal file
105
tests/test_graph_topo_order_perf.py
Normal file
|
|
@ -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="<pending>",
|
||||
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}"
|
||||
Loading…
Reference in a new issue