C++ core integration #93
Conversation
networmix
added
the
breaking
There was a problem hiding this comment.
Pull request overview
This PR integrates a new C++ core library (NetGraph-Core) to replace Python-based graph algorithms, achieving a ~20x performance improvement. The changes involve removing deprecated Python algorithm implementations, introducing new adapter layers for Core integration, restructuring the failure management system, and updating traffic demand placement workflows.
Key changes:
- Removed Python implementations of path utilities, max-flow algorithms, edge selection, and capacity calculation tests
- Introduced NetGraph-Core adapter layer for graph building and algorithm execution
- Refactored traffic matrix placement and failure management to use Core-based execution
- Updated workflow steps to validate networks via NetworkX instead of building algorithm graphs
- Restructured model classes to support EdgeRef-based path representations
Reviewed changes
Copilot reviewed 97 out of 212 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/algorithms/test_path_utils.py | Removed Python path resolution tests (functionality moved to Core) |
| tests/algorithms/test_max_flow.py | Removed Python max-flow algorithm tests (replaced by Core implementation) |
| tests/algorithms/test_edge_select.py | Removed Python edge selection tests (logic integrated into Core) |
| tests/algorithms/test_calc_capacity.py | Removed Python capacity calculation tests (Core handles capacity now) |
| tests/algorithms/test_bugs_algorithms.py | Removed algorithm bug regression tests (to be recreated for Core APIs) |
| tests/algorithms/sample_graphs.py | Removed Python test fixture graphs (no longer needed for algorithm tests) |
| tests/adapters/test_adapters.py | Added placeholder test file for new adapter module |
| pyproject.toml | Bumped version to 0.10.0, added netgraph-core dependency and PyRight venv config |
| ngraph/workflow/traffic_matrix_placement_step.py | Updated to use Core-based demand placement via new model structure |
| ngraph/workflow/parse.py | Extracted workflow step parsing logic from scenario.py |
| ngraph/workflow/network_stats.py | Simplified filtering to remove NetworkView dependency |
| ngraph/workflow/maximum_supported_demand_step.py | Migrated to Core-based demand placement with expanded validation |
| ngraph/workflow/max_flow_step.py | Updated imports to reflect new module structure |
| ngraph/workflow/cost_power.py | Updated imports and simplified component resolution logic |
| ngraph/workflow/build_graph.py | Changed from algorithm graph building to NetworkX validation/export |
| ngraph/types/dto.py | Introduced EdgeRef and FlowSummary data structures for Core integration |
| ngraph/types/base.py | Removed PathElement and PathTuple (replaced by EdgeRef-based paths) |
| ngraph/solver/paths.py | Rewrote shortest path solvers to use NetGraph-Core algorithms |
| ngraph/solver/maxflow.py | Complete rewrite to use NetGraph-Core max-flow with caching support |
| ngraph/solver/helpers.py | Removed empty helper module |
| ngraph/solver/init.py | Updated docstring to remove NetworkView reference |
| ngraph/scenario.py | Extracted parsing logic, delegated to specialized builders |
| ngraph/results/snapshot.py | Extracted scenario snapshot building from scenario.py |
| ngraph/results/flow.py | Optimized FlowEntry/FlowSummary serialization, added float formatting helper |
| ngraph/results/artifacts.py | Removed PlacementResultSet (deprecated) |
| ngraph/profiling/reporter.py | Removed empty profiling reporter module |
| ngraph/paths/bundle.py | Removed PathBundle (no longer needed with Core) |
| ngraph/paths/init.py | Removed paths package documentation |
| ngraph/monte_carlo/results.py | Removed Monte Carlo result structures (deprecated) |
| ngraph/monte_carlo/functions.py | Removed Monte Carlo analysis functions (deprecated) |
| ngraph/monte_carlo/init.py | Removed Monte Carlo package exports |
| ngraph/model/view.py | Removed NetworkView (replaced by mask-based exclusions in Core) |
| ngraph/model/path.py | Updated Path to use EdgeRef instead of integer edge keys |
| ngraph/model/network.py | Removed algorithm methods, added Core graph building support |
| ngraph/model/flow/policy_config.py | Introduced FlowPolicyPreset and factory for Core flow policies |
| ngraph/model/failure/policy_set.py | Updated import path for FailurePolicy |
| ngraph/model/failure/parser.py | Extracted failure policy parsing from scenario.py |
| ngraph/model/failure/init.py | Updated docstring to remove NetworkView reference |
| ngraph/model/demand/spec.py | Updated TrafficDemand to use FlowPolicyPreset instead of FlowPolicyConfig |
Comments suppressed due to low confidence (2)
ngraph/workflow/maximum_supported_demand_step.py:1
- Redundant ternary expression. 'max_path_cost_factor if max_path_cost_factor is not None else None' simplifies to just 'max_path_cost_factor'.
"""Maximum Supported Demand (MSD) workflow step.
ngraph/solver/paths.py:1
- Using strict=False in zip() defeats the purpose of data integrity checking. If costs and flows arrays must be equal length, use strict=True; otherwise, add explicit length validation before the zip.
"""Shortest-path solver wrappers bound to the model layer.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| workflow_data: List[Dict[str, Any]], | ||
| derive_seed, | ||
| ) -> List[WorkflowStep]: |
There was a problem hiding this comment.
Missing type hint for 'derive_seed' parameter. Should specify 'Callable[[str], int | None]' for clarity.
| logger = get_logger(__name__) | ||
|
|
||
|
|
||
| def _fmt_float_key(x: float, places: int = 9) -> str: |
There was a problem hiding this comment.
[nitpick] Function '_fmt_float_key' is used but lacks explanation of why it was extracted from the original method. Consider adding a module-level docstring explaining this is a shared utility.
| out.append( | ||
| type( | ||
| "_Rule", | ||
| (), | ||
| { | ||
| "entity_scope": entity_scope, | ||
| "conditions": conditions, | ||
| "logic": rule_dict.get("logic", "or"), | ||
| "rule_type": rule_dict.get("rule_type", "all"), | ||
| "probability": rule_dict.get("probability", 1.0), | ||
| "count": rule_dict.get("count", 1), | ||
| "weight_by": rule_dict.get("weight_by"), | ||
| }, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Dynamic type creation using type() creates objects without proper class definition. Consider defining a proper FailureRule class or dataclass for better type safety and maintainability.
ngraph/model/failure/parser.py
Outdated
| policy = build_failure_policy( | ||
| fp_data, | ||
| policy_name=name, | ||
| derive_seed=lambda n: derive_seed(f"failure_policy:{n}"), |
There was a problem hiding this comment.
Lambda captures 'derive_seed' from outer scope and calls it recursively with modified argument. This appears to be a namespace collision - the inner 'derive_seed' shadows the parameter. Should use a different name for the lambda parameter.
ngraph/model/path.py
Outdated
| # Cost recalculation with EdgeRef requires mapping back to graph edges | ||
| # For now, we leave cost at 0 (caller can recalculate if needed) |
There was a problem hiding this comment.
Setting cost to 0 when graph is None silently produces incorrect results. Consider raising an error or returning None to make the limitation explicit rather than returning a Path with invalid cost.
ngraph/model/network.py
Outdated
| try: | ||
| import netgraph_core # noqa: F401 | ||
| except ImportError as e: | ||
| raise ImportError( | ||
| "netgraph_core module not found. Ensure NetGraph-Core is installed." | ||
| ) from e |
There was a problem hiding this comment.
[nitpick] Import check at module level causes network.py to fail early if netgraph_core is missing. Consider moving this check to a lazy import location or the actual usage sites to allow partial module use for testing/development.
| try: | |
| import netgraph_core # noqa: F401 | |
| except ImportError as e: | |
| raise ImportError( | |
| "netgraph_core module not found. Ensure NetGraph-Core is installed." | |
| ) from e |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
ngraph/solver/maxflow.py
Outdated
| node_mask = None | ||
| edge_mask = None | ||
| if excluded_nodes or excluded_links: | ||
| node_mask = _build_node_mask(_cache, excluded_nodes) | ||
| edge_mask = _build_edge_mask(_cache, excluded_links) |
There was a problem hiding this comment.
In the cached branch of max_flow the node/edge masks are only built when excluded_nodes or excluded_links are explicitly provided (node_mask/edge_mask stay None otherwise), so the disabled_node_ids/disabled_link_ids precomputed in the cache are never applied. As a result, calling max_flow on a network with disabled nodes or links will still route flow through those components unless the caller manually passes them as exclusions, which is a regression from the previous implementation that automatically removed disabled items when building the graph.
Useful? React with 👍 / 👎.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
This PR is being reviewed by Cursor Bugbot
Details
You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
dev/perf/runner.py
Outdated
| # Create a closure that captures the graph and source | ||
| def run_spf(): | ||
| return nx.dijkstra_predecessor_and_distance(graph, source) | ||
| return nx.dijkstra_predecessor_and_distance(nx_graph, source) |
There was a problem hiding this comment.
Bug: NetworkX SPF benchmark ignores edge costs
The _execute_spf_networkx_benchmark function adds edges with cost=link.cost attribute but then calls nx.dijkstra_predecessor_and_distance(nx_graph, source) without specifying weight='cost'. NetworkX's Dijkstra defaults to looking for a weight attribute, so the actual edge costs are ignored and all edges are treated as having weight 1. This makes the NetworkX vs NetGraph-Core SPF benchmark comparison potentially misleading since they may not be computing the same thing.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 97 out of 213 changed files in this pull request and generated 4 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| max_cost_factor=max_path_cost_factor | ||
| if max_path_cost_factor is not None | ||
| else None, |
There was a problem hiding this comment.
[nitpick] The conditional expression spans multiple lines unnecessarily. Consider simplifying to a single line: max_cost_factor=max_path_cost_factor, since None is a valid value for the parameter.
| max_cost_factor=max_path_cost_factor | |
| if max_path_cost_factor is not None | |
| else None, | |
| max_cost_factor=max_path_cost_factor, |
| logger = get_logger(__name__) | ||
|
|
||
|
|
||
| def _fmt_float_key(x: float, places: int = 9) -> str: |
There was a problem hiding this comment.
[nitpick] The function docstring doesn't explain why places=9 was chosen. Consider documenting the rationale (e.g., sufficient precision for cost values while avoiding floating-point artifacts).
| ) | ||
|
|
||
| # Convert to node-link format for serialization | ||
| # Use edges="edges" for forward compatibility with NetworkX 3.6+ |
There was a problem hiding this comment.
The comment mentions 'forward compatibility with NetworkX 3.6+' but NetworkX 3.6 hasn't been released as of the knowledge cutoff (January 2025). Verify this version exists or update the comment to reflect the actual future version being targeted.
| # Use edges="edges" for forward compatibility with NetworkX 3.6+ | |
| # Use edges="edges" for forward compatibility with future NetworkX versions (>=3.5) |
| # Cost set to infinity to explicitly signal recalculation is needed. | ||
| # EdgeRef-based cost calculation requires mapping back to graph edges. | ||
| return Path(tuple(new_elements), float("inf")) |
There was a problem hiding this comment.
[nitpick] Setting cost to float('inf') silently indicates incomplete functionality. Consider raising NotImplementedError with a clear message about graph parameter being required for cost recalculation, rather than returning a Path with misleading infinite cost.
| # Cost set to infinity to explicitly signal recalculation is needed. | |
| # EdgeRef-based cost calculation requires mapping back to graph edges. | |
| return Path(tuple(new_elements), float("inf")) | |
| # Cost recalculation requires mapping back to graph edges. | |
| if graph is None: | |
| raise NotImplementedError( | |
| "Cost recalculation for sub-paths requires a graph parameter. " | |
| "Please provide a graph to enable cost computation, or implement this functionality." | |
| ) | |
| # Future implementation: recalculate cost using graph and cost_attr. | |
| # For now, raise error if graph is not provided. | |
| # Example (to implement): cost = graph.calculate_path_cost(new_elements, cost_attr) | |
| # return Path(tuple(new_elements), cost) |
A major overhaul of the NetGraph with the split into a C++ algo-core (NetGraph-Core) and a high-level DSL + orchestration (NetGraph). Provided ~20x improvement in performance.
Note
Integrates NetGraph-Core and overhauls APIs (solver, path, workflow, failure/components), replacing Python impls with C++-backed functionality and introducing breaking changes and new flow presets.
0.10.0and addnetgraph-coredependency.ngraph.solver.{maxflow,paths}using Core; add mask-based exclusions.NetworkView; callers passexcluded_nodes/links.max_flow_with_detailsreturns Core-backedFlowSummarywith cost distribution.ngraph.model.path.Pathnow usesEdgeRef(link_id + direction).ngraph.types.dto(EdgeRef,FlowSummary).FlowPolicyPresetandcreate_flow_policy()(TE ECMP/WCMP, LSP limits).BuildGraphnow emits NetworkX node-link JSON; reverse-edge control viaadd_reverse.ngraph.model.failure.*; add builders/parsers.ngraph.model.components.results.snapshot, optimizeresults.flowserialization; remove legacy artifacts/MC helpers.Written by Cursor Bugbot for commit 13ab444. This will update automatically on new commits. Configure here.