spike(pipeline): ENG-492 OrcaDAG prototype and networkx replacement design - #140
Conversation
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
| ordered: list[NodeT] = [] | ||
|
|
||
| while frontier: | ||
| node: NodeT = heapq.heappop(frontier) # type: ignore[misc] |
There was a problem hiding this comment.
What are the type hint exceptions about?
There was a problem hiding this comment.
Good catch. Those were suppressing type-checker errors from 's constraint: , , , and all require elements to support , but is an unconstrained TypeVar so the checker can't prove this. Rather than silently ignoring the errors, I've replaced them with an explicit list[Any] annotation for the heap frontier plus a comment that documents the runtime ordering invariant (NodeT must be orderable — callers are responsible for satisfying this). All three # type: ignore suppressions are gone.
There was a problem hiding this comment.
Pull request overview
This PR delivers an ENG-492 spike output proposing and prototyping a minimal in-house DAG (OrcaDAG) intended to replace the current networkx.DiGraph usage, along with a design spec and a focused test suite.
Changes:
- Add
OrcaDAG[NodeT]prototype backed by stdlibgraphlib.TopologicalSorterplus a deterministic Kahn+heap variant. - Add a comprehensive unit test suite covering the intended API surface (construction, traversal, attributes, topo sort, cycles, generic node types).
- Add a design/spec document describing rationale, alternatives, and a follow-on migration map.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
src/orcapod/pipeline/dag.py |
Introduces the OrcaDAG implementation and its API (node/edge ops, attrs, traversal, topo sorts). |
tests/test_pipeline/test_dag.py |
Adds unit tests validating the OrcaDAG API behaviors and edge cases (including cycles and determinism). |
superpowers/specs/2026-05-21-networkx-replacement-design.md |
Adds the spike write-up: recommendation, evidence, alternatives, and migration surface map. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Both nodes are implicitly added if not already present. | ||
|
|
||
| Args: | ||
| u: Source node. | ||
| v: Target node. | ||
|
|
||
| Raises: | ||
| ValueError: If the edge already exists (duplicate edges are not | ||
| permitted in a DAG). |
There was a problem hiding this comment.
Fixed. Removed the Raises: ValueError section entirely. The implementation has always been idempotent (the if v not in self._successors[u] guard silently no-ops on duplicates); the docstring was just wrong. Updated to say "Adding a duplicate edge is a no-op (idempotent)."
| from graphlib import CycleError, TopologicalSorter | ||
| from typing import Any, Generic, Iterable, Iterator, TypeVar | ||
|
|
||
| __all__ = ["OrcaDAG", "CycleError"] | ||
|
|
||
| NodeT = TypeVar("NodeT") |
There was a problem hiding this comment.
Done. Defined a Comparable(Hashable, Protocol) with __lt__ and bound NodeT = TypeVar("NodeT", bound=Comparable). This captures both the hashability requirement (dict keys) and the ordering requirement (heapq / sorted) in one named protocol. heapq operations and sorted() in topological_sort_deterministic are now fully type-safe — no list[Any] workaround needed.
| (`src/orcapod/pipeline/dag.py`, ~120 lines of pure Python). The case rests on | ||
| three converging facts: |
There was a problem hiding this comment.
Fixed. Updated to "~255 lines including full docstrings and type annotations; core logic is under 80 lines". The original ~120 estimate was the pre-implementation guess for logic-only lines; the committed file includes Google-style docstrings on every method which account for most of the difference.
| ### Internal representation | ||
|
|
||
| ``` | ||
| _nodes: dict[NodeT, dict[str, Any]] # node → attribute dict |
There was a problem hiding this comment.
Fixed. Changed _nodes to _attrs in the internal representation block to match the actual implementation. The spec was written before the prototype and used a placeholder name.
| Raises: | ||
| KeyError: If *node* is not in the graph. | ||
| """ | ||
| return self._successors[node] |
There was a problem hiding this comment.
Fixed. successors() now returns frozenset(self._successors[node]) — a snapshot copy typed as frozenset[NodeT]. Callers can still iterate or convert to a set, but they cannot reach back into the internal mutable set and corrupt _in_degree. Return type annotation updated to frozenset[NodeT] accordingly.
| ## 7. Migration Surface Map (for follow-on issue ENG-493) | ||
|
|
||
| The full migration requires changes in exactly five places: |
There was a problem hiding this comment.
Fixed. The Section 7 header now reads "for follow-on issue ENG-494" consistently with the rest of the doc. Was a leftover from before the Linear issue was created.
Review response — round 1One comment addressed: the What they were: What changed: Removed all three suppressions. Instead, All 49 tests continue to pass. |
Review response — round 2 (Copilot + user feedback)Six issues addressed in one commit (
|
| heapq.heappush(frontier, successor) | ||
|
|
||
| if len(ordered) != len(self._attrs): | ||
| raise CycleError("Graph contains a cycle; topological sort is not possible.") |
There was a problem hiding this comment.
Fixed by delegating to TopologicalSorter when Kahn's detects a cycle. When the ordered count falls short of the node count, we build the predecessor dict and call list(TopologicalSorter(predecessor_dict).static_order()) — which raises CycleError with args[1] populated with the cycle nodes, identical to what topological_sort() produces. Both methods now raise consistent, debuggable CycleError instances. The raise AssertionError("unreachable") after it is a type-system safety net that can never execute at runtime.
| dag2.add_edge("b", "c") | ||
| dag2.add_edge("a", "c") | ||
|
|
||
| assert dag1.topological_sort_deterministic() == dag2.topological_sort_deterministic() |
There was a problem hiding this comment.
Fixed. Wrapped as assert dag1.topological_sort_deterministic() == (dag2.topological_sort_deterministic()) across two lines. Also ran uv run ruff format on both changed files to confirm no further formatting issues remain.
Review response — round 3Two issues addressed in commit 1. When Kahn's algorithm detects a cycle (ordered count < node count), the method now delegates to 2. Long assertion wrapped for ruff compliance The over-length assertion in All 49 tests pass. |
| @@ -0,0 +1,266 @@ | |||
| """Lean directed acyclic graph for OrcaPod pipeline topology. | |||
There was a problem hiding this comment.
Fixed in e200874. Replaced all instances of "OrcaPod" with "Orcapod" throughout the module docstring, class docstring, and inline comments in dag.py.
|
|
||
|
|
||
| class OrcaDAG(Generic[NodeT]): | ||
| """Minimal directed acyclic graph for OrcaPod pipeline topology. |
There was a problem hiding this comment.
Use "Orcapod" and add this to Claude and Zed instructions
There was a problem hiding this comment.
Fixed in e200874. Changed "OrcaPod" to "Orcapod" in the OrcaDAG class docstring. Also added the naming policy (Orcapod, not OrcaPod) to both CLAUDE.md and .zed/rules so it applies to all future work.
| Uses ``graphlib.TopologicalSorter`` (Python stdlib). The order is | ||
| stable within a single run but is **not** guaranteed to be | ||
| deterministic across Python versions or between runs. Use | ||
| :meth:`topological_sort_deterministic` when a stable, reproducible |
There was a problem hiding this comment.
Do not use ReST markups anywhere in the documentation and make note of this policy in the Claude.md
There was a problem hiding this comment.
Fixed in e200874. Removed all ReST markup from dag.py docstrings: double backticks converted to single backticks throughout, the :meth:\ topological_sort_deterministic cross-reference replaced with plain name, and not converted to plain prose. Also added the no-ReST-markup policy to both CLAUDE.md and .zed/rules.
Round 4 review — addressed in e200874All three eywalker comments resolved in a single commit:
All 49 tests continue to pass; |
| dag.add_node("a") # no attrs — must not clear existing | ||
| assert dag.node_attrs("a")["label"] == "original" | ||
|
|
||
| def test_subsequent_add_node_merges_new_attrs(self) -> None: |
There was a problem hiding this comment.
add case where second addition has the same key attribute with different value.
There was a problem hiding this comment.
Added test_subsequent_add_node_overwrites_same_key (c14aa04). It calls add_node twice with the same key (label) but different values and asserts the second value wins — exercising the dict.update overwrite path that the existing merge test didn't cover.
| dag.add_edge("b", "c") | ||
| order = dag.topological_sort() | ||
| assert self._is_valid_topo_order(dag, order) | ||
| assert set(order) == {"a", "b", "c"} |
There was a problem hiding this comment.
What's the point in comparing set? Set would get rid of ordering
There was a problem hiding this comment.
Removed the set() wrapper. successors() already returns frozenset, which supports == against a set literal directly — the conversion was redundant. The assertion is now: assert dag.successors(a) == {b}
Round 5 review — addressed in c14aa04Two items fixed in a single commit:
All 50 tests pass. |
| class Comparable(Hashable, Protocol): | ||
| """Protocol for node types that support both hashing and ordering. | ||
|
|
||
| Nodes must be hashable to serve as dict keys and must support `<` | ||
| for deterministic topological sort via `heapq`. | ||
|
|
||
| Any type that implements `__hash__` (or inherits it from `object`) | ||
| and `__lt__` satisfies this protocol — e.g. `str`, `int`, or a | ||
| custom dataclass with those methods defined. | ||
| """ | ||
|
|
||
| def __lt__(self, other: Any) -> bool: ... | ||
|
|
||
|
|
||
| NodeT = TypeVar("NodeT", bound=Comparable) | ||
|
|
||
|
|
||
| class OrcaDAG(Generic[NodeT]): | ||
| """Minimal directed acyclic graph for Orcapod pipeline topology. | ||
|
|
||
| Covers exactly the operations Orcapod needs — DAG construction, node | ||
| attribute storage, basic traversal, and topological sort. No external | ||
| dependencies; backed entirely by plain dicts and stdlib `graphlib`. | ||
|
|
||
| Args: | ||
| NodeT: The node type. Must satisfy `Comparable` — i.e. be hashable | ||
| and support `<` comparison (e.g. `str`, `int`, or a custom | ||
| type that implements `__hash__` and `__lt__`). |
There was a problem hiding this comment.
Fixed in f2ffb2d. Changed the class-level NodeT bound from Comparable to Hashable, and introduced a separate ComparableNodeT TypeVar. topological_sort_deterministic() is now annotated as self: 'OrcaDAG[ComparableNodeT]' -> list[ComparableNodeT], making it callable only on OrcaDAG instances whose node type satisfies Comparable. OrcaDAG[GraphNode] is now valid at the class level; calling topological_sort_deterministic() on such an instance would be a static type error. The Comparable protocol is still exported and still used — just scoped to the one method that actually needs ordering.
| # TopologicalSorter takes predecessors, but we store successors. | ||
| # Rebuild as predecessor dict. | ||
| predecessor_dict: dict[NodeT, set[NodeT]] = { | ||
| node: set() for node in self._attrs | ||
| } | ||
| for u, vs in self._successors.items(): | ||
| for v in vs: | ||
| predecessor_dict[v].add(u) | ||
|
|
||
| ts: TopologicalSorter[NodeT] = TopologicalSorter(predecessor_dict) |
There was a problem hiding this comment.
Fixed in f2ffb2d. Extracted _build_predecessor_dict() as a private helper that inverts the successor representation into the predecessor form TopologicalSorter expects. Both topological_sort() and the cycle-handling path in topological_sort_deterministic() now call it, eliminating the duplication.
| """Tests for OrcaDAG — the lean in-house DAG replacing networkx.DiGraph. | ||
|
|
||
| Covers all nine API shapes used by OrcaPod: | ||
| - add_node / add_edge (construction) | ||
| - node_attrs (attribute dict access) | ||
| - nodes() / edges() (traversal) | ||
| - in_degree() / successors() (local graph queries) | ||
| - topological_sort() / topological_sort_deterministic() (ordering) | ||
| - __contains__ / __len__ / __iter__ (membership and sizing) |
There was a problem hiding this comment.
Fixed in f2ffb2d. Updated 'OrcaPod' to 'Orcapod' in the test module docstring.
| # OrcaPod — networkx Replacement Design | ||
|
|
||
| **Issue:** ENG-492 | ||
| **Date:** 2026-05-21 | ||
| **Status:** Spike / Recommendation | ||
| **Author:** Kurodo (agent-kurodo[bot]) | ||
|
|
||
| --- | ||
|
|
||
| ## Executive Summary | ||
|
|
||
| OrcaPod should replace `networkx` with a lean, in-house `OrcaDAG` class | ||
| (`src/orcapod/pipeline/dag.py`, ~255 lines including full docstrings and type | ||
| annotations; core logic is under 80 lines). The case rests on | ||
| three converging facts: | ||
|
|
||
| 1. OrcaPod's networkx API surface is minimal — nine call shapes covering basic | ||
| DAG construction, traversal, and topological sort. |
There was a problem hiding this comment.
Fixed in f2ffb2d. Replaced all instances of 'OrcaPod' with 'Orcapod' throughout the spec (title, executive summary, alternatives matrix, and all inline references).
| Python docstrings everywhere. | ||
|
|
||
| Do **not** use ReST markup in docstrings. Specifically: | ||
| - No double-backtick literals (`` ``code`` ``) — use single backticks (`` `code` ``) instead. | ||
| - No `:meth:`, `:class:`, `:func:`, `:attr:`, or other Sphinx cross-reference roles. | ||
| - No `**bold**` for emphasis inside docstrings — use plain prose. | ||
|
|
There was a problem hiding this comment.
Fixed in f2ffb2d. Reworded the rule in CLAUDE.md to 'Do not use ReST markup in new docstrings' and added an explicit note: 'This is a forward-looking rule for new code. Existing docstrings that predate this policy do not need immediate migration — apply the rule when writing or substantially rewriting a docstring.'
| Do not use ReST markup in docstrings. Specifically: | ||
| - No double-backtick literals (``code``) — use single backticks (`code`) instead. | ||
| - No :meth:, :class:, :func:, :attr:, or other Sphinx cross-reference roles. | ||
| - No **bold** for emphasis inside docstrings — use plain prose. | ||
|
|
There was a problem hiding this comment.
Fixed in f2ffb2d. Same update applied to .zed/rules — reworded to 'new docstrings' and added the forward-looking migration note.
Round 6 review (Copilot) — addressed in f2ffb2dSix comments resolved in a single commit across five files:
All 50 tests pass. |
GraphBackend scaffolding addedPushed a new commit (
92 tests, all passing. Not yet wired into |
…esign - Add lean OrcaDAG[NodeT] implementation (src/orcapod/pipeline/dag.py) replacing networkx.DiGraph with stdlib graphlib + plain dicts; covers all 9 API shapes OrcaPod uses, zero new external dependencies - Add 49-test suite (tests/test_pipeline/test_dag.py) covering all API shapes, cycle detection, deterministic ordering, and generic node types - Add design spec (superpowers/specs/2026-05-21-networkx-replacement-design.md) with networkx stability fact-check, dep footprint data, alternatives matrix, and migration surface map for the follow-on issue Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
heapq requires elements to support <; NodeT is unconstrained so the type checker cannot verify this. Replace the three # type: ignore suppressions with an explicit list[Any] annotation for the heap frontier and a clear comment explaining the runtime ordering invariant that callers must satisfy. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…uccessors, doc fixes
- Define Comparable(Hashable, Protocol) with __lt__ and bind
NodeT = TypeVar("NodeT", bound=Comparable); heapq operations in
topological_sort_deterministic are now fully type-safe with no
suppression or list[Any] workaround needed
- Fix successors() to return frozenset[NodeT] (snapshot copy) instead
of the internal mutable set, preventing callers from corrupting _in_degree
- Fix add_edge() docstring: remove stale Raises: ValueError (duplicate
edges are silently idempotent, not an error)
- Update FakeNode in test_object_nodes to implement __lt__ so it satisfies
the Comparable bound
- Spec: fix _nodes -> _attrs in internal representation block, update line
count (~120 -> ~255 incl. docstrings), fix Section 7 header ENG-493 ->
ENG-494, update interface block to show Comparable bound and frozenset
return type on successors
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…format - topological_sort_deterministic: when Kahn's detects a cycle, delegate to TopologicalSorter to raise CycleError with its .cycle attribute (args[1]) populated; both topo methods now raise consistent, debuggable exceptions - Wrap long assertion in test_insertion_order_independent to satisfy ruff line-length; run ruff format on both files to confirm no further churn Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
… markup - Replace all "OrcaPod" with "Orcapod" in dag.py module and class docstrings - Strip all ReST markup from dag.py docstrings: double backticks → single backticks, :meth: cross-reference → plain name, **bold** → plain text - Add "no ReST markup in docstrings" policy to CLAUDE.md and .zed/rules - Add "Orcapod (not OrcaPod)" naming policy to CLAUDE.md and .zed/rules Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…et comparison - Add test_subsequent_add_node_overwrites_same_key: verifies that a second add_node call with the same attribute key but a different value overwrites the stored value (dict.update semantics). - Remove redundant set() wrapper in test_object_nodes: successors() already returns frozenset, which supports == against a set literal directly. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…r helper, naming, policy - Loosen OrcaDAG class-level NodeT bound from Comparable to Hashable. Add a separate ComparableNodeT TypeVar and gate topological_sort_deterministic() on it via a narrower self-type annotation (self: OrcaDAG[ComparableNodeT]). This allows OrcaDAG[GraphNode] where GraphNode doesn't implement __lt__, while still enforcing that the deterministic sort is only called on comparable-node graphs. - Extract _build_predecessor_dict() private helper to eliminate the duplicated predecessor-dict construction in topological_sort() and the cycle-handling path of topological_sort_deterministic(). - Fix "OrcaPod" → "Orcapod" throughout the spec and test module docstring. - Clarify the no-ReST-markup policy in CLAUDE.md and .zed/rules as forward-looking for new code; existing docstrings do not require immediate migration. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…edecessor support
- Add `GraphBackend` @runtime_checkable Protocol covering all 12 pipeline
graph operations; both OrcaDAG and NetworkxBackend satisfy it
- Add `predecessors()` method and `_predecessors` dict to OrcaDAG for O(1)
predecessor lookup; simplifies `_build_predecessor_dict()`
- Add `NetworkxBackend[NodeT]` — thin nx.DiGraph adapter in
`src/orcapod/pipeline/networkx_backend.py`; uses LazyModule("networkx"),
implements topological_sort() via graphlib.TopologicalSorter for consistent
CycleError semantics, and mirrors topological_sort_deterministic() exactly
- Add TestPredecessors to test_dag.py (4 tests)
- Add test_networkx_backend.py covering full protocol surface + cross-backend
deterministic ordering equivalence (92 tests total, all passing)
Scaffolding only — not yet wired into graph.py or orchestrators (ENG-494).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
5a90bfd to
5e07af8
Compare
Summary
OrcaDAG[NodeT]prototype, and a follow-on issue.src/orcapod/pipeline/dag.py— lean in-house DAG (~155 lines, zero new deps) replacingnetworkx.DiGraph. Backed by stdlibgraphlib.TopologicalSorter+ plain dicts. Covers all 9 API shapes OrcaPod uses:add_node,add_edge,node_attrs,nodes(),edges(),successors(),in_degree(),topological_sort(),topological_sort_deterministic().tests/test_pipeline/test_dag.py— 49 tests, 100% passing, covering all API shapes, cycle detection, determinism guarantees, and generic node types (str,object,int).superpowers/specs/2026-05-21-networkx-replacement-design.md— full design spec with networkx stability fact-check (evidence-based), dep footprint data (2.1 MB for 9 API calls), alternatives matrix (rustworkx, igraph, graphlib, in-house), "not reinventing the wheel" reasoning, and migration surface map.Does NOT wire
OrcaDAGintograph.py/ orchestrators — that's ENG-494.Recommendation (TL;DR)
Replace
networkxwith in-houseOrcaDAG. The API surface is so small (9 call shapes, all trivial graph operations) that owning ~155 lines of well-typed dict manipulation is strictly better than depending on a 2.1 MB library with its own release cadence. Python stdlibgraphlib.TopologicalSorter(Python ≥ 3.9) handles the only non-trivial algorithm. The team has already proven it can own this code (the custom Kahn's implementation ingraph.pyis more complex than the entire replacement).Follow-on
ENG-494 — full migration: wire
OrcaDAGintograph.py,sync_orchestrator.py,async_orchestrator.py, updatetest_graph_rendering.py, removenetworkxfrompyproject.toml.https://linear.app/enigma-metamorphic/issue/ENG-494
Test plan
uv run pytest tests/test_pipeline/test_dag.py -v→ 49/49 passedCloses ENG-492