Static paths, netgraph-core 0.8.0 floor, and max-flow doc accuracy - #110
Conversation
The section had grown to 83 bullets averaging 259 characters, with one 1225-character entry containing 13 semicolons. Bugs that silently produced wrong results sat at positions 20-24 of 40, at the same visual weight as a CLI table sorting fix. - Group Fixed by consequence: analyses that returned wrong numbers first, then inputs that were silently accepted, then everything else. - Split run-on bullets that packed several unrelated changes behind semicolons; collapse five Performance bullets into two. - Drop content a user cannot act on: internal refactor narration, dead-code removal, how changes were verified, and an enumeration of individual documentation corrections. - Remove cross-section duplication. Independently verified afterwards that no user-facing fact was lost and that no reworded claim became inaccurate; six dropped facts were restored (new errors from add_link, fully-overlapping combine demands, non-string path selectors, and malformed DSL values; the removed module-level mask helpers; the prepared_rg_index parameter). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A demand can name the routes its traffic must follow instead of letting the
flow policy choose them, modelling MPLS-style LSPs:
static_paths:
- ["A", "B", "C"] # node names
- links: ["A|D|0", "D|C|0"] # or link ids, for a specific parallel link
One flow is created per route, in the order listed. A route broken by a
failure carries nothing rather than rerouting, which is what distinguishes a
pinned route from ordinary routing.
Built on netgraph-core 0.8.0's FlowPolicy.set_static_paths and
PredDAG.from_edges, so the dependency floor moves to 0.8.0. That release also
adds a max-flow completion phase, so max_flow can return more than it did on
0.7.x; the design reference is updated to describe reverse residual arcs
returning placed flow rather than serving only min-cut reachability.
Because routes run between two concrete nodes, a pinned demand must use
mode: pairwise with selectors matching exactly one source and one target.
Combine mode routes through pseudo endpoints that no operator-supplied route
can start from, so it is rejected with that explanation.
Resolution details:
- A node hop takes the cheapest enabled link between the pair, ties broken by
link id, so the choice is stable across identical scenario builds. Disabled
links are never chosen, and naming one explicitly is an error: a route
pinned to a disabled link could never carry traffic.
- Hops resolve through the graph's adjacency rows rather than a scan of every
edge, so cost is proportional to node degree rather than graph size.
- Bundles depend only on the static graph, not on per-iteration masks, so they
are resolved once per analysis context and reused across Monte Carlo
iterations and MSD probes.
Known limitation: two demands pinned between the same source, target and
priority are rejected, because netgraph-core assigns flow ids per policy
starting at zero and they would collide. List every route on one demand, or
separate the demands by priority.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Publishing netgraph-core 0.8.0 made several statements in this release inaccurate. An audit that ran ngraph against 0.7.2 and 0.8.0 side by side, cross-checked against networkx.maximum_flow_value on thousands of random topologies, found 0.7.2 understates max flow on cost-asymmetric graphs and 0.8.0 is exact -- so the completion phase is a correctness fix, and the docs should describe it rather than the tier loop alone. The min-cut duality guarantee announced under BREAKING was unqualified. It holds for the default max-flow configuration (PROPORTIONAL, require_capacity, not shortest_path) -- the same gate the C++ completion phase uses -- and not for the placement models. Scoped in the changelog and in api.md. Also corrected, all wrong on both this branch and its parent: - cost_distribution keys: completion-phase entries are marginal costs (forward edge costs minus the cancelled flow's cost), so a key need not match any traversable path. design.md and the MaxFlowResult docstring said "path cost tier", which is only true of the tier loop. - The MAX_FLOW pseudocode ended at the tier loop, so a reader implementing it would reproduce 0.7.x's smaller answer. Added the completion phase. - The complexity bound was justified by "placed flow is never removed from an edge", which the completion phase does. The bound itself still holds -- Edmonds-Karp at O(VE^2) is dominated -- so only the justification changed. - "Does not re-route previously placed flow" now says which phase does not. No behavior change; docs and changelog only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1a1aedd378
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
|
||
|
|
||
| @dataclass(frozen=True) | ||
| class StaticPath: |
There was a problem hiding this comment.
Export StaticPath through the public demand API
StaticPath is required to construct pinned demands programmatically, but it is not re-exported by either ngraph.model.demand or the top-level ngraph package alongside TrafficDemand. Consequently, from ngraph import StaticPath and from ngraph.model.demand import StaticPath both fail, forcing users of this public feature to depend on the internal ngraph.model.demand.spec module. Add the class to the package imports and __all__ lists.
Useful? React with 👍 / 👎.
Lands the three commits that were on
review/correctness-perf-docs-overhaulbut never reachedmain. #108 was squash-merged at head9ead387, before the changelog rewrite was pushed to that branch, somainis missing all three. The tree here is byte-identical to the reviewed branch tip, andmain's tree matches9ead387exactly, so the cherry-pick was clean.Reviewed as #109 (merged into the review branch, which never reached
main). Automated review raised one P2 — see the bottom section.1. Static paths — pinning demands to explicit routes
A demand can name the routes its traffic must follow instead of letting the flow policy choose, modelling MPLS-style LSPs:
One flow per route, created in listed order. A route broken by a failure carries nothing rather than rerouting — that is what separates a pinned route from ordinary routing, and most of the 31 new tests pin it down.
Two
netgraph-coreconstraints shaped the design, both verified in the C++ rather than assumed.PredDAGis read-only from Python and can only be built withfrom_edges, one edge per hop, so a route is a strict explicit route; where parallel links connect a pair, use thelinksform to select one. And Core rejects amax_flow_countthat differs from the bundle count, socreate_flow_policynow routes all five presets through one construction point takingstatic_path_count. Pinned demands requiremode: pairwisewith selectors matching exactly one source and target, since combine mode routes through pseudo endpoints no operator route can start from.The
linksform is only usable because #108 made link ids deterministic (A|B|0).2. Dependency floor
>=0.8.0— a correctness fix, not a feature requirementset_static_pathsandPredDAG.from_edgesare 0.8.0 APIs, so the floor has to move. Butmaintoday carries a documented guarantee its own floor cannot deliver, which this also fixes.#108's changelog announces, as BREAKING, that
MaxFlowResult.min_cutcapacity equals the max flow. Measured with the source held constant and only the engine swapped: under 0.7.2 the default configuration violates duality — a positivetotal_flowpaired with an emptymin_cut— while under 0.8.0 it holds in 5999/5999 sweeps. Cross-checked againstnetworkx.maximum_flow_valueover thousands of random topologies, 0.7.2 understates max flow on cost-asymmetric graphs and 0.8.0 is wrong in zero cases. One hand-verifiable example: 0.7.2 reports 4.0 with an empty cut where the true min cut is 5.So 0.8.0 is a correctness fix, and the floor bump is what makes the existing BREAKING entry true. Values can increase against 0.7.x.
No upper bound: no other dependency here is capped, and a cap on a library propagates conflicts downstream.
3. Max-flow documentation corrected for the 0.8.0 engine
The min-cut duality guarantee was stated unconditionally. It holds for the default max-flow configuration (
PROPORTIONAL,require_capacity=True,shortest_path=False) — the same gate the C++ completion phase uses — and not for the placement models. Scoped inCHANGELOG.mdanddocs/reference/api.md.Four further statements were stale on both branches:
cost_distributionkeys: completion-phase entries are marginal costs (forward edge costs minus the cancelled flow's cost), so a key need not match any traversable path. Previously described as "flow volume placed at each path cost tier", true only of the tier loop.MAX_FLOWpseudocode ended at the tier loop, so a reader implementing it would reproduce 0.7.x's smaller answer. Completion phase added.4. Changelog rewrite
[Unreleased]restructured for signal: 83 bullets to 74, average length 259 to 207 characters, longest 1225 to 451. Fixed is grouped under prose leads, the first being "Analyses that previously returned wrong numbers without any error. If you rely on results from an earlier version, re-run them."Automated review
Codex raised one P2: inserting
static_pathsbeforeattrs/idshifts positional constructor order. Closed without a code change, after checking rather than asserting. The signature it asks to preserve belongs to the unmerged parent, not to any released version — on v0.21.0 the 8th positional isflow_policyandattrsis 10th, and #108 already broke positional compatibility by removingvolume_placed/flow_policy_obj(documented under Removed). A v0.21.0-shaped positional call at every arity 1–11 behaves identically on both branches, sostatic_pathsadds no new breakage, and all 61TrafficDemand(call sites construct by keyword.kw_only=Truewas considered and rejected: it would newly breakTrafficDemand("A","B",10.0), which works on main today.Verification
make check-cigreen on this exact tree: 1217 passed, 91.89% coverage.make validateandmake docsgreen.