From 52780f28f0819e68198a6bfe0c13475c4e9f9a95 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 13:57:05 +0100 Subject: [PATCH 01/11] v0.8.0 per changelog Co-Authored-By: Claude Fable 5 --- CHANGELOG.md | 17 ++ CMakeLists.txt | 12 +- Makefile | 3 +- pyproject.toml | 2 +- src/flow_policy.cpp | 14 ++ src/flow_state.cpp | 8 +- src/k_shortest_paths.cpp | 56 +++++-- src/max_flow.cpp | 142 +++++++++++++--- src/shortest_paths.cpp | 69 +++++--- src/strict_multidigraph.cpp | 16 +- tests/cpp/max_flow_tests.cpp | 80 +++++++++ tests/cpp/shortest_paths_tests.cpp | 43 +++++ tests/py/test_review_regressions.py | 248 ++++++++++++++++++++++++++++ 13 files changed, 651 insertions(+), 59 deletions(-) create mode 100644 tests/py/test_review_regressions.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e625a9..54b2106 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,23 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.8.0] - 2026-08-22 + +### Fixed + +- **Max-Flow**: `calc_max_flow` could return less than the true maximum, since the tier loop augments only along forward SPF DAGs and never cancels an earlier placement (the reported `min_cut` then contradicted `total_flow`). Added a residual completion phase with reverse arcs for `Proportional` + `require_capacity` + `shortest_path=false`; results increase where the previous value was suboptimal. +- **Shortest Paths**: Zero-cost edges produced a cyclic predecessor DAG, making `EqualBalanced` placement return 0 flow and `resolve_to_paths` hang. Equal-cost predecessors are now recorded only while a node is unsettled, and `resolve_to_paths` guards against cycles in caller-supplied DAGs. +- **K-Shortest Paths**: Spur enumeration materialized every equal-cost path, exponential in ECMP fan-out (a 22-stage ladder needed ~19 s and ~3.8 GB for `k=3`). Enumeration is now bounded by the number of candidates still acceptable; tie-breaking among equal-cost paths may differ, but path counts and costs are unchanged. +- **Flow Policy**: `place_demand` silently routed a second `(src, dst)` pair over the first pair's paths. It now raises `invalid_argument`; use one policy per demand or call `remove_demand()` first. +- **Graph Construction**: `from_arrays` now rejects a total edge cost at or above 2^62, which overflows the int64 path arithmetic in SPF and silently corrupts results. +- **Flow State**: `compute_min_cut` and max-flow reachability derived placed flow from `capacity - residual`, overstating it for a `FlowState` built with a custom `residual_init`. Both now use `edge_flow`. +- **Build**: `NETGRAPH_CORE_SANITIZE` passed its flags as one quoted string, so sanitizer builds never compiled, and the test target was missing them entirely. `make sanitize-test` also no longer sets `detect_leaks=1` on macOS, where it aborts. + +### Changed + +- **Shortest Paths**: Replaced nested per-node predecessor vectors with flat intrusive lists, removing the allocation churn that dominated the hot path (~3-4x faster; output unchanged). +- **Max-Flow**: Parallelized `batch_max_flow` across source/destination pairs using `std::async`; thread count controlled by `NGRAPH_CORE_BATCH_THREADS` env or hardware concurrency. + ## [0.7.2] - 2026-03-26 ### Fixed diff --git a/CMakeLists.txt b/CMakeLists.txt index 461f44e..5dc5f41 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -218,8 +218,16 @@ endif() if(NETGRAPH_CORE_SANITIZE) message(STATUS "Enabling sanitizers (disables optimizations)") if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") - set(SAN_FLAGS "-fsanitize=address,undefined -fno-omit-frame-pointer") - foreach(tgt netgraph_core _netgraph_core) + # Must be a CMake list: a single quoted string reaches the compiler as one + # argument and clang rejects "-fsanitize=address,undefined -fno-omit-frame-pointer". + set(SAN_FLAGS -fsanitize=address,undefined -fno-omit-frame-pointer) + set(SAN_TARGETS netgraph_core _netgraph_core) + if(TARGET netgraph_core_tests) + # The test executable links the sanitized static library, so it must be + # built and linked with the same flags to pull in the sanitizer runtime. + list(APPEND SAN_TARGETS netgraph_core_tests) + endif() + foreach(tgt ${SAN_TARGETS}) target_compile_options(${tgt} PRIVATE ${SAN_FLAGS}) target_link_options(${tgt} PRIVATE ${SAN_FLAGS}) endforeach() diff --git a/Makefile b/Makefile index 0cab396..08e2e39 100644 --- a/Makefile +++ b/Makefile @@ -296,7 +296,8 @@ sanitize-test: if command -v ninja >/dev/null 2>&1; then GEN_ARGS="-G Ninja"; fi; \ cmake -S . -B "$$BUILD_DIR" -DNETGRAPH_CORE_BUILD_TESTS=ON -DNETGRAPH_CORE_SANITIZE=ON -DCMAKE_BUILD_TYPE=Debug $$GEN_ARGS; \ cmake --build "$$BUILD_DIR" --config Debug -j; \ - ASAN_OPTIONS=detect_leaks=1 ctest --test-dir "$$BUILD_DIR" --output-on-failure || echo "⚠️ Some sanitizer tests failed" + if [ "$$(uname -s)" = "Darwin" ]; then ASAN_ENV="ASAN_OPTIONS=detect_leaks=0"; else ASAN_ENV="ASAN_OPTIONS=detect_leaks=1"; fi; \ + env $$ASAN_ENV ctest --test-dir "$$BUILD_DIR" --output-on-failure || echo "⚠️ Some sanitizer tests failed" # Clean + reinstall in dev mode (respects CMAKE_ARGS and MACOSX_DEPLOYMENT_TARGET) # Uses active PYTHON (venv or PATH) to avoid environment mismatches diff --git a/pyproject.toml b/pyproject.toml index a0aedec..b575881 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -4,7 +4,7 @@ build-backend = "scikit_build_core.build" [project] name = "netgraph-core" -version = "0.7.2" +version = "0.8.0" description = "C++ implementation of graph algorithms for network flow analysis and traffic engineering with Python bindings" readme = "README.md" requires-python = ">=3.11" diff --git a/src/flow_policy.cpp b/src/flow_policy.cpp index 0201bd1..00958ba 100644 --- a/src/flow_policy.cpp +++ b/src/flow_policy.cpp @@ -202,6 +202,20 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, std::optional min_flow) { NGRAPH_PROFILE_SCOPE("place_demand"); + // A FlowPolicy manages flows for a single demand. Placing a different + // (src, dst) pair on a policy that already holds flows would silently route + // the new volume over the previous pair's paths (the round-robin loop reads + // src/dst from the existing flow records), so reject it loudly. + if (!flows_.empty()) { + const auto& existing = flows_.begin()->second; + if (existing.src != src || existing.dst != dst) { + throw std::invalid_argument( + "FlowPolicy::place_demand: this policy already manages a demand for a " + "different (src, dst) pair; use a separate FlowPolicy per demand or " + "call remove_demand() first"); + } + } + // Compute target flow per flow-record. // target: the volume to place per flow (or globally if target_per_flow is unset). // per_target: refined target for EqualBalanced mode (considers source capacity). diff --git a/src/flow_state.cpp b/src/flow_state.cpp index bdb39da..1f7127f 100644 --- a/src/flow_state.cpp +++ b/src/flow_state.cpp @@ -481,8 +481,12 @@ MinCut FlowState::compute_min_cut(NodeId src, std::span node_mask, s auto eid = static_cast(in_aei[j]); if (use_edge_mask && !edge_mask[eid]) continue; if (use_node_mask && !node_mask[static_cast(w)]) continue; - double flow_e = g_->capacity_view()[eid] - residual_[eid]; - if (flow_e > kMinFlow && !visited[static_cast(w)]) { + // Reverse arcs exist where THIS state has placed cancellable flow. Using + // edge_flow_ (not capacity - residual) keeps the traversal correct when + // the state was constructed with a custom residual_init, where + // capacity - residual also counts pre-existing usage this state never + // placed and cannot cancel. + if (edge_flow_[eid] > kMinFlow && !visited[static_cast(w)]) { visited[static_cast(w)] = 1; q.push(w); } diff --git a/src/k_shortest_paths.cpp b/src/k_shortest_paths.cpp index 6cb5b62..39a387b 100644 --- a/src/k_shortest_paths.cpp +++ b/src/k_shortest_paths.cpp @@ -36,6 +36,13 @@ struct Candidate { bool operator>(const Candidate& o) const { return cost > o.cost; } }; +struct VectorHash { size_t operator()(const std::vector& v) const noexcept { + size_t h = 1469598103934665603ull; + for (auto x : v) { h ^= static_cast(x + 0x9e3779b97f4a7c15ull); h *= 1099511628211ull; } + return h; +}}; +using PathSet = std::unordered_set, VectorHash>; + static std::optional dijkstra_single(const StrictMultiDiGraph& g, NodeId s, NodeId t, const std::vector* node_mask, @@ -107,9 +114,15 @@ static std::optional dijkstra_single(const StrictMultiDiGraph& g, NodeId s return p; } -// Enumerate all shortest spur paths from spur->t using PredDAG produced by -// shortest_paths(spur, ..., multipath=true). Returns a list of (nodes, edges) -// sequences in forward order. +// Enumerate shortest spur paths from spur->t using the PredDAG produced by +// shortest_paths(spur, ..., multipath=true), in deterministic DFS order. +// All spur paths in the DAG share the same (shortest) cost, and at most +// max_count candidates can ever be accepted downstream, so enumeration stops +// after emitting max_count paths. Without the bound this walk materializes +// every equal-cost path — exponential in ECMP fan-out (2^k paths on a k-stage +// ladder). When dedup is supplied (unique mode), paths whose full edge +// sequence (prefix + spur segment) was already visited are skipped without +// consuming the bound, so the cap cannot starve fresh candidates. static void dfs_spur_paths(NodeId spur, NodeId v, const std::vector& off, const std::vector& parents, @@ -117,7 +130,11 @@ static void dfs_spur_paths(NodeId spur, NodeId v, std::vector& nodes_rev, std::vector& edges_rev, std::vector>& out_nodes, - std::vector>& out_edges) { + std::vector>& out_edges, + std::size_t max_count, + const std::vector* prefix_edges, + const PathSet* dedup) { + if (out_nodes.size() >= max_count) return; if (v == spur) { // build forward std::vector n; n.reserve(nodes_rev.size() + 1); @@ -125,6 +142,13 @@ static void dfs_spur_paths(NodeId spur, NodeId v, for (auto it = nodes_rev.rbegin(); it != nodes_rev.rend(); ++it) n.push_back(*it); std::vector e; e.reserve(edges_rev.size()); for (auto it = edges_rev.rbegin(); it != edges_rev.rend(); ++it) e.push_back(*it); + if (dedup != nullptr) { + std::vector full; + full.reserve(prefix_edges->size() + e.size()); + full.insert(full.end(), prefix_edges->begin(), prefix_edges->end()); + full.insert(full.end(), e.begin(), e.end()); + if (dedup->find(full) != dedup->end()) return; // already visited; free the slot + } out_nodes.push_back(std::move(n)); out_edges.push_back(std::move(e)); return; @@ -132,11 +156,13 @@ static void dfs_spur_paths(NodeId spur, NodeId v, auto s = off[static_cast(v)]; auto e = off[static_cast(v) + 1]; for (std::int32_t i = s; i < e; ++i) { + if (out_nodes.size() >= max_count) return; auto u = parents[static_cast(i)]; auto eid = via[static_cast(i)]; edges_rev.push_back(static_cast(eid)); nodes_rev.push_back(v); - dfs_spur_paths(spur, u, off, parents, via, nodes_rev, edges_rev, out_nodes, out_edges); + dfs_spur_paths(spur, u, off, parents, via, nodes_rev, edges_rev, out_nodes, out_edges, + max_count, prefix_edges, dedup); nodes_rev.pop_back(); edges_rev.pop_back(); } @@ -219,12 +245,7 @@ std::vector, PredDAG>> k_shortest_paths( auto cost_view = g.cost_view(); std::priority_queue, std::greater> B; auto path_signature = [&](const std::vector& edges){ return edges; }; - struct VectorHash { size_t operator()(const std::vector& v) const noexcept { - size_t h = 1469598103934665603ull; - for (auto x : v) { h ^= static_cast(x + 0x9e3779b97f4a7c15ull); h *= 1099511628211ull; } - return h; - }}; - std::unordered_set, VectorHash> visited; + PathSet visited; visited.insert(path_signature(p0->edges)); for (int i = 1; i < k; ++i) { @@ -282,12 +303,23 @@ std::vector, PredDAG>> k_shortest_paths( dag_spur.parent_offsets[static_cast(dst)] == dag_spur.parent_offsets[static_cast(dst + 1)]) { continue; } + // Every spur path in the DAG costs dist_spur[dst]; skip the whole family + // if the resulting candidates would exceed the cost ceiling. + if (static_cast(prefix_cost[j]) + static_cast(dist_spur[static_cast(dst)]) > max_cost) { + continue; + } + // At most (k - accepted) further paths can ever be accepted, so that many + // fresh candidates from this spur family suffice. + const std::size_t want = static_cast(k) - paths.size(); + std::vector prefix_edges(last.edges.begin(), + last.edges.begin() + static_cast(j)); std::vector nodes_rev; std::vector edges_rev; std::vector> spur_nodes_list; std::vector> spur_edges_list; dfs_spur_paths(spur_node, dst, dag_spur.parent_offsets, dag_spur.parents, dag_spur.via_edges, - nodes_rev, edges_rev, spur_nodes_list, spur_edges_list); + nodes_rev, edges_rev, spur_nodes_list, spur_edges_list, + want, &prefix_edges, unique ? &visited : nullptr); for (std::size_t si = 0; si < spur_nodes_list.size(); ++si) { const auto& spur_nodes = spur_nodes_list[si]; const auto& spur_edges = spur_edges_list[si]; diff --git a/src/max_flow.cpp b/src/max_flow.cpp index a970a2f..d5e6565 100644 --- a/src/max_flow.cpp +++ b/src/max_flow.cpp @@ -31,12 +31,12 @@ namespace netgraph::core { namespace { -std::size_t sensitivity_thread_budget(std::size_t candidate_count) { +std::size_t thread_budget_from_env(const char* env_name, std::size_t candidate_count) { if (candidate_count <= 1) { return 1; } - const char* env = std::getenv("NGRAPH_CORE_SENSITIVITY_THREADS"); + const char* env = std::getenv(env_name); if (env != nullptr && env[0] != '\0') { char* end = nullptr; unsigned long parsed = std::strtoul(env, &end, 10); @@ -151,6 +151,93 @@ calc_max_flow(const StrictMultiDiGraph& g, NodeId src, NodeId dst, if (shortest_path) break; } + // Completion phase. The tier loop above augments only along forward SPF DAGs + // and can never cancel an earlier placement, so it may terminate below the + // true maximum (its own min-cut then contradicts total_flow). Finish with + // BFS augmentation on the full residual graph, where traversing an edge + // backwards returns previously placed flow. Only meaningful for Proportional + // max-flow semantics: EqualBalanced models ECMP admission and + // require_capacity=false models fixed-cost IP routing, both of which are + // placement models rather than max-flow computations. + if (placement == FlowPlacement::Proportional && !shortest_path && require_capacity) { + const auto row = g.row_offsets_view(); + const auto col = g.col_indices_view(); + const auto aei = g.adj_edge_index_view(); + const auto irow = g.in_row_offsets_view(); + const auto icol = g.in_col_indices_view(); + const auto iaei = g.in_adj_edge_index_view(); + const auto costv = g.cost_view(); + const auto residual = fs.residual_view(); + const auto eflow = fs.edge_flow_view(); + std::vector prev_node(static_cast(N)); + std::vector prev_edge(static_cast(N)); + std::vector prev_fwd(static_cast(N)); + std::vector seen(static_cast(N)); + std::vector> fwd_deltas, rev_deltas; + while (true) { + std::fill(seen.begin(), seen.end(), 0); + std::queue bfs; + seen[static_cast(src)] = 1; + bfs.push(src); + bool found = false; + while (!bfs.empty() && !found) { + auto u = bfs.front(); bfs.pop(); + auto us = static_cast(u); + // Forward residual arcs u -> v. + for (auto j = static_cast(row[us]); j < static_cast(row[us + 1]); ++j) { + auto v = static_cast(col[j]); + auto eid = static_cast(aei[j]); + if (use_edge_mask && !edge_mask[eid]) continue; + if (use_node_mask && !node_mask[v]) continue; + if (seen[v] || residual[eid] <= kMinCap) continue; + seen[v] = 1; prev_node[v] = u; prev_edge[v] = static_cast(eid); prev_fwd[v] = 1; + if (static_cast(v) == dst) { found = true; break; } + bfs.push(static_cast(v)); + } + if (found) break; + // Reverse residual arcs u -> w along edges w -> u carrying flow. + for (auto j = static_cast(irow[us]); j < static_cast(irow[us + 1]); ++j) { + auto w = static_cast(icol[j]); + auto eid = static_cast(iaei[j]); + if (use_edge_mask && !edge_mask[eid]) continue; + if (use_node_mask && !node_mask[w]) continue; + if (seen[w] || eflow[eid] <= kMinFlow) continue; + seen[w] = 1; prev_node[w] = u; prev_edge[w] = static_cast(eid); prev_fwd[w] = 0; + if (static_cast(w) == dst) { found = true; break; } + bfs.push(static_cast(w)); + } + } + if (!found) break; + // Bottleneck and marginal path cost (forward costs minus cancelled costs). + double bottleneck = std::numeric_limits::infinity(); + Cost path_cost = 0; + for (auto v = dst; v != src; v = prev_node[static_cast(v)]) { + auto vs = static_cast(v); + auto eid = static_cast(prev_edge[vs]); + if (prev_fwd[vs]) { + bottleneck = std::min(bottleneck, static_cast(residual[eid])); + path_cost += costv[eid]; + } else { + bottleneck = std::min(bottleneck, static_cast(eflow[eid])); + path_cost -= costv[eid]; + } + } + if (!(bottleneck >= kMinFlow)) break; + fwd_deltas.clear(); rev_deltas.clear(); + for (auto v = dst; v != src; v = prev_node[static_cast(v)]) { + auto vs = static_cast(v); + if (prev_fwd[vs]) fwd_deltas.emplace_back(prev_edge[vs], static_cast(bottleneck)); + else rev_deltas.emplace_back(prev_edge[vs], static_cast(bottleneck)); + } + fs.apply_deltas(fwd_deltas, /*add=*/true); + fs.apply_deltas(rev_deltas, /*add=*/false); + total += static_cast(bottleneck); + bool merged2 = false; + for (auto& pr : cost_dist) { if (pr.first == path_cost) { pr.second += bottleneck; merged2 = true; break; } } + if (!merged2) cost_dist.emplace_back(path_cost, static_cast(bottleneck)); + } + } + summary.total_flow = total; if (with_edge_flows) { auto ef = fs.edge_flow_view(); @@ -177,7 +264,7 @@ calc_max_flow(const StrictMultiDiGraph& g, NodeId src, NodeId dst, if (with_reachable) { summary.reachable_nodes.assign(static_cast(g.num_nodes()), 0u); auto residual = fs.residual_view(); - auto capv = fs.capacity_view(); + auto eflows = fs.edge_flow_view(); const bool reach_use_node_mask = use_node_mask; const bool reach_use_edge_mask = use_edge_mask; const auto N = static_cast(g.num_nodes()); @@ -214,8 +301,7 @@ calc_max_flow(const StrictMultiDiGraph& g, NodeId src, NodeId dst, auto eid = static_cast(iae[p]); if (reach_use_edge_mask && !edge_mask[eid]) continue; if (reach_use_node_mask && !node_mask[u]) continue; - auto flow = capv[eid] - residual[eid]; - if (flow > kMinFlow && !summary.reachable_nodes[u]) { + if (eflows[eid] > kMinFlow && !summary.reachable_nodes[u]) { stack.push_back(static_cast(u)); } } @@ -247,21 +333,37 @@ batch_max_flow(const StrictMultiDiGraph& g, } } - std::vector out; - out.reserve(pairs.size()); - for (std::size_t i = 0; i < pairs.size(); ++i) { - auto pr = pairs[i]; - std::span nm = (i < node_masks.size() ? node_masks[i] : std::span{}); - std::span em = (i < edge_masks.size() ? edge_masks[i] : std::span{}); - auto [val, summary] = calc_max_flow(g, pr.first, pr.second, - placement, shortest_path, - require_capacity, - with_edge_flows, - with_reachable, - with_residuals, - nm, em); - out.push_back(std::move(summary)); + std::vector out(pairs.size()); + auto run_range = [&](std::size_t begin, std::size_t end) { + for (std::size_t i = begin; i < end; ++i) { + auto pr = pairs[i]; + std::span nm = (i < node_masks.size() ? node_masks[i] : std::span{}); + std::span em = (i < edge_masks.size() ? edge_masks[i] : std::span{}); + auto [val, summary] = calc_max_flow(g, pr.first, pr.second, + placement, shortest_path, + require_capacity, + with_edge_flows, + with_reachable, + with_residuals, + nm, em); + out[i] = std::move(summary); + } + }; + // Pairs are independent over an immutable graph; evaluate chunks in parallel. + // Thread count from NGRAPH_CORE_BATCH_THREADS env or hardware concurrency. + const auto thread_budget = thread_budget_from_env("NGRAPH_CORE_BATCH_THREADS", pairs.size()); + if (thread_budget <= 1) { + run_range(0, pairs.size()); + return out; + } + const auto chunk_size = (pairs.size() + thread_budget - 1) / thread_budget; + std::vector> futures; + futures.reserve(thread_budget); + for (std::size_t begin = 0; begin < pairs.size(); begin += chunk_size) { + const std::size_t end = std::min(begin + chunk_size, pairs.size()); + futures.emplace_back(std::async(std::launch::async, run_range, begin, end)); } + for (auto& f : futures) f.get(); return out; } @@ -312,7 +414,7 @@ sensitivity_analysis(const StrictMultiDiGraph& g, NodeId src, NodeId dst, results.reserve(candidates.size()); // Step 2: Iterate candidates, testing flow reduction when each is removed - const auto thread_budget = sensitivity_thread_budget(candidates.size()); + const auto thread_budget = thread_budget_from_env("NGRAPH_CORE_SENSITIVITY_THREADS", candidates.size()); if (thread_budget <= 1) { for (EdgeId eid : candidates) { auto maybe_result = evaluate_sensitivity_candidate( diff --git a/src/shortest_paths.cpp b/src/shortest_paths.cpp index b55231e..3e9f46c 100644 --- a/src/shortest_paths.cpp +++ b/src/shortest_paths.cpp @@ -49,9 +49,14 @@ resolve_to_paths(const PredDAG& dag, NodeId src, NodeId dst, struct Frame { NodeId node; std::size_t idx; std::vector>> groups; }; std::vector stack; stack.reserve(16); + // on_path[v] marks nodes on the current DFS stack. A well-formed PredDAG is + // acyclic, but zero-cost edges (or a caller-supplied DAG) can contain cycles; + // without this guard the enumeration below would walk them forever. + std::vector on_path(dag.parent_offsets.size() > 0 ? dag.parent_offsets.size() - 1 : 0, 0); // start from dst Frame start; start.node = dst; start.idx = 0; group_parents(dag, dst, start.groups); stack.push_back(std::move(start)); + on_path[static_cast(dst)] = 1; std::vector>> current; // reversed path accum @@ -59,11 +64,14 @@ resolve_to_paths(const PredDAG& dag, NodeId src, NodeId dst, auto& top = stack.back(); if (top.idx >= top.groups.size()) { // backtrack + on_path[static_cast(top.node)] = 0; stack.pop_back(); if (!current.empty()) current.pop_back(); continue; } auto [parent, edges] = top.groups[top.idx++]; + // Skip parents already on the current path (cycle in the input DAG). + if (parent != src && on_path[static_cast(parent)]) continue; current.emplace_back(top.node, std::move(edges)); if (parent == src) { // reached src; build forward segments: for each hop prev->next store (next, edges) @@ -141,6 +149,7 @@ resolve_to_paths(const PredDAG& dag, NodeId src, NodeId dst, current.pop_back(); continue; } + on_path[static_cast(parent)] = 1; stack.push_back(std::move(next)); } @@ -205,11 +214,25 @@ shortest_paths_core(const StrictMultiDiGraph& g, NodeId src, min_residual_to_node[static_cast(src)] = std::numeric_limits::max(); } - // pred_lists[v] stores predecessors for node v as (parent_node, [edges_from_parent]). - // In multipath mode, multiple parents with equal-cost paths are retained. - std::vector>>> pred_lists(static_cast(N)); + // Predecessor storage as a flat intrusive list: pred_head/pred_tail index into + // the ent_* arrays, whose entries are (parent, via_edge) pairs appended in + // discovery order. This avoids the per-node/per-group vector allocations that + // previously dominated SPF runtime (~70% of samples in malloc/free). + std::vector pred_head(static_cast(N), -1); + std::vector pred_tail(static_cast(N), -1); + std::vector ent_parent; ent_parent.reserve(static_cast(g.num_edges())); + std::vector ent_edge; ent_edge.reserve(static_cast(g.num_edges())); + std::vector ent_next; ent_next.reserve(static_cast(g.num_edges())); + auto pred_clear = [&](std::size_t v){ pred_head[v] = -1; pred_tail[v] = -1; }; + auto pred_append = [&](std::size_t v, NodeId p, EdgeId e){ + const auto idx = static_cast(ent_parent.size()); + ent_parent.push_back(p); ent_edge.push_back(e); ent_next.push_back(-1); + if (pred_head[v] < 0) { pred_head[v] = idx; } + else { ent_next[static_cast(pred_tail[v])] = idx; } + pred_tail[v] = idx; + }; if (src_allowed) { - pred_lists[static_cast(src)] = {}; + // source has no predecessors } else { // Source is out of range or masked out: no traversal, return empty DAG. PredDAG dag; @@ -234,6 +257,13 @@ shortest_paths_core(const StrictMultiDiGraph& g, NodeId src, const bool require_cap = selection.require_capacity || has_residual; const bool multipath = multipath_arg; + // settled[v] is set once v is popped at its final distance. Equal-cost + // predecessor updates are only accepted while v is unsettled: with positive + // edge costs every equal-cost parent is discovered before v settles, and with + // zero-cost edges this guard is what keeps the PredDAG acyclic (previously a + // zero-cost pair u<->v recorded each node as the other's parent). + std::vector settled(static_cast(N), 0); + std::vector sel_buf; sel_buf.reserve(16); while (!pq.empty()) { // Extract min-cost node from priority queue. // Structured binding: auto [d_u, neg_res_u, u] = ... destructures the tuple. @@ -244,6 +274,7 @@ shortest_paths_core(const StrictMultiDiGraph& g, NodeId src, // Skip residual-stale entries in single-path mode (same cost but outdated residual). if (!multipath && d_u == dist[static_cast(u)] && -neg_res_u < min_residual_to_node[static_cast(u)] - kEpsilon) continue; + settled[static_cast(u)] = 1; // Early exit optimization: record when we first reach destination. if (early_exit && u == dst_node && !have_best_dst) { best_dst_cost = d_u; have_best_dst = true; } @@ -266,7 +297,7 @@ shortest_paths_core(const StrictMultiDiGraph& g, NodeId src, // Select best edge(s) from u to v according to policy. Cost min_edge_cost = std::numeric_limits::max(); - std::vector selected_edges; + std::vector& selected_edges = sel_buf; selected_edges.clear(); double best_rem_for_min_cost = -1.0; std::size_t j = i; int best_edge_id = -1; @@ -333,17 +364,18 @@ shortest_paths_core(const StrictMultiDiGraph& g, NodeId src, // Relaxation: found shorter path to v, or equal-cost path with better capacity (single-path mode). if (new_cost < dist[v_idx] || - (!multipath && new_cost == dist[v_idx] && + (!multipath && new_cost == dist[v_idx] && !settled[v_idx] && path_residual > min_residual_to_node[v_idx] + kEpsilon)) { dist[v_idx] = new_cost; min_residual_to_node[v_idx] = path_residual; - pred_lists[v_idx].clear(); - pred_lists[v_idx].push_back({u, std::move(selected_edges)}); + pred_clear(v_idx); + for (auto sel_e : selected_edges) pred_append(v_idx, u, sel_e); pq.emplace(new_cost, -path_residual, v); // Negate residual for max-heap behavior } - // Multipath: found equal-cost alternative path to v. - else if (multipath && new_cost == dist[v_idx]) { - pred_lists[v_idx].push_back({u, std::move(selected_edges)}); + // Multipath: found equal-cost alternative path to v (only while v is + // unsettled; see the settled[] comment above). + else if (multipath && new_cost == dist[v_idx] && !settled[v_idx]) { + for (auto sel_e : selected_edges) pred_append(v_idx, u, sel_e); // Note: In multipath mode, we don't update min_residual_to_node because // we're collecting all equal-cost paths, not choosing based on residual. } @@ -353,7 +385,7 @@ shortest_paths_core(const StrictMultiDiGraph& g, NodeId src, if (have_best_dst) { if (pq.empty() || std::get<0>(pq.top()) > best_dst_cost) break; } } - // Convert pred_lists to PredDAG using CSR-like layout. + // Convert flat predecessor lists to PredDAG using CSR-like layout. // parent_offsets[v]:parent_offsets[v+1] gives the range in parents/via_edges for node v. PredDAG dag; dag.parent_offsets.assign(static_cast(N+1), 0); @@ -361,7 +393,7 @@ shortest_paths_core(const StrictMultiDiGraph& g, NodeId src, // Step 1: Count total predecessor entries per node. for (std::int32_t v=0; v(v)]) c += pe.second.size(); + for (std::int32_t i = pred_head[static_cast(v)]; i >= 0; i = ent_next[static_cast(i)]) ++c; dag.parent_offsets[static_cast(v+1)] = static_cast(c); } @@ -375,13 +407,10 @@ shortest_paths_core(const StrictMultiDiGraph& g, NodeId src, for (std::int32_t v=0; v(dag.parent_offsets[static_cast(v)]); std::size_t k = 0; - for (auto const& pe : pred_lists[static_cast(v)]) { - NodeId p = pe.first; - for (auto e : pe.second) { - dag.parents[base+k] = p; - dag.via_edges[base+k] = e; - ++k; - } + for (std::int32_t i = pred_head[static_cast(v)]; i >= 0; i = ent_next[static_cast(i)]) { + dag.parents[base+k] = ent_parent[static_cast(i)]; + dag.via_edges[base+k] = ent_edge[static_cast(i)]; + ++k; } } return {std::move(dist), std::move(dag)}; diff --git a/src/strict_multidigraph.cpp b/src/strict_multidigraph.cpp index 34dfc39..c582270 100644 --- a/src/strict_multidigraph.cpp +++ b/src/strict_multidigraph.cpp @@ -37,7 +37,14 @@ StrictMultiDiGraph StrictMultiDiGraph::from_arrays( throw std::invalid_argument("number of edges exceeds INT32_MAX"); } - // Invariants: ids within [0, num_nodes), non-negative weights + // Invariants: ids within [0, num_nodes), non-negative weights. + // SPF accumulates path costs as int64 without overflow checks and uses + // INT64_MAX as the unreachable sentinel, so the total of all edge costs + // (an upper bound on any simple path, and on one reverse-edge bounce) must + // stay below 2^62 or accumulated costs could wrap negative and silently + // corrupt results. + constexpr std::uint64_t kMaxTotalCost = (std::uint64_t{1} << 62); + std::uint64_t total_cost = 0; for (std::size_t i = 0; i < m; ++i) { if (src[i] < 0 || dst[i] < 0 || src[i] >= num_nodes || dst[i] >= num_nodes) { throw std::out_of_range("edge index out of range of num_nodes"); @@ -46,6 +53,13 @@ StrictMultiDiGraph StrictMultiDiGraph::from_arrays( throw std::invalid_argument("capacity must be >= 0"); } if (cost[i] < 0) { throw std::invalid_argument("cost must be >= 0"); } + const auto c = static_cast(cost[i]); + if (c >= kMaxTotalCost - total_cost) { + throw std::invalid_argument( + "total edge cost must stay below 2^62: larger accumulated path costs " + "overflow int64 cost arithmetic and silently corrupt results"); + } + total_cost += c; } // Gather initial arrays std::vector src_v(src.begin(), src.end()); diff --git a/tests/cpp/max_flow_tests.cpp b/tests/cpp/max_flow_tests.cpp index 75f4cdc..cf14a92 100644 --- a/tests/cpp/max_flow_tests.cpp +++ b/tests/cpp/max_flow_tests.cpp @@ -1256,3 +1256,83 @@ TEST(MaxFlow, Sensitivity_ShortestPathVsMaxFlow) { } } } + +// ============================================================================ +// Regression: residual completion phase (flow cancellation) +// ============================================================================ + +// Successive shortest paths over forward-only SPF DAGs can strand capacity that +// only reverse-arc cancellation can recover. On this full-duplex topology the +// first tier routes 0->1->2->3 (cost 3), blocking both the direct 1->3 exit and +// the 2->3 exit; the true maximum needs to reroute it. Expected max flow is 3: +// 0->1->3 (1), 0->2->1->3 (1), 0->2->3 (1); the cut {0,2}|{1,3} has capacity 3. +TEST(MaxFlow, CompletionPhase_RecoversCancellation) { + std::vector src, dst; + std::vector cap; + std::vector cost; + auto add_duplex = [&](std::int32_t u, std::int32_t v, double c, std::int64_t k) { + src.push_back(u); dst.push_back(v); cap.push_back(c); cost.push_back(k); + src.push_back(v); dst.push_back(u); cap.push_back(c); cost.push_back(k); + }; + add_duplex(1, 3, 3.0, 17); + add_duplex(0, 2, 8.0, 17); + add_duplex(1, 2, 1.0, 1); + add_duplex(1, 0, 1.0, 1); + add_duplex(3, 2, 1.0, 1); + auto g = StrictMultiDiGraph::from_arrays(4, src, dst, cap, cost); + + auto be = make_cpu_backend(); + Algorithms algs(be); + auto gh = algs.build_graph(g); + + MaxFlowOptions opts; + opts.placement = FlowPlacement::Proportional; + opts.shortest_path = false; + + auto [total, summary] = algs.max_flow(gh, 0, 3, opts); + EXPECT_NEAR(total, 3.0, 1e-9) << "true max flow requires cancelling the first tier"; + + // Max-flow/min-cut duality: the reported cut capacity must equal the flow. + double cut_cap = 0.0; + auto capv = g.capacity_view(); + for (auto eid : summary.min_cut.edges) cut_cap += capv[static_cast(eid)]; + EXPECT_NEAR(cut_cap, total, 1e-9) << "min-cut capacity must equal total flow"; + + // Cost distribution must account for exactly the total flow. + double dist_sum = 0.0; + for (auto f : summary.flows) dist_sum += f; + EXPECT_NEAR(dist_sum, total, 1e-9); +} + +// The completion phase must respect masks: masking node 2 on the topology above +// limits the flow to the direct 0->1->3 route. +TEST(MaxFlow, CompletionPhase_RespectsNodeMask) { + std::vector src, dst; + std::vector cap; + std::vector cost; + auto add_duplex = [&](std::int32_t u, std::int32_t v, double c, std::int64_t k) { + src.push_back(u); dst.push_back(v); cap.push_back(c); cost.push_back(k); + src.push_back(v); dst.push_back(u); cap.push_back(c); cost.push_back(k); + }; + add_duplex(1, 3, 3.0, 17); + add_duplex(0, 2, 8.0, 17); + add_duplex(1, 2, 1.0, 1); + add_duplex(1, 0, 1.0, 1); + add_duplex(3, 2, 1.0, 1); + auto g = StrictMultiDiGraph::from_arrays(4, src, dst, cap, cost); + + auto be = make_cpu_backend(); + Algorithms algs(be); + auto gh = algs.build_graph(g); + + auto mask = make_bool_mask(4); + mask[2] = false; + + MaxFlowOptions opts; + opts.placement = FlowPlacement::Proportional; + opts.shortest_path = false; + opts.node_mask = std::span(mask.get(), 4); + + auto [total, summary] = algs.max_flow(gh, 0, 3, opts); + EXPECT_NEAR(total, 1.0, 1e-9) << "without node 2 only 0->1->3 remains (bottleneck 1)"; +} diff --git a/tests/cpp/shortest_paths_tests.cpp b/tests/cpp/shortest_paths_tests.cpp index 0080af2..261f964 100644 --- a/tests/cpp/shortest_paths_tests.cpp +++ b/tests/cpp/shortest_paths_tests.cpp @@ -298,3 +298,46 @@ TEST(ShortestPaths, RejectsMaskLengthMismatch) { opts2.edge_mask = std::span(edge_mask.get(), static_cast(g.num_edges() - 1)); EXPECT_THROW({ (void)algs.spf(gh, 0, opts2); }, std::invalid_argument); } + +// ============================================================================ +// Regression: zero-cost edges must not create cycles in the PredDAG +// ============================================================================ + +// A zero-cost pair 1<->2 previously recorded each node as the other's parent +// (both relaxations see an "equal cost" path), producing a cyclic PredDAG. +// EqualBalanced placement then stalled (returned 0 flow) and path enumeration +// walked the cycle forever. Equal-cost predecessors are now only accepted while +// the child is unsettled, which keeps the DAG acyclic by settle order. +TEST(ShortestPaths, ZeroCostEdges_PredDAGIsAcyclic) { + std::int32_t src_arr[4] = {0, 1, 2, 2}; + std::int32_t dst_arr[4] = {1, 2, 1, 3}; + double cap_arr[4] = {10.0, 10.0, 10.0, 10.0}; + std::int64_t cost_arr[4] = {1, 0, 0, 1}; + auto g = StrictMultiDiGraph::from_arrays(4, + std::span(src_arr, 4), std::span(dst_arr, 4), + std::span(cap_arr, 4), std::span(cost_arr, 4)); + + EdgeSelection sel; + sel.multi_edge = true; + auto [dist, dag] = shortest_paths(g, 0, std::nullopt, /*multipath=*/true, sel); + + EXPECT_EQ(dist[3], 2); + + // No 2-cycles: v's parent u must not also have v as a parent. + for (std::int32_t v = 0; v < 4; ++v) { + for (auto i = dag.parent_offsets[static_cast(v)]; + i < dag.parent_offsets[static_cast(v) + 1]; ++i) { + auto u = dag.parents[static_cast(i)]; + for (auto j = dag.parent_offsets[static_cast(u)]; + j < dag.parent_offsets[static_cast(u) + 1]; ++j) { + EXPECT_NE(dag.parents[static_cast(j)], v) + << "cycle " << u << "<->" << v << " in PredDAG"; + } + } + } + + // Path enumeration must terminate and yield the single simple path. + auto paths = resolve_to_paths(dag, 0, 3); + ASSERT_EQ(paths.size(), 1u); + EXPECT_EQ(paths[0].size(), 4u); // 0 -> 1 -> 2 -> 3 +} diff --git a/tests/py/test_review_regressions.py b/tests/py/test_review_regressions.py new file mode 100644 index 0000000..4c230a1 --- /dev/null +++ b/tests/py/test_review_regressions.py @@ -0,0 +1,248 @@ +"""Regression tests for the 2026-08 code review findings. + +Each test locks in the fixed behavior for a confirmed defect: + +1. max_flow under-reported when the SSP tier loop needed flow cancellation; + its own min_cut then contradicted total_flow (duality violation). +2. Zero-cost edges produced a cyclic PredDAG: EqualBalanced placement returned + 0 flow and resolve_to_paths never terminated. +3. k_shortest_paths enumerated every equal-cost spur path (exponential in ECMP + width; a 22-stage ladder needed ~19 s and ~3.8 GB for k=3). +4. batch_max_flow results must be identical to per-pair calls (now computed in + parallel). +5. FlowPolicy.place_demand silently routed a second (src, dst) pair over the + first pair's paths. +6. StrictMultiDiGraph accepted cost totals that overflow int64 path arithmetic. +7. FlowState.compute_min_cut treated pre-existing usage from a custom + residual_init as cancellable flow. +""" + +from __future__ import annotations + +import numpy as np +import pytest + +import netgraph_core as ngc + + +def _graph(num_nodes, edges): + """edges: list of (src, dst, capacity, cost).""" + s, d, c, k = zip(*edges, strict=False) + return ngc.StrictMultiDiGraph.from_arrays( + num_nodes, + np.array(s, dtype=np.int32), + np.array(d, dtype=np.int32), + np.array(c, dtype=np.float64), + np.array(k, dtype=np.int64), + np.arange(len(s), dtype=np.int64), + ) + + +def _bidir(links): + out = [] + for u, v, cap, cost in links: + out.append((u, v, cap, cost)) + out.append((v, u, cap, cost)) + return out + + +@pytest.fixture +def algs(): + return ngc.Algorithms(ngc.Backend.cpu()) + + +class TestMaxFlowCompletion: + """Finding 1: forward-only SSP stranded capacity that needs cancellation.""" + + def _cancellation_graph(self): + # s=0, a=1, b=2, t=3; all links full duplex. True max flow is 3.0: + # 0->1->3 (1), 0->2->1->3 (1), 0->2->3 (1); cut {0,2}|{1,3} has cap 3. + return _graph( + 4, + _bidir( + [ + (1, 3, 3.0, 17), + (0, 2, 8.0, 17), + (1, 2, 1.0, 1), + (1, 0, 1.0, 1), + (3, 2, 1.0, 1), + ] + ), + ) + + def test_exact_max_flow(self, algs): + g = self._cancellation_graph() + pg = algs.build_graph(g) + total, _ = algs.max_flow(pg, 0, 3) + assert total == pytest.approx(3.0) + + def test_min_cut_duality(self, algs): + g = self._cancellation_graph() + pg = algs.build_graph(g) + total, summary = algs.max_flow(pg, 0, 3) + cap = np.asarray(g.capacity_view()) + cut = np.asarray(summary.min_cut.edges) + assert cut.size > 0 + assert float(cap[cut].sum()) == pytest.approx(total) + + def test_cost_distribution_accounts_for_total(self, algs): + g = self._cancellation_graph() + pg = algs.build_graph(g) + total, summary = algs.max_flow(pg, 0, 3) + assert float(np.asarray(summary.flows).sum()) == pytest.approx(total) + + def test_completion_respects_masks(self, algs): + g = self._cancellation_graph() + pg = algs.build_graph(g) + mask = np.ones(4, dtype=bool) + mask[2] = False # only 0->1->3 remains; bottleneck is cap(0->1) = 1 + total, _ = algs.max_flow(pg, 0, 3, node_mask=mask) + assert total == pytest.approx(1.0) + + +class TestZeroCostEdges: + """Finding 2: zero-cost edges made the PredDAG cyclic.""" + + def _zero_cost_graph(self): + # 0 ->(1) 1 <->(0) 2 ->(1) 3 + return _graph( + 4, + [(0, 1, 10.0, 1), (1, 2, 10.0, 0), (2, 1, 10.0, 0), (2, 3, 10.0, 1)], + ) + + def test_pred_dag_acyclic(self, algs): + g = self._zero_cost_graph() + pg = algs.build_graph(g) + _, dag = algs.spf(pg, 0, None, multipath=True) + po = np.asarray(dag.parent_offsets) + pa = np.asarray(dag.parents) + for v in range(4): + for i in range(po[v], po[v + 1]): + u = pa[i] + assert v not in pa[po[u] : po[u + 1]], f"cycle {u}<->{v}" + + def test_equal_balanced_places_flow(self, algs): + g = self._zero_cost_graph() + pg = algs.build_graph(g) + total, _ = algs.max_flow( + pg, 0, 3, flow_placement=ngc.FlowPlacement.EQUAL_BALANCED + ) + assert total == pytest.approx(10.0) + + def test_resolve_to_paths_terminates(self, algs): + g = self._zero_cost_graph() + pg = algs.build_graph(g) + _, dag = algs.spf(pg, 0, None, multipath=True) + paths = dag.resolve_to_paths(0, 3) # previously never returned + assert [n for n, _ in paths[0]] == [0, 1, 2, 3] + + +class TestKspEcmpLadder: + """Finding 3: spur enumeration was exponential in equal-cost path count.""" + + def test_ladder_is_tractable(self, algs): + # 30 stages of parallel 2-hop diamonds: 2^30 equal-cost paths. + # Pre-fix this was unreachable (22 stages already took ~19 s / 3.8 GB). + stages = 30 + edges = [] + nid = 0 + + def new(): + nonlocal nid + nid += 1 + return nid - 1 + + main = [new()] + for _ in range(stages): + nxt, a, b = new(), new(), new() + for u, v in ((main[-1], a), (a, nxt), (main[-1], b), (b, nxt)): + edges.append((u, v, 10.0, 1)) + main.append(nxt) + g = _graph(nid, edges) + pg = algs.build_graph(g) + out = algs.ksp(pg, main[0], main[-1], k=4) + assert len(out) == 4 + # All returned paths are shortest (equal cost) and distinct. + costs = [dist[main[-1]] for dist, _ in out] + assert costs == [2 * stages] * 4 + sigs = {tuple(dag.via_edges) for _, dag in out} + assert len(sigs) == 4 + + +class TestBatchMaxFlowParity: + """Finding 4: batch results must equal per-pair results (now parallel).""" + + def test_batch_equals_serial(self, algs): + rng = np.random.default_rng(7) + n = 12 + edges = [] + for _ in range(40): + u, v = rng.integers(0, n, size=2) + if u != v: + edges.append((int(u), int(v), float(rng.integers(1, 8)), 1)) + g = _graph(n, edges) + pg = algs.build_graph(g) + pairs = np.array([[i, (i + 5) % n] for i in range(8)], dtype=np.int32) + batch = algs.batch_max_flow(pg, pairs, with_edge_flows=True) + for i, (a, b) in enumerate(pairs): + total, summary = algs.max_flow(pg, int(a), int(b), with_edge_flows=True) + assert batch[i].total_flow == pytest.approx(total) + np.testing.assert_array_equal( + np.asarray(batch[i].edge_flows), np.asarray(summary.edge_flows) + ) + + +class TestFlowPolicySrcDstGuard: + """Finding 5: reusing a policy for a different pair corrupted placement.""" + + def test_mismatched_pair_raises(self, algs): + g = _graph(4, [(0, 1, 10.0, 1), (2, 3, 10.0, 1)]) + pg = algs.build_graph(g) + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + placed, _ = policy.place_demand(fg, 0, 1, 0, 5.0) + assert placed == pytest.approx(5.0) + with pytest.raises(ValueError, match="different \\(src, dst\\)"): + policy.place_demand(fg, 2, 3, 0, 5.0) + + def test_same_pair_and_after_remove_ok(self, algs): + g = _graph(4, [(0, 1, 10.0, 1), (2, 3, 10.0, 1)]) + pg = algs.build_graph(g) + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + assert policy.place_demand(fg, 0, 1, 0, 5.0)[0] == pytest.approx(5.0) + assert policy.place_demand(fg, 0, 1, 0, 2.0)[0] == pytest.approx(2.0) + policy.remove_demand(fg) + assert policy.place_demand(fg, 2, 3, 0, 5.0)[0] == pytest.approx(5.0) + np.testing.assert_allclose(np.asarray(fg.edge_flow_view()), [0.0, 5.0]) + + +class TestCostOverflowGuard: + """Finding 6: cost totals near INT64_MAX silently wrapped in SPF.""" + + def test_total_at_2_pow_62_rejected(self): + big = 1 << 62 + with pytest.raises(ValueError, match="2\\^62"): + _graph(3, [(0, 1, 10.0, big), (1, 2, 10.0, big)]) + + def test_large_but_safe_costs_accepted(self, algs): + big = 1 << 60 + g = _graph(3, [(0, 1, 10.0, big), (1, 2, 10.0, big)]) + pg = algs.build_graph(g) + dist, _ = algs.spf(pg, 0, None, dtype="int64") + assert dist[2] == 2 * big # no wraparound + + +class TestFlowStateCustomResidual: + """Finding 7: custom residual_init desynced min-cut's notion of flow.""" + + def test_min_cut_uses_own_placed_flow(self): + g = _graph(3, [(0, 1, 10.0, 1), (1, 2, 10.0, 1)]) + fs = ngc.FlowState(g, np.array([4.0, 4.0])) + placed = fs.place_max_flow(0, 2, ngc.FlowPlacement.PROPORTIONAL) + assert placed == pytest.approx(4.0) + # Reverse arcs must be keyed on this state's own edge_flow (4.0), not + # capacity - residual (10.0, which counts pre-existing usage). + np.testing.assert_allclose(np.asarray(fs.edge_flow_view()), [4.0, 4.0]) + cut = np.asarray(fs.compute_min_cut(0).edges) + assert cut.size > 0 From 4c932fb34e5fd65d1e13f68eeda0bf88188fb1c9 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 14:03:40 +0100 Subject: [PATCH 02/11] Use atomic profile counters for coverage builds 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 --- CMakeLists.txt | 13 ++++++++++++- Makefile | 1 + 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 5dc5f41..2726b19 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -27,7 +27,11 @@ add_executable(netgraph_core_tests target_link_libraries(netgraph_core_tests PRIVATE netgraph_core GTest::gtest_main) target_include_directories(netgraph_core_tests PRIVATE tests/cpp) if(NETGRAPH_CORE_COVERAGE AND (CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang")) + # Atomic counters: see the NETGRAPH_CORE_COVERAGE block below. target_compile_options(netgraph_core_tests PRIVATE -O0 -g --coverage) + if(CMAKE_CXX_COMPILER_ID MATCHES "GNU") + target_compile_options(netgraph_core_tests PRIVATE -fprofile-update=atomic) + endif() target_link_options(netgraph_core_tests PRIVATE --coverage) endif() include(GoogleTest) @@ -206,8 +210,15 @@ endif() if(NETGRAPH_CORE_COVERAGE) message(STATUS "Enabling coverage instrumentation (disables optimizations)") if(CMAKE_CXX_COMPILER_ID MATCHES "GNU|Clang") + # batch_max_flow and sensitivity_analysis run worker threads, and the default + # non-atomic profile counters corrupt under concurrent updates (gcov then + # emits impossible values such as "branch 4 taken -1"). + set(COV_FLAGS -O0 -g --coverage) + if(CMAKE_CXX_COMPILER_ID MATCHES "GNU") + list(APPEND COV_FLAGS -fprofile-update=atomic) + endif() foreach(tgt netgraph_core _netgraph_core) - target_compile_options(${tgt} PRIVATE -O0 -g --coverage) + target_compile_options(${tgt} PRIVATE ${COV_FLAGS}) target_link_options(${tgt} PRIVATE --coverage) endforeach() else() diff --git a/Makefile b/Makefile index 08e2e39..4a9a92c 100644 --- a/Makefile +++ b/Makefile @@ -281,6 +281,7 @@ cov: --object-directory build/cpp-tests-cov \ --filter 'include/netgraph' --filter 'src' --exclude 'tests' --exclude 'bindings/.*' --exclude '.*pybind11.*' --exclude '_deps/pybind11-src/.*' \ --gcov-ignore-errors=all \ + --gcov-ignore-parse-errors=negative_hits.warn_once_per_file \ --xml-pretty -o build/coverage/coverage-cpp.xml @echo "" @echo "================ Python + C++ coverage (summary) ================" From 01236c9d4fc1e538ca22f9903ce6e9f657784a16 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 14:07:47 +0100 Subject: [PATCH 03/11] Fix batch_max_flow rejecting int32 pairs on Windows 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 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 --- bindings/python/module.cpp | 8 +++++- tests/py/test_review_regressions.py | 39 +++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/bindings/python/module.cpp b/bindings/python/module.cpp index 30249d1..19d7dc8 100644 --- a/bindings/python/module.cpp +++ b/bindings/python/module.cpp @@ -267,9 +267,15 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { py::object node_masks, py::object edge_masks, FlowPlacement placement, bool shortest_path, bool require_capacity, bool with_edge_flows, bool with_reachable, bool with_residuals){ + // Check the dtype, not the buffer format string: NumPy spells int32 as + // NPY_LONG ('l') on LLP64 (Windows) and NPY_INT ('i') on LP64, while + // format_descriptor is always 'i'. Comparing format strings + // therefore rejects a genuine int32 array on Windows. isinstance uses + // dtype equivalence, matching as_span() above. + if (!py::isinstance>(pairs)) throw py::type_error("pairs dtype must be int32"); + if (!(pairs.flags() & py::array::c_style)) throw py::type_error("pairs must be C-contiguous (use np.ascontiguousarray)"); auto buf = pairs.request(); if (buf.ndim != 2 || buf.shape[1] != 2) throw py::type_error("pairs must be shape [B,2]"); - if (buf.format != py::format_descriptor::format()) throw py::type_error("pairs dtype must be int32"); const std::size_t B = static_cast(buf.shape[0]); std::vector> pp; pp.reserve(B); auto* p = static_cast(buf.ptr); diff --git a/tests/py/test_review_regressions.py b/tests/py/test_review_regressions.py index 4c230a1..18114f8 100644 --- a/tests/py/test_review_regressions.py +++ b/tests/py/test_review_regressions.py @@ -15,6 +15,8 @@ 6. StrictMultiDiGraph accepted cost totals that overflow int64 path arithmetic. 7. FlowState.compute_min_cut treated pre-existing usage from a custom residual_init as cancellable flow. +8. batch_max_flow rejected a genuine int32 `pairs` array on Windows, and read + non-contiguous input as if it were packed. """ from __future__ import annotations @@ -246,3 +248,40 @@ def test_min_cut_uses_own_placed_flow(self): np.testing.assert_allclose(np.asarray(fs.edge_flow_view()), [4.0, 4.0]) cut = np.asarray(fs.compute_min_cut(0).edges) assert cut.size > 0 + + +class TestBatchPairsDtype: + """Finding 8: `batch_max_flow` rejected a genuine int32 array on Windows. + + The dtype check compared buffer format strings. NumPy spells int32 as + NPY_LONG ('l') on LLP64 (Windows) but NPY_INT ('i') on LP64, while + pybind11's `format_descriptor` is always 'i', so the check threw on + Windows for a correctly-typed array. Parametrizing over both 32-bit + spellings exercises whichever one is the platform's alias for int32. + """ + + def _line_graph(self): + return _graph(3, [(0, 1, 5.0, 1), (1, 2, 5.0, 1)]) + + @pytest.mark.parametrize("dtype", [np.int32, np.intc]) + def test_accepts_every_int32_spelling(self, algs, dtype): + g = self._line_graph() + pg = algs.build_graph(g) + pairs = np.array([[0, 2]], dtype=dtype) + assert np.dtype(dtype).itemsize == 4 and np.dtype(dtype).kind == "i" + out = algs.batch_max_flow(pg, pairs) + assert out[0].total_flow == pytest.approx(5.0) + + def test_rejects_wrong_dtype(self, algs): + pg = algs.build_graph(self._line_graph()) + with pytest.raises(TypeError, match="int32"): + algs.batch_max_flow(pg, np.array([[0, 2]], dtype=np.int64)) + + def test_rejects_non_contiguous(self, algs): + # A strided view would otherwise be read as if it were packed, silently + # turning [[0, 2]] into the pair (0, 99). + pg = algs.build_graph(self._line_graph()) + strided = np.array([[0, 99, 2, 99]], dtype=np.int32)[:, ::2] + assert not strided.flags["C_CONTIGUOUS"] + with pytest.raises(TypeError, match="C-contiguous"): + algs.batch_max_flow(pg, strided) From 9aa2b79ac0570ee6abac0fc43cb15b414ece57b6 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 14:11:05 +0100 Subject: [PATCH 04/11] Document Windows dtype and coverage build fixes in changelog Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 ++ 1 file changed, 2 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 54b2106..31ff9c2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,7 +15,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Flow Policy**: `place_demand` silently routed a second `(src, dst)` pair over the first pair's paths. It now raises `invalid_argument`; use one policy per demand or call `remove_demand()` first. - **Graph Construction**: `from_arrays` now rejects a total edge cost at or above 2^62, which overflows the int64 path arithmetic in SPF and silently corrupts results. - **Flow State**: `compute_min_cut` and max-flow reachability derived placed flow from `capacity - residual`, overstating it for a `FlowState` built with a custom `residual_init`. Both now use `edge_flow`. +- **Python Bindings**: `batch_max_flow` rejected a valid int32 `pairs` array on Windows, because the dtype check compared buffer format strings and NumPy spells int32 as `NPY_LONG` on LLP64 but `NPY_INT` on LP64. It now compares dtype equivalence, and rejects non-contiguous `pairs` rather than reading them as if packed. - **Build**: `NETGRAPH_CORE_SANITIZE` passed its flags as one quoted string, so sanitizer builds never compiled, and the test target was missing them entirely. `make sanitize-test` also no longer sets `detect_leaks=1` on macOS, where it aborts. +- **Build**: Coverage builds now use `-fprofile-update=atomic` on GCC; the default non-atomic counters corrupt under the threads used by `batch_max_flow` and `sensitivity_analysis`. ### Changed From bbbb2b4a55dc20a3e987d79d3567f59d88c978c2 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 15:30:59 +0100 Subject: [PATCH 05/11] Fix API/doc accuracy and remaining hot-path allocations 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 --- CHANGELOG.md | 11 +- CONTRIBUTING.md | 2 +- README.md | 12 +- bindings/python/module.cpp | 37 +++- include/netgraph/core/backend.hpp | 5 +- include/netgraph/core/flow_policy.hpp | 17 +- include/netgraph/core/flow_state.hpp | 3 + include/netgraph/core/max_flow.hpp | 21 ++- include/netgraph/core/shortest_paths.hpp | 14 +- include/netgraph/core/strict_multidigraph.hpp | 12 ++ python/netgraph_core/__init__.py | 14 ++ python/netgraph_core/_docs.py | 167 ++++++++++++++---- src/flow_policy.cpp | 62 +++++-- src/flow_state.cpp | 105 +++++++---- src/max_flow.cpp | 19 +- tests/py/test_review_regressions.py | 113 ++++++++++++ 16 files changed, 502 insertions(+), 112 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 31ff9c2..05cf27b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,7 +22,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed - **Shortest Paths**: Replaced nested per-node predecessor vectors with flat intrusive lists, removing the allocation churn that dominated the hot path (~3-4x faster; output unchanged). -- **Max-Flow**: Parallelized `batch_max_flow` across source/destination pairs using `std::async`; thread count controlled by `NGRAPH_CORE_BATCH_THREADS` env or hardware concurrency. +- **Flow Placement**: `place_on_dag` now rebuilds its `(parent, child)` edge groups into a reused arena instead of nested per-node vectors, which was ~70% of its runtime (~2-2.4x faster; output unchanged). +- **Max-Flow**: Parallelized `batch_max_flow` across source/destination pairs using `std::async`; workers claim pairs from a shared counter, so a batch whose costs are unevenly distributed still parallelizes. Thread count from `NGRAPH_CORE_BATCH_THREADS` env or hardware concurrency. +- **Flow Policy**: `place_demand` computes one SPF when seeding initial flows instead of repeating an identical one per flow; seeding cost no longer scales with `min_flow_count`. +- **Python Bindings**: A wrong-typed `graph`/`algorithms` argument now raises `TypeError` instead of an opaque `RuntimeError: Unable to cast ... to C++ type '?'`, `Algorithms.spf` raises `TypeError` for a wrong `residual` length (matching every other length check), `Algorithms.ksp` validates `dtype` before running, and `batch_max_flow` rejects out-of-range node ids like the single-pair entry points. +- **Python Typing**: Corrected `_docs.py` (pybind11 enums and classes are not `enum.Enum`/`dataclass`, `MinCut.edges` is an `int32` array, removed a `Path` class that never existed) and widened the type-checking re-exports from 6 names to all 16, so `Algorithms`, `FlowPolicy`, `StrictMultiDiGraph` and friends are no longer `Unknown` to type checkers despite the shipped `py.typed`. +- **Docs**: Corrected README/CONTRIBUTING (required CMake is 3.23 not 3.15, `make py-test` does not exist, `make test` does not collect coverage), the `sensitivity_analysis` description in README and `backend.hpp` (it measures flow *lost* on edge removal, not gain from relaxing capacity), the zero-copy and GIL claims, and header contracts for `from_arrays`, `PredDAG`, `FlowSummary.costs`, `calc_max_flow` and `FlowPolicy`. + +### Removed + +- **Python Bindings**: The unreachable `Flow` class (bound from `FlowRecord`). No binding constructed or returned one, it was absent from `__all__`, and the name collided with the C++ alias `using Flow = double`. ## [0.7.2] - 2026-03-26 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 0fdc19e..415c708 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -47,7 +47,7 @@ Run specific test suites: ```bash make cpp-test # C++ tests (GoogleTest) -make py-test # Python tests (pytest) +make test # Python tests (pytest) ``` ### Code Style diff --git a/README.md b/README.md index 7ec5a36..8088c06 100644 --- a/README.md +++ b/README.md @@ -29,12 +29,12 @@ NetGraph-Core provides a specialized graph implementation for networking problem - Configurable constraints on cost factors (e.g., paths within 1.5x of optimal). - **Max-Flow**: - - **Algorithm**: Iterative augmentation using Successive Shortest Path on residual graphs, pushing flow across full ECMP/WCMP DAGs at each step. + - **Algorithm**: Iterative augmentation using Successive Shortest Path on residual graphs, pushing flow across full ECMP/WCMP DAGs at each step. For `Proportional` placement with `require_capacity=True`, a final residual completion phase augments with reverse arcs so the result is a true maximum flow; the other modes are placement models and may report less. - **Traffic Engineering (TE) Mode**: Routing adapts to residual capacity (progressive fill). - **IP Routing Mode**: Cost-only routing (ECMP/WCMP) ignoring capacity constraints. - **Analysis**: - - **Sensitivity Analysis**: Identifies bottleneck edges where capacity relaxation increases total flow. Supports `shortest_path` mode to analyze only edges used under ECMP routing (IP/IGP networks) vs. full max-flow (SDN/TE networks). + - **Sensitivity Analysis**: Identifies critical edges by removing each saturated edge and measuring how much total flow is *lost*. Supports `shortest_path` mode to analyze only edges used under ECMP routing (IP/IGP networks) vs. full max-flow (SDN/TE networks). - **Min-Cut**: Computes minimum cuts on residual graphs. ### 3. Flow Policy Engine @@ -49,8 +49,8 @@ Unified configuration object (`FlowPolicy`) that models diverse routing behavior ### 4. Python Integration -- **Zero-Copy**: Exposes C++ internal buffers to Python as read-only NumPy arrays (float64/int64). -- **Concurrency**: Releases the Python GIL during graph algorithms to enable threading. +- **Zero-Copy**: `FlowState` and `FlowGraph` `*_view()` methods expose C++ buffers as read-only NumPy arrays that track mutation in place. `StrictMultiDiGraph.*_view()` returns copies instead, so the graph's immutability cannot be violated. +- **Concurrency**: Releases the Python GIL during the long-running algorithms (SPF, KSP, max-flow, placement) to enable threading. `PredDAG.resolve_to_paths` holds the GIL. `batch_max_flow` and `sensitivity_analysis` also use internal worker threads, sized by `NGRAPH_CORE_BATCH_THREADS` / `NGRAPH_CORE_SENSITIVITY_THREADS`. ## Installation @@ -89,7 +89,7 @@ tests/py/ # Python tests (pytest) make dev # Setup: venv, dependencies, pre-commit hooks make check # Run all tests and linting (auto-fix formatting) make check-ci # Strict checks without auto-fix (for CI) -make test # Python tests with coverage +make test # Python tests make cpp-test # C++ tests only make cov # Combined coverage report (C++ + Python) ``` @@ -98,7 +98,7 @@ make cov # Combined coverage report (C++ + Python) - **C++:** C++20 compiler (GCC 10+, Clang 12+, MSVC 2019+) - **Python:** 3.11+ -- **Build:** CMake 3.15+, scikit-build-core +- **Build:** CMake 3.23+, scikit-build-core - **Dependencies:** pybind11, NumPy ## License diff --git a/bindings/python/module.cpp b/bindings/python/module.cpp index 19d7dc8..c951840 100644 --- a/bindings/python/module.cpp +++ b/bindings/python/module.cpp @@ -160,8 +160,14 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { .def_property_readonly("num_edges", [](const PyGraph& pg){ return pg.num_edges; }); py::class_>(m, "Algorithms") - .def(py::init([](const PyBackend& b){ return std::make_shared(b.impl); })) + .def(py::init([](const PyBackend& b){ return std::make_shared(b.impl); }), py::arg("backend")) .def("build_graph", [](const Algorithms& algs, py::object graph_obj){ + // graph_obj is py::object so PyGraph can keep it alive; check the type + // explicitly, since a bare .cast<> failure surfaces as an opaque + // "RuntimeError: Unable to cast ... to C++ type '?'". + if (!py::isinstance(graph_obj)) { + throw py::type_error("build_graph: graph must be a StrictMultiDiGraph"); + } const StrictMultiDiGraph& g = graph_obj.cast(); auto gh = algs.build_graph(g); // Keep the Python graph object alive alongside the handle when @@ -199,6 +205,12 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { if (!(arr.flags() & py::array::c_style)) throw py::type_error("residual must be C-contiguous (np.ascontiguousarray)"); auto buf = arr.request(); if (buf.ndim != 1 || buf.format != py::format_descriptor::format()) throw py::type_error("residual must be 1-D float64"); + // Check length here so this matches the TypeError raised by every other + // length check (masks, FlowState residual); otherwise the core throws + // std::invalid_argument, which reaches Python as ValueError. + if (static_cast(buf.shape[0]) != pg.num_edges) { + throw py::type_error("residual length must equal " + std::to_string(pg.num_edges)); + } residual_vec.resize(static_cast(buf.shape[0])); std::memcpy(residual_vec.data(), buf.ptr, residual_vec.size()*sizeof(double)); opts.residual = std::span(residual_vec.data(), residual_vec.size()); @@ -228,6 +240,7 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { int k, py::object max_cost_factor, bool unique, py::object node_mask, py::object edge_mask, std::string dtype){ if (src < 0 || src >= pg.num_nodes || dst < 0 || dst >= pg.num_nodes) throw py::value_error("src/dst out of range"); if (k <= 0) throw py::value_error("k must be >= 1"); + if (dtype != "float64" && dtype != "int64") throw py::value_error("dtype must be 'float64' or 'int64'"); KspOptions opts; opts.k = k; opts.unique = unique; if (!max_cost_factor.is_none()) opts.max_cost_factor = py::cast(max_cost_factor); auto node_bs = to_bool_span_from_numpy(node_mask, static_cast(pg.num_nodes), "node_mask"); auto edge_bs = to_bool_span_from_numpy(edge_mask, static_cast(pg.num_edges), "edge_mask"); @@ -280,6 +293,14 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { std::vector> pp; pp.reserve(B); auto* p = static_cast(buf.ptr); for (std::size_t i=0;i= pg.num_nodes || + pp[i].second < 0 || pp[i].second >= pg.num_nodes) { + throw py::value_error("pairs[" + std::to_string(i) + "] has a src/dst out of range"); + } + } std::vector node_bufs, edge_bufs; std::vector> node_spans; std::vector> edge_spans; auto parse_mask_list = [&](py::object list_obj, std::size_t expected_len, const char* what, @@ -484,12 +505,11 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { .def("get_flow_edges", [](const FlowGraph& fg, const FlowIndex& idx){ auto v = fg.get_flow_edges(idx); py::list out; for (auto const& pr : v) { out.append(py::make_tuple(pr.first, pr.second)); } return out; }) .def("get_flow_path", [](const FlowGraph& fg, const FlowIndex& idx){ auto v = fg.get_flow_path(idx); py::list out; for (auto e : v) out.append(e); return out; }); - py::class_(m, "Flow") - .def_property_readonly("index", [](const FlowRecord& f){ return f.index; }) - .def_readonly("src", &FlowRecord::src) - .def_readonly("dst", &FlowRecord::dst) - .def_readonly("cost", &FlowRecord::cost) - .def_readonly("placed_flow", &FlowRecord::placed_flow); + // NOTE: FlowRecord is deliberately not bound. No binding constructs or returns one + // (FlowPolicy.flows yields plain tuples), so the class was unreachable, and the + // Python name "Flow" collided with the C++ alias `using Flow = double`. If per-flow + // records become part of the public API, bind it under an unambiguous name such as + // "FlowRecord" and have FlowPolicy.flows return it instead of tuples. py::enum_(m, "PathAlg") .value("SPF", PathAlg::SPF); @@ -568,6 +588,9 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { py::class_(m, "FlowPolicy") .def("__init__", [](FlowPolicy& self, py::object algs_obj, const PyGraph& pg, FlowPolicyConfig cfg, py::object node_mask, py::object edge_mask){ + if (!py::isinstance(algs_obj)) { + throw py::type_error("FlowPolicy: algorithms must be an Algorithms instance"); + } std::shared_ptr algs = py::cast>(algs_obj); auto node_bs = to_bool_span_from_numpy(node_mask, static_cast(pg.num_nodes), "node_mask"); diff --git a/include/netgraph/core/backend.hpp b/include/netgraph/core/backend.hpp index 8ad21b6..c1c295d 100644 --- a/include/netgraph/core/backend.hpp +++ b/include/netgraph/core/backend.hpp @@ -121,8 +121,9 @@ class Backend { // opts: Configuration options. // // Returns: - // A vector of pairs (EdgeId, Flow gain), indicating how much flow would - // increase if the edge's capacity were relaxed. + // A vector of pairs (EdgeId, FlowDelta) for edges whose removal reduces + // total flow, where FlowDelta is how much flow is LOST without that edge. + // This is a criticality measure, not the gain from relaxing capacity. [[nodiscard]] virtual std::vector> sensitivity_analysis( const GraphHandle& gh, NodeId src, NodeId dst, const MaxFlowOptions& opts) = 0; }; diff --git a/include/netgraph/core/flow_policy.hpp b/include/netgraph/core/flow_policy.hpp index 8e9c4e4..3f1652f 100644 --- a/include/netgraph/core/flow_policy.hpp +++ b/include/netgraph/core/flow_policy.hpp @@ -97,7 +97,17 @@ class FlowPolicy { [[nodiscard]] int flow_count() const noexcept { return static_cast(flows_.size()); } [[nodiscard]] double placed_demand() const noexcept; - // Core operations + // Core operations. + // + // A FlowPolicy manages the flows of a SINGLE demand. Once it holds flows, calling + // place_demand() with a different (src, dst) throws std::invalid_argument, because + // the round-robin loop routes using the src/dst stored on the existing flow records. + // Use one policy per demand, or call remove_demand() first to retarget it. + // + // `fg` must wrap the same StrictMultiDiGraph as the policy's own graph handle; + // placing onto a FlowGraph built from a different graph is rejected. + // + // Returns (total_placed, remaining_volume). [[nodiscard]] std::pair place_demand(FlowGraph& fg, NodeId src, NodeId dst, FlowClass flowClass, @@ -105,6 +115,8 @@ class FlowPolicy { std::optional target_per_flow = std::nullopt, std::optional min_flow = std::nullopt); + // Re-places the currently placed volume so each flow carries ~target_per_flow. + // (src, dst) must match the demand this policy already manages. [[nodiscard]] std::pair rebalance_demand(FlowGraph& fg, NodeId src, NodeId dst, FlowClass flowClass, @@ -120,6 +132,9 @@ class FlowPolicy { private: // Helpers + // Throws std::invalid_argument if fg wraps a different graph than this policy, or if + // (src, dst) differs from the demand already managed here. `what` names the caller. + void check_demand_target(const FlowGraph& fg, NodeId src, NodeId dst, const char* what) const; [[nodiscard]] std::optional> get_path_bundle(const FlowGraph& fg, NodeId src, NodeId dst, std::optional min_flow); diff --git a/include/netgraph/core/flow_state.hpp b/include/netgraph/core/flow_state.hpp index afd510c..b8b5339 100644 --- a/include/netgraph/core/flow_state.hpp +++ b/include/netgraph/core/flow_state.hpp @@ -90,6 +90,9 @@ class FlowState { // Apply or revert a set of edge flow allocations directly. // When add==true, treats each (eid, flow) as additional placed flow on the edge. // When add==false, removes placed flow (reverts allocations), clamping to [0, capacity]. + // NOTE: residual is recomputed as capacity - edge_flow, so for a FlowState built with + // a custom residual_init this discards the initial offset. Entries with flow <= 0 and + // out-of-range edge ids are ignored. void apply_deltas(std::span> deltas, bool add) noexcept; private: diff --git a/include/netgraph/core/max_flow.hpp b/include/netgraph/core/max_flow.hpp index 08e8391..3497e6c 100644 --- a/include/netgraph/core/max_flow.hpp +++ b/include/netgraph/core/max_flow.hpp @@ -16,8 +16,14 @@ struct MinCut { std::vector edges; }; struct FlowSummary { Flow total_flow {0.0}; MinCut min_cut {}; - // Parallel arrays: costs[i] has placed flow flows[i] at that total path cost. - // Costs are ascending and unique; flows are raw volumes (not normalized). + // Parallel arrays: costs[i] carries placed flow flows[i]. Costs are ascending and + // unique; flows are raw volumes (not normalized). + // + // Entries from the SPF tier loop are the total cost of the shortest-path DAG used. + // Entries produced by the residual completion phase are MARGINAL costs: the cost of + // the augmenting path's forward edges minus the cost of the flow it cancelled. Such + // an entry need not correspond to any traversable s-t path, so treat costs[] as a + // cost-weighted breakdown of the total flow rather than a list of path costs. std::vector costs; std::vector flows; std::vector edge_flows; // filled if requested @@ -26,6 +32,17 @@ struct FlowSummary { std::vector reachable_nodes; // length == g.num_nodes(); 0/1 flags }; +// Computes maximum flow from src to dst. +// +// The SPF tier loop augments along forward shortest-path DAGs only. For +// FlowPlacement::Proportional with require_capacity=true and shortest_path=false a +// residual completion phase then augments with reverse arcs, so the result is a true +// maximum flow and summary.min_cut satisfies max-flow/min-cut duality. +// +// The other configurations are placement models rather than max-flow computations and +// may report less than the maximum: EqualBalanced models single-pass ECMP admission, +// require_capacity=false models fixed cost-only IP routing, and shortest_path=true +// restricts flow to the first cost tier. [[nodiscard]] std::pair calc_max_flow(const StrictMultiDiGraph& g, NodeId src, NodeId dst, FlowPlacement placement, bool shortest_path, diff --git a/include/netgraph/core/shortest_paths.hpp b/include/netgraph/core/shortest_paths.hpp index 47924f9..fad97cb 100644 --- a/include/netgraph/core/shortest_paths.hpp +++ b/include/netgraph/core/shortest_paths.hpp @@ -11,9 +11,16 @@ namespace netgraph::core { -// PredDAG (Predecessor Directed Acyclic Graph): compact representation of all equal-cost +// PredDAG (Predecessor Directed Acyclic Graph): compact representation of the equal-cost // shortest paths from a source node. Stored in CSR format for efficiency. // +// The DAG is always acyclic. With strictly positive edge costs it captures *every* +// equal-cost shortest path. Zero-cost edges are the one exception: a predecessor is +// recorded only while the child is still unsettled, so among nodes that are mutually +// reachable at equal distance the settle order decides which alternatives are kept. +// Without that rule a zero-cost pair u<->v would record each node as the other's +// parent, making the structure cyclic and path enumeration non-terminating. +// // For each node v, predecessors are stored in parents[parent_offsets[v]:parent_offsets[v+1]] // with corresponding EdgeIds in via_edges[parent_offsets[v]:parent_offsets[v+1]]. // Multiple parallel edges are represented by multiple entries with the same parent. @@ -55,7 +62,10 @@ shortest_paths(const StrictMultiDiGraph& g, NodeId src, // Enumerate concrete paths represented by a PredDAG from src to dst. // Each path is returned as a sequence of (node_id, (edge_ids...)) pairs ending with (dst, ()). // When split_parallel_edges=false, parallel edges per hop are grouped in the tuple. -// When true, one edge per hop is selected to produce concrete paths; enumeration may be capped with max_paths. +// When true, the cartesian product over parallel edges is enumerated, yielding one +// concrete single-edge path per combination; cap it with max_paths. +// Enumeration is exponential in the DAG's branching, so pass max_paths on wide ECMP DAGs. +// NOTE: this function does not release the Python GIL when called through the bindings. [[nodiscard]] std::vector>>> resolve_to_paths(const PredDAG& dag, NodeId src, NodeId dst, bool split_parallel_edges = false, diff --git a/include/netgraph/core/strict_multidigraph.hpp b/include/netgraph/core/strict_multidigraph.hpp index deb2890..5f14498 100644 --- a/include/netgraph/core/strict_multidigraph.hpp +++ b/include/netgraph/core/strict_multidigraph.hpp @@ -18,6 +18,18 @@ namespace netgraph::core { class StrictMultiDiGraph { public: + // Builds a graph from parallel edge arrays, which must all have the same length. + // Edges are reordered by (cost, src, dst); ext_edge_ids is permuted alongside them. + // Self-loops and duplicate (src, dst) pairs are permitted (this is a multigraph). + // + // Throws std::invalid_argument or std::out_of_range if: + // - the arrays differ in length, or ext_edge_ids is neither empty nor that length + // - num_nodes < 0, or any src/dst falls outside [0, num_nodes) + // - any capacity < 0, or any cost < 0 + // - the number of edges exceeds INT32_MAX + // - the TOTAL of all edge costs reaches 2^62. SPF accumulates path costs as int64 + // and uses INT64_MAX as the unreachable sentinel, so a larger total could wrap + // negative and silently corrupt results. [[nodiscard]] static StrictMultiDiGraph from_arrays( std::int32_t num_nodes, std::span src, diff --git a/python/netgraph_core/__init__.py b/python/netgraph_core/__init__.py index f8180ff..a4814cf 100644 --- a/python/netgraph_core/__init__.py +++ b/python/netgraph_core/__init__.py @@ -34,17 +34,31 @@ __version__ = version("netgraph-core") # Provide richer type information for editors/type-checkers without affecting runtime. +# Every name here must also appear in the runtime import above; the aliases only +# change what a type checker sees. Keep this list in sync with _docs.py, and keep +# _docs.py in sync with the bindings -- a wrong stub is worse than a missing one, +# because it turns "unchecked" into "confidently wrong". try: # pragma: no cover - typing-only import from typing import TYPE_CHECKING as _TYPE_CHECKING if _TYPE_CHECKING: # noqa: SIM108 from ._docs import ( # noqa: I001 + Algorithms as Algorithms, + Backend as Backend, EdgeSelection as EdgeSelection, EdgeTieBreak as EdgeTieBreak, + FlowGraph as FlowGraph, + FlowIndex as FlowIndex, FlowPlacement as FlowPlacement, + FlowPolicy as FlowPolicy, + FlowPolicyConfig as FlowPolicyConfig, + FlowState as FlowState, FlowSummary as FlowSummary, + Graph as Graph, MinCut as MinCut, + PathAlg as PathAlg, PredDAG as PredDAG, + StrictMultiDiGraph as StrictMultiDiGraph, ) except ImportError: # Safe fallback if _docs.py changes; runtime bindings above remain authoritative. diff --git a/python/netgraph_core/_docs.py b/python/netgraph_core/_docs.py index 06dc98d..eec30b6 100644 --- a/python/netgraph_core/_docs.py +++ b/python/netgraph_core/_docs.py @@ -9,9 +9,7 @@ from __future__ import annotations -from dataclasses import dataclass -from enum import Enum -from typing import TYPE_CHECKING, Optional +from typing import TYPE_CHECKING, ClassVar, Optional if TYPE_CHECKING: # only for typing; runtime comes from extension import numpy as np # type: ignore[reportMissingImports] @@ -19,19 +17,36 @@ # to prevent circular imports during type checking. -class EdgeTieBreak(Enum): - DETERMINISTIC = 1 - PREFER_HIGHER_RESIDUAL = 2 +class EdgeTieBreak: + """Tie-break rule among equal-cost parallel edges (pybind11 enum).""" + + DETERMINISTIC: ClassVar[EdgeTieBreak] + PREFER_HIGHER_RESIDUAL: ClassVar[EdgeTieBreak] + __members__: ClassVar[dict[str, EdgeTieBreak]] + + @property + def name(self) -> str: ... + @property + def value(self) -> int: ... -@dataclass class EdgeSelection: - multi_edge: bool = True - require_capacity: bool = False - tie_break: EdgeTieBreak = EdgeTieBreak.DETERMINISTIC + """Edge selection policy. The constructor is keyword-only.""" + multi_edge: bool + require_capacity: bool + tie_break: EdgeTieBreak -class FlowPlacement(Enum): + def __init__( + self, + *, + multi_edge: bool = True, + require_capacity: bool = False, + tie_break: EdgeTieBreak = ..., + ) -> None: ... + + +class FlowPlacement: """How to place flow across equal-cost predecessors during augmentation. PROPORTIONAL (WCMP-like): Distributes flow proportionally to available capacity. @@ -44,8 +59,14 @@ class FlowPlacement(Enum): ECMP = Equal-Cost Multi-Path; WCMP = Weighted-Cost Multi-Path. """ - PROPORTIONAL = 1 - EQUAL_BALANCED = 2 + PROPORTIONAL: ClassVar[FlowPlacement] + EQUAL_BALANCED: ClassVar[FlowPlacement] + __members__: ClassVar[dict[str, FlowPlacement]] + + @property + def name(self) -> str: ... + @property + def value(self) -> int: ... class PredDAG: @@ -68,8 +89,14 @@ def resolve_to_paths( ) -> list[tuple[tuple[int, tuple[int, ...]], ...]]: ... -class PathAlg(Enum): - SPF = 1 +class PathAlg: + SPF: ClassVar[PathAlg] + __members__: ClassVar[dict[str, PathAlg]] + + @property + def name(self) -> str: ... + @property + def value(self) -> int: ... class Backend: @@ -118,7 +145,7 @@ def place( dst: int, dag: "PredDAG", amount: float, - flow_placement: FlowPlacement = FlowPlacement.PROPORTIONAL, + flow_placement: FlowPlacement = ..., ) -> float: ... def remove(self, index: "FlowIndex") -> None: ... @@ -187,7 +214,7 @@ def place_on_dag( dst: int, dag: "PredDAG", requested_flow: float = float("inf"), - flow_placement: FlowPlacement = FlowPlacement.PROPORTIONAL, + flow_placement: FlowPlacement = ..., ) -> float: """Place flow along a predecessor DAG. @@ -205,7 +232,7 @@ def place_max_flow( self, src: int, dst: int, - flow_placement: FlowPlacement = FlowPlacement.PROPORTIONAL, + flow_placement: FlowPlacement = ..., shortest_path: bool = False, require_capacity: bool = True, *, @@ -256,9 +283,89 @@ def compute_min_cut( class StrictMultiDiGraph: - """Opaque graph structure provided by the runtime extension (typing stub only).""" + """Immutable directed multigraph with CSR and reverse-CSR adjacency. - ... + Edges are reordered by (cost, src, dst) at construction, so EdgeId values are + positions in that canonical order rather than the caller's input order. Use + `ext_edge_ids` to map results back to caller-side identifiers. + + Every `*_view()` method returns a fresh, writable COPY of the internal buffer, + so mutating the result cannot violate the graph's immutability. (This differs + from `FlowState`/`FlowGraph` views, which are live read-only views.) + """ + + @staticmethod + def from_arrays( + num_nodes: int, + src: "np.ndarray", + dst: "np.ndarray", + capacity: "np.ndarray", + cost: "np.ndarray", + ext_edge_ids: "np.ndarray", + ) -> "StrictMultiDiGraph": + """Build a graph from parallel edge arrays (all must be 1-D, C-contiguous). + + Args: + num_nodes: Node count; every src/dst must lie in [0, num_nodes). + src: int32[E] source node ids. + dst: int32[E] destination node ids. + capacity: float64[E] capacities, each >= 0. + cost: int64[E] costs, each >= 0. Their TOTAL must stay below 2**62, + since SPF accumulates path costs as int64. + ext_edge_ids: int64[E] caller-side ids, permuted alongside the edges. + + Raises: + TypeError: On a wrong dtype, a non-contiguous array, or a wrong shape. + ValueError: On a negative capacity/cost, a node id out of range, a + length mismatch, or a total cost at or above 2**62. + """ + ... + + def num_nodes(self) -> int: ... + def num_edges(self) -> int: ... + def capacity_view(self) -> "np.ndarray": + """Copy of per-edge capacities, float64[E].""" + ... + + def cost_view(self) -> "np.ndarray": + """Copy of per-edge costs, int64[E].""" + ... + + def edge_src_view(self) -> "np.ndarray": + """Copy of per-edge source node ids, int32[E].""" + ... + + def edge_dst_view(self) -> "np.ndarray": + """Copy of per-edge destination node ids, int32[E].""" + ... + + def ext_edge_ids_view(self) -> "np.ndarray": + """Copy of caller-supplied external edge ids, int64[E].""" + ... + + def row_offsets_view(self) -> "np.ndarray": + """CSR row offsets over outgoing edges, int32[num_nodes + 1].""" + ... + + def col_indices_view(self) -> "np.ndarray": + """CSR neighbour node ids for outgoing edges, int32[E].""" + ... + + def adj_edge_index_view(self) -> "np.ndarray": + """EdgeId for each CSR outgoing entry, int32[E].""" + ... + + def in_row_offsets_view(self) -> "np.ndarray": + """Reverse-CSR row offsets over incoming edges, int32[num_nodes + 1].""" + ... + + def in_col_indices_view(self) -> "np.ndarray": + """Reverse-CSR predecessor node ids, int32[E].""" + ... + + def in_adj_edge_index_view(self) -> "np.ndarray": + """EdgeId for each reverse-CSR entry, int32[E].""" + ... class FlowIndex: @@ -355,18 +462,10 @@ def remove_demand(self, flow_graph: "FlowGraph") -> None: ... def flows(self) -> dict[tuple[int, int, int, int], tuple[int, int, int, float]]: ... -@dataclass(frozen=True) -class Path: - nodes: np.ndarray - edges: np.ndarray - cost: float - - class MinCut: - edges: list[int] + edges: "np.ndarray" # int32[K], copy of the cut edge ids -@dataclass(frozen=True) class FlowSummary: total_flow: float min_cut: MinCut @@ -451,7 +550,9 @@ def spf( as no traversal can begin from an excluded source. Raises: - TypeError: If arrays have wrong dtype, ndim, or length. + TypeError: If arrays have wrong dtype, ndim, or length + (residual must have length num_edges; masks must match + num_nodes / num_edges). ValueError: If src/dst out of range. """ ... @@ -491,7 +592,7 @@ def max_flow( src: int, dst: int, *, - flow_placement: FlowPlacement = FlowPlacement.PROPORTIONAL, + flow_placement: FlowPlacement = ..., shortest_path: bool = False, require_capacity: bool = True, with_edge_flows: bool = False, @@ -540,7 +641,7 @@ def batch_max_flow( *, node_masks: Optional[list["np.ndarray"]] = None, edge_masks: Optional[list["np.ndarray"]] = None, - flow_placement: FlowPlacement = FlowPlacement.PROPORTIONAL, + flow_placement: FlowPlacement = ..., shortest_path: bool = False, require_capacity: bool = True, with_edge_flows: bool = False, @@ -566,7 +667,7 @@ def sensitivity_analysis( src: int, dst: int, *, - flow_placement: FlowPlacement = FlowPlacement.PROPORTIONAL, + flow_placement: FlowPlacement = ..., shortest_path: bool = False, require_capacity: bool = True, node_mask: Optional["np.ndarray"] = None, diff --git a/src/flow_policy.cpp b/src/flow_policy.cpp index 00958ba..8ea1072 100644 --- a/src/flow_policy.cpp +++ b/src/flow_policy.cpp @@ -26,6 +26,31 @@ namespace netgraph::core { +/* Reject uses that would silently produce a wrong answer: + - a FlowGraph wrapping a different graph than the policy routes on (SPF would run + on one topology while flow is placed on another); + - a (src, dst) pair different from the demand this policy already manages (the + round-robin loop routes using src/dst from the existing flow records). */ +void FlowPolicy::check_demand_target(const FlowGraph& fg, NodeId src, NodeId dst, + const char* what) const { + const auto* policy_graph = ctx_.graph.graph.get(); + if (policy_graph != nullptr && policy_graph != &fg.graph()) { + throw std::invalid_argument( + std::string("FlowPolicy::") + what + + ": the FlowGraph wraps a different StrictMultiDiGraph than this policy; " + "paths would be selected on one topology and placed on another"); + } + if (!flows_.empty()) { + const auto& existing = flows_.begin()->second; + if (existing.src != src || existing.dst != dst) { + throw std::invalid_argument( + std::string("FlowPolicy::") + what + + ": this policy already manages a demand for a different (src, dst) pair; " + "use a separate FlowPolicy per demand or call remove_demand() first"); + } + } +} + double FlowPolicy::placed_demand() const noexcept { double s = 0.0; for (auto const& kv : flows_) s += kv.second.placed_flow; @@ -202,19 +227,7 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, std::optional min_flow) { NGRAPH_PROFILE_SCOPE("place_demand"); - // A FlowPolicy manages flows for a single demand. Placing a different - // (src, dst) pair on a policy that already holds flows would silently route - // the new volume over the previous pair's paths (the round-robin loop reads - // src/dst from the existing flow records), so reject it loudly. - if (!flows_.empty()) { - const auto& existing = flows_.begin()->second; - if (existing.src != src || existing.dst != dst) { - throw std::invalid_argument( - "FlowPolicy::place_demand: this policy already manages a demand for a " - "different (src, dst) pair; use a separate FlowPolicy per demand or " - "call remove_demand() first"); - } - } + check_demand_target(fg, src, dst, "place_demand"); // Compute target flow per flow-record. // target: the volume to place per flow (or globally if target_per_flow is unset). @@ -268,11 +281,20 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, if (max_flow_count_.has_value()) { initial = std::min(initial, *max_flow_count_); } - for (int i=0;i(per_target) - : min_flow; - [[maybe_unused]] auto* created = create_flow(fg, src, dst, flowClass, min_req); + auto min_req = (flow_placement_ == FlowPlacement::EqualBalanced && max_flow_count_.has_value()) + ? std::optional(per_target) + : min_flow; + // Seeding places no flow, so residuals do not change between iterations and + // every create_flow() here would recompute the identical SPF. Compute the + // bundle once and copy it into each seeded flow. + if (initial > 0) { + if (auto pb = get_path_bundle(fg, src, dst, min_req)) { + for (int i = 0; i < initial; ++i) { + FlowIndex idx{src, dst, flowClass, next_flow_id_++}; + FlowRecord f(idx, src, dst, pb->first, pb->second); + flows_.emplace(idx, std::move(f)); + } + } } } } @@ -403,6 +425,10 @@ std::pair FlowPolicy::rebalance_demand(FlowGraph& fg, NodeId src, NodeId dst, FlowClass flowClass, double target_per_flow) { + // Must run before remove_demand() empties flows_, or the check inside + // place_demand() would have nothing left to compare against and would + // silently retarget this policy's volume onto a different node pair. + check_demand_target(fg, src, dst, "rebalance_demand"); double vol = placed_demand(); remove_demand(fg); return place_demand(fg, src, dst, flowClass, vol, target_per_flow, std::nullopt); diff --git a/src/flow_state.cpp b/src/flow_state.cpp index 1f7127f..8e19b20 100644 --- a/src/flow_state.cpp +++ b/src/flow_state.cpp @@ -21,6 +21,7 @@ #include #include #include +#include #include #include #include @@ -98,68 +99,99 @@ struct FlowWorkspace { struct EdgeGroup { std::int32_t from; // child v (destination of grouped edges) std::int32_t to; // parent u (source of grouped edges) - std::vector eids; // underlying forward edges u->v (may be multiple parallel edges) + // Underlying forward edges u->v (parallel edges) as a slice of GroupSet::eids. + // Storing a slice rather than a std::vector keeps group building allocation-free; + // rebuilding groups after every augmentation used to dominate place_on_dag. + std::int32_t eid_begin {0}; + std::int32_t eid_count {0}; Cap sum_cap {0.0}; // sum of residual capacities for Proportional placement Cap min_cap {0.0}; // min residual capacity for EqualBalanced placement }; +// Groups plus the arena backing their edge slices, and the scratch used to build +// them. One instance is reused across every rebuild within a place_on_dag call so +// the buffers keep their capacity. +struct GroupSet { + std::vector groups; + std::vector eids; // arena: group gi owns [eid_begin, +eid_count) + std::vector reach; // scratch: backward reachability from t + std::vector uniq; // scratch: distinct parents of one node + std::vector bfs; // scratch: BFS queue + + [[nodiscard]] std::span edges_of(const EdgeGroup& gr) const noexcept { + return std::span(eids.data() + gr.eid_begin, + static_cast(gr.eid_count)); + } +}; + // Build grouped edges by (parent u, child v) that can reach destination t, // using the current residual snapshot. -static std::vector build_groups_residual(const StrictMultiDiGraph& g, - const PredDAG& dag, NodeId t, - const std::vector& residual) { - std::vector groups; +static void build_groups_residual(const StrictMultiDiGraph& g, + const PredDAG& dag, NodeId t, + const std::vector& residual, + GroupSet& gs) { + gs.groups.clear(); + gs.eids.clear(); const auto& offsets = dag.parent_offsets; const auto& parents = dag.parents; const auto& via = dag.via_edges; const auto N = g.num_nodes(); // Compute reachability: BFS backward from destination t to identify nodes on SPF DAG. - std::vector reach(static_cast(N), 0); + gs.reach.assign(static_cast(N), 0); if (t >= 0 && t < N) { - std::queue q; q.push(t); reach[static_cast(t)] = 1; - while (!q.empty()) { - auto v = q.front(); q.pop(); + gs.bfs.clear(); + gs.bfs.push_back(t); + gs.reach[static_cast(t)] = 1; + for (std::size_t head = 0; head < gs.bfs.size(); ++head) { + const auto v = gs.bfs[head]; // Iterate over v's predecessors (parents in the DAG). std::size_t s = static_cast(offsets[static_cast(v)]); std::size_t e = static_cast(offsets[static_cast(v + 1)]); for (std::size_t i = s; i < e; ++i) { auto u = parents[i]; - if (!reach[static_cast(u)]) { reach[static_cast(u)] = 1; q.push(u); } + if (!gs.reach[static_cast(u)]) { gs.reach[static_cast(u)] = 1; gs.bfs.push_back(u); } } } } // For each reachable node v, group its incoming DAG edges by parent node u. // This creates one group per (u, v) pair, aggregating parallel edges. + // Two passes per node (collect distinct parents, then gather each parent's edges) + // keep every group's edges contiguous in the arena without any allocation. for (std::int32_t v = 0; v < N; ++v) { - if (!reach[static_cast(v)]) continue; - // Small linear grouping by parent (faster than a hash for typical degrees). - std::vector>> by_parent; + if (!gs.reach[static_cast(v)]) continue; const std::size_t s = static_cast(offsets[static_cast(v)]); const std::size_t e = static_cast(offsets[static_cast(v + 1)]); + // Pass 1: distinct parents, in first-appearance order (matches the previous + // by_parent ordering, so emitted groups keep the same order as before). + gs.uniq.clear(); for (std::size_t i = s; i < e; ++i) { const auto u = parents[i]; bool found = false; - for (auto& pr : by_parent) { - if (pr.first == u) { pr.second.push_back(via[i]); found = true; break; } - } - if (!found) by_parent.emplace_back(u, std::vector{ via[i] }); + for (auto pu : gs.uniq) { if (pu == u) { found = true; break; } } + if (!found) gs.uniq.push_back(u); } - for (auto& kv : by_parent) { - EdgeGroup gr; gr.from = v; gr.to = kv.first; gr.eids.clear(); + // Pass 2: gather each parent's admissible parallel edges contiguously. + for (auto u : gs.uniq) { + EdgeGroup gr; + gr.from = v; gr.to = u; + gr.eid_begin = static_cast(gs.eids.size()); gr.sum_cap = static_cast(0.0); gr.min_cap = std::numeric_limits::infinity(); - for (auto eid0 : kv.second) { + for (std::size_t i = s; i < e; ++i) { + if (parents[i] != u) continue; + const auto eid0 = via[i]; const Cap c = residual[static_cast(eid0)]; if (c >= kMinCap) { - gr.eids.push_back(eid0); + gs.eids.push_back(eid0); gr.sum_cap += c; gr.min_cap = std::min(gr.min_cap, c); } } + gr.eid_count = static_cast(gs.eids.size()) - gr.eid_begin; if (gr.min_cap == std::numeric_limits::infinity()) gr.min_cap = static_cast(0.0); - if (!gr.eids.empty()) groups.push_back(std::move(gr)); + if (gr.eid_count > 0) gs.groups.push_back(gr); + // else: nothing was appended, so the arena is already back at eid_begin. } } - return groups; } // Construct reversed residual graph for Dinic BFS/DFS using group capacities. @@ -216,8 +248,11 @@ Flow FlowState::place_on_dag(NodeId src, NodeId dst, const PredDAG& dag, const auto N = g_->num_nodes(); if (src < 0 || src >= N || dst < 0 || dst >= N || src == dst) return 0.0; - // Build groups using current residual - auto groups = build_groups_residual(*g_, dag, dst, residual_); + // Build groups using current residual. `gs` is reused across rebuilds so its + // buffers keep their capacity for the whole call. + GroupSet gs; + build_groups_residual(*g_, dag, dst, residual_, gs); + const auto& groups = gs.groups; Flow placed = static_cast(0.0); double remaining = static_cast(requested_flow); @@ -251,7 +286,7 @@ Flow FlowState::place_on_dag(NodeId src, NodeId dst, const PredDAG& dag, // Guard against division by zero when group capacity is numerically zero. double denom = gr.sum_cap > kMinCap ? gr.sum_cap : 1.0; // Proportional split: each edge gets share = sent * (edge_residual / sum_residual). - for (auto eid : gr.eids) { + for (auto eid : gs.edges_of(gr)) { Cap base = residual_[static_cast(eid)]; double share = sent * (static_cast(base) / denom); edge_flow_[static_cast(eid)] += static_cast(share); @@ -261,7 +296,7 @@ Flow FlowState::place_on_dag(NodeId src, NodeId dst, const PredDAG& dag, } } // Rebuild groups for next tier using updated residual - groups = build_groups_residual(*g_, dag, dst, residual_); + build_groups_residual(*g_, dag, dst, residual_, gs); build_reversed_residual(ws, N, groups); } } else { @@ -274,9 +309,9 @@ Flow FlowState::place_on_dag(NodeId src, NodeId dst, const PredDAG& dag, std::vector rev_cap(groups.size(), 0.0); for (std::size_t gi = 0; gi < groups.size(); ++gi) { const auto& gr = groups[gi]; - if (gr.eids.empty()) continue; + if (gr.eid_count == 0) continue; // EB: group admissible total = min_edge_residual * |edges| - const double cap_rev = static_cast(gr.min_cap) * static_cast(gr.eids.size()); + const double cap_rev = static_cast(gr.min_cap) * static_cast(gr.eid_count); if (cap_rev >= kMinCap) { succ[static_cast(gr.to)].push_back(gi); // u -> v (group index) rev_cap[gi] = cap_rev; @@ -301,7 +336,7 @@ Flow FlowState::place_on_dag(NodeId src, NodeId dst, const PredDAG& dag, for (std::size_t u = 0; u < succ.size(); ++u) { if (!reach[u]) continue; int s = 0; - for (auto gi : succ[u]) s += static_cast(groups[gi].eids.size()); + for (auto gi : succ[u]) s += static_cast(groups[gi].eid_count); node_split[u] = s; } @@ -332,9 +367,9 @@ Flow FlowState::place_on_dag(NodeId src, NodeId dst, const PredDAG& dag, if (split <= 0) continue; for (auto gi : succ[static_cast(u)]) { const auto& gr = groups[gi]; - if (gr.eids.empty()) continue; + if (gr.eid_count == 0) continue; // Group share proportional to number of edges (equal per-edge split). - double push = f_in * (static_cast(gr.eids.size()) / static_cast(split)); + double push = f_in * (static_cast(gr.eid_count) / static_cast(split)); if (push < kEpsilon) continue; assigned[gi] += push; auto v = static_cast(gr.from); @@ -371,11 +406,11 @@ Flow FlowState::place_on_dag(NodeId src, NodeId dst, const PredDAG& dag, placed += use; // Apply scaled group assignments equally over parallel edges. for (std::size_t gi = 0; gi < groups.size(); ++gi) { - const auto& gr = groups[gi]; if (gr.eids.empty()) continue; + const auto& gr = groups[gi]; if (gr.eid_count == 0) continue; double flow_scaled = assigned[gi] * static_cast(use); if (flow_scaled < kMinFlow) continue; - double per_edge = flow_scaled / static_cast(gr.eids.size()); - for (auto eid : gr.eids) { + double per_edge = flow_scaled / static_cast(gr.eid_count); + for (auto eid : gs.edges_of(gr)) { edge_flow_[static_cast(eid)] += static_cast(per_edge); double base = static_cast(residual_[static_cast(eid)]); residual_[static_cast(eid)] = static_cast(std::max(0.0, base - per_edge)); diff --git a/src/max_flow.cpp b/src/max_flow.cpp index d5e6565..58aa192 100644 --- a/src/max_flow.cpp +++ b/src/max_flow.cpp @@ -14,6 +14,7 @@ #include "netgraph/core/constants.hpp" #include +#include #include #include #include @@ -356,12 +357,22 @@ batch_max_flow(const StrictMultiDiGraph& g, run_range(0, pairs.size()); return out; } - const auto chunk_size = (pairs.size() + thread_budget - 1) / thread_budget; + // Claim pairs one at a time from a shared counter rather than handing each worker a + // fixed contiguous chunk. Per-pair cost varies by orders of magnitude (a dense + // cross-fabric pair versus a disconnected one), so static chunking leaves workers + // idle whenever the expensive pairs happen to land in the same chunk. + std::atomic next_pair{0}; + auto run_dynamic = [&]() { + for (;;) { + const std::size_t i = next_pair.fetch_add(1, std::memory_order_relaxed); + if (i >= pairs.size()) break; + run_range(i, i + 1); + } + }; std::vector> futures; futures.reserve(thread_budget); - for (std::size_t begin = 0; begin < pairs.size(); begin += chunk_size) { - const std::size_t end = std::min(begin + chunk_size, pairs.size()); - futures.emplace_back(std::async(std::launch::async, run_range, begin, end)); + for (std::size_t w = 0; w < thread_budget; ++w) { + futures.emplace_back(std::async(std::launch::async, run_dynamic)); } for (auto& f : futures) f.get(); return out; diff --git a/tests/py/test_review_regressions.py b/tests/py/test_review_regressions.py index 18114f8..a43f189 100644 --- a/tests/py/test_review_regressions.py +++ b/tests/py/test_review_regressions.py @@ -285,3 +285,116 @@ def test_rejects_non_contiguous(self, algs): assert not strided.flags["C_CONTIGUOUS"] with pytest.raises(TypeError, match="C-contiguous"): algs.batch_max_flow(pg, strided) + + +class TestFlowPolicyTargetGuards: + """Findings 9-10: FlowPolicy accepted uses that silently produced wrong answers.""" + + def _two_graphs_same_edge_count(self): + # Same edge count, different topology: the residual-length check that caught + # the mismatched-graph case by accident does not fire here. + a = _graph(4, [(0, 1, 5.0, 1), (1, 3, 5.0, 1), (0, 2, 1.0, 1)]) + b = _graph(4, [(0, 2, 9.0, 1), (2, 3, 9.0, 1), (0, 1, 1.0, 1)]) + return a, b + + def test_flowgraph_from_a_different_graph_is_rejected(self, algs): + ga, gb = self._two_graphs_same_edge_count() + policy = ngc.FlowPolicy(algs, algs.build_graph(ga), ngc.FlowPolicyConfig()) + with pytest.raises(ValueError, match="different StrictMultiDiGraph"): + policy.place_demand(ngc.FlowGraph(gb), 0, 3, 0, 100.0) + + def test_rebalance_demand_rejects_a_different_pair(self, algs): + # remove_demand() empties flows_, so rebalance_demand must check the pair + # itself rather than relying on the check inside place_demand. + g = _graph(4, [(0, 1, 10.0, 1), (2, 3, 10.0, 1)]) + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, algs.build_graph(g), ngc.FlowPolicyConfig()) + policy.place_demand(fg, 0, 1, 0, 5.0) + with pytest.raises(ValueError, match="different \\(src, dst\\)"): + policy.rebalance_demand(fg, 2, 3, 0, 5.0) + # The matching pair still rebalances, and retargeting after remove_demand works. + policy.rebalance_demand(fg, 0, 1, 0, 2.0) + policy.remove_demand(fg) + assert policy.place_demand(fg, 2, 3, 0, 5.0)[0] == pytest.approx(5.0) + + +class TestBindingValidationConsistency: + """Findings 11-12: validation that differed between analogous entry points.""" + + def _line(self): + return _graph(3, [(0, 1, 5.0, 1), (1, 2, 5.0, 1)]) + + def test_batch_max_flow_rejects_out_of_range_like_max_flow(self, algs): + pg = algs.build_graph(self._line()) + # Previously this returned total_flow=0.0 and the bad id vanished into the batch. + with pytest.raises(ValueError, match="out of range"): + algs.batch_max_flow(pg, np.array([[0, 99]], dtype=np.int32)) + with pytest.raises(ValueError, match="out of range"): + algs.max_flow(pg, 0, 99) + + def test_ksp_validates_dtype_even_when_no_paths_exist(self, algs): + pg = algs.build_graph(self._line()) + # 2 -> 0 is unreachable, so the results loop never runs; the dtype check used + # to live inside that loop and silently accepted the bad value. + with pytest.raises(ValueError, match="dtype must be"): + algs.ksp(pg, 2, 0, k=1, dtype="float32") + + def test_algorithms_backend_argument_is_nameable(self): + assert isinstance(ngc.Algorithms(backend=ngc.Backend.cpu()), ngc.Algorithms) + + def test_unreachable_flow_record_class_is_not_exported(self): + import _netgraph_core + + # Nothing could construct or receive it, and the name collided with the C++ + # alias `using Flow = double`. + assert not hasattr(_netgraph_core, "Flow") + + +class TestTypeStubAccuracy: + """Finding 13: _docs.py declared types that did not match runtime.""" + + def test_min_cut_edges_is_an_int32_array_not_a_list(self, algs): + g = _graph(3, [(0, 1, 5.0, 1), (1, 2, 5.0, 1)]) + _, summary = algs.max_flow(algs.build_graph(g), 0, 2) + edges = summary.min_cut.edges + assert isinstance(edges, np.ndarray) + assert edges.dtype == np.int32 + + def test_stub_declares_only_names_that_exist(self): + import _netgraph_core + + from netgraph_core import _docs + + declared = { + n + for n in dir(_docs) + if not n.startswith("_") and isinstance(getattr(_docs, n), type) + } + missing = {n for n in declared if not hasattr(_netgraph_core, n)} + assert not missing, f"_docs.py declares nonexistent runtime types: {missing}" + + +class TestErrorTypeConsistency: + """Finding 14: the same class of user error raised different exception types.""" + + @pytest.fixture + def pg(self, algs): + return algs.build_graph(_graph(3, [(0, 1, 5.0, 1), (1, 2, 5.0, 1)])) + + def test_spf_residual_length_raises_type_error_like_masks(self, algs, pg): + # Previously the residual length check fell through to the C++ core, which + # throws std::invalid_argument -> ValueError, while every mask length check + # in the binding layer raises TypeError. + with pytest.raises(TypeError, match="residual length must equal"): + algs.spf(pg, 0, residual=np.zeros(5)) + with pytest.raises(TypeError, match="node_mask length must equal"): + algs.spf(pg, 0, node_mask=np.ones(7, dtype=bool)) + + def test_wrong_typed_arguments_raise_type_error_not_runtime_error(self, algs, pg): + # These hand-rolled casts used to surface as + # "RuntimeError: Unable to cast ... to C++ type '?'", which no `except + # TypeError` catches and which names nothing actionable. + with pytest.raises(TypeError, match="must be a StrictMultiDiGraph"): + algs.build_graph(5) + with pytest.raises(TypeError, match="must be an Algorithms"): + ngc.FlowPolicy("not-algorithms", pg, ngc.FlowPolicyConfig()) From 5ced75a997b1dd91f78742d60843d1d7d7343db9 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 19:47:24 +0100 Subject: [PATCH 06/11] Fix k_shortest_paths crash and remove dead code across all 88 files 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 --- .gcovr.cfg | 20 --------- CHANGELOG.md | 3 ++ CMakeLists.txt | 1 - Makefile | 2 +- bindings/python/module.cpp | 2 - include/netgraph/core/backend.hpp | 1 - include/netgraph/core/flow_policy.hpp | 1 - include/netgraph/core/flow_state.hpp | 1 - include/netgraph/core/max_flow.hpp | 1 - include/netgraph/core/profiling.hpp | 8 ++-- python/netgraph_core/__init__.py | 45 +++++++++---------- src/cpu_backend.cpp | 6 ++- src/flow_policy.cpp | 1 - src/flow_state.cpp | 2 - src/k_shortest_paths.cpp | 6 ++- src/max_flow.cpp | 1 - src/shortest_paths.cpp | 19 +++----- tests/cpp/k_shortest_paths_tests.cpp | 24 ++++++++++ tests/cpp/masking_tests.cpp | 2 +- tests/cpp/test_utils.hpp | 33 -------------- tests/py/conftest.py | 10 ----- tests/py/test_flow_policy.py | 5 --- tests/py/test_flow_policy_validation.py | 15 ------- tests/py/test_graph_from_arrays.py | 13 +++--- .../py/test_policy_vs_maxflow_equivalence.py | 19 -------- tests/py/test_review_regressions.py | 30 +++++++++++++ tests/py/test_thread_safety.py | 25 +++++++---- 27 files changed, 121 insertions(+), 175 deletions(-) delete mode 100644 .gcovr.cfg diff --git a/.gcovr.cfg b/.gcovr.cfg deleted file mode 100644 index 9aa166e..0000000 --- a/.gcovr.cfg +++ /dev/null @@ -1,20 +0,0 @@ -[gcovr] -root = . - -# Object dirs for Python extension and standalone C++ test builds -object-directory = build -object-directory = build/cpp-tests-cov - -# Only include our sources -filter = include/netgraph -filter = src - -# Exclude tests and third-party deps -exclude = tests -exclude = bindings/.* -exclude-directories = .*/_deps/.* -exclude-directories = .*/venv/.* -exclude-directories = .*/site-packages/.* - -# Work around odd temp working dirs on macOS/pybind11 -gcov-ignore-errors = all diff --git a/CHANGELOG.md b/CHANGELOG.md index 05cf27b..c5962ec 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Flow Policy**: `place_demand` silently routed a second `(src, dst)` pair over the first pair's paths. It now raises `invalid_argument`; use one policy per demand or call `remove_demand()` first. - **Graph Construction**: `from_arrays` now rejects a total edge cost at or above 2^62, which overflows the int64 path arithmetic in SPF and silently corrupts results. - **Flow State**: `compute_min_cut` and max-flow reachability derived placed flow from `capacity - residual`, overstating it for a `FlowState` built with a custom `residual_init`. Both now use `edge_flow`. +- **K-Shortest Paths**: `k_shortest_paths` read `paths.back()` on an empty vector when `max_cost_factor < 1.0` put the cost ceiling below the shortest path, crashing for `k > 1` (the `k == 1` early return masked it). All `k` now return no paths for such a factor. - **Python Bindings**: `batch_max_flow` rejected a valid int32 `pairs` array on Windows, because the dtype check compared buffer format strings and NumPy spells int32 as `NPY_LONG` on LLP64 but `NPY_INT` on LP64. It now compares dtype equivalence, and rejects non-contiguous `pairs` rather than reading them as if packed. - **Build**: `NETGRAPH_CORE_SANITIZE` passed its flags as one quoted string, so sanitizer builds never compiled, and the test target was missing them entirely. `make sanitize-test` also no longer sets `detect_leaks=1` on macOS, where it aborts. - **Build**: Coverage builds now use `-fprofile-update=atomic` on GCC; the default non-atomic counters corrupt under the threads used by `batch_max_flow` and `sensitivity_analysis`. @@ -27,10 +28,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Flow Policy**: `place_demand` computes one SPF when seeding initial flows instead of repeating an identical one per flow; seeding cost no longer scales with `min_flow_count`. - **Python Bindings**: A wrong-typed `graph`/`algorithms` argument now raises `TypeError` instead of an opaque `RuntimeError: Unable to cast ... to C++ type '?'`, `Algorithms.spf` raises `TypeError` for a wrong `residual` length (matching every other length check), `Algorithms.ksp` validates `dtype` before running, and `batch_max_flow` rejects out-of-range node ids like the single-pair entry points. - **Python Typing**: Corrected `_docs.py` (pybind11 enums and classes are not `enum.Enum`/`dataclass`, `MinCut.edges` is an `int32` array, removed a `Path` class that never existed) and widened the type-checking re-exports from 6 names to all 16, so `Algorithms`, `FlowPolicy`, `StrictMultiDiGraph` and friends are no longer `Unknown` to type checkers despite the shipped `py.typed`. +- **Tests**: Several assertions could never fail and now do: a self-loop assertion sat inside `except Exception: pass`, a thread-safety test ended in `assert True` while its workers called `pytest.fail()` from a thread where it cannot fail the test, and two tautologies (`assert size >= 0`) were replaced with the properties actually under test. - **Docs**: Corrected README/CONTRIBUTING (required CMake is 3.23 not 3.15, `make py-test` does not exist, `make test` does not collect coverage), the `sensitivity_analysis` description in README and `backend.hpp` (it measures flow *lost* on edge removal, not gain from relaxing capacity), the zero-copy and GIL claims, and header contracts for `from_arrays`, `PredDAG`, `FlowSummary.costs`, `calc_max_flow` and `FlowPolicy`. ### Removed +- **Dead code**: `.gcovr.cfg` (never read -- gcovr's default config name has no leading dot, and `make cov` passes every flag explicitly), the unreachable `FlowGraph` include and `expect_flow_conservation()` helper in the C++ test utilities, unused test helpers/fixtures, `_USE_MATH_DEFINES` (no `M_*` macro is used), and unused ``/``/``/``/`` includes across `src/` and `include/`. - **Python Bindings**: The unreachable `Flow` class (bound from `FlowRecord`). No binding constructed or returned one, it was absent from `__all__`, and the name collided with the C++ alias `using Flow = double`. ## [0.7.2] - 2026-03-26 diff --git a/CMakeLists.txt b/CMakeLists.txt index 2726b19..a05aa3c 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -89,7 +89,6 @@ target_include_directories(netgraph_core PUBLIC $ $ ) -target_compile_definitions(netgraph_core PRIVATE _USE_MATH_DEFINES) ## C++ standard is set globally above. target_compile_features(netgraph_core PUBLIC cxx_std_20) diff --git a/Makefile b/Makefile index 4a9a92c..b10aa7d 100644 --- a/Makefile +++ b/Makefile @@ -52,7 +52,7 @@ help: @echo " make check-ci - Run non-mutating lint + tests (CI entrypoint)" @echo " make lint - Run only linting (non-mutating: ruff + pyright)" @echo " make format - Auto-format code with ruff" - @echo " make test - Run tests with coverage" + @echo " make test - Run Python tests (pytest)" @echo " make qt - Run quick tests only (exclude slow/benchmark)" @echo " make cpp-test - Build and run C++ tests" @echo " make cov - Coverage summary + XML + single-page combined HTML" diff --git a/bindings/python/module.cpp b/bindings/python/module.cpp index c951840..94dcd52 100644 --- a/bindings/python/module.cpp +++ b/bindings/python/module.cpp @@ -260,8 +260,6 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { auto* outp = dist_arr.mutable_data(); for (std::size_t i=0;i::infinity() : static_cast(dist[i]); out.append(py::make_tuple(std::move(dist_arr), pr.second)); - } else { - throw py::value_error("dtype must be 'float64' or 'int64'"); } } return out; diff --git a/include/netgraph/core/backend.hpp b/include/netgraph/core/backend.hpp index c1c295d..0a75618 100644 --- a/include/netgraph/core/backend.hpp +++ b/include/netgraph/core/backend.hpp @@ -7,7 +7,6 @@ #pragma once #include -#include #include #include #include diff --git a/include/netgraph/core/flow_policy.hpp b/include/netgraph/core/flow_policy.hpp index 3f1652f..7502962 100644 --- a/include/netgraph/core/flow_policy.hpp +++ b/include/netgraph/core/flow_policy.hpp @@ -24,7 +24,6 @@ struct ExecutionContext { std::shared_ptr algorithms; GraphHandle graph; - // Constructor with validation ExecutionContext(std::shared_ptr algs, const GraphHandle& gh) noexcept : algorithms(std::move(algs)), graph(gh) {} }; diff --git a/include/netgraph/core/flow_state.hpp b/include/netgraph/core/flow_state.hpp index b8b5339..bdc327e 100644 --- a/include/netgraph/core/flow_state.hpp +++ b/include/netgraph/core/flow_state.hpp @@ -7,7 +7,6 @@ #pragma once #include -#include #include #include #include diff --git a/include/netgraph/core/max_flow.hpp b/include/netgraph/core/max_flow.hpp index 3497e6c..347b30d 100644 --- a/include/netgraph/core/max_flow.hpp +++ b/include/netgraph/core/max_flow.hpp @@ -1,7 +1,6 @@ /* Max-flow utility APIs with summaries and batch evaluation. */ #pragma once -#include #include #include #include diff --git a/include/netgraph/core/profiling.hpp b/include/netgraph/core/profiling.hpp index 0561d3d..40eb9ea 100644 --- a/include/netgraph/core/profiling.hpp +++ b/include/netgraph/core/profiling.hpp @@ -2,8 +2,7 @@ Enable via environment variable: NGRAPH_CORE_PROFILE=1 -When disabled (default), overhead is minimal: ~2-4 CPU cycles per scope -due to a single static bool check with branch prediction. +When disabled (default), each instrumented scope costs one cached-bool check. Usage: #include "netgraph/core/profiling.hpp" @@ -22,7 +21,6 @@ From Python: */ #pragma once -#include #include #include #include @@ -113,8 +111,8 @@ class ScopedTimer { #define NGRAPH_CONCAT_IMPL(a, b) a##b #define NGRAPH_CONCAT(a, b) NGRAPH_CONCAT_IMPL(a, b) -// Main profiling macro. Expands to a ScopedTimer only if profiling is enabled. -// When disabled, the check is a single static bool read (~1-2 cycles). +// Main profiling macro. Expands to a ScopedTimer only if profiling is enabled; +// when disabled the timer is constructed with a null name and does no work. #define NGRAPH_PROFILE_SCOPE(name) \ ::netgraph::core::ScopedTimer NGRAPH_CONCAT(_ngraph_timer_, __LINE__)( \ ::netgraph::core::profiling_enabled() ? (name) : nullptr) diff --git a/python/netgraph_core/__init__.py b/python/netgraph_core/__init__.py index a4814cf..4c39e1c 100644 --- a/python/netgraph_core/__init__.py +++ b/python/netgraph_core/__init__.py @@ -8,6 +8,7 @@ # pyright: reportMissingImports=false from importlib.metadata import version +from typing import TYPE_CHECKING from _netgraph_core import ( Algorithms, @@ -38,31 +39,25 @@ # change what a type checker sees. Keep this list in sync with _docs.py, and keep # _docs.py in sync with the bindings -- a wrong stub is worse than a missing one, # because it turns "unchecked" into "confidently wrong". -try: # pragma: no cover - typing-only import - from typing import TYPE_CHECKING as _TYPE_CHECKING - - if _TYPE_CHECKING: # noqa: SIM108 - from ._docs import ( # noqa: I001 - Algorithms as Algorithms, - Backend as Backend, - EdgeSelection as EdgeSelection, - EdgeTieBreak as EdgeTieBreak, - FlowGraph as FlowGraph, - FlowIndex as FlowIndex, - FlowPlacement as FlowPlacement, - FlowPolicy as FlowPolicy, - FlowPolicyConfig as FlowPolicyConfig, - FlowState as FlowState, - FlowSummary as FlowSummary, - Graph as Graph, - MinCut as MinCut, - PathAlg as PathAlg, - PredDAG as PredDAG, - StrictMultiDiGraph as StrictMultiDiGraph, - ) -except ImportError: - # Safe fallback if _docs.py changes; runtime bindings above remain authoritative. - pass +if TYPE_CHECKING: # pragma: no cover - typing-only + from ._docs import ( # noqa: I001 + Algorithms as Algorithms, + Backend as Backend, + EdgeSelection as EdgeSelection, + EdgeTieBreak as EdgeTieBreak, + FlowGraph as FlowGraph, + FlowIndex as FlowIndex, + FlowPlacement as FlowPlacement, + FlowPolicy as FlowPolicy, + FlowPolicyConfig as FlowPolicyConfig, + FlowState as FlowState, + FlowSummary as FlowSummary, + Graph as Graph, + MinCut as MinCut, + PathAlg as PathAlg, + PredDAG as PredDAG, + StrictMultiDiGraph as StrictMultiDiGraph, + ) __all__ = [ "__version__", diff --git a/src/cpu_backend.cpp b/src/cpu_backend.cpp index a37a131..879cb63 100644 --- a/src/cpu_backend.cpp +++ b/src/cpu_backend.cpp @@ -26,8 +26,10 @@ class CpuBackend final : public Backend { const GraphHandle& gh, NodeId src, const SpfOptions& opts) override { const StrictMultiDiGraph& g = *gh.graph; // Validate mask lengths strictly; mismatches are user errors. - // NOTE: Keep this as the single public boundary check; deeper layers - // (shortest_paths) should rely on this to avoid redundant validation. + // NOTE: this check is deliberately duplicated with the one inside + // shortest_paths(): the backend layer produces a caller-facing message, and the + // algorithm layer must stand on its own because it is also part of the public + // C++ API and callable without going through a Backend. if (!opts.node_mask.empty() && opts.node_mask.size() != static_cast(g.num_nodes())) { throw std::invalid_argument("CpuBackend::spf: node_mask length mismatch"); } diff --git a/src/flow_policy.cpp b/src/flow_policy.cpp index 8ea1072..9062c07 100644 --- a/src/flow_policy.cpp +++ b/src/flow_policy.cpp @@ -22,7 +22,6 @@ #include #include #include -#include namespace netgraph::core { diff --git a/src/flow_state.cpp b/src/flow_state.cpp index 8e19b20..3789853 100644 --- a/src/flow_state.cpp +++ b/src/flow_state.cpp @@ -19,10 +19,8 @@ #include #include #include -#include #include #include -#include #include #include #include diff --git a/src/k_shortest_paths.cpp b/src/k_shortest_paths.cpp index 39a387b..bb6c8f9 100644 --- a/src/k_shortest_paths.cpp +++ b/src/k_shortest_paths.cpp @@ -7,7 +7,6 @@ #include "netgraph/core/k_shortest_paths.hpp" #include -#include #include #include #include @@ -210,6 +209,11 @@ std::vector, PredDAG>> k_shortest_paths( max_cost = best_cost * (*max_cost_factor); } if (p0->cost <= max_cost) paths.push_back(*p0); + // A max_cost_factor below 1.0 puts the ceiling under the shortest path itself, so + // nothing was admitted. Return now: the k > 1 loop below opens with paths.back(), + // which is undefined behaviour on an empty vector. (The k == 1 branch would have + // masked this, so the guard must sit ahead of it.) + if (paths.empty()) return {}; if (k == 1) { // Convert and return std::vector, PredDAG>> items; diff --git a/src/max_flow.cpp b/src/max_flow.cpp index 58aa192..81e3b29 100644 --- a/src/max_flow.cpp +++ b/src/max_flow.cpp @@ -24,7 +24,6 @@ #include #include #include -#include #include #include diff --git a/src/shortest_paths.cpp b/src/shortest_paths.cpp index 3e9f46c..5ccedb6 100644 --- a/src/shortest_paths.cpp +++ b/src/shortest_paths.cpp @@ -1,10 +1,16 @@ /* Path enumeration from PredDAG (resolve_to_paths). */ #include "netgraph/core/shortest_paths.hpp" +#include "netgraph/core/constants.hpp" #include "netgraph/core/profiling.hpp" #include +#include +#include +#include #include -#include +#include +#include +#include #include #include @@ -169,17 +175,6 @@ resolve_to_paths(const PredDAG& dag, NodeId src, NodeId dst, * Node-level tie-breaking for equal-cost nodes (prefers higher bottleneck capacity) - Early exit when specific destination is reached */ -#include "netgraph/core/shortest_paths.hpp" -#include -#include -#include -#include -#include -#include -#include -#include -#include "netgraph/core/constants.hpp" - namespace netgraph::core { namespace { diff --git a/tests/cpp/k_shortest_paths_tests.cpp b/tests/cpp/k_shortest_paths_tests.cpp index 019b24e..30b1dc6 100644 --- a/tests/cpp/k_shortest_paths_tests.cpp +++ b/tests/cpp/k_shortest_paths_tests.cpp @@ -305,3 +305,27 @@ TEST(KShortestPaths, LargerOutOfOrderTopology) { } } } + +// Regression: a max_cost_factor below 1.0 puts the cost ceiling under the shortest +// path, so no path is admitted. The k > 1 loop opened with paths.back() on that empty +// vector -- undefined behaviour. The k == 1 early return masked it, so only k >= 2 hit +// it. All k must now agree and return no paths. +TEST(KSP, MaxCostFactorBelowOne_ReturnsEmptyForAllK) { + auto g = make_line_graph(3); + auto be = make_cpu_backend(); + Algorithms algs(be); + auto gh = algs.build_graph(g); + + for (int k : {1, 2, 5}) { + KspOptions opts; + opts.k = k; + opts.max_cost_factor = 0.5; + auto out = algs.ksp(gh, 0, 2, opts); + EXPECT_TRUE(out.empty()) << "k=" << k << " should admit no path under a sub-1.0 factor"; + } + // A factor of exactly 1.0 still admits the shortest path. + KspOptions ok; + ok.k = 3; + ok.max_cost_factor = 1.0; + EXPECT_EQ(algs.ksp(gh, 0, 2, ok).size(), 1u); +} diff --git a/tests/cpp/masking_tests.cpp b/tests/cpp/masking_tests.cpp index 7df9824..7af26d3 100644 --- a/tests/cpp/masking_tests.cpp +++ b/tests/cpp/masking_tests.cpp @@ -410,7 +410,7 @@ TEST(MaskingTests, KSPWithEdgeMask) { std::span(edge_mask.get(), g.num_edges())); // Should find remaining path(s) - EXPECT_GE(results.size(), 0); + EXPECT_EQ(results.size(), 1u); for (const auto& [dist, dag] : results) { if (dist[2] < std::numeric_limits::max()) { diff --git a/tests/cpp/test_utils.hpp b/tests/cpp/test_utils.hpp index 86a6b0e..f041799 100644 --- a/tests/cpp/test_utils.hpp +++ b/tests/cpp/test_utils.hpp @@ -5,7 +5,6 @@ #include #include "netgraph/core/strict_multidigraph.hpp" #include "netgraph/core/shortest_paths.hpp" -#include "netgraph/core/flow_graph.hpp" #include "netgraph/core/max_flow.hpp" namespace netgraph::core::test { @@ -436,38 +435,6 @@ inline void expect_pred_dag_semantically_valid(const StrictMultiDiGraph& g, } } -inline void expect_flow_conservation(const FlowGraph& fg, NodeId src, NodeId dst) { - const auto& g = fg.graph(); - auto row = g.row_offsets_view(); - auto col = g.col_indices_view(); - auto aei = g.adj_edge_index_view(); - auto in_row = g.in_row_offsets_view(); - auto in_col = g.in_col_indices_view(); - auto in_aei = g.in_adj_edge_index_view(); - auto flows = fg.edge_flow_view(); - - // For each intermediate node, inflow should equal outflow - for (std::int32_t u = 0; u < g.num_nodes(); ++u) { - if (u == src || u == dst) continue; - - double outflow = 0.0; - auto s = static_cast(row[static_cast(u)]); - auto e = static_cast(row[static_cast(u) + 1]); - for (std::size_t j = s; j < e; ++j) { - outflow += flows[static_cast(aei[j])]; - } - - double inflow = 0.0; - auto is = static_cast(in_row[static_cast(u)]); - auto ie = static_cast(in_row[static_cast(u) + 1]); - for (std::size_t j = is; j < ie; ++j) { - inflow += flows[static_cast(in_aei[j])]; - } - - EXPECT_NEAR(inflow, outflow, 1e-9) << "Flow not conserved at node " << u; - } -} - // FlowSummary validation helpers inline void validate_capacity_constraints(const StrictMultiDiGraph& g, const FlowSummary& summary) { ASSERT_EQ(summary.edge_flows.size(), static_cast(g.num_edges())) diff --git a/tests/py/conftest.py b/tests/py/conftest.py index 5e84fb3..ed33a8e 100644 --- a/tests/py/conftest.py +++ b/tests/py/conftest.py @@ -438,16 +438,6 @@ def _convert(g: ngc.StrictMultiDiGraph, dag: ngc.PredDAG): return _convert -@pytest.fixture -def make_pred_map(dag_to_pred_map): - """Alias fixture for converting PredDAG to {node: {parent: [EdgeId]}}. - - Provided to make intent clearer at call sites. - """ - - return dag_to_pred_map - - @pytest.fixture def assert_paths_concrete(): """Validate path tuples returned by resolve_to_paths. diff --git a/tests/py/test_flow_policy.py b/tests/py/test_flow_policy.py index 6bd2abc..774e903 100644 --- a/tests/py/test_flow_policy.py +++ b/tests/py/test_flow_policy.py @@ -7,7 +7,6 @@ from __future__ import annotations -import math from typing import Dict, Tuple import numpy as np @@ -90,10 +89,6 @@ def make_policy(config: str, algs: ngc.Algorithms, gh) -> ngc.FlowPolicy: } -def _almost_equal(a: float, b: float, tol: float = 1e-9) -> bool: - return math.isclose(a, b, rel_tol=0, abs_tol=tol) - - def _run_case( g: ngc.StrictMultiDiGraph, label: str, diff --git a/tests/py/test_flow_policy_validation.py b/tests/py/test_flow_policy_validation.py index c5e5ce4..9f2c62b 100644 --- a/tests/py/test_flow_policy_validation.py +++ b/tests/py/test_flow_policy_validation.py @@ -45,21 +45,6 @@ def parallel_paths_varying_capacities(): return _make_graph(num_nodes, src, dst, cap, cost) -@pytest.fixture -def parallel_paths_varying_costs(): - """3 parallel paths with different costs for cost-aware testing.""" - # S -> [M1, M2, M3] -> T - # Paths: S-M1-T (cost 10), S-M2-T (cost 20), S-M3-T (cost 30) - num_nodes = 5 - src = np.array([0, 0, 0, 1, 2, 3], dtype=np.int32) - dst = np.array([1, 2, 3, 4, 4, 4], dtype=np.int32) - cap = np.array([100.0, 100.0, 100.0, 100.0, 100.0, 100.0], dtype=np.float64) - # First hop costs: 10, 20, 30; Second hop costs: 10, 20, 30 - cost = np.array([10, 20, 30, 10, 20, 30], dtype=np.int64) - - return _make_graph(num_nodes, src, dst, cap, cost) - - # ============================================================================ # TEST 1: Path Distribution and Balance # ============================================================================ diff --git a/tests/py/test_graph_from_arrays.py b/tests/py/test_graph_from_arrays.py index 29e0316..19f8595 100644 --- a/tests/py/test_graph_from_arrays.py +++ b/tests/py/test_graph_from_arrays.py @@ -61,10 +61,9 @@ def test_from_arrays_self_loops_behavior(): dst = np.array([0], dtype=np.int32) # self-loop cap = np.array([1.0], dtype=np.float64) cost = np.array([1], dtype=np.int64) - # Either allowed or rejected; assert it doesn't crash and creates 1 edge or raises - try: - g = ngc.StrictMultiDiGraph.from_arrays(n, src, dst, cap, cost, _make_ext_ids(1)) - assert g.num_edges() >= 1 - except Exception: - # Accept rejection behavior too - pass + # Self-loops are accepted: StrictMultiDiGraph is a multigraph and from_arrays + # only rejects out-of-range ids and negative capacity/cost. + g = ngc.StrictMultiDiGraph.from_arrays(n, src, dst, cap, cost, _make_ext_ids(1)) + assert g.num_edges() == 1 + assert g.edge_src_view()[0] == 0 + assert g.edge_dst_view()[0] == 0 diff --git a/tests/py/test_policy_vs_maxflow_equivalence.py b/tests/py/test_policy_vs_maxflow_equivalence.py index 0d51d9a..cf5815a 100644 --- a/tests/py/test_policy_vs_maxflow_equivalence.py +++ b/tests/py/test_policy_vs_maxflow_equivalence.py @@ -60,25 +60,6 @@ def _policy_for( return ngc.FlowPolicy(algs, gh, cfg) -def _max_flow_for( - algs: ngc.Algorithms, - gh, - placement: ngc.FlowPlacement, - shortest_path: bool, - require_capacity: bool, -) -> float: - total, _ = algs.max_flow( - gh, - 0, - 1, # src, dst are supplied by fixtures through to_handle(graph) below; we override per case - flow_placement=placement, - shortest_path=shortest_path, - require_capacity=require_capacity, - with_edge_flows=False, - ) - return float(total) - - @pytest.mark.parametrize( "graph_label", [ diff --git a/tests/py/test_review_regressions.py b/tests/py/test_review_regressions.py index a43f189..b6032d5 100644 --- a/tests/py/test_review_regressions.py +++ b/tests/py/test_review_regressions.py @@ -17,6 +17,14 @@ residual_init as cancellable flow. 8. batch_max_flow rejected a genuine int32 `pairs` array on Windows, and read non-contiguous input as if it were packed. +9-10. FlowPolicy accepted a FlowGraph built from a different graph, and + rebalance_demand bypassed the (src, dst) guard. +11-12. batch_max_flow and ksp validated inputs later (or not at all) compared + with their single-pair counterparts. +13. _docs.py declared types that did not match runtime. +14. The same class of user error raised different exception types. +15. k_shortest_paths read paths.back() on an empty vector when + max_cost_factor < 1.0 and k > 1. """ from __future__ import annotations @@ -398,3 +406,25 @@ def test_wrong_typed_arguments_raise_type_error_not_runtime_error(self, algs, pg algs.build_graph(5) with pytest.raises(TypeError, match="must be an Algorithms"): ngc.FlowPolicy("not-algorithms", pg, ngc.FlowPolicyConfig()) + + +class TestKspCostCeiling: + """Finding 15: k>1 with max_cost_factor < 1.0 read paths.back() on an empty vector. + + The ceiling lands below the shortest path, so nothing is admitted. The k == 1 + branch returned early and masked the undefined behaviour; k >= 2 fell into the + spur loop, which opens with `paths.back()`. + """ + + @pytest.mark.parametrize("k", [1, 2, 5]) + @pytest.mark.parametrize("factor", [0.5, 0.99]) + def test_sub_unit_factor_returns_empty_for_every_k(self, algs, k, factor): + g = _graph(3, [(0, 1, 5.0, 1), (1, 2, 5.0, 1)]) + pg = algs.build_graph(g) + assert algs.ksp(pg, 0, 2, k=k, max_cost_factor=factor) == [] + + @pytest.mark.parametrize("k", [1, 2, 5]) + def test_unit_factor_still_admits_the_shortest_path(self, algs, k): + g = _graph(3, [(0, 1, 5.0, 1), (1, 2, 5.0, 1)]) + pg = algs.build_graph(g) + assert len(algs.ksp(pg, 0, 2, k=k, max_cost_factor=1.0)) == 1 diff --git a/tests/py/test_thread_safety.py b/tests/py/test_thread_safety.py index 059d9f7..9720542 100644 --- a/tests/py/test_thread_safety.py +++ b/tests/py/test_thread_safety.py @@ -166,7 +166,10 @@ def mutate_mask(): # Should get consistent results (either with or without node 2) num_paths = results[0] - assert num_paths >= 0 # Should not crash or return corrupted data + # The mask must be copied before the GIL is released, so the run sees one + # consistent snapshot: 2 paths (mask untouched) or 1 (node 2 blocked, leaving + # 0->1->4). Any other count means it observed a torn mask. + assert num_paths in (1, 2) class TestMaxFlowThreadSafety: @@ -277,20 +280,27 @@ def test_repeated_concurrent_mutations(self): graph = algs.build_graph(g) node_mask = np.ones(n, dtype=bool) + # pytest.fail() inside a worker thread cannot fail the test: the exception + # stays in that thread and join() does not re-raise it. Record failures and + # assert on them from the main thread instead. + errors: list[str] = [] def run_algorithms(): """Run various algorithms repeatedly.""" for _ in range(20): try: algs.spf(graph, 0, node_mask=node_mask) - except Exception as e: - pytest.fail(f"Algorithm raised exception: {e}") + except Exception as e: # noqa: BLE001 - reported below + errors.append(f"algorithm raised: {e!r}") def mutate_continuously(): """Continuously mutate the mask.""" - for _ in range(100): - node_mask[:] = np.random.rand(n) > 0.5 - time.sleep(0.001) + try: + for _ in range(100): + node_mask[:] = np.random.rand(n) > 0.5 + time.sleep(0.001) + except Exception as e: # noqa: BLE001 - reported below + errors.append(f"mutator raised: {e!r}") algo_thread = threading.Thread(target=run_algorithms) mutator_thread = threading.Thread(target=mutate_continuously) @@ -301,8 +311,7 @@ def mutate_continuously(): algo_thread.join() mutator_thread.join() - # If we got here without crashes/exceptions, thread safety is working - assert True, "Thread safety test completed without crashes" + assert not errors, "; ".join(errors) class TestMemoryOrdering: From caf555eea6c41a32173a9e9c96244bdcd15f45b1 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 19:54:44 +0100 Subject: [PATCH 07/11] Restore where std::sort is used Removing the unused 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 for the same reason. Co-Authored-By: Claude Opus 5 --- bindings/python/module.cpp | 1 + src/profiling.cpp | 1 + 2 files changed, 2 insertions(+) diff --git a/bindings/python/module.cpp b/bindings/python/module.cpp index 94dcd52..14207fe 100644 --- a/bindings/python/module.cpp +++ b/bindings/python/module.cpp @@ -10,6 +10,7 @@ #include #include #include +#include #include "netgraph/core/k_shortest_paths.hpp" #include "netgraph/core/max_flow.hpp" diff --git a/src/profiling.cpp b/src/profiling.cpp index cbbefa9..7acfed9 100644 --- a/src/profiling.cpp +++ b/src/profiling.cpp @@ -9,6 +9,7 @@ */ #include "netgraph/core/profiling.hpp" +#include // std::sort in dump() #include namespace netgraph::core { From 303e5d05fb161ff2a8e5314dcb655ccf1afd902c Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 21:24:53 +0100 Subject: [PATCH 08/11] Remove vestigial build_graph_from_arrays and its ownership chain 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 --- CHANGELOG.md | 2 ++ bindings/python/module.cpp | 21 +-------------------- include/netgraph/core/algorithms.hpp | 4 ---- include/netgraph/core/backend.hpp | 14 +++----------- python/netgraph_core/_docs.py | 12 ------------ src/cpu_backend.cpp | 4 ---- 6 files changed, 6 insertions(+), 51 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index c5962ec..13f98cf 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -34,6 +34,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed - **Dead code**: `.gcovr.cfg` (never read -- gcovr's default config name has no leading dot, and `make cov` passes every flag explicitly), the unreachable `FlowGraph` include and `expect_flow_conservation()` helper in the C++ test utilities, unused test helpers/fixtures, `_USE_MATH_DEFINES` (no `M_*` macro is used), and unused ``/``/``/``/`` includes across `src/` and `include/`. +- **Python Bindings**: `Algorithms.build_graph_from_arrays`. Nothing used it (no test, and not NetGraph), and the `Graph` handle it returned could not be passed to `FlowGraph` or `FlowPolicy`, so it only ever served the stateless algorithms while looking like a peer of `build_graph`. Build the graph with `StrictMultiDiGraph.from_arrays()` and pass it to `Algorithms.build_graph()`. +- **C++ API**: The `build_graph(std::shared_ptr)` overload on `Algorithms` and `Backend`, which existed solely to serve that binding. Removing it also drops a pure virtual that every `Backend` implementation had to provide. Callers wanting the handle to own the graph can construct it directly: `GraphHandle{my_shared_ptr}`. - **Python Bindings**: The unreachable `Flow` class (bound from `FlowRecord`). No binding constructed or returned one, it was absent from `__all__`, and the name collided with the C++ alias `using Flow = double`. ## [0.7.2] - 2026-03-26 diff --git a/bindings/python/module.cpp b/bindings/python/module.cpp index 14207fe..234ed73 100644 --- a/bindings/python/module.cpp +++ b/bindings/python/module.cpp @@ -156,7 +156,7 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { .def_static("cpu", [](){ return PyBackend{ make_cpu_backend() }; }); py::class_(m, "Graph") - // Opaque holder; constructed via Algorithms.build_graph or build_graph_from_arrays + // Opaque holder; constructed via Algorithms.build_graph .def_property_readonly("num_nodes", [](const PyGraph& pg){ return pg.num_nodes; }) .def_property_readonly("num_edges", [](const PyGraph& pg){ return pg.num_edges; }); @@ -175,25 +175,6 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { // referencing a non-owned graph instance. return PyGraph{ gh, graph_obj, g.num_nodes(), g.num_edges() }; }, py::arg("graph")) - .def("build_graph_from_arrays", [](const Algorithms& algs, - std::int32_t num_nodes, - py::array src, py::array dst, - py::array capacity, py::array cost, - py::array ext_edge_ids){ - // Build graph and construct shared ownership directly - auto ext_s = as_span(ext_edge_ids, "ext_edge_ids"); - auto sp = std::make_shared( - StrictMultiDiGraph::from_arrays( - num_nodes, - as_span(src, "src"), - as_span(dst, "dst"), - as_span(capacity, "capacity"), - as_span(cost, "cost"), - ext_s)); - auto gh = algs.build_graph(std::static_pointer_cast(sp)); - // GraphHandle holds shared ownership; no additional Python-side owner needed - return PyGraph{ gh, py::none(), sp->num_nodes(), sp->num_edges() }; - }, py::arg("num_nodes"), py::arg("src"), py::arg("dst"), py::arg("capacity"), py::arg("cost"), py::arg("ext_edge_ids")) .def("spf", [](const Algorithms& algs, const PyGraph& pg, std::int32_t src, py::object dst, py::object selection_obj, py::object residual_obj, py::object node_mask, py::object edge_mask, bool multipath, std::string dtype) -> py::tuple { diff --git a/include/netgraph/core/algorithms.hpp b/include/netgraph/core/algorithms.hpp index e7d3f98..c5ca458 100644 --- a/include/netgraph/core/algorithms.hpp +++ b/include/netgraph/core/algorithms.hpp @@ -16,10 +16,6 @@ class Algorithms { return backend_->build_graph(g); } - [[nodiscard]] GraphHandle build_graph(std::shared_ptr g) const { - return backend_->build_graph(std::move(g)); - } - [[nodiscard]] std::pair, PredDAG> spf(const GraphHandle& gh, NodeId src, const SpfOptions& opts) const { return backend_->spf(gh, src, opts); diff --git a/include/netgraph/core/backend.hpp b/include/netgraph/core/backend.hpp index 0a75618..1c2ba73 100644 --- a/include/netgraph/core/backend.hpp +++ b/include/netgraph/core/backend.hpp @@ -30,7 +30,9 @@ class Backend { // Prepares a backend-specific graph handle from an existing graph reference. // - // The CPU backend creates a non-owning shared_ptr with a no-op deleter. + // The CPU backend creates a non-owning shared_ptr with a no-op deleter, so the + // caller must keep `g` alive for as long as the handle is used. To hand the + // handle shared ownership instead, construct it directly: GraphHandle{my_sp}. // // Arguments: // g: The source graph to wrap. @@ -39,16 +41,6 @@ class Backend { // A GraphHandle containing the backend-specific graph representation. [[nodiscard]] virtual GraphHandle build_graph(const StrictMultiDiGraph& g) = 0; - // Prepares a backend-specific graph handle that takes shared ownership of the - // provided graph instance. - // - // Arguments: - // g: The source graph as a shared_ptr. - // - // Returns: - // A GraphHandle that shares ownership of the graph. - [[nodiscard]] virtual GraphHandle build_graph(std::shared_ptr g) = 0; - // Computes shortest paths from a source node. // // Arguments: diff --git a/python/netgraph_core/_docs.py b/python/netgraph_core/_docs.py index eec30b6..68c6e84 100644 --- a/python/netgraph_core/_docs.py +++ b/python/netgraph_core/_docs.py @@ -500,18 +500,6 @@ def build_graph(self, graph: "StrictMultiDiGraph") -> "Graph": """ ... - def build_graph_from_arrays( - self, - num_nodes: int, - src: "np.ndarray", - dst: "np.ndarray", - capacity: "np.ndarray", - cost: "np.ndarray", - ext_edge_ids: "np.ndarray", - ) -> "Graph": - """Build graph directly from arrays (graph is owned by the handle).""" - ... - def spf( self, graph: "Graph", diff --git a/src/cpu_backend.cpp b/src/cpu_backend.cpp index 879cb63..2f64537 100644 --- a/src/cpu_backend.cpp +++ b/src/cpu_backend.cpp @@ -18,10 +18,6 @@ class CpuBackend final : public Backend { return GraphHandle{ std::shared_ptr(&g, [](const StrictMultiDiGraph*){}) }; } - GraphHandle build_graph(std::shared_ptr g) override { - return GraphHandle{ std::move(g) }; - } - std::pair, PredDAG> spf( const GraphHandle& gh, NodeId src, const SpfOptions& opts) override { const StrictMultiDiGraph& g = *gh.graph; From e5f199c1bd81f23a9ac0029eb3998a6a36b88a34 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 22:22:15 +0100 Subject: [PATCH 09/11] Design and implement FlowPolicy static paths (pinned path bundles) 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 --- CHANGELOG.md | 6 + bindings/python/module.cpp | 21 ++ include/netgraph/core/flow_policy.hpp | 59 ++- include/netgraph/core/shortest_paths.hpp | 21 ++ python/netgraph_core/_docs.py | 73 +++- src/flow_policy.cpp | 456 +++++++++++++++++++---- src/k_shortest_paths.cpp | 44 +-- src/shortest_paths.cpp | 65 ++++ tests/cpp/flow_policy_tests.cpp | 85 +++++ tests/py/test_static_paths.py | 392 +++++++++++++++++++ 10 files changed, 1094 insertions(+), 128 deletions(-) create mode 100644 tests/py/test_static_paths.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 13f98cf..08ecdf1 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [0.8.0] - 2026-08-22 +### Added + +- **Flow Policy**: `set_static_paths(src, dst, paths)` pins a demand to explicit path bundles (MPLS-style routing), finishing a feature that was ported but never bound and whose port bound every flow to the first bundle. One flow per usable bundle, each bound to its own bundle in supply order; bundles are validated against the graph and pruned against the policy's masks, and a bundle with no surviving `src->dst` walk is down (no reroute). `EqualBalanced` spreads over the up bundles only. With static paths the policy neither grows its flow set nor reoptimizes; `max_path_cost`/`-factor`, `min_flow_count` and `reoptimize_flows_on_each_placement` are inert. +- **Shortest Paths**: `PredDAG.from_edges(graph, edges)` builds a single-path `PredDAG` from a contiguous edge-id sequence (`make_path_dag` in C++) -- the missing constructor for operator-defined explicit paths. Extracting it also deduplicates the two identical path-to-DAG conversion blocks in `k_shortest_paths` (outputs unchanged). + ### Fixed - **Max-Flow**: `calc_max_flow` could return less than the true maximum, since the tier loop augments only along forward SPF DAGs and never cancels an earlier placement (the reported `min_cut` then contradicted `total_flow`). Added a residual completion phase with reverse arcs for `Proportional` + `require_capacity` + `shortest_path=false`; results increase where the previous value was suboptimal. @@ -26,6 +31,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Flow Placement**: `place_on_dag` now rebuilds its `(parent, child)` edge groups into a reused arena instead of nested per-node vectors, which was ~70% of its runtime (~2-2.4x faster; output unchanged). - **Max-Flow**: Parallelized `batch_max_flow` across source/destination pairs using `std::async`; workers claim pairs from a shared counter, so a batch whose costs are unevenly distributed still parallelizes. Thread count from `NGRAPH_CORE_BATCH_THREADS` env or hardware concurrency. - **Flow Policy**: `place_demand` computes one SPF when seeding initial flows instead of repeating an identical one per flow; seeding cost no longer scales with `min_flow_count`. +- **Flow Policy**: The `EqualBalanced` rebalance recursion (`place_demand` -> `rebalance_demand` -> `place_demand`) is now an iterative loop with numerically identical results (returned leftover may differ by ~1 ulp from floating-point summation order); with many pinned bundles of heterogeneous capacity the recursion depth grew like `U * ln(imbalance/kMinFlow)`, a stack-overflow risk on worker threads. - **Python Bindings**: A wrong-typed `graph`/`algorithms` argument now raises `TypeError` instead of an opaque `RuntimeError: Unable to cast ... to C++ type '?'`, `Algorithms.spf` raises `TypeError` for a wrong `residual` length (matching every other length check), `Algorithms.ksp` validates `dtype` before running, and `batch_max_flow` rejects out-of-range node ids like the single-pair entry points. - **Python Typing**: Corrected `_docs.py` (pybind11 enums and classes are not `enum.Enum`/`dataclass`, `MinCut.edges` is an `int32` array, removed a `Path` class that never existed) and widened the type-checking re-exports from 6 names to all 16, so `Algorithms`, `FlowPolicy`, `StrictMultiDiGraph` and friends are no longer `Unknown` to type checkers despite the shipped `py.typed`. - **Tests**: Several assertions could never fail and now do: a self-loop assertion sat inside `except Exception: pass`, a thread-safety test ended in `assert True` while its workers called `pytest.fail()` from a thread where it cannot fail the test, and two tautologies (`assert size >= 0`) were replaced with the properties actually under test. diff --git a/bindings/python/module.cpp b/bindings/python/module.cpp index 234ed73..1cde885 100644 --- a/bindings/python/module.cpp +++ b/bindings/python/module.cpp @@ -328,6 +328,16 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { .def_property_readonly("via_edges", [](const PredDAG& d){ return copy_to_numpy(std::span(d.via_edges.data(), d.via_edges.size())); }) + .def_static("from_edges", [](const StrictMultiDiGraph& g, py::sequence edges){ + std::vector e; + e.reserve(py::len(edges)); + for (auto item : edges) e.push_back(py::cast(item)); + return make_path_dag(g, e); + }, py::arg("graph"), py::arg("edges"), + "Build a single-path PredDAG from a contiguous edge-id sequence.\n" + "Validates that each edge exists, consecutive edges connect, and the walk " + "is a simple path. The result works anywhere a PredDAG is accepted " + "(place_on_dag, FlowGraph.place, FlowPolicy.set_static_paths).") .def("resolve_to_paths", [](const PredDAG& dag, std::int32_t src, std::int32_t dst, bool split_parallel_edges, py::object max_paths){ std::optional mp; if (!max_paths.is_none()) mp = py::cast(max_paths); @@ -594,6 +604,17 @@ PYBIND11_MODULE(_netgraph_core, m, py::mod_gil_not_used()) { .def("rebalance_demand", [](FlowPolicy& p, FlowGraph& fg, std::int32_t src, std::int32_t dst, FlowClass flowClass, double target){ py::gil_scoped_release rel; auto pr = p.rebalance_demand(fg, src, dst, flowClass, target); py::gil_scoped_acquire acq; return py::make_tuple(pr.first, pr.second); }, py::arg("flow_graph"), py::arg("src"), py::arg("dst"), py::arg("flowClass"), py::arg("target")) .def("remove_demand", [](FlowPolicy& p, FlowGraph& fg){ py::gil_scoped_release rel; p.remove_demand(fg); py::gil_scoped_acquire acq; }) + .def("set_static_paths", [](FlowPolicy& p, std::int32_t src, std::int32_t dst, py::sequence paths){ + std::vector bundles; + bundles.reserve(py::len(paths)); + for (auto item : paths) bundles.push_back(py::cast(item)); + py::gil_scoped_release rel; + p.set_static_paths(src, dst, std::move(bundles)); + py::gil_scoped_acquire acq; + }, py::arg("src"), py::arg("dst"), py::arg("paths"), + "Pin this policy's demand to explicit path bundles (one flow per usable " + "bundle, bound in supply order). See FlowPolicy docs for validation, mask " + "(failure) semantics, and which config knobs become inert.") .def_property_readonly("flows", [](const FlowPolicy& p){ py::dict out; for (auto const& kv : p.flows()) { const auto& idx = kv.first; const auto& f = kv.second; out[py::make_tuple(idx.src, idx.dst, idx.flowClass, idx.flowId)] = py::make_tuple(f.src, f.dst, f.cost, f.placed_flow); } return out; }); // Profiling functions (enabled via NGRAPH_CORE_PROFILE=1 environment variable) diff --git a/include/netgraph/core/flow_policy.hpp b/include/netgraph/core/flow_policy.hpp index 7502962..32e1ea2 100644 --- a/include/netgraph/core/flow_policy.hpp +++ b/include/netgraph/core/flow_policy.hpp @@ -106,7 +106,11 @@ class FlowPolicy { // `fg` must wrap the same StrictMultiDiGraph as the policy's own graph handle; // placing onto a FlowGraph built from a different graph is rejected. // - // Returns (total_placed, remaining_volume). + // Returns (placed, remaining). NOTE for EqualBalanced: when the equalizing + // rebalance runs, `placed` is the policy's TOTAL placed demand (cumulative + // across calls, since rebalancing re-places previously placed volume too) and + // `placed + remaining` can therefore exceed this call's `volume`. Use + // placed_demand() deltas for strict per-call accounting. [[nodiscard]] std::pair place_demand(FlowGraph& fg, NodeId src, NodeId dst, FlowClass flowClass, @@ -125,15 +129,49 @@ class FlowPolicy { [[nodiscard]] const std::unordered_map& flows() const noexcept { return flows_; } -// Configure static paths for flow creation. Each entry is (src, dst, dag, cost). -// max_flow_count must equal the number of static paths if set. - void set_static_paths(std::vector> paths); + // Pin this policy's demand to explicit path bundles (MPLS-style routing). + // + // One flow is created per USABLE bundle at the next placement, in supply order, + // each permanently bound to its own bundle. A single-path bundle (e.g. from + // make_path_dag / PredDAG.from_edges or ksp) models a strict-ERO LSP: any failed + // hop takes it down. A multi-walk bundle (e.g. an SPF DAG) models a pinned DAG / + // SR-TE-style policy that renormalizes over its surviving walks. + // + // Each bundle is validated against the policy's graph (shape, id ranges, + // edge-endpoint consistency, acyclicity, and an src->dst walk), then pruned + // against the policy's node/edge masks. A bundle with no surviving src->dst walk + // is DOWN: it creates no flow and carries nothing (a pinned path does not + // reroute around failures). Each flow's cost is the min-cost src->dst walk of + // its PRUNED bundle, computed from the graph's edge costs. + // + // With static paths configured the policy neither creates additional flows nor + // reoptimizes: max_path_cost, max_path_cost_factor, min_flow_count and + // reoptimize_flows_on_each_placement are inert. EqualBalanced spreads over the + // usable (up) bundles only. flow_count() reports the usable count U; the + // supplied count N is the caller's, so down LSPs = N - flow_count(). + // + // Throws std::invalid_argument if bundles is empty, the policy already holds + // flows (remove_demand() first), shortest_path=true is configured (single- + // augmentation IP semantics contradict pinned multi-LSP placement), src/dst is + // invalid or src == dst, a user-supplied max_flow_count differs from + // bundles.size(), or any bundle is malformed. Calling again before placement + // replaces the previous pinning. + void set_static_paths(NodeId src, NodeId dst, std::vector bundles); private: // Helpers // Throws std::invalid_argument if fg wraps a different graph than this policy, or if // (src, dst) differs from the demand already managed here. `what` names the caller. void check_demand_target(const FlowGraph& fg, NodeId src, NodeId dst, const char* what) const; + // Placement core: target computation, flow creation, round-robin placement, and + // the optional post-placement reoptimization. The public place_demand() wraps it + // with the iterative EqualBalanced rebalance rounds. + [[nodiscard]] std::pair place_demand_body(FlowGraph& fg, + NodeId src, NodeId dst, + FlowClass flowClass, + double volume, + std::optional target_per_flow, + std::optional min_flow); [[nodiscard]] std::optional> get_path_bundle(const FlowGraph& fg, NodeId src, NodeId dst, std::optional min_flow); @@ -171,8 +209,17 @@ class FlowPolicy { Cost best_path_cost_ { std::numeric_limits::max() }; FlowId next_flow_id_ { 0 }; - // Static paths (optional) - std::vector> static_paths_; + // Static paths (optional): usable (mask-pruned) bundles, one flow per bundle. + struct StaticBundle { + PredDAG dag; // pruned to mask-surviving entries + Cost cost {0}; // min-cost src->dst walk within the pruned bundle + }; + NodeId static_src_ {-1}; + NodeId static_dst_ {-1}; + int static_supplied_count_ {0}; // N as supplied; usable U = static_bundles_.size() + std::vector static_bundles_; + + [[nodiscard]] bool has_static_paths() const noexcept { return !static_bundles_.empty() || static_supplied_count_ > 0; } }; } // namespace netgraph::core diff --git a/include/netgraph/core/shortest_paths.hpp b/include/netgraph/core/shortest_paths.hpp index fad97cb..6c91d3d 100644 --- a/include/netgraph/core/shortest_paths.hpp +++ b/include/netgraph/core/shortest_paths.hpp @@ -59,6 +59,27 @@ shortest_paths(const StrictMultiDiGraph& g, NodeId src, std::span node_mask = {}, std::span edge_mask = {}); +// Build the SPF-compatible (distances, PredDAG) pair for one concrete path given +// as node/edge sequences (nodes[i] -> nodes[i+1] via edges[i]; so nodes.size() == +// edges.size() + 1, or a single node with no edges). Distances are cumulative edge +// costs along the path, INT64_MAX elsewhere; the DAG stores one parent per visited +// node. Inputs are trusted (no validation) -- used by KSP's result conversion. +[[nodiscard]] std::pair, PredDAG> +path_to_pred_dag(const StrictMultiDiGraph& g, + std::span nodes, + std::span edges); + +// Build a single-path PredDAG from a contiguous edge sequence (the missing +// constructor for operator-defined explicit paths; PredDAGs from SPF/KSP work +// directly wherever a PredDAG is accepted). +// +// Throws std::invalid_argument if edges is empty, any id is out of range, the +// sequence is not contiguous (dst(e_i) != src(e_{i+1})), or the walk repeats a +// node (a repeated node cannot be represented as one-parent-per-node, and a +// pinned path is a simple path by definition). +[[nodiscard]] PredDAG make_path_dag(const StrictMultiDiGraph& g, + std::span edges); + // Enumerate concrete paths represented by a PredDAG from src to dst. // Each path is returned as a sequence of (node_id, (edge_ids...)) pairs ending with (dst, ()). // When split_parallel_edges=false, parallel edges per hop are grouped in the tuple. diff --git a/python/netgraph_core/_docs.py b/python/netgraph_core/_docs.py index 68c6e84..e0174be 100644 --- a/python/netgraph_core/_docs.py +++ b/python/netgraph_core/_docs.py @@ -9,7 +9,7 @@ from __future__ import annotations -from typing import TYPE_CHECKING, ClassVar, Optional +from typing import TYPE_CHECKING, ClassVar, Optional, Sequence if TYPE_CHECKING: # only for typing; runtime comes from extension import numpy as np # type: ignore[reportMissingImports] @@ -79,6 +79,22 @@ class PredDAG: parents: np.ndarray via_edges: np.ndarray + @staticmethod + def from_edges(graph: "StrictMultiDiGraph", edges: "Sequence[int]") -> "PredDAG": + """Build a single-path PredDAG from a contiguous edge-id sequence. + + The missing constructor for operator-defined explicit paths; PredDAGs + returned by Algorithms.spf/ksp work directly wherever a PredDAG is + accepted (FlowState.place_on_dag, FlowGraph.place, + FlowPolicy.set_static_paths). + + Raises: + ValueError: If edges is empty, an id is out of range, consecutive + edges do not connect, or the walk revisits a node (a pinned + path must be a simple path). + """ + ... + def resolve_to_paths( self, src: int, @@ -409,9 +425,12 @@ class FlowPolicyConfig: class FlowPolicy: """Flow policy for demand placement. - When static_paths is empty the policy may refresh the DAG per round using - residual-aware shortest paths. This progressively prunes saturated next-hops - (traffic-engineering style) and differs from one-shot ECMP admission. + Dynamic policies may refresh the DAG per round using residual-aware + shortest paths, progressively pruning saturated next-hops (traffic- + engineering style), which differs from one-shot ECMP admission. + + A policy can instead be pinned to explicit path bundles with + set_static_paths(); see that method for the pinned semantics. Args: algorithms: Algorithms instance (kept alive by FlowPolicy) @@ -445,7 +464,18 @@ def place_demand( volume: float, target_per_flow: Optional[float] = None, min_flow: Optional[float] = None, - ) -> tuple[float, float]: ... + ) -> tuple[float, float]: + """Place `volume` of demand; returns (placed, remaining). + + EQUAL_BALANCED note: when the equalizing rebalance runs, `placed` is + the policy's TOTAL placed demand (cumulative across calls, because + rebalancing re-places previously placed volume too), so + `placed + remaining` can exceed this call's `volume`. Use + placed_demand() deltas for strict per-call accounting. EB placement is + also not incremental: each call's volume defines the per-flow target, + so a second, smaller call may place nothing. + """ + ... def rebalance_demand( self, @@ -457,6 +487,39 @@ def rebalance_demand( ) -> tuple[float, float]: ... def remove_demand(self, flow_graph: "FlowGraph") -> None: ... + def set_static_paths(self, src: int, dst: int, paths: "Sequence[PredDAG]") -> None: + """Pin this policy's demand to explicit path bundles (MPLS-style). + + One flow is created per USABLE bundle at the next placement, in supply + order, each permanently bound to its own bundle. A single-path bundle + (from PredDAG.from_edges or ksp) models a strict-ERO LSP: any failed + hop takes it down. A multi-walk bundle (an SPF DAG) models a pinned + DAG / SR-TE-style policy that renormalizes over surviving walks. + + Bundles are validated against the policy's graph, then pruned against + the policy's node/edge masks; a bundle with no surviving src->dst walk + is DOWN and creates no flow (pinned paths do not reroute around + failures). Each flow's cost is the min-cost src->dst walk of its + PRUNED bundle. EqualBalanced spreads over the usable (up) bundles + only; down LSPs = len(paths) - flow_count(). + + With static paths configured the policy neither creates additional + flows nor reoptimizes: max_path_cost, max_path_cost_factor, + min_flow_count and reoptimize_flows_on_each_placement are inert. + Placement is not incremental across place_demand calls for + EqualBalanced (each call's volume defines the per-flow target). + + Raises: + ValueError: If paths is empty; the policy already holds flows + (call remove_demand() first); shortest_path=True is + configured; src/dst is out of range or equal; a user-supplied + max_flow_count differs from len(paths); or a bundle is + malformed (wrong shape, out-of-range ids, an edge that does + not connect its parent to its node in this graph, a cycle, or + no src->dst walk). Calling again before placement replaces + the previous pinning. + """ + ... @property def flows(self) -> dict[tuple[int, int, int, int], tuple[int, int, int, float]]: ... diff --git a/src/flow_policy.cpp b/src/flow_policy.cpp index 9062c07..3e1a773 100644 --- a/src/flow_policy.cpp +++ b/src/flow_policy.cpp @@ -61,16 +61,10 @@ double FlowPolicy::placed_demand() const noexcept { std::optional> FlowPolicy::get_path_bundle(const FlowGraph& fg, NodeId src, NodeId dst, std::optional min_flow) { - // Static path handling: if static paths are configured, use them exclusively. - if (!static_paths_.empty()) { - // Search for a static path matching this (src, dst) pair. - for (auto const& t : static_paths_) { - if (std::get<0>(t) == src && std::get<1>(t) == dst) { - return std::make_optional(std::make_pair(std::get<2>(t), std::get<3>(t))); - } - } - return std::nullopt; // No static path for this pair - } + // With static paths, flows are created directly from the pinned bundles in + // place_demand_body and never reoptimized, so no dynamic bundle exists to hand + // out. Callers reaching here with static paths configured get nothing. + if (has_static_paths()) return std::nullopt; if (path_alg_ != PathAlg::SPF) return std::nullopt; // Use configured selection for per-adjacency edge behavior (multi-edge, tie-breaking). @@ -151,9 +145,19 @@ std::optional> FlowPolicy::get_path_bundle(const FlowGr // max_path_cost: absolute upper bound on path cost. // max_path_cost_factor: relative multiplier on best path cost (e.g. 1.5 = allow 50% longer). if (max_path_cost_.has_value() || max_path_cost_factor_.has_value()) { - double maxf = max_path_cost_factor_.value_or(1.0); - Cost absmax = max_path_cost_.value_or(std::numeric_limits::max()); - if (dst_cost > std::min(absmax, static_cast(static_cast(best_path_cost_) * maxf))) return std::nullopt; + const Cost absmax = max_path_cost_.value_or(std::numeric_limits::max()); + // Apply the relative bound only when a best cost exists: multiplying the + // INT64_MAX "no best yet" sentinel by the factor and casting back is + // undefined behavior (the product exceeds the int64 range). + Cost factor_bound = std::numeric_limits::max(); + if (max_path_cost_factor_.has_value() && + best_path_cost_ != std::numeric_limits::max()) { + const double prod = static_cast(best_path_cost_) * *max_path_cost_factor_; + if (prod < static_cast(std::numeric_limits::max())) { + factor_bound = static_cast(prod); + } + } + if (dst_cost > std::min(absmax, factor_bound)) return std::nullopt; } // Ensure there is at least one predecessor for dst if (static_cast(dst) >= dag.parent_offsets.size()-1) return std::nullopt; @@ -189,6 +193,11 @@ FlowRecord* FlowPolicy::create_flow(FlowGraph& fg, NodeId src, NodeId dst, FlowC Reoptimization is useful when a flow's current path becomes suboptimal due to network changes or when seeking additional capacity. */ FlowRecord* FlowPolicy::reoptimize_flow(FlowGraph& fg, const FlowIndex& idx, double headroom) { + // Pinned means pinned: a static-path flow is never rerouted. (Deliberate + // divergence from the original Python port, where reoptimization could + // silently move a pinned flow onto an SPF path.) Must precede any + // remove/re-place churn below. + if (has_static_paths()) return nullptr; auto it = flows_.find(idx); if (it == flows_.end()) return nullptr; FlowRecord& cur = it->second; @@ -228,15 +237,81 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, check_demand_target(fg, src, dst, "place_demand"); + auto [total_placed, remaining] = place_demand_body(fg, src, dst, flowClass, volume, + target_per_flow, min_flow); + + // For EQUAL_BALANCED placement, rebalance flows to maintain equal volumes. + // + // Iterative on purpose: the previous implementation recursed + // place_demand -> rebalance_demand -> place_demand until balanced, and with many + // pinned bundles of heterogeneous capacity the depth grows like + // U * ln(imbalance / kMinFlow) -- a stack-overflow risk on worker threads. Each + // round below performs exactly what one recursion level performed (re-place the + // currently placed volume at the equal-share target), and the recursion's return + // value telescoped to (placed_demand(), pre-rebalance leftover + volume lost in + // rebalancing), which is reproduced after the loop. + if (flow_placement_ == FlowPlacement::EqualBalanced && !flows_.empty()) { + // Restore the reoptimize flag even if a round throws (bad_alloc is the only + // realistic thrower here); otherwise the policy would stay permanently + // non-reoptimizing. + struct ReoptRestore { + bool& flag; + bool prev; + ~ReoptRestore() { flag = prev; } + } reopt_restore{reoptimize_flows_on_each_placement_, reoptimize_flows_on_each_placement_}; + reoptimize_flows_on_each_placement_ = false; + const double pre_rounds_placed = placed_demand(); + bool rebalanced = false; + // Backstop only; each round strictly reduces the imbalance in practice. + constexpr int kMaxRebalanceRounds = 65536; + for (int round = 0; round < kMaxRebalanceRounds && !flows_.empty(); ++round) { + const double target_eq = placed_demand() / static_cast(flows_.size()); + bool unbalanced = false; + for (auto const& kv : flows_) { + if (std::abs(target_eq - kv.second.placed_flow) >= kMinFlow) { unbalanced = true; break; } + } + if (!unbalanced) break; + rebalanced = true; + const double vol = placed_demand(); + remove_demand(fg); + (void)place_demand_body(fg, src, dst, flowClass, vol, target_eq, std::nullopt); + } + if (rebalanced) { + const double final_placed = placed_demand(); + remaining += pre_rounds_placed - final_placed; // volume shed while rebalancing + total_placed = final_placed; + } + } + return { total_placed, remaining }; +} + +/* Placement core; see place_demand for the public contract. */ +std::pair FlowPolicy::place_demand_body(FlowGraph& fg, + NodeId src, NodeId dst, + FlowClass flowClass, + double volume, + std::optional target_per_flow, + std::optional min_flow) { + const bool is_static = has_static_paths(); + // Compute target flow per flow-record. // target: the volume to place per flow (or globally if target_per_flow is unset). // per_target: refined target for EqualBalanced mode (considers source capacity). double target = target_per_flow.value_or(volume); double per_target = target; - // For EqualBalanced mode with max_flow_count, compute a per-flow target based on - // available source capacity and the requested volume, divided by the number of flows. - if (flow_placement_ == FlowPlacement::EqualBalanced && max_flow_count_.has_value()) { + // EqualBalanced divisor: the number of flows the volume is split across. For a + // pinned policy that is the number of USABLE bundles (a head-end hashes over up + // LSPs only); dynamically it is the configured max_flow_count. + int eb_divisor = 0; + if (flow_placement_ == FlowPlacement::EqualBalanced) { + if (is_static) eb_divisor = static_cast(static_bundles_.size()); + else if (max_flow_count_.has_value()) eb_divisor = *max_flow_count_; + } + + // For EqualBalanced, compute a per-flow target based on available source + // capacity and the requested volume, divided by the number of flows. + if (eb_divisor > 0) { const auto& g = fg.graph(); auto row = g.row_offsets_view(); auto aei = g.adj_edge_index_view(); @@ -252,27 +327,24 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, src_cap += static_cast(residual[eid]); } } - // Compute per-flow target as the minimum of: - // - requested volume / max_flow_count - // - source capacity / max_flow_count - double per_req = target / static_cast(*max_flow_count_); - double per_src = src_cap / static_cast(*max_flow_count_); + // Per-flow target: min of requested volume and source capacity, per flow. + double per_req = target / static_cast(eb_divisor); + double per_src = src_cap / static_cast(eb_divisor); per_target = std::max(kMinFlow, std::min(per_req, per_src)); } // Initialize flows if none exist yet. if (flows_.empty()) { - if (!static_paths_.empty()) { - // Static paths: create one flow per static path. - if (max_flow_count_.has_value() && static_cast(static_paths_.size()) != *max_flow_count_) { - throw std::invalid_argument("If set, max_flow_count must be equal to the number of static paths."); + if (is_static) { + // Pinned bundles: one flow per usable bundle, in supply order, each bound to + // its own (pruned) DAG and post-prune cost. + if (src != static_src_ || dst != static_dst_) { + throw std::invalid_argument( + "Source and destination nodes of static paths do not match demand."); } - for (auto const& t : static_paths_) { - if (std::get<0>(t) == src && std::get<1>(t) == dst) { - [[maybe_unused]] auto* created = create_flow(fg, src, dst, flowClass, std::nullopt); - } else { - throw std::invalid_argument("Source and destination nodes of static paths do not match demand."); - } + for (auto const& b : static_bundles_) { + FlowIndex idx{src, dst, flowClass, next_flow_id_++}; + flows_.emplace(idx, FlowRecord(idx, src, dst, b.dag, b.cost)); } } else { // Dynamic paths: seed initial flows. @@ -300,6 +372,13 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, // Round-robin placement: iterate over flows and try to place volume on each. std::deque q; for (auto const& kv : flows_) q.push_back(kv.first); + if (is_static) { + // Pinned bundles have a documented precedence: bundle-supply order. Flow ids + // are assigned in that order, so sorting makes the visit order deterministic + // across platforms (unordered_map iteration order is not). + std::sort(q.begin(), q.end(), + [](const FlowIndex& a, const FlowIndex& b) { return a.flowId < b.flowId; }); + } double total_placed = 0.0; int no_progress = 0; // counter for consecutive iterations with no progress int iters = 0; @@ -325,7 +404,7 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, // For multipath flows, this tracks saturated edges within the DAG. // For tunnel flows, this allows different tunnels to discover different paths // as residuals change, enabling natural fan-out across equal-cost paths. - if (flow_placement_ == FlowPlacement::EqualBalanced && static_paths_.empty()) { + if (flow_placement_ == FlowPlacement::EqualBalanced && !is_static) { if (auto pb = get_path_bundle(fg, f->src, f->dst, std::optional(per_target))) { f->dag = std::move(pb->first); f->cost = pb->second; @@ -335,7 +414,7 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, if (target_per_flow.has_value()) { // When a per-flow target is specified (e.g., during rebalancing), cap by remaining per-flow target. need = std::max(0.0, target - f->placed_flow); - } else if (flow_placement_ == FlowPlacement::EqualBalanced && max_flow_count_.has_value()) { + } else if (eb_divisor > 0) { // For EqualBalanced, request only the remaining deficit toward per-target for this flow. need = std::max(0.0, per_target - f->placed_flow); } else { @@ -352,8 +431,11 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, if (shortest_path_) { break; } - // track recent placements - if (diminishing_returns_enabled_) { + // Track recent placements. For a pinned policy, arm the window only once every + // flow has been visited: with more bundles than the window, the early rounds of + // small per-flow placements must not starve the bundles not yet visited. + if (diminishing_returns_enabled_ && + (!is_static || iters >= static_cast(flows_.size()))) { recent.push_back(placed); if (static_cast(recent.size()) > diminishing_returns_window_) recent.pop_front(); if (static_cast(recent.size()) == diminishing_returns_window_) { @@ -369,52 +451,39 @@ std::pair FlowPolicy::place_demand(FlowGraph& fg, } else { no_progress = 0; } - if (flow_placement_ == FlowPlacement::EqualBalanced) { - if (max_flow_count_.has_value()) { - // Bounded EB: add flows up to configured maximum. - if (static_cast(flows_.size()) < *max_flow_count_) { - if (auto* nf = create_flow(fg, src, dst, flowClass, std::optional(per_target))) q.push_back(nf->index); + // A pinned policy neither grows its flow set nor reoptimizes: the pinned-ness + // guard is explicit, never inferred from flow-count arithmetic. + if (!is_static) { + if (flow_placement_ == FlowPlacement::EqualBalanced) { + if (max_flow_count_.has_value()) { + // Bounded EB: add flows up to configured maximum. + if (static_cast(flows_.size()) < *max_flow_count_) { + if (auto* nf = create_flow(fg, src, dst, flowClass, std::optional(per_target))) q.push_back(nf->index); + } + } else { + // Unbounded EB: rely on a single flow to equalize over the DAG. + // Do not create additional flows implicitly. } } else { - // Unbounded EB: rely on a single flow to equalize over the DAG. - // Do not create additional flows implicitly. - } - } else { - if (target - f->placed_flow >= kMinFlow) { - if (!max_flow_count_ || static_cast(flows_.size()) < *max_flow_count_) { - if (auto* nf = create_flow(fg, src, dst, flowClass, std::nullopt)) q.push_back(nf->index); - } else { - if (auto* rf = reoptimize_flow(fg, f->index, kMinFlow)) q.push_back(rf->index); + if (target - f->placed_flow >= kMinFlow) { + if (!max_flow_count_ || static_cast(flows_.size()) < *max_flow_count_) { + if (auto* nf = create_flow(fg, src, dst, flowClass, std::nullopt)) q.push_back(nf->index); + } else { + if (auto* rf = reoptimize_flow(fg, f->index, kMinFlow)) q.push_back(rf->index); + } } } } if (iters >= max_total_iterations_) break; } - // Reoptimize all flows after placement if enabled + // Reoptimize all flows after placement if enabled (no-op for pinned policies: + // reoptimize_flow returns immediately when static paths are configured). if (reoptimize_flows_on_each_placement_) { for (auto& kv : flows_) { (void)reoptimize_flow(fg, kv.first, kMinFlow); } } - - // For EQUAL_BALANCED placement, rebalance flows to maintain equal volumes. - if (flow_placement_ == FlowPlacement::EqualBalanced && !flows_.empty()) { - double target_eq = placed_demand() / static_cast(flows_.size()); - bool unbalanced = false; - for (auto const& kv : flows_) { - if (std::abs(target_eq - kv.second.placed_flow) >= kMinFlow) { unbalanced = true; break; } - } - if (unbalanced) { - bool prev_reopt = reoptimize_flows_on_each_placement_; - reoptimize_flows_on_each_placement_ = false; - auto pr = rebalance_demand(fg, src, dst, flowClass, target_eq); - // pr.first = placed in rebalanced pass, pr.second = excess - volume += pr.second; // leave remaining volume - reoptimize_flows_on_each_placement_ = prev_reopt; - total_placed = placed_demand(); - } - } return { total_placed, volume }; } @@ -443,16 +512,253 @@ void FlowPolicy::remove_demand(FlowGraph& fg) { best_path_cost_ = std::numeric_limits::max(); } -/* Configure static paths to be used instead of dynamic SPF selection. If - `max_flow_count` is not set, it is set to the number of provided paths. */ -void FlowPolicy::set_static_paths(std::vector> paths) { - static_paths_ = std::move(paths); - if (max_flow_count_.has_value() && static_cast(static_paths_.size()) != *max_flow_count_) { +namespace { + +/* Validate one pinned bundle against the graph, prune it against the policy's + masks, and compute its post-prune min cost. Throws std::invalid_argument on a + malformed bundle; returns nullopt for a structurally valid bundle that has no + surviving src->dst walk under the masks (a DOWN LSP). */ +std::optional> +prepare_static_bundle(const StrictMultiDiGraph& g, const PredDAG& dag, + NodeId src, NodeId dst, + std::span node_mask, + std::span edge_mask) { + const auto N = static_cast(g.num_nodes()); + const auto E = static_cast(g.num_edges()); + const auto& off = dag.parent_offsets; + const auto& par = dag.parents; + const auto& via = dag.via_edges; + + // Shape: CSR offsets over N nodes, entry arrays sized by the final offset. + if (off.size() != N + 1 || off.front() != 0) { + throw std::invalid_argument("static path bundle: parent_offsets must have length num_nodes + 1 and start at 0"); + } + for (std::size_t v = 0; v + 1 < off.size(); ++v) { + if (off[v] > off[v + 1]) { + throw std::invalid_argument("static path bundle: parent_offsets must be non-decreasing"); + } + } + const auto entries = static_cast(off.back()); + if (par.size() != entries || via.size() != entries) { + throw std::invalid_argument("static path bundle: parents/via_edges size must equal parent_offsets.back()"); + } + + // Entries: ids in range, and each via edge must actually connect parent -> child + // in THIS graph. Without the endpoint check, a well-shaped DAG built for a + // different graph (or with shuffled edge ids) would silently place flow on + // arbitrary edges. + const auto esrc = g.edge_src_view(); + const auto edst = g.edge_dst_view(); + for (std::size_t v = 0; v < N; ++v) { + for (auto i = static_cast(off[v]); i < static_cast(off[v + 1]); ++i) { + const auto pnode = par[i]; + const auto e = via[i]; + if (pnode < 0 || static_cast(pnode) >= N) { + throw std::invalid_argument("static path bundle: parent node id out of range"); + } + if (e < 0 || static_cast(e) >= E) { + throw std::invalid_argument("static path bundle: via edge id out of range"); + } + if (esrc[static_cast(e)] != pnode || + edst[static_cast(e)] != static_cast(v)) { + throw std::invalid_argument( + "static path bundle: via edge does not connect its parent to its node in " + "this graph (bundle built for a different graph?)"); + } + } + } + + // Acyclicity (Kahn over parent -> child entries). + { + std::vector indeg(N, 0); + for (std::size_t v = 0; v < N; ++v) { + indeg[v] = static_cast(off[v + 1] - off[v]); + } + std::vector stack; + stack.reserve(N); + for (std::size_t v = 0; v < N; ++v) { + if (indeg[v] == 0) stack.push_back(static_cast(v)); + } + std::size_t seen = 0; + // Child adjacency: parent -> list of children, derived on the fly. + std::vector> children(N); + for (std::size_t v = 0; v < N; ++v) { + for (auto i = static_cast(off[v]); i < static_cast(off[v + 1]); ++i) { + children[static_cast(par[i])].push_back(static_cast(v)); + } + } + while (!stack.empty()) { + auto u = stack.back(); stack.pop_back(); + ++seen; + for (auto v : children[static_cast(u)]) { + if (--indeg[static_cast(v)] == 0) stack.push_back(v); + } + } + if (seen != N) { + throw std::invalid_argument("static path bundle: predecessor structure contains a cycle"); + } + } + + // Structural src->dst connectivity ignoring masks: a bundle that never had an + // src->dst walk is malformed for this demand, distinct from being down. + auto backward_reaches_src = [&](auto&& admit_entry) { + std::vector reach(N, 0); + std::vector bfs; + bfs.push_back(dst); + reach[static_cast(dst)] = 1; + for (std::size_t head = 0; head < bfs.size(); ++head) { + const auto v = static_cast(bfs[head]); + for (auto i = static_cast(off[v]); i < static_cast(off[v + 1]); ++i) { + if (!admit_entry(i)) continue; + const auto pnode = par[i]; + if (!reach[static_cast(pnode)]) { + reach[static_cast(pnode)] = 1; + bfs.push_back(pnode); + } + } + } + return reach[static_cast(src)] != 0; + }; + if (!backward_reaches_src([](std::size_t) { return true; })) { + throw std::invalid_argument("static path bundle: no src->dst walk exists in the bundle"); + } + + // Prune against the policy's masks (failure exclusions), then re-check + // connectivity: no surviving walk means the LSP is DOWN. + const bool use_nm = !node_mask.empty(); + const bool use_em = !edge_mask.empty(); + auto entry_up = [&](std::size_t i) { + const auto pnode = static_cast(par[i]); + const auto e = static_cast(via[i]); + if (use_em && !edge_mask[e]) return false; + if (use_nm && !node_mask[pnode]) return false; + return true; + }; + auto node_up = [&](NodeId v) { return !use_nm || node_mask[static_cast(v)]; }; + if (!node_up(src) || !node_up(dst)) return std::nullopt; + + PredDAG pruned; + pruned.parent_offsets.assign(N + 1, 0); + for (std::size_t v = 0; v < N; ++v) { + std::int32_t c = 0; + if (node_up(static_cast(v))) { + for (auto i = static_cast(off[v]); i < static_cast(off[v + 1]); ++i) { + if (entry_up(i)) ++c; + } + } + pruned.parent_offsets[v + 1] = pruned.parent_offsets[v] + c; + } + pruned.parents.resize(static_cast(pruned.parent_offsets.back())); + pruned.via_edges.resize(static_cast(pruned.parent_offsets.back())); + for (std::size_t v = 0; v < N; ++v) { + auto w = static_cast(pruned.parent_offsets[v]); + if (!node_up(static_cast(v))) continue; + for (auto i = static_cast(off[v]); i < static_cast(off[v + 1]); ++i) { + if (!entry_up(i)) continue; + pruned.parents[w] = par[i]; + pruned.via_edges[w] = via[i]; + ++w; + } + } + + const auto& poff = pruned.parent_offsets; + { + std::vector reach(N, 0); + std::vector bfs; + bfs.push_back(dst); + reach[static_cast(dst)] = 1; + for (std::size_t head = 0; head < bfs.size(); ++head) { + const auto v = static_cast(bfs[head]); + for (auto i = static_cast(poff[v]); i < static_cast(poff[v + 1]); ++i) { + const auto pnode = pruned.parents[i]; + if (!reach[static_cast(pnode)]) { + reach[static_cast(pnode)] = 1; + bfs.push_back(pnode); + } + } + } + if (!reach[static_cast(src)]) return std::nullopt; // DOWN + } + + // Cost: min-cost src->dst walk within the PRUNED bundle, so the flow reports the + // metric of a walk it can actually use. DP in a topological order of the pruned + // DAG (acyclicity established above; pruning cannot introduce cycles). + Cost best = std::numeric_limits::max(); + { + const auto costs = g.cost_view(); + std::vector dist(N, std::numeric_limits::max()); + dist[static_cast(src)] = 0; + std::vector indeg(N, 0); + std::vector>> children(N); + for (std::size_t v = 0; v < N; ++v) { + indeg[v] = static_cast(poff[v + 1] - poff[v]); + for (auto i = static_cast(poff[v]); i < static_cast(poff[v + 1]); ++i) { + children[static_cast(pruned.parents[i])].emplace_back( + static_cast(v), pruned.via_edges[i]); + } + } + std::vector stack; + for (std::size_t v = 0; v < N; ++v) { + if (indeg[v] == 0) stack.push_back(static_cast(v)); + } + while (!stack.empty()) { + auto u = stack.back(); stack.pop_back(); + const auto du = dist[static_cast(u)]; + for (auto [v, e] : children[static_cast(u)]) { + if (du != std::numeric_limits::max()) { + const Cost cand = du + costs[static_cast(e)]; + if (cand < dist[static_cast(v)]) dist[static_cast(v)] = cand; + } + if (--indeg[static_cast(v)] == 0) stack.push_back(v); + } + } + best = dist[static_cast(dst)]; + } + return std::make_optional(std::make_pair(std::move(pruned), best)); +} + +} // namespace + +/* See flow_policy.hpp for the contract. */ +void FlowPolicy::set_static_paths(NodeId src, NodeId dst, std::vector bundles) { + const auto* g = ctx_.graph.graph.get(); + if (g == nullptr) { + throw std::invalid_argument("FlowPolicy::set_static_paths: policy has no graph"); + } + if (!flows_.empty()) { + throw std::invalid_argument( + "FlowPolicy::set_static_paths: policy already holds flows; call remove_demand() first"); + } + if (bundles.empty()) { + throw std::invalid_argument("FlowPolicy::set_static_paths: bundles must be non-empty"); + } + if (shortest_path_) { + throw std::invalid_argument( + "FlowPolicy::set_static_paths: incompatible with shortest_path=true (single-" + "augmentation IP semantics contradict pinned multi-LSP placement)"); + } + const auto N = g->num_nodes(); + if (src < 0 || src >= N || dst < 0 || dst >= N || src == dst) { + throw std::invalid_argument("FlowPolicy::set_static_paths: src/dst out of range or equal"); + } + // Validate the USER's max_flow_count against the supplied bundle count. The + // configured value is never overwritten: the usable (up) count U drives the + // per-flow math at placement time, and N - flow_count() is the down-LSP count. + if (max_flow_count_.has_value() && *max_flow_count_ != static_cast(bundles.size())) { throw std::invalid_argument("If set, max_flow_count must be equal to the number of static paths."); } - if (!max_flow_count_.has_value()) { - max_flow_count_ = static_cast(static_paths_.size()); + + std::vector usable; + usable.reserve(bundles.size()); + for (auto const& dag : bundles) { + if (auto prepared = prepare_static_bundle(*g, dag, src, dst, node_mask_, edge_mask_)) { + usable.push_back(StaticBundle{std::move(prepared->first), prepared->second}); + } } + static_src_ = src; + static_dst_ = dst; + static_supplied_count_ = static_cast(bundles.size()); + static_bundles_ = std::move(usable); } } // namespace netgraph::core diff --git a/src/k_shortest_paths.cpp b/src/k_shortest_paths.cpp index bb6c8f9..ce410e6 100644 --- a/src/k_shortest_paths.cpp +++ b/src/k_shortest_paths.cpp @@ -218,29 +218,8 @@ std::vector, PredDAG>> k_shortest_paths( // Convert and return std::vector, PredDAG>> items; items.reserve(paths.size()); - auto cost_view = g.cost_view(); for (auto const& P : paths) { - std::vector dist(static_cast(g.num_nodes()), std::numeric_limits::max()); - PredDAG dag; - dag.parent_offsets.assign(static_cast(g.num_nodes() + 1), 0); - // Fill distances along path and one-parent predecessors - if (!P.nodes.empty()) { - dist[static_cast(P.nodes.front())] = 0; - for (std::size_t i = 1; i < P.nodes.size(); ++i) { - auto u = P.nodes[i-1]; auto v = P.nodes[i]; auto e = P.edges[i-1]; - dist[static_cast(v)] = dist[static_cast(u)] + static_cast(cost_view[static_cast(e)]); - dag.parent_offsets[static_cast(v+1)] = 1; - } - for (std::size_t v = 1; v < dag.parent_offsets.size(); ++v) dag.parent_offsets[v] += dag.parent_offsets[v-1]; - dag.parents.resize(static_cast(dag.parent_offsets.back())); - dag.via_edges.resize(static_cast(dag.parent_offsets.back())); - for (std::size_t i = 1; i < P.nodes.size(); ++i) { - auto v = P.nodes[i]; - auto base = static_cast(dag.parent_offsets[static_cast(v)]); - dag.parents[base] = P.nodes[i-1]; dag.via_edges[base] = P.edges[i-1]; - } - } - items.emplace_back(std::move(dist), std::move(dag)); + items.push_back(path_to_pred_dag(g, P.nodes, P.edges)); } return items; } @@ -362,26 +341,7 @@ std::vector, PredDAG>> k_shortest_paths( std::vector, PredDAG>> items; items.reserve(paths.size()); for (auto const& P : paths) { - std::vector dist(static_cast(g.num_nodes()), std::numeric_limits::max()); - PredDAG dag; - dag.parent_offsets.assign(static_cast(g.num_nodes() + 1), 0); - if (!P.nodes.empty()) { - dist[static_cast(P.nodes.front())] = 0; - for (std::size_t i = 1; i < P.nodes.size(); ++i) { - auto u = P.nodes[i-1]; auto v = P.nodes[i]; auto e = P.edges[i-1]; - dist[static_cast(v)] = dist[static_cast(u)] + static_cast(cost_view[static_cast(e)]); - dag.parent_offsets[static_cast(v+1)] = 1; - } - for (std::size_t v = 1; v < dag.parent_offsets.size(); ++v) dag.parent_offsets[v] += dag.parent_offsets[v-1]; - dag.parents.resize(static_cast(dag.parent_offsets.back())); - dag.via_edges.resize(static_cast(dag.parent_offsets.back())); - for (std::size_t i = 1; i < P.nodes.size(); ++i) { - auto v = P.nodes[i]; - auto base = static_cast(dag.parent_offsets[static_cast(v)]); - dag.parents[base] = P.nodes[i-1]; dag.via_edges[base] = P.edges[i-1]; - } - } - items.emplace_back(std::move(dist), std::move(dag)); + items.push_back(path_to_pred_dag(g, P.nodes, P.edges)); } return items; } diff --git a/src/shortest_paths.cpp b/src/shortest_paths.cpp index 5ccedb6..cd31d8c 100644 --- a/src/shortest_paths.cpp +++ b/src/shortest_paths.cpp @@ -36,6 +36,71 @@ static inline void group_parents(const PredDAG& dag, NodeId v, } } +std::pair, PredDAG> +path_to_pred_dag(const StrictMultiDiGraph& g, + std::span nodes, + std::span edges) { + const auto cost_view = g.cost_view(); + std::vector dist(static_cast(g.num_nodes()), + std::numeric_limits::max()); + PredDAG dag; + dag.parent_offsets.assign(static_cast(g.num_nodes() + 1), 0); + if (!nodes.empty()) { + dist[static_cast(nodes.front())] = 0; + for (std::size_t i = 1; i < nodes.size(); ++i) { + auto u = nodes[i - 1]; auto v = nodes[i]; auto e = edges[i - 1]; + dist[static_cast(v)] = + dist[static_cast(u)] + cost_view[static_cast(e)]; + dag.parent_offsets[static_cast(v + 1)] = 1; + } + for (std::size_t v = 1; v < dag.parent_offsets.size(); ++v) + dag.parent_offsets[v] += dag.parent_offsets[v - 1]; + dag.parents.resize(static_cast(dag.parent_offsets.back())); + dag.via_edges.resize(static_cast(dag.parent_offsets.back())); + for (std::size_t i = 1; i < nodes.size(); ++i) { + auto v = nodes[i]; + auto base = static_cast(dag.parent_offsets[static_cast(v)]); + dag.parents[base] = nodes[i - 1]; + dag.via_edges[base] = edges[i - 1]; + } + } + return {std::move(dist), std::move(dag)}; +} + +PredDAG make_path_dag(const StrictMultiDiGraph& g, std::span edges) { + if (edges.empty()) { + throw std::invalid_argument("make_path_dag: edges must be non-empty"); + } + const auto E = static_cast(g.num_edges()); + const auto esrc = g.edge_src_view(); + const auto edst = g.edge_dst_view(); + std::vector nodes; + nodes.reserve(edges.size() + 1); + std::vector seen(static_cast(g.num_nodes()), 0); + for (std::size_t i = 0; i < edges.size(); ++i) { + const auto e = edges[i]; + if (e < 0 || static_cast(e) >= E) { + throw std::invalid_argument("make_path_dag: edge id out of range"); + } + const auto u = esrc[static_cast(e)]; + const auto v = edst[static_cast(e)]; + if (i == 0) { + nodes.push_back(u); + seen[static_cast(u)] = 1; + } else if (u != nodes.back()) { + throw std::invalid_argument( + "make_path_dag: edges are not contiguous (edge source does not match the " + "previous edge's destination)"); + } + if (seen[static_cast(v)]) { + throw std::invalid_argument("make_path_dag: path revisits a node (must be a simple path)"); + } + seen[static_cast(v)] = 1; + nodes.push_back(v); + } + return path_to_pred_dag(g, nodes, edges).second; +} + std::vector>>> resolve_to_paths(const PredDAG& dag, NodeId src, NodeId dst, bool split_parallel_edges, diff --git a/tests/cpp/flow_policy_tests.cpp b/tests/cpp/flow_policy_tests.cpp index 5f51809..4b53ea2 100644 --- a/tests/cpp/flow_policy_tests.cpp +++ b/tests/cpp/flow_policy_tests.cpp @@ -6,6 +6,7 @@ #include "netgraph/core/algorithms.hpp" #include "netgraph/core/strict_multidigraph.hpp" #include "netgraph/core/types.hpp" +#include "test_utils.hpp" using namespace netgraph::core; @@ -349,3 +350,87 @@ TEST(FlowPolicyCore, EqualBalanced_ShortestPath_IgnoresHigherCostTier) { // Only shortest tier edges should carry flow expect_edge_flows_by_uv(fg, {{0,1,10.0}, {1,4,10.0}, {0,2,0.0}, {2,4,0.0}}); } + +// ============================================================================ +// Static paths (pinned path bundles) +// ============================================================================ + +// Each flow binds to ITS OWN bundle in supply order (the original Python +// semantics; the first C++ port bound every flow to bundle[0]). +TEST(FlowPolicyStatic, PerFlowBundleBinding) { + auto g = make_square1(); + FlowGraph fg(g); + auto be = make_cpu_backend(); auto algs = std::make_shared(be); auto gh = algs->build_graph(g); + ExecutionContext ctx(algs, gh); + FlowPolicy policy(ctx, FlowPolicyConfig{}); + + // Edge ids after (cost, src, dst) reordering match construction order here. + EdgeId short_path[2] = {0, 1}; // A->B->C, cost 2, cap 1 + EdgeId long_path[2] = {2, 3}; // A->D->C, cost 4, cap 2 + std::vector bundles; + bundles.push_back(make_path_dag(g, std::span(short_path, 2))); + bundles.push_back(make_path_dag(g, std::span(long_path, 2))); + policy.set_static_paths(0, 2, std::move(bundles)); + + auto [placed, left] = policy.place_demand(fg, 0, 2, 0, 3.0); + EXPECT_NEAR(placed, 3.0, 1e-9); + EXPECT_NEAR(left, 0.0, 1e-9); + ASSERT_EQ(policy.flow_count(), 2); + // Creation order == supply order; costs prove the binding. + std::vector> got; + for (auto const& kv : policy.flows()) got.emplace_back(kv.first.flowId, kv.second.cost); + std::sort(got.begin(), got.end()); + EXPECT_EQ(got[0].second, 2); + EXPECT_EQ(got[1].second, 4); +} + +// A bundle whose every walk is masked out is DOWN: no flow, no volume. +TEST(FlowPolicyStatic, MaskedBundleIsDown) { + using netgraph::core::test::make_bool_mask; + auto g = make_square1(); + FlowGraph fg(g); + auto be = make_cpu_backend(); auto algs = std::make_shared(be); auto gh = algs->build_graph(g); + ExecutionContext ctx(algs, gh); + + auto edge_mask = make_bool_mask(4); + edge_mask[0] = false; // A->B down -> the short bundle has no surviving walk + FlowPolicyConfig cfg; + cfg.edge_mask = std::span(edge_mask.get(), 4); + FlowPolicy policy(ctx, cfg); + + EdgeId short_path[2] = {0, 1}; + EdgeId long_path[2] = {2, 3}; + std::vector bundles; + bundles.push_back(make_path_dag(g, std::span(short_path, 2))); + bundles.push_back(make_path_dag(g, std::span(long_path, 2))); + policy.set_static_paths(0, 2, std::move(bundles)); + + auto [placed, left] = policy.place_demand(fg, 0, 2, 0, 3.0); + EXPECT_EQ(policy.flow_count(), 1); + EXPECT_NEAR(placed, 2.0, 1e-9); // only the long bundle carries + EXPECT_NEAR(left, 1.0, 1e-9); +} + +// Malformed bundles are rejected loudly; a foreign-graph DAG cannot slip through. +TEST(FlowPolicyStatic, ValidationRejectsForeignAndDisconnectedBundles) { + auto g = make_square1(); + auto be = make_cpu_backend(); auto algs = std::make_shared(be); auto gh = algs->build_graph(g); + ExecutionContext ctx(algs, gh); + FlowPolicy policy(ctx, FlowPolicyConfig{}); + + EdgeId short_path[2] = {0, 1}; + auto dag = make_path_dag(g, std::span(short_path, 2)); + + // Connects 0->2, not 1->3: structurally wrong for that demand. + { + std::vector b; b.push_back(dag); + EXPECT_THROW(policy.set_static_paths(1, 3, std::move(b)), std::invalid_argument); + } + // Shuffled via edge: entry no longer connects its parent to its node. + { + PredDAG bad = dag; + bad.via_edges[0] = 3; // D->C edge in place of a parent link + std::vector b; b.push_back(std::move(bad)); + EXPECT_THROW(policy.set_static_paths(0, 2, std::move(b)), std::invalid_argument); + } +} diff --git a/tests/py/test_static_paths.py b/tests/py/test_static_paths.py new file mode 100644 index 0000000..eb94689 --- /dev/null +++ b/tests/py/test_static_paths.py @@ -0,0 +1,392 @@ +"""FlowPolicy.set_static_paths: pinned path bundles (MPLS-style routing). + +Semantics under test (design-panel reviewed): +- One flow per usable bundle, each permanently bound to its own bundle in + supply order (the original Python semantics; the first C++ port bound every + flow to bundle[0]). +- Bundles are validated against the graph and pruned against the policy's + masks; a bundle with no surviving src->dst walk is DOWN and creates no flow. +- Flow cost is the min-cost src->dst walk of the PRUNED bundle. +- Pinned policies never grow their flow set and never reoptimize; + max_path_cost/-factor, min_flow_count and reoptimize_flows_on_each_placement + are inert. +- EqualBalanced spreads over the usable (up) bundles only, and the rebalance + loop is iterative (many heterogeneous pinned bundles must not blow the stack). +- PredDAG.from_edges builds a single-path bundle from a contiguous edge list. +""" + +from __future__ import annotations + +import numpy as np +import pytest + +import netgraph_core as ngc + + +def _graph(num_nodes, edges): + """edges: list of (src, dst, capacity, cost).""" + s, d, c, k = zip(*edges, strict=False) + return ngc.StrictMultiDiGraph.from_arrays( + num_nodes, + np.array(s, dtype=np.int32), + np.array(d, dtype=np.int32), + np.array(c, dtype=np.float64), + np.array(k, dtype=np.int64), + np.arange(len(s), dtype=np.int64), + ) + + +def _eid(g, u, v): + """Edge id of u->v after the graph's (cost, src, dst) reordering.""" + es, ed = g.edge_src_view(), g.edge_dst_view() + (idx,) = np.where((es == u) & (ed == v)) + assert idx.size == 1, f"expected exactly one {u}->{v} edge" + return int(idx[0]) + + +@pytest.fixture +def algs(): + return ngc.Algorithms(ngc.Backend.cpu()) + + +@pytest.fixture +def square(algs): + """0->1->2 (cost 1+1, cap 1 each) and 0->3->2 (cost 2+2, cap 2 each).""" + g = _graph(4, [(0, 1, 1.0, 1), (1, 2, 1.0, 1), (0, 3, 2.0, 2), (3, 2, 2.0, 2)]) + pg = algs.build_graph(g) + short = ngc.PredDAG.from_edges(g, [_eid(g, 0, 1), _eid(g, 1, 2)]) + long = ngc.PredDAG.from_edges(g, [_eid(g, 0, 3), _eid(g, 3, 2)]) + return g, pg, short, long + + +class TestFromEdges: + def test_builds_usable_single_path_dag(self, algs, square): + g, pg, short, _ = square + fs = ngc.FlowState(g) + placed = fs.place_on_dag( + 0, 2, short, float("inf"), ngc.FlowPlacement.PROPORTIONAL + ) + assert placed == pytest.approx(1.0) # bottleneck of the 0->1->2 path + + def test_rejects_empty_out_of_range_non_contiguous_and_revisits(self, algs): + g = _graph(3, [(0, 1, 1.0, 1), (1, 2, 1.0, 1), (2, 0, 1.0, 1)]) + e01, e12, e20 = _eid(g, 0, 1), _eid(g, 1, 2), _eid(g, 2, 0) + with pytest.raises(ValueError, match="non-empty"): + ngc.PredDAG.from_edges(g, []) + with pytest.raises(ValueError, match="out of range"): + ngc.PredDAG.from_edges(g, [99]) + with pytest.raises(ValueError, match="not contiguous"): + ngc.PredDAG.from_edges(g, [e01, e20]) + with pytest.raises(ValueError, match="simple path"): + ngc.PredDAG.from_edges(g, [e01, e12, e20]) # returns to node 0 + + +class TestPerFlowBinding: + """The port bug this redesign fixes: every flow bound to bundle[0].""" + + def test_each_flow_bound_to_its_own_bundle_in_supply_order(self, algs, square): + g, pg, short, long = square + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + policy.set_static_paths(0, 2, [short, long]) + placed, left = policy.place_demand(fg, 0, 2, 0, 3.0) + assert placed == pytest.approx(3.0) # 1 on the short path + 2 on the long + assert left == pytest.approx(0.0) + # Ordinal binding: creation order == supply order; costs prove which + # bundle each flow holds (short = 2, long = 4). + by_creation = sorted(policy.flows.items(), key=lambda kv: kv[0][3]) + assert [v[2] for _, v in by_creation] == [2, 4] + assert [v[3] for _, v in by_creation] == pytest.approx([1.0, 2.0]) + + def test_supply_order_is_placement_precedence(self, algs, square): + """Proportional pinned placement is greedy in bundle-supply order.""" + g, pg, short, long = square + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + policy.set_static_paths(0, 2, [long, short]) # long first this time + placed, _ = policy.place_demand(fg, 0, 2, 0, 2.0) + assert placed == pytest.approx(2.0) + by_creation = sorted(policy.flows.items(), key=lambda kv: kv[0][3]) + # First-supplied bundle (long, cost 4) absorbed everything. + assert [v[2] for _, v in by_creation] == [4, 2] + assert [v[3] for _, v in by_creation] == pytest.approx([2.0, 0.0]) + + +class TestEqualBalanced: + def test_eb_equalizes_to_bottleneck_bundle(self, algs, square): + g, pg, short, long = square + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy( + algs, + pg, + ngc.FlowPolicyConfig(flow_placement=ngc.FlowPlacement.EQUAL_BALANCED), + ) + policy.set_static_paths(0, 2, [short, long]) + placed, _ = policy.place_demand(fg, 0, 2, 0, 3.0) + # ECMP over 2 LSPs; the short path caps each at ~1.0. + assert placed == pytest.approx(2.0, abs=1e-3) + vols = [v[3] for v in policy.flows.values()] + assert max(vols) - min(vols) < 1e-3 + + def test_eb_spreads_over_up_bundles_only(self, algs, square): + g, pg, short, long = square + em = np.ones(4, dtype=bool) + em[_eid(g, 0, 1)] = False # short path down + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy( + algs, + pg, + ngc.FlowPolicyConfig(flow_placement=ngc.FlowPlacement.EQUAL_BALANCED), + edge_mask=em, + ) + policy.set_static_paths(0, 2, [short, long]) + placed, _ = policy.place_demand(fg, 0, 2, 0, 3.0) + # One up LSP; head-end ECMP over up tunnels only -> full 2.0 on the long + # path, not volume/2 as a supplied-count divisor would give. + assert policy.flow_count() == 1 + assert placed == pytest.approx(2.0, abs=1e-3) + + def test_many_heterogeneous_bundles_do_not_blow_the_stack(self, algs): + """256 pinned parallel LSPs with one small bundle: the EB rebalance loop + (previously recursion of depth ~U * ln(imbalance/kMinFlow)) must survive.""" + n_paths = 256 + edges = [] + # src=0, dst=1, path i via node 2+i with capacity 100 except one runt. + for i in range(n_paths): + mid = 2 + i + cap = 0.5 if i == 0 else 100.0 + edges.append((0, mid, cap, 1)) + edges.append((mid, 1, cap, 1)) + g = _graph(2 + n_paths, edges) + pg = algs.build_graph(g) + bundles = [ + ngc.PredDAG.from_edges(g, [_eid(g, 0, 2 + i), _eid(g, 2 + i, 1)]) + for i in range(n_paths) + ] + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy( + algs, + pg, + ngc.FlowPolicyConfig(flow_placement=ngc.FlowPlacement.EQUAL_BALANCED), + ) + policy.set_static_paths(0, 1, bundles) + placed, _ = policy.place_demand(fg, 0, 1, 0, 10_000.0) + # ECMP: the runt bundle (cap 0.5) bounds every LSP. + assert placed == pytest.approx(n_paths * 0.5, rel=1e-2) + + def test_second_placement_is_not_incremental(self, algs, square): + """Documented: per-call volume defines the EB per-flow target, so a second + smaller placement adds nothing (parity with dynamic EB + max_flow_count).""" + g, pg, short, long = square + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy( + algs, + pg, + ngc.FlowPolicyConfig(flow_placement=ngc.FlowPlacement.EQUAL_BALANCED), + ) + policy.set_static_paths(0, 2, [short, long]) + first, _ = policy.place_demand(fg, 0, 2, 0, 2.0) + assert first == pytest.approx(2.0, abs=1e-3) + # The smaller per-call volume yields a smaller per-flow target that the + # flows already meet, so nothing new is placed and the new volume is + # returned as leftover. + second, second_left = policy.place_demand(fg, 0, 2, 0, 1.0) + assert second == pytest.approx(0.0, abs=1e-3) + assert second_left == pytest.approx(1.0, abs=1e-3) + assert policy.placed_demand() == pytest.approx(first, abs=1e-3) + + +class TestDiminishingReturnsArming: + def test_more_bundles_than_window_all_receive_volume(self, algs): + """16 LSPs x cap 1 against volume 10000: the diminishing-returns window + (8) must not fire before every pinned flow was visited once.""" + n_paths = 16 + edges = [] + for i in range(n_paths): + mid = 2 + i + edges.append((0, mid, 1.0, 1)) + edges.append((mid, 1, 1.0, 1)) + g = _graph(2 + n_paths, edges) + pg = algs.build_graph(g) + bundles = [ + ngc.PredDAG.from_edges(g, [_eid(g, 0, 2 + i), _eid(g, 2 + i, 1)]) + for i in range(n_paths) + ] + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + policy.set_static_paths(0, 1, bundles) + placed, _ = policy.place_demand(fg, 0, 1, 0, 10_000.0) + assert placed == pytest.approx(float(n_paths)) # all 16 units of capacity + + +class TestMaskSemantics: + def test_down_bundle_creates_no_flow_and_cost_is_post_prune(self, algs): + """A bundle whose cheapest walk is masked reports the surviving walk's + cost; a bundle with no surviving walk is down.""" + # Two parallel two-hop walks 0->1: via node 2 (cost 1+1) and node 3 (cost 5+0). + g = _graph( + 4, + [(0, 2, 5.0, 1), (2, 1, 5.0, 1), (0, 3, 5.0, 5), (3, 1, 5.0, 0)], + ) + pg = algs.build_graph(g) + cheap = [_eid(g, 0, 2), _eid(g, 2, 1)] + dear = [_eid(g, 0, 3), _eid(g, 3, 1)] + # One multi-walk bundle containing both walks: merge the two single-path + # DAGs by using an SPF-style DAG is not possible here (costs differ), so + # build the union manually via two bundles for the down test, and use the + # dear-only bundle for the cost check after masking the cheap walk. + b_cheap = ngc.PredDAG.from_edges(g, cheap) + b_dear = ngc.PredDAG.from_edges(g, dear) + + em = np.ones(4, dtype=bool) + em[cheap[0]] = False # kill the cheap walk entirely + + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig(), edge_mask=em) + policy.set_static_paths(0, 1, [b_cheap, b_dear]) + placed, _ = policy.place_demand(fg, 0, 1, 0, 100.0) + assert policy.flow_count() == 1 # cheap bundle is DOWN + ((_, flow),) = policy.flows.items() + assert flow[2] == 5 # cost of the surviving (dear) walk + assert placed == pytest.approx(5.0) + + def test_multiwalk_bundle_survives_partial_failure_with_surviving_cost(self, algs): + """An SPF DAG used as one bundle renormalizes over surviving walks and + reports the post-prune min cost (SR-TE-style semantics).""" + # Equal-cost diamond: 0->1 via 2 or via 3, all edges cost 1 cap 5. + g = _graph(4, [(0, 2, 5.0, 1), (2, 1, 5.0, 1), (0, 3, 5.0, 1), (3, 1, 5.0, 1)]) + pg = algs.build_graph(g) + algs_local = algs + _, dag = algs_local.spf(pg, 0, None, multipath=True) + + em = np.ones(4, dtype=bool) + em[_eid(g, 0, 2)] = False # one branch of the DAG fails + + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig(), edge_mask=em) + policy.set_static_paths(0, 1, [dag]) + placed, _ = policy.place_demand(fg, 0, 1, 0, 100.0) + assert policy.flow_count() == 1 + assert placed == pytest.approx(5.0) # surviving branch only + ((_, flow),) = policy.flows.items() + assert flow[2] == 2 + + def test_all_bundles_down_places_nothing_and_is_idempotent(self, algs, square): + g, pg, short, long = square + nm = np.ones(4, dtype=bool) + nm[1] = False + nm[3] = False # both intermediate nodes down + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig(), node_mask=nm) + policy.set_static_paths(0, 2, [short, long]) + for _ in range(2): + placed, left = policy.place_demand(fg, 0, 2, 0, 3.0) + assert (placed, left) == (0.0, 3.0) + assert policy.flow_count() == 0 + + +class TestPinnedInertness: + def test_reoptimize_on_each_placement_is_inert(self, algs, square): + """With reoptimization enabled (as the TE presets set), pinned flows keep + their bundles: deliberate divergence from the original Python, where + reoptimization silently rerouted pinned flows onto SPF paths.""" + g, pg, short, long = square + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy( + algs, pg, ngc.FlowPolicyConfig(reoptimize_flows_on_each_placement=True) + ) + policy.set_static_paths(0, 2, [short, long]) + placed, _ = policy.place_demand(fg, 0, 2, 0, 3.0) + assert placed == pytest.approx(3.0) + by_creation = sorted(policy.flows.items(), key=lambda kv: kv[0][3]) + assert [v[2] for _, v in by_creation] == [2, 4] # bindings unchanged + + def test_max_path_cost_is_inert_for_pinned_bundles(self, algs, square): + g, pg, short, long = square + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig(max_path_cost=1)) + policy.set_static_paths(0, 2, [short, long]) # costs 2 and 4, both > 1 + placed, _ = policy.place_demand(fg, 0, 2, 0, 3.0) + assert placed == pytest.approx(3.0) # pinned bundles bypass the cost gate + + def test_lifecycle_remove_and_rebalance(self, algs, square): + g, pg, short, long = square + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + policy.set_static_paths(0, 2, [short, long]) + policy.place_demand(fg, 0, 2, 0, 3.0) + policy.remove_demand(fg) + assert np.asarray(fg.edge_flow_view()).sum() == pytest.approx(0.0) + placed, _ = policy.place_demand(fg, 0, 2, 0, 3.0) # re-pins from bundles + assert placed == pytest.approx(3.0) + placed_r, _ = policy.rebalance_demand(fg, 0, 2, 0, 1.0) + assert placed_r == pytest.approx(2.0) # 1.0 per flow target + + +class TestValidation: + def test_repin_before_placement_replaces(self, algs, square): + g, pg, short, long = square + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + policy.set_static_paths(0, 2, [short]) + policy.set_static_paths(0, 2, [short, long]) # no spurious throw + fg = ngc.FlowGraph(g) + placed, _ = policy.place_demand(fg, 0, 2, 0, 3.0) + assert placed == pytest.approx(3.0) + assert policy.flow_count() == 2 + + def test_repin_with_masks_pruning_is_not_spurious(self, algs, square): + """The design-panel blocker: derived max_flow_count must not make + re-pinning the same bundles throw once masks prune some of them.""" + g, pg, short, long = square + em = np.ones(4, dtype=bool) + em[_eid(g, 0, 1)] = False # short bundle will be down (U=1 < N=2) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig(), edge_mask=em) + policy.set_static_paths(0, 2, [short, long]) + policy.set_static_paths(0, 2, [short, long]) # must not throw + assert policy.flow_count() == 0 # no flows until placement + + def test_rejects_bundle_from_a_different_graph(self, algs, square): + g, pg, short, _ = square + other = _graph( + 4, [(0, 3, 1.0, 7), (3, 2, 1.0, 7), (0, 1, 1.0, 9), (1, 2, 1.0, 9)] + ) + foreign = ngc.PredDAG.from_edges(other, [_eid(other, 0, 1), _eid(other, 1, 2)]) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + with pytest.raises(ValueError, match="different graph|does not connect"): + policy.set_static_paths(0, 2, [foreign]) + + def test_rejects_bundle_without_src_dst_walk(self, algs, square): + g, pg, short, _ = square + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + with pytest.raises(ValueError, match="no src->dst walk"): + policy.set_static_paths(1, 3, [short]) # short connects 0->2, not 1->3 + + def test_set_after_placement_and_wrong_demand_throw(self, algs, square): + g, pg, short, long = square + fg = ngc.FlowGraph(g) + policy = ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()) + policy.set_static_paths(0, 2, [short]) + policy.place_demand(fg, 0, 2, 0, 1.0) + with pytest.raises(ValueError, match="already holds flows"): + policy.set_static_paths(0, 2, [long]) + policy.remove_demand(fg) + policy.set_static_paths(0, 2, [long]) # allowed again after removal + with pytest.raises(ValueError, match="do not match demand"): + policy.place_demand(fg, 0, 1, 0, 1.0) + + def test_config_incompatibilities(self, algs, square): + g, pg, short, long = square + with pytest.raises(ValueError, match="max_flow_count"): + ngc.FlowPolicy( + algs, pg, ngc.FlowPolicyConfig(max_flow_count=3) + ).set_static_paths(0, 2, [short, long]) + with pytest.raises(ValueError, match="shortest_path"): + ngc.FlowPolicy( + algs, pg, ngc.FlowPolicyConfig(shortest_path=True) + ).set_static_paths(0, 2, [short]) + with pytest.raises(ValueError, match="non-empty"): + ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()).set_static_paths(0, 2, []) + with pytest.raises(ValueError, match="out of range or equal"): + ngc.FlowPolicy(algs, pg, ngc.FlowPolicyConfig()).set_static_paths( + 2, 2, [short] + ) From 503d4011e587990029543f0989ae230d381807ff Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Sun, 23 Aug 2026 23:58:05 +0100 Subject: [PATCH 10/11] Fix stub constructors that broke downstream type checking 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 --- CHANGELOG.md | 2 +- python/netgraph_core/_docs.py | 48 ++++++++++++++++++-- tests/py/test_review_regressions.py | 69 +++++++++++++++++++++++++++++ 3 files changed, 114 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 08ecdf1..9777a00 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -33,7 +33,7 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 - **Flow Policy**: `place_demand` computes one SPF when seeding initial flows instead of repeating an identical one per flow; seeding cost no longer scales with `min_flow_count`. - **Flow Policy**: The `EqualBalanced` rebalance recursion (`place_demand` -> `rebalance_demand` -> `place_demand`) is now an iterative loop with numerically identical results (returned leftover may differ by ~1 ulp from floating-point summation order); with many pinned bundles of heterogeneous capacity the recursion depth grew like `U * ln(imbalance/kMinFlow)`, a stack-overflow risk on worker threads. - **Python Bindings**: A wrong-typed `graph`/`algorithms` argument now raises `TypeError` instead of an opaque `RuntimeError: Unable to cast ... to C++ type '?'`, `Algorithms.spf` raises `TypeError` for a wrong `residual` length (matching every other length check), `Algorithms.ksp` validates `dtype` before running, and `batch_max_flow` rejects out-of-range node ids like the single-pair entry points. -- **Python Typing**: Corrected `_docs.py` (pybind11 enums and classes are not `enum.Enum`/`dataclass`, `MinCut.edges` is an `int32` array, removed a `Path` class that never existed) and widened the type-checking re-exports from 6 names to all 16, so `Algorithms`, `FlowPolicy`, `StrictMultiDiGraph` and friends are no longer `Unknown` to type checkers despite the shipped `py.typed`. +- **Python Typing**: Corrected `_docs.py` (pybind11 enums and classes are not `enum.Enum`/`dataclass`, `MinCut.edges` is an `int32` array, removed a `Path` class that never existed, and added the constructors for `FlowIndex`, `FlowPolicyConfig` and the three enums) and widened the type-checking re-exports from 6 names to all 16, so `Algorithms`, `FlowPolicy`, `StrictMultiDiGraph` and friends are no longer `Unknown` to type checkers despite the shipped `py.typed`. A missing stub constructor is now worse than no stub, so a test asserts every stub constructor matches its binding. - **Tests**: Several assertions could never fail and now do: a self-loop assertion sat inside `except Exception: pass`, a thread-safety test ended in `assert True` while its workers called `pytest.fail()` from a thread where it cannot fail the test, and two tautologies (`assert size >= 0`) were replaced with the properties actually under test. - **Docs**: Corrected README/CONTRIBUTING (required CMake is 3.23 not 3.15, `make py-test` does not exist, `make test` does not collect coverage), the `sensitivity_analysis` description in README and `backend.hpp` (it measures flow *lost* on edge removal, not gain from relaxing capacity), the zero-copy and GIL claims, and header contracts for `from_arrays`, `PredDAG`, `FlowSummary.costs`, `calc_max_flow` and `FlowPolicy`. diff --git a/python/netgraph_core/_docs.py b/python/netgraph_core/_docs.py index e0174be..5cddcbd 100644 --- a/python/netgraph_core/_docs.py +++ b/python/netgraph_core/_docs.py @@ -24,6 +24,8 @@ class EdgeTieBreak: PREFER_HIGHER_RESIDUAL: ClassVar[EdgeTieBreak] __members__: ClassVar[dict[str, EdgeTieBreak]] + def __init__(self, value: int) -> None: ... + @property def name(self) -> str: ... @property @@ -63,6 +65,8 @@ class FlowPlacement: EQUAL_BALANCED: ClassVar[FlowPlacement] __members__: ClassVar[dict[str, FlowPlacement]] + def __init__(self, value: int) -> None: ... + @property def name(self) -> str: ... @property @@ -109,6 +113,8 @@ class PathAlg: SPF: ClassVar[PathAlg] __members__: ClassVar[dict[str, PathAlg]] + def __init__(self, value: int) -> None: ... + @property def name(self) -> str: ... @property @@ -385,10 +391,21 @@ def in_adj_edge_index_view(self) -> "np.ndarray": class FlowIndex: - src: int - dst: int - flowClass: int - flowId: int + """Identity of one flow: (src, dst, flowClass, flowId). + + The constructor takes exactly four POSITIONAL arguments (the binding + declares no argument names), and the attributes are read-only. + """ + + def __init__(self, src: int, dst: int, flowClass: int, flowId: int, /) -> None: ... + @property + def src(self) -> int: ... + @property + def dst(self) -> int: ... + @property + def flowClass(self) -> int: ... + @property + def flowId(self) -> int: ... class FlowPolicyConfig: @@ -421,6 +438,29 @@ class FlowPolicyConfig: diminishing_returns_window: int diminishing_returns_epsilon_frac: float + def __init__( + self, + *, + path_alg: PathAlg = ..., + flow_placement: FlowPlacement = ..., + selection: EdgeSelection = ..., + require_capacity: bool = True, + multipath: bool = True, + min_flow_count: int = 1, + max_flow_count: Optional[int] = None, + max_path_cost: Optional[int] = None, + max_path_cost_factor: Optional[float] = None, + shortest_path: bool = False, + reoptimize_flows_on_each_placement: bool = False, + max_no_progress_iterations: int = 100, + max_total_iterations: int = 10000, + diminishing_returns_enabled: bool = True, + diminishing_returns_window: int = 8, + diminishing_returns_epsilon_frac: float = 1e-3, + ) -> None: + """All parameters are keyword-only; the no-argument form is also valid.""" + ... + class FlowPolicy: """Flow policy for demand placement. diff --git a/tests/py/test_review_regressions.py b/tests/py/test_review_regressions.py index b6032d5..ea83104 100644 --- a/tests/py/test_review_regressions.py +++ b/tests/py/test_review_regressions.py @@ -428,3 +428,72 @@ def test_unit_factor_still_admits_the_shortest_path(self, algs, k): g = _graph(3, [(0, 1, 5.0, 1), (1, 2, 5.0, 1)]) pg = algs.build_graph(g) assert len(algs.ksp(pg, 0, 2, k=k, max_cost_factor=1.0)) == 1 + + +class TestStubConstructorConsistency: + """Finding 16: widened stubs declared no __init__ for classes that need one. + + `_docs.py` is wired to type checkers, so a stub whose constructor disagrees + with the binding turns "unchecked" into "confidently wrong": pyright rejected + every real `FlowIndex(src, dst, cls, id)` call in the downstream consumer. + This repo's own pyright missed it because `tests/**` is excluded, so assert + the correspondence at runtime instead. + """ + + def _runtime_classes(self): + import _netgraph_core + + for name in sorted(n for n in dir(_netgraph_core) if not n.startswith("_")): + obj = getattr(_netgraph_core, name) + if isinstance(obj, type): + yield name, obj + + def test_stub_declares_init_wherever_construction_needs_arguments(self): + from netgraph_core import _docs + + missing = [] + for name, rt in self._runtime_classes(): + stub = getattr(_docs, name, None) + if stub is None: + continue # not part of the typed surface + try: + rt() + continue # zero-arg construction works; a bare stub is fine + except Exception: + pass + doc = rt.__init__.__doc__ or "" + if "__init__" not in doc: + continue # no bound constructor at all (e.g. result-only types) + if "__init__" not in vars(stub): + missing.append(name) + assert not missing, ( + f"stub classes need an __init__ matching the binding: {missing}" + ) + + def test_stub_attributes_exist_at_runtime(self): + from netgraph_core import _docs + + wrong = [] + for name, rt in self._runtime_classes(): + stub = getattr(_docs, name, None) + if stub is None: + continue + declared = { + a + for a in vars(stub) + if not a.startswith("_") and not callable(vars(stub)[a]) + } + declared |= set(getattr(stub, "__annotations__", {})) + for attr in declared: + if not hasattr(rt, attr): + wrong.append(f"{name}.{attr}") + assert not wrong, f"_docs.py declares attributes absent at runtime: {wrong}" + + def test_flow_index_constructor_matches_the_binding(self): + # The exact call shape the downstream consumer uses. + idx = ngc.FlowIndex(1, 2, 3, 4) + assert (idx.src, idx.dst, idx.flowClass, idx.flowId) == (1, 2, 3, 4) + with pytest.raises(TypeError): + ngc.FlowIndex(src=1, dst=2, flowClass=3, flowId=4) # positional-only + with pytest.raises(AttributeError): + idx.src = 9 # read-only, as the stub's properties declare From 89a0613540632a9445151e83487cf599aba258f5 Mon Sep 17 00:00:00 2001 From: Andrey Golovanov Date: Mon, 24 Aug 2026 00:42:14 +0100 Subject: [PATCH 11/11] Pre-release review: correct changelog claims and release mechanics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .github/workflows/release.yml | 21 ++++++++++++++++- CHANGELOG.md | 26 ++++++++++++--------- CONTRIBUTING.md | 18 +++++++++----- README.md | 39 ++++++++++++++++++++++++++++++- pyproject.toml | 4 ++-- python/netgraph_core/__init__.py | 3 +++ python/netgraph_core/_docs.py | 40 +++++++++++++++++++++++++++++--- 7 files changed, 127 insertions(+), 24 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 9aa3aae..85d5023 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -10,6 +10,25 @@ on: workflow_dispatch: jobs: + # A tag push publishes whatever version pyproject.toml declares, so a mistyped + # tag would ship the wrong version silently. Fail fast instead. + check_version: + name: Tag matches project version + runs-on: ubuntu-latest + if: github.event_name == 'push' && startsWith(github.ref, 'refs/tags/v') + steps: + - uses: actions/checkout@v4 + - name: Compare tag with pyproject version + run: | + python -c " + import tomllib, os, sys + version = tomllib.load(open('pyproject.toml','rb'))['project']['version'] + tag = os.environ['GITHUB_REF_NAME'].removeprefix('v') + if tag != version: + sys.exit(f'tag {tag!r} != pyproject version {version!r}') + print(f'tag matches project version: {version}') + " + build_wheels: name: Wheel (${{ matrix.os }}, ${{ matrix.cibw_archs }}) runs-on: ${{ matrix.os }} @@ -95,7 +114,7 @@ jobs: publish_pypi: name: Publish to PyPI - needs: [build_wheels, build_sdist, test_wheels_centos_stream_9] + needs: [check_version, build_wheels, build_sdist, test_wheels_centos_stream_9] runs-on: ubuntu-latest environment: pypi permissions: diff --git a/CHANGELOG.md b/CHANGELOG.md index 9777a00..78b6c81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,7 +5,7 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). -## [0.8.0] - 2026-08-22 +## [0.8.0] - 2026-08-24 ### Added @@ -14,11 +14,11 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed -- **Max-Flow**: `calc_max_flow` could return less than the true maximum, since the tier loop augments only along forward SPF DAGs and never cancels an earlier placement (the reported `min_cut` then contradicted `total_flow`). Added a residual completion phase with reverse arcs for `Proportional` + `require_capacity` + `shortest_path=false`; results increase where the previous value was suboptimal. -- **Shortest Paths**: Zero-cost edges produced a cyclic predecessor DAG, making `EqualBalanced` placement return 0 flow and `resolve_to_paths` hang. Equal-cost predecessors are now recorded only while a node is unsettled, and `resolve_to_paths` guards against cycles in caller-supplied DAGs. -- **K-Shortest Paths**: Spur enumeration materialized every equal-cost path, exponential in ECMP fan-out (a 22-stage ladder needed ~19 s and ~3.8 GB for `k=3`). Enumeration is now bounded by the number of candidates still acceptable; tie-breaking among equal-cost paths may differ, but path counts and costs are unchanged. -- **Flow Policy**: `place_demand` silently routed a second `(src, dst)` pair over the first pair's paths. It now raises `invalid_argument`; use one policy per demand or call `remove_demand()` first. -- **Graph Construction**: `from_arrays` now rejects a total edge cost at or above 2^62, which overflows the int64 path arithmetic in SPF and silently corrupts results. +- **Max-Flow**: `calc_max_flow` could return less than the true maximum, since the tier loop augments only along forward SPF DAGs and never cancels an earlier placement (the reported `min_cut` then contradicted `total_flow`). Added a residual completion phase with reverse arcs. **This is the default configuration** of `max_flow`/`batch_max_flow` (`Proportional` + `require_capacity=True` + `shortest_path=False`), so recorded expected values, regression baselines and stored results must be regenerated: `total_flow` increases where the previous value was suboptimal, and `min_cut`, `edge_flows`, `residual_capacity` and `reachable_nodes` change with it. `sensitivity_analysis`, which is built on `calc_max_flow`, is affected too. +- **Shortest Paths**: Zero-cost edges produced a cyclic predecessor DAG, making `EqualBalanced` placement return 0 flow and `resolve_to_paths` hang. Equal-cost predecessors are now recorded only while a node is unsettled, and `resolve_to_paths` guards against cycles in caller-supplied DAGs. Side effect on graphs containing zero-cost edges: among nodes mutually reachable at equal distance the settle order now decides which alternatives are kept, so a `PredDAG` can hold fewer equal-cost predecessors than in 0.7.2 and ECMP fan-out narrows accordingly. Graphs with strictly positive costs are unaffected (verified bit-identical). +- **K-Shortest Paths**: Spur enumeration materialized every equal-cost path, exponential in ECMP fan-out (a 67-node ECMP ladder needed ~4 s and ~3.5 GiB for `k=3`, and a 73-node one ~19 s; both are now instant in ~34 MB). Enumeration is now bounded by the number of candidates still acceptable; tie-breaking among equal-cost paths may differ, but path counts and costs are unchanged. +- **Flow Policy**: `place_demand` silently routed a second `(src, dst)` pair over the first pair's paths, and neither `place_demand` nor `rebalance_demand` checked that the supplied `FlowGraph` wrapped the policy's own graph -- with equal edge counts that selected paths on one topology and placed flow on another. Both now raise `invalid_argument` (`ValueError` from Python); use one policy per demand, or call `remove_demand()` first to retarget it. +- **Graph Construction**: `from_arrays` now rejects a total edge cost at or above 2^62, which overflows the int64 path arithmetic in SPF and silently corrupts results. The bound is on the sum of all edge costs -- a conservative upper bound on any path -- so it can reject a graph whose individual paths could not actually overflow. - **Flow State**: `compute_min_cut` and max-flow reachability derived placed flow from `capacity - residual`, overstating it for a `FlowState` built with a custom `residual_init`. Both now use `edge_flow`. - **K-Shortest Paths**: `k_shortest_paths` read `paths.back()` on an empty vector when `max_cost_factor < 1.0` put the cost ceiling below the shortest path, crashing for `k > 1` (the `k == 1` early return masked it). All `k` now return no paths for such a factor. - **Python Bindings**: `batch_max_flow` rejected a valid int32 `pairs` array on Windows, because the dtype check compared buffer format strings and NumPy spells int32 as `NPY_LONG` on LLP64 but `NPY_INT` on LP64. It now compares dtype equivalence, and rejects non-contiguous `pairs` rather than reading them as if packed. @@ -27,12 +27,16 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Changed +- **Max-Flow**: `FlowSummary.costs`/`flows` is now a cost-weighted breakdown of the total flow rather than a list of path costs. Entries added by the new completion phase are *marginal* costs -- the augmenting path's forward edge costs minus the cost of the flow it cancels -- so an entry need not correspond to any traversable `src->dst` path. Consumers that treat these as path costs (for example a cost-distribution report) should be reviewed. +- **Flow Policy**: `max_path_cost` no longer implies a relative factor of 1.0. Previously `max_path_cost_factor` defaulted to 1.0 whenever either limit was set, so setting `max_path_cost` alone also rejected every path costing more than the best path found -- making the absolute limit unreachable. The two limits are now independent, and a policy with only `max_path_cost` set admits every path up to that cost. (Found while fixing undefined behavior in the same expression: with an unreachable destination, `best_path_cost_` is the `INT64_MAX` sentinel and multiplying it by the factor overflowed the cast back to `int64`.) - **Shortest Paths**: Replaced nested per-node predecessor vectors with flat intrusive lists, removing the allocation churn that dominated the hot path (~3-4x faster; output unchanged). - **Flow Placement**: `place_on_dag` now rebuilds its `(parent, child)` edge groups into a reused arena instead of nested per-node vectors, which was ~70% of its runtime (~2-2.4x faster; output unchanged). -- **Max-Flow**: Parallelized `batch_max_flow` across source/destination pairs using `std::async`; workers claim pairs from a shared counter, so a batch whose costs are unevenly distributed still parallelizes. Thread count from `NGRAPH_CORE_BATCH_THREADS` env or hardware concurrency. +- **Max-Flow**: Parallelized `batch_max_flow` across source/destination pairs using `std::async`; workers claim pairs from a shared counter, so a batch whose costs are unevenly distributed still parallelizes. Thread count from `NGRAPH_CORE_BATCH_THREADS` env or hardware concurrency; set it to `1` when calling from your own worker pool to avoid oversubscription. +- **BREAKING** **C++ API**: `FlowPolicy::set_static_paths` changed signature from `(std::vector>)` to `(NodeId src, NodeId dst, std::vector bundles)`, and its behavior changed: the previous implementation handed every flow the first matching bundle instead of one bundle per flow. It was never bound to Python, so only C++ callers are affected. - **Flow Policy**: `place_demand` computes one SPF when seeding initial flows instead of repeating an identical one per flow; seeding cost no longer scales with `min_flow_count`. - **Flow Policy**: The `EqualBalanced` rebalance recursion (`place_demand` -> `rebalance_demand` -> `place_demand`) is now an iterative loop with numerically identical results (returned leftover may differ by ~1 ulp from floating-point summation order); with many pinned bundles of heterogeneous capacity the recursion depth grew like `U * ln(imbalance/kMinFlow)`, a stack-overflow risk on worker threads. -- **Python Bindings**: A wrong-typed `graph`/`algorithms` argument now raises `TypeError` instead of an opaque `RuntimeError: Unable to cast ... to C++ type '?'`, `Algorithms.spf` raises `TypeError` for a wrong `residual` length (matching every other length check), `Algorithms.ksp` validates `dtype` before running, and `batch_max_flow` rejects out-of-range node ids like the single-pair entry points. +- **Python Bindings**: Three input errors now raise where they previously did not, or raise a different type -- adjust `except` clauses accordingly. `Algorithms.spf` raises `TypeError` (was `ValueError`) for a wrong `residual` length, matching every other length check; `batch_max_flow` raises `ValueError` for out-of-range node ids that previously produced a zero-flow summary absorbed silently into the batch; and `Algorithms.ksp` validates `dtype` up front, where an invalid value previously went unnoticed whenever the query returned no paths. +- **Python Bindings**: A wrong-typed `graph`/`algorithms` argument now raises `TypeError` instead of an opaque `RuntimeError: Unable to cast ... to C++ type '?'`. - **Python Typing**: Corrected `_docs.py` (pybind11 enums and classes are not `enum.Enum`/`dataclass`, `MinCut.edges` is an `int32` array, removed a `Path` class that never existed, and added the constructors for `FlowIndex`, `FlowPolicyConfig` and the three enums) and widened the type-checking re-exports from 6 names to all 16, so `Algorithms`, `FlowPolicy`, `StrictMultiDiGraph` and friends are no longer `Unknown` to type checkers despite the shipped `py.typed`. A missing stub constructor is now worse than no stub, so a test asserts every stub constructor matches its binding. - **Tests**: Several assertions could never fail and now do: a self-loop assertion sat inside `except Exception: pass`, a thread-safety test ended in `assert True` while its workers called `pytest.fail()` from a thread where it cannot fail the test, and two tautologies (`assert size >= 0`) were replaced with the properties actually under test. - **Docs**: Corrected README/CONTRIBUTING (required CMake is 3.23 not 3.15, `make py-test` does not exist, `make test` does not collect coverage), the `sensitivity_analysis` description in README and `backend.hpp` (it measures flow *lost* on edge removal, not gain from relaxing capacity), the zero-copy and GIL claims, and header contracts for `from_arrays`, `PredDAG`, `FlowSummary.costs`, `calc_max_flow` and `FlowPolicy`. @@ -40,9 +44,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Removed - **Dead code**: `.gcovr.cfg` (never read -- gcovr's default config name has no leading dot, and `make cov` passes every flag explicitly), the unreachable `FlowGraph` include and `expect_flow_conservation()` helper in the C++ test utilities, unused test helpers/fixtures, `_USE_MATH_DEFINES` (no `M_*` macro is used), and unused ``/``/``/``/`` includes across `src/` and `include/`. -- **Python Bindings**: `Algorithms.build_graph_from_arrays`. Nothing used it (no test, and not NetGraph), and the `Graph` handle it returned could not be passed to `FlowGraph` or `FlowPolicy`, so it only ever served the stateless algorithms while looking like a peer of `build_graph`. Build the graph with `StrictMultiDiGraph.from_arrays()` and pass it to `Algorithms.build_graph()`. -- **C++ API**: The `build_graph(std::shared_ptr)` overload on `Algorithms` and `Backend`, which existed solely to serve that binding. Removing it also drops a pure virtual that every `Backend` implementation had to provide. Callers wanting the handle to own the graph can construct it directly: `GraphHandle{my_shared_ptr}`. -- **Python Bindings**: The unreachable `Flow` class (bound from `FlowRecord`). No binding constructed or returned one, it was absent from `__all__`, and the name collided with the C++ alias `using Flow = double`. +- **BREAKING** **Python Bindings**: `Algorithms.build_graph_from_arrays`. Nothing used it (no test, and not NetGraph), and the `Graph` handle it returned could not be passed to `FlowGraph` or `FlowPolicy`, so it only ever served the stateless algorithms while looking like a peer of `build_graph`. Build the graph with `StrictMultiDiGraph.from_arrays()` and pass it to `Algorithms.build_graph()`. +- **BREAKING** **C++ API**: The `build_graph(std::shared_ptr)` overload on `Algorithms` and `Backend`, which existed solely to serve that binding. It was a pure virtual, so an out-of-tree `Backend` implementation must delete its now-dangling `override` to compile. Callers wanting the handle to own the graph construct it directly: `GraphHandle{my_shared_ptr}`. +- **BREAKING** **Python Bindings**: The `Flow` class (bound from `FlowRecord`). It was reachable only as `_netgraph_core.Flow`, never exported from `netgraph_core`, and no binding constructed or returned one, so nothing could obtain an instance; the name also collided with the C++ alias `using Flow = double`. ## [0.7.2] - 2026-03-26 diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 415c708..6cd3577 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -65,13 +65,19 @@ make format Releases are automated via GitHub Actions when a new tag is pushed. -1. Bump version in `pyproject.toml`. -2. Commit and push. -3. Create and push a tag: +1. Move the pending notes into a new `## [X.Y.Z] - YYYY-MM-DD` section in `CHANGELOG.md`, + dated the day you tag. Mark anything that breaks consumers as **BREAKING**. +2. Bump `version` in `pyproject.toml` to the same `X.Y.Z`. CI does not cross-check the + tag against this value, so a mismatch publishes the wrong version silently. +3. Commit and push. +4. Create and push a matching tag: ```bash - git tag v0.1.0 - git push origin v0.1.0 + git tag v0.8.0 + git push origin v0.8.0 ``` -4. The CI pipeline will build wheels, sdist, and publish to PyPI. +5. The CI pipeline builds wheels and the sdist, tests them, and publishes to PyPI via + Trusted Publishing. `make publish` / `make publish-test` upload whatever is in your + local `dist/` and are for Test PyPI or emergencies only -- real releases go through + the tag. diff --git a/README.md b/README.md index 8088c06..c5d7fba 100644 --- a/README.md +++ b/README.md @@ -8,7 +8,7 @@ NetGraph-Core provides a specialized graph implementation for networking problem - **Determinism**: Guaranteed reproducible edge ordering by (cost, src, dst). - **Flow Modeling**: Native support for multi-commodity flow state, residual tracking, and ECMP/WCMP placement. -- **Performance**: Immutable CSR (Compressed Sparse Row) adjacency and zero-copy NumPy views. +- **Performance**: Immutable CSR (Compressed Sparse Row) adjacency, and zero-copy NumPy views over mutable flow state. ## Core Features @@ -28,6 +28,10 @@ NetGraph-Core provides a specialized graph implementation for networking problem - Yen's algorithm returning DAG-wrapped paths. - Configurable constraints on cost factors (e.g., paths within 1.5x of optimal). +- **Explicit Paths**: + - `PredDAG.from_edges(graph, edges)` builds a path bundle from an operator-supplied + edge sequence, usable anywhere a `PredDAG` is accepted. + - **Max-Flow**: - **Algorithm**: Iterative augmentation using Successive Shortest Path on residual graphs, pushing flow across full ECMP/WCMP DAGs at each step. For `Proportional` placement with `require_capacity=True`, a final residual completion phase augments with reverse arcs so the result is a true maximum flow; the other modes are placement models and may report less. - **Traffic Engineering (TE) Mode**: Routing adapts to residual capacity (progressive fill). @@ -46,12 +50,37 @@ Unified configuration object (`FlowPolicy`) that models diverse routing behavior - `EqualBalanced`: **ECMP** (equal splitting) - equal distribution across next-hops and parallel edges. - `Proportional`: **WCMP** (weighted splitting) - distribution proportional to residual capacity. - **Lifecycle Management**: Handles demand placement, re-optimization of existing flows, and constraints (path cost, stretch factor, flow counts). +- **Static (Pinned) Paths**: `FlowPolicy.set_static_paths()` pins a demand to explicit path bundles (MPLS-style). One flow per usable bundle; a bundle with no path surviving the failure masks is *down* and carries nothing, since a pinned path does not reroute. ### 4. Python Integration - **Zero-Copy**: `FlowState` and `FlowGraph` `*_view()` methods expose C++ buffers as read-only NumPy arrays that track mutation in place. `StrictMultiDiGraph.*_view()` returns copies instead, so the graph's immutability cannot be violated. - **Concurrency**: Releases the Python GIL during the long-running algorithms (SPF, KSP, max-flow, placement) to enable threading. `PredDAG.resolve_to_paths` holds the GIL. `batch_max_flow` and `sensitivity_analysis` also use internal worker threads, sized by `NGRAPH_CORE_BATCH_THREADS` / `NGRAPH_CORE_SENSITIVITY_THREADS`. +## Quick Start + +```python +import numpy as np +import netgraph_core as ngc + +# Two parallel paths from node 0 to node 3. +graph = ngc.StrictMultiDiGraph.from_arrays( + num_nodes=4, + src=np.array([0, 1, 0, 2], dtype=np.int32), + dst=np.array([1, 3, 2, 3], dtype=np.int32), + capacity=np.array([10.0, 10.0, 5.0, 5.0], dtype=np.float64), + cost=np.array([1, 1, 1, 1], dtype=np.int64), + ext_edge_ids=np.arange(4, dtype=np.int64), # your own stable edge ids +) + +algs = ngc.Algorithms(ngc.Backend.cpu()) +handle = algs.build_graph(graph) + +total, summary = algs.max_flow(handle, 0, 3) +print(total) # 15.0 +print(summary.min_cut.edges.tolist()) # [0, 1] -- bottleneck edge ids +``` + ## Installation ```bash @@ -94,6 +123,14 @@ make cpp-test # C++ tests only make cov # Combined coverage report (C++ + Python) ``` +## Environment Variables + +| Variable | Effect | +| --- | --- | +| `NGRAPH_CORE_PROFILE=1` | Enable profiling of C++ hot paths (`profiling_dump()` / `profiling_reset()`). | +| `NGRAPH_CORE_BATCH_THREADS` | Worker threads for `batch_max_flow` (default: hardware concurrency). Set to `1` when calling from your own worker pool. | +| `NGRAPH_CORE_SENSITIVITY_THREADS` | Worker threads for `sensitivity_analysis` (default: hardware concurrency). | + ## Requirements - **C++:** C++20 compiler (GCC 10+, Clang 12+, MSVC 2019+) diff --git a/pyproject.toml b/pyproject.toml index b575881..5ab88d5 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,5 +1,5 @@ [build-system] -requires = ["scikit-build-core>=0.10", "pybind11>=3", "numpy>=1.22"] +requires = ["scikit-build-core>=0.10", "pybind11>=3", "numpy>=1.23.2"] build-backend = "scikit_build_core.build" [project] @@ -26,7 +26,7 @@ classifiers = [ "Programming Language :: Python :: Implementation :: CPython", "Operating System :: OS Independent", ] -dependencies = ["numpy>=1.22"] +dependencies = ["numpy>=1.23.2"] [project.optional-dependencies] dev = [ diff --git a/python/netgraph_core/__init__.py b/python/netgraph_core/__init__.py index 4c39e1c..3bb8a27 100644 --- a/python/netgraph_core/__init__.py +++ b/python/netgraph_core/__init__.py @@ -57,6 +57,9 @@ PathAlg as PathAlg, PredDAG as PredDAG, StrictMultiDiGraph as StrictMultiDiGraph, + profiling_dump as profiling_dump, + profiling_enabled as profiling_enabled, + profiling_reset as profiling_reset, ) __all__ = [ diff --git a/python/netgraph_core/_docs.py b/python/netgraph_core/_docs.py index 5cddcbd..c873f0e 100644 --- a/python/netgraph_core/_docs.py +++ b/python/netgraph_core/_docs.py @@ -131,9 +131,15 @@ def cpu() -> "Backend": class Graph: - """Opaque graph handle provided by the runtime extension (typing stub only).""" + """Opaque backend graph handle produced by Algorithms.build_graph. - ... + Keeps the underlying StrictMultiDiGraph alive for as long as it is used. + """ + + @property + def num_nodes(self) -> int: ... + @property + def num_edges(self) -> int: ... class FlowGraph: @@ -507,6 +513,12 @@ def place_demand( ) -> tuple[float, float]: """Place `volume` of demand; returns (placed, remaining). + Raises: + ValueError: If this policy already manages a different (src, dst) + pair (call remove_demand() first, or use one policy per + demand), or if `flow_graph` wraps a different + StrictMultiDiGraph than the policy's own graph handle. + EQUAL_BALANCED note: when the equalizing rebalance runs, `placed` is the policy's TOTAL placed demand (cumulative across calls, because rebalancing re-places previously placed volume too), so @@ -526,7 +538,10 @@ def rebalance_demand( target: float, ) -> tuple[float, float]: ... - def remove_demand(self, flow_graph: "FlowGraph") -> None: ... + def remove_demand(self, flow_graph: "FlowGraph") -> None: + """Remove all of this policy's flows, releasing its (src, dst) binding.""" + ... + def set_static_paths(self, src: int, dst: int, paths: "Sequence[PredDAG]") -> None: """Pin this policy's demand to explicit path bundles (MPLS-style). @@ -572,6 +587,10 @@ class MinCut: class FlowSummary: total_flow: float min_cut: MinCut + # Cost-weighted breakdown of total_flow, ascending and unique. Entries from + # the SPF tier loop are shortest-path-DAG costs; entries from the residual + # completion phase are MARGINAL costs (forward edge costs minus the cost of + # the flow they cancel) and need not match any traversable src->dst path. costs: "np.ndarray" # int64[K] flows: "np.ndarray" # float64[K] edge_flows: "np.ndarray" @@ -794,3 +813,18 @@ def sensitivity_analysis( whose removal would reduce total flow by flow_delta. """ ... + + +def profiling_enabled() -> bool: + """True if profiling is enabled (NGRAPH_CORE_PROFILE=1 in the environment).""" + ... + + +def profiling_dump() -> None: + """Print collected profiling statistics to stderr.""" + ... + + +def profiling_reset() -> None: + """Clear all collected profiling statistics.""" + ...