v0.8.0: fix max-flow exactness, zero-cost DAG cycles, and SPF/KSP performance - #5
Merged
Merged
Conversation
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
batch_max_flow now runs worker threads, and gcov's default non-atomic
counters corrupt under concurrent updates, producing impossible values
("branch 4 taken -1") that abort the gcovr report. Build coverage
targets with -fprofile-update=atomic on GCC, and let gcovr warn rather
than fail on the known negative-hits gcov bug.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The dtype check compared buffer format strings, but NumPy spells int32 as
NPY_LONG ('l') on LLP64 and NPY_INT ('i') on LP64, while pybind11's
format_descriptor<int32_t> is always 'i'. A correctly-typed int32 array
was therefore rejected on Windows. Check dtype equivalence via isinstance
instead, matching as_span() elsewhere in the bindings.
Also reject non-contiguous pairs, which were previously read as if packed
and silently yielded wrong node ids.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audit of the public API, documentation and hot paths. API safety: - FlowPolicy rejects a FlowGraph wrapping a different graph (paths were selected on one topology and placed on another) and rebalance_demand now checks (src, dst) before remove_demand() clears the flows it would have been compared against. - Wrong-typed graph/algorithms arguments raise TypeError instead of an opaque "RuntimeError: Unable to cast ... to C++ type '?'"; spf raises TypeError for a bad residual length like every other length check; ksp validates dtype up front; batch_max_flow rejects out-of-range node ids. - Removed the unreachable Flow class: nothing constructed or returned one and the name collided with the C++ alias `using Flow = double`. Docs: corrected the sensitivity_analysis semantics (flow lost on removal, not gain from relaxing capacity), the zero-copy and GIL claims, the required CMake version, a nonexistent make target, and the header contracts for from_arrays, PredDAG, FlowSummary.costs and calc_max_flow. Typing: _docs.py described pybind11 enums as enum.Enum and classes as dataclasses, typed MinCut.edges as list[int], and declared a Path class that never existed. Corrected it and widened the re-exports from 6 names to 16, so the shipped py.typed no longer promises types that resolve to Unknown. Performance (all outputs bit-identical): - place_on_dag rebuilds edge groups into a reused arena rather than nested per-node vectors, ~70% of its runtime: 2-2.4x faster. - batch_max_flow workers claim pairs from a shared counter instead of fixed chunks, so a cost-skewed batch still scales (5.3x on 8 threads). - place_demand runs one SPF when seeding instead of one per flow. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Audit of every tracked file for dead code and obsolete statements. k_shortest_paths read paths.back() on an empty vector whenever max_cost_factor < 1.0 put the cost ceiling below the shortest path, so nothing was admitted. The k == 1 early return masked it; k >= 2 fell into the spur loop and segfaulted. All k now return no paths for such a factor. Several test assertions could never fail and now can: an assertion inside 'except Exception: pass', a thread-safety test ending in 'assert True' whose workers called pytest.fail() from a thread where it cannot fail the test, and two tautologies over unsigned/len() values. Removed: .gcovr.cfg (gcovr's default config name has no leading dot and make cov passes every flag explicitly, so it was never read), an unused C++ test helper and the include it alone needed, three unused Python test helpers/fixtures, _USE_MATH_DEFINES with no M_* macro anywhere, an unreachable dtype else-arm and an unreachable except ImportError, and unused includes across src/ and include/. Corrected comments that no longer matched the code: a false claim that cpu_backend is the single validation boundary, two disagreeing profiling cycle counts, and a stale constructor note. Flow, SPF and KSP outputs are bit-identical; 151 C++ tests pass under ASan/UBSan. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Removing the unused <algorithm> from profiling.hpp broke MSVC: profiling.cpp calls std::sort in dump() and had been getting the header transitively. libc++ still supplied it, so it only surfaced on the Windows builds. Add the include to profiling.cpp, which is the file that actually uses std::sort, and give module.cpp its own <optional> for the same reason. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Nothing used Algorithms.build_graph_from_arrays: no test in this repo and
nothing in NetGraph. Worse, the Graph handle it returned could not be
passed to FlowGraph or FlowPolicy (both raise TypeError), so it silently
served only the stateless algorithms while presenting itself as a peer of
build_graph. Callers use StrictMultiDiGraph.from_arrays() followed by
Algorithms.build_graph().
It was also the only caller of the build_graph(shared_ptr) overload, so
that goes too, in Algorithms, in the CpuBackend override, and as a pure
virtual on Backend that every implementation had to provide. Nothing is
lost: GraphHandle is a public aggregate, so a C++ caller wanting shared
ownership writes GraphHandle{sp} directly.
Flow and SPF outputs are bit-identical; NetGraph's 1186 tests still pass.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Finishes the static-paths feature that was ported from the original Python FlowPolicy but never bound to Python, and whose port bound every flow to the first bundle instead of its own (the original tests assert per-flow binding). Design reviewed by a three-lens panel and the implementation adversarially verified, including a differential run of 6000 randomized scenarios against a binary built from the pre-change sources: dynamic policies are numerically identical (EB leftovers may differ by ~1 ulp from summation order). set_static_paths(src, dst, bundles) pins the demand to explicit path bundles: one flow per usable bundle, bound in supply order. Bundles are validated against the graph (shape, id ranges, edge-endpoint consistency, acyclicity, src->dst walk) and pruned against the policy's masks; a bundle with no surviving walk is down and creates no flow (pinned paths do not reroute). Flow cost is the min-cost walk of the pruned bundle. EqualBalanced spreads over the up bundles only; the user's max_flow_count is validated against the supplied count but never mutated, so re-pinning under masks cannot throw spuriously. Pinned policies never grow their flow set and never reoptimize. PredDAG.from_edges(graph, edges) / make_path_dag builds a single-path bundle from a contiguous edge list, extracting the conversion that k_shortest_paths duplicated verbatim in two places (outputs unchanged). The EqualBalanced rebalance recursion is now an iterative loop: with many pinned bundles of heterogeneous capacity its depth grew like U*ln(imbalance/kMinFlow), a stack-overflow risk on worker threads (256-bundle regression test included). Also fixes UB found by UBSan in the max_path_cost_factor gate (INT64_MAX sentinel times factor cast back to int64) and makes the rebalance flag restore exception-safe. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Widening the type-checking re-exports from 6 names to 16 made _docs.py authoritative for far more of the API, and five of those stubs declared no constructor. Because the stubs are wired to type checkers, that turned 'unchecked' into 'confidently wrong': running NetGraph's pyright against this build produced three errors on every real FlowIndex(src, dst, flowClass, flowId) call, which pyright read as a zero-argument constructor. Add the real constructors for FlowIndex (four positional-only arguments, read-only attributes), FlowPolicyConfig (keyword-only, matching the binding's defaults), and the three pybind11 enums (constructible from their integer value). This repo's own pyright could not catch it because pyproject excludes tests/**, and ngraph/ is the only non-test code constructing a FlowIndex. Assert the correspondence at runtime instead: every stub class whose binding needs constructor arguments must declare __init__, and every attribute a stub declares must exist at runtime. Verified the guard fails on the exact bug before restoring the fix. NetGraph now passes clean against a real 0.8.0 wheel: 1186 tests (98 slow included, none skipped), 91.65% coverage, pyright 0 errors, ruff clean, schema validation clean. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A four-lens review of the release candidate found two changelog claims that would have misled consumers, both now stated plainly. FlowSummary.costs/flows changed meaning in the DEFAULT configuration: the completion phase appends MARGINAL costs (forward edges minus the flow it cancels), which need not match any traversable path. That was documented only in max_flow.hpp, while a downstream consumer zips these straight into a cost distribution. Now a Changed bullet and a note in _docs.py. max_path_cost silently changed behavior when I fixed the UB in its cast: max_path_cost_factor previously defaulted to 1.0 whenever either limit was set, so setting max_path_cost alone also rejected anything costlier than the best path. The limits are now independent -- an improvement, but a behavior change that needed saying. Also: mark the three API removals BREAKING with a migration line for out-of-tree Backend implementers; file the set_static_paths C++ signature change under Changed rather than Added; split the exception-type changes (spf ValueError->TypeError, batch_max_flow, ksp dtype) out of a bullet that read as polish; note the zero-cost SPF fix narrows ECMP fan-out; and correct the KSP figures, which conflated the 22-stage and 24-stage runs. Release mechanics: release.yml now fails a tag push whose version does not match pyproject.toml (publish previously would have shipped the wrong version silently), CONTRIBUTING documents the changelog step and that make publish bypasses the tag flow, and the numpy floor moves to 1.23.2 — no cp311 wheel exists below it, so numpy>=1.22 was unsatisfiable. Docs: README gains the two new features, a verified runnable Quick Start (it is the PyPI landing page and had no example), and the env vars; _docs.py gains Graph members, place_demand's Raises, and the profiling stubs so py.typed covers the whole exported surface. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Release v0.8.0. Six correctness defects (one a crash, one a silent wrong answer in the
default configuration), the completion of a half-wired feature, three performance fixes,
and a documentation/API pass. Every finding below was reproduced against
mainbeforebeing fixed, and the full CHANGELOG entry carries the migration detail.
Correctness
Max-flow returned less than the maximum.
calc_max_flowaugments only along forwardSPF DAGs and can never cancel an earlier placement. On a 4-node full-duplex graph it
returned
2.0where the true maximum is3.0— and reported an emptymin_cutalongside it, violating the duality its own
compute_min_cutcomputes correctly. This isthe default configuration, so recorded expected values must be regenerated. Fixed with
a residual completion phase; a 60,000-trial differential fuzz against
networkx.maximum_flow_valuenow finds 0 mismatches (was 51–64 per 20k).Zero-cost edges produced a cyclic "PredDAG". An equal-cost predecessor could be
recorded for an already-settled node, so a zero-cost pair
u<->vmade each node theother's parent.
EqualBalancedthen returned 0 flow andresolve_to_pathshungforever without releasing the GIL. A single
cost: 0link triggered both.k_shortest_pathssegfaulted fork > 1whenmax_cost_factor < 1.0put the costceiling below the shortest path (
paths.back()on an empty vector; thek == 1earlyreturn masked it).
Plus:
FlowPolicysilently routed a second(src, dst)pair over the first pair's pathsand never checked that the
FlowGraphwrapped its own graph;from_arraysaccepted costtotals that overflow int64 path arithmetic;
compute_min_cutmis-derived placed flow for acustom
residual_init; andbatch_max_flowrejected valid int32pairson Windows,which had left Windows CI red on
mainfor ~5 months.Feature: pinned path bundles
FlowPolicy.set_static_paths(src, dst, bundles)finishes a feature that was ported fromNetGraph's original Python
FlowPolicybut never bound — and whose port bound every flowto the first bundle rather than its own, contradicting the original's own tests. One flow
per usable bundle, validated against the graph and pruned against the policy's masks; a
bundle with no surviving walk is down and carries nothing, since a pinned path does not
reroute.
PredDAG.from_edgesprovides the missing explicit-path constructor.The design went through a three-lens panel before implementation (it caught a blocker in
the first draft), and the implementation through an adversarial review that compiled the
pre-change sources into a separate binary and ran 6,000 randomized differential scenarios:
dynamic policies are numerically identical.
Performance — all outputs bit-identical
place_on_dag(576 nodes)batch_max_flow, skewed batchSPF was ~70% malloc/free;
place_on_dag's group rebuild was ~70% of its runtime; KSP wasexponential in ECMP fan-out. The
EqualBalancedrebalance recursion became an iterativeloop — its depth grew like
U·ln(imbalance/kMinFlow), a stack overflow at 256 pinned LSPs.API, docs, and dead code
Removed three vestigial APIs (
build_graph_from_arrays, the unreachableFlowclass, andthe
build_graph(shared_ptr)overload) — all marked BREAKING with migration notes.Corrected documentation that was factually wrong, including
sensitivity_analysisdescribed backwards in two of four places, and the shipped
py.typedpromising types thatresolved to
Unknown. An exhaustive dead-code audit covered all 88 tracked files.Three test assertions that could never fail now can — including a thread-safety test whose
workers called
pytest.fail()from a thread where it cannot fail the test.Also fixes three pre-existing build bugs:
make sanitize-testnever compiled at all,coverage counters corrupted under threads, and Windows CI.
Verification
154 C++ tests in Release and under ASan/UBSan · 430+ Python tests · downstream NetGraph
passes 1217 tests against a real 0.8.0 wheel with pyright clean ·
FLOW_SHA256andSPF_POSCOST_SHA256gates bit-identical · clean-room wheel install verified ·twine checkpasses on wheel and sdist.
🤖 Generated with Claude Code