Unified Node Queue + Diving Node and Iteration Limits #718
Conversation
…iteration and node limit to the diving threads.
nguidotti
added
non-breaking
nguidotti
changed the title
📝 WalkthroughWalkthroughThis PR refactors the branch-and-bound solver's node management system by replacing heap-based storage with a unified node_queue abstraction, introducing integrated statistics tracking via bnb_stats_t, updating the variable selection API to return objective estimates alongside branch variables, and consolidating diving and best-first node handling. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
cpp/src/dual_simplex/node_queue.hpp (1)
36-41: Minor: Redundant&&instd::forward.
std::forward<Args&&>works butstd::forward<Args>is the canonical form.template <typename... Args> void emplace(Args&&... args) { - buffer.emplace_back(std::forward<Args&&>(args)...); + buffer.emplace_back(std::forward<Args>(args)...); std::push_heap(buffer.begin(), buffer.end(), comp); }cpp/src/dual_simplex/branch_and_bound.cpp (4)
599-604: Potential early termination before any LP work is done.The iteration limit is calculated as
0.05 * bnb_lp_iters - stats.total_lp_iters. On the first call when both values are small, this could be zero or negative, causing immediateITERATION_LIMITreturn before any LP iterations are performed. This could cause diving threads to spin without making progress.Consider adding a minimum iteration allowance:
if (thread_type != thread_type_t::EXPLORATION) { i_t bnb_lp_iters = exploration_stats_.total_lp_iters; f_t max_iter = 0.05 * bnb_lp_iters; - lp_settings.iteration_limit = max_iter - stats.total_lp_iters; - if (lp_settings.iteration_limit <= 0) { return node_solve_info_t::ITERATION_LIMIT; } + lp_settings.iteration_limit = std::max(static_cast<f_t>(100), max_iter - stats.total_lp_iters); + if (max_iter > 0 && stats.total_lp_iters >= max_iter) { return node_solve_info_t::ITERATION_LIMIT; } }
1127-1131: Consider documenting initialization ofdive_stats.The explicit initialization of
dive_statsfields to zero is clear, but sincebnb_stats_talready has default member initializers (line 72-76 in header), this could be simplified. However, the explicit initialization is not incorrect and provides clarity.bnb_stats_t<i_t, f_t> dive_stats; - dive_stats.total_lp_iters = 0; - dive_stats.total_lp_solve_time = 0; - dive_stats.nodes_explored = 0; - dive_stats.nodes_unexplored = 0; + // dive_stats uses default initialization from bnb_stats_t
1144-1145: Consider making dive limits configurable.The hardcoded limits (time limit check and
dive_stats.nodes_explored > 500) are reasonable heuristics but could benefit from being configurable viasimplex_solver_settings_tfor tuning on different problem types.
1180-1182: Consider documenting the sibling pruning heuristic.The condition
stack.front()->depth - stack.back()->depth > 5prunes siblings that are more than 5 levels apart. This is a valid heuristic to keep dives focused, but a brief comment explaining the rationale would improve maintainability.+ // Prune distant siblings to keep the dive focused on a single path if (stack.size() > 1 && stack.front()->depth - stack.back()->depth > 5) { stack.pop_back(); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cpp/src/dual_simplex/bounds_strengthening.cpp(2 hunks)cpp/src/dual_simplex/branch_and_bound.cpp(18 hunks)cpp/src/dual_simplex/branch_and_bound.hpp(6 hunks)cpp/src/dual_simplex/diving_queue.hpp(0 hunks)cpp/src/dual_simplex/mip_node.hpp(4 hunks)cpp/src/dual_simplex/node_queue.hpp(1 hunks)cpp/src/dual_simplex/pseudo_costs.cpp(5 hunks)cpp/src/dual_simplex/pseudo_costs.hpp(1 hunks)
💤 Files with no reviewable changes (1)
- cpp/src/dual_simplex/diving_queue.hpp
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cu,cuh,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh,cpp,hpp,h}: Track GPU device memory allocations and deallocations to prevent memory leaks; ensure cudaMalloc/cudaFree balance and cleanup of streams/events
Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Check numerical stability: prevent overflow/underflow, precision loss, division by zero/near-zero, and use epsilon comparisons for floating-point equality checks
Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
For concurrent CUDA operations (barriers, async operations), explicitly create and manage dedicated streams instead of reusing the default stream; document stream lifecycle
Eliminate unnecessary host-device synchronization (cudaDeviceSynchronize) in hot paths that blocks GPU pipeline; use streams and events for async execution
Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse
Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)
Ensure race conditions are absent in multi-GPU code and multi-threaded server implementations; verify proper synchronization of shared state
Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Check that hard-coded GPU de...
Files:
cpp/src/dual_simplex/pseudo_costs.hppcpp/src/dual_simplex/bounds_strengthening.cppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/node_queue.hppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hppcpp/src/dual_simplex/pseudo_costs.cpp
**/*.{h,hpp,py}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Verify C API does not break ABI stability (no struct layout changes, field reordering); maintain backward compatibility in Python and server APIs with deprecation warnings
Files:
cpp/src/dual_simplex/pseudo_costs.hppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/node_queue.hppcpp/src/dual_simplex/branch_and_bound.hpp
**/*.{cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cpp,hpp,h}: Check for unclosed file handles when reading MPS/QPS problem files; ensure RAII patterns or proper cleanup in exception paths
Validate input sanitization to prevent buffer overflows and resource exhaustion attacks; avoid unsafe deserialization of problem files
Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state
Files:
cpp/src/dual_simplex/pseudo_costs.hppcpp/src/dual_simplex/bounds_strengthening.cppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/node_queue.hppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hppcpp/src/dual_simplex/pseudo_costs.cpp
**/*.{cu,cpp,hpp,h}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Files:
cpp/src/dual_simplex/pseudo_costs.hppcpp/src/dual_simplex/bounds_strengthening.cppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/node_queue.hppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hppcpp/src/dual_simplex/pseudo_costs.cpp
🧠 Learnings (10)
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
Applied to files:
cpp/src/dual_simplex/pseudo_costs.hppcpp/src/dual_simplex/bounds_strengthening.cppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/node_queue.hppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hppcpp/src/dual_simplex/pseudo_costs.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Validate correct initialization of variable bounds, constraint coefficients, and algorithm state before solving; ensure reset when transitioning between algorithm phases (presolve, simplex, diving, crossover)
Applied to files:
cpp/src/dual_simplex/bounds_strengthening.cppcpp/src/dual_simplex/mip_node.hppcpp/src/dual_simplex/node_queue.hppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hppcpp/src/dual_simplex/pseudo_costs.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*test*.{cpp,cu,py} : Add tests for algorithm phase transitions: verify correct initialization of bounds and state when transitioning from presolve to simplex to diving to crossover
Applied to files:
cpp/src/dual_simplex/bounds_strengthening.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Identify assertions with overly strict numerical tolerances that fail on legitimate degenerate/edge cases (near-zero pivots, singular matrices, empty problems)
Applied to files:
cpp/src/dual_simplex/bounds_strengthening.cpp
📚 Learning: 2025-12-04T20:09:09.264Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 602
File: cpp/src/linear_programming/solve.cu:732-742
Timestamp: 2025-12-04T20:09:09.264Z
Learning: In cpp/src/linear_programming/solve.cu, the barrier solver does not currently return INFEASIBLE or UNBOUNDED status. It only returns OPTIMAL, TIME_LIMIT, NUMERICAL_ISSUES, or CONCURRENT_LIMIT.
Applied to files:
cpp/src/dual_simplex/node_queue.hppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Ensure variables and constraints are accessed from the correct problem context (original vs presolve vs folded vs postsolve); verify index mapping consistency across problem transformations
Applied to files:
cpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hppcpp/src/dual_simplex/pseudo_costs.cpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Reduce tight coupling between solver components (presolve, simplex, basis, barrier); increase modularity and reusability of optimization algorithms
Applied to files:
cpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Refactor code duplication in solver components (3+ occurrences) into shared utilities; for GPU kernels, use templated device functions to avoid duplication
Applied to files:
cpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hpp
📚 Learning: 2025-11-25T10:20:49.822Z
Learnt from: CR
Repo: NVIDIA/cuopt PR: 0
File: .github/.coderabbit_review_guide.md:0-0
Timestamp: 2025-11-25T10:20:49.822Z
Learning: Applies to **/*.{cu,cuh,cpp,hpp,h} : Assess algorithmic complexity for large-scale problems (millions of variables/constraints); ensure O(n log n) or better complexity, not O(n²) or worse
Applied to files:
cpp/src/dual_simplex/branch_and_bound.cppcpp/src/dual_simplex/branch_and_bound.hpp
📚 Learning: 2025-12-04T04:11:12.640Z
Learnt from: chris-maes
Repo: NVIDIA/cuopt PR: 500
File: cpp/src/dual_simplex/scaling.cpp:68-76
Timestamp: 2025-12-04T04:11:12.640Z
Learning: In the cuOPT dual simplex solver, CSR/CSC matrices (including the quadratic objective matrix Q) are required to have valid dimensions and indices by construction. Runtime bounds checking in performance-critical paths like matrix scaling is avoided to prevent slowdowns. Validation is performed via debug-only check_matrix() calls wrapped in #ifdef CHECK_MATRIX.
Applied to files:
cpp/src/dual_simplex/branch_and_bound.hpp
🧬 Code graph analysis (3)
cpp/src/dual_simplex/node_queue.hpp (1)
cpp/src/dual_simplex/mip_node.hpp (4)
lower(97-108)lower(97-99)lower(112-130)lower(112-114)
cpp/src/dual_simplex/branch_and_bound.cpp (4)
cpp/src/dual_simplex/branch_and_bound.hpp (3)
node_ptr(245-257)node_ptr(260-260)node(209-212)cpp/src/dual_simplex/mip_node.hpp (6)
node_ptr(277-283)node_ptr(277-277)log(329-337)log(329-332)log(339-354)log(339-344)cpp/src/dual_simplex/pseudo_costs.hpp (3)
node_ptr(31-31)fractional(46-49)fractional(51-52)cpp/src/dual_simplex/node_queue.hpp (4)
node(24-28)node(24-24)node(30-34)node(30-30)
cpp/src/dual_simplex/pseudo_costs.cpp (1)
cpp/src/dual_simplex/pseudo_costs.hpp (3)
fractional(46-49)fractional(51-52)num_initialized_down(41-44)
🪛 Cppcheck (2.18.0)
cpp/src/dual_simplex/pseudo_costs.cpp
[warning] 320-320: Array index -1 is out of bounds.
(negativeContainerIndex)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.0.2, 3.10, amd64, rockylinux8
- GitHub Check: checks / check-style
🔇 Additional comments (30)
cpp/src/dual_simplex/bounds_strengthening.cpp (2)
157-165: LGTM: Debug logging for infeasibility detection.Switching to
settings.log.debug()aligns with the project's logging conventions and keeps diagnostic output manageable in production.
214-216: LGTM: Consistent debug logging for bound violation.Same pattern applied for the variable bound infeasibility check.
cpp/src/dual_simplex/pseudo_costs.hpp (1)
46-49: LGTM: API update for combined variable selection and objective estimation.The new signature returning
std::pair<i_t, f_t>cleanly provides both the branch variable and the objective estimate in a single call, reducing redundant computation and aligning with the pseudocost-based node ordering in the diving heap.cpp/src/dual_simplex/mip_node.hpp (3)
48-49: LGTM: Proper default initialization of objective_estimate.Initializing to infinity is correct—nodes start with the worst possible estimate until computed.
85-86: LGTM: Parent-to-child propagation of objective_estimate.Propagating the parent's estimate to children is correct; the estimate will be updated when the child node is processed by
variable_selection_and_obj_estimate.
233-240: LGTM: detach_copy preserves objective_estimate.The detached copy correctly includes all node state including the new
objective_estimatefield.cpp/src/dual_simplex/pseudo_costs.cpp (6)
202-202: LGTM: RAII-based locking.Using
std::lock_guardinstead of manual lock/unlock ensures the mutex is released even if an exception occurs.
262-262: LGTM: Thread-safe variable selection.Acquiring the lock protects the pseudo-cost data structures during read operations.
268-268: LGTM: Objective estimate initialization.Starting the estimate from
lower_boundis correct—the pseudocost contributions are then added to this base.
301-304: LGTM: Pseudocost-based objective estimate accumulation.The estimate uses
min(down_cost, up_cost)per fractional variable, which is a standard optimistic estimate for the child node's objective (assuming optimal branching direction). This aligns with Achterberg's approach cited in the PR description.
323-323: LGTM: Return type matches new API.Returning the pair
{branch_var, estimate}correctly fulfills the newvariable_selection_and_obj_estimatecontract.
306-321: Undefined behavior concern is invalid—both callers guarantee non-emptyfractional.The first call site in
solve_node()is protected by anelse ifthat only executes whenleaf_num_fractional != 0(line 17). The second call site in root solving checksif (num_fractional == 0)and returns early (lines 13–35), ensuringvariable_selection_and_obj_estimate()is never called with emptyfractional. The suggested defensive guard is unnecessary.Likely an incorrect or invalid review comment.
cpp/src/dual_simplex/node_queue.hpp (5)
18-61: LGTM: Clean STL-based heap wrapper.The generic
heap_tcorrectly leveragesstd::push_heap/std::pop_heapwhile exposing the underlying container for direct access when needed.
67-76: LGTM: Shared heap entry design.Using
heap_entry_twithshared_ptrallows both heaps to reference the same node without duplication, directly supporting the PR's memory reduction goal.
103-109: LGTM: Dual-heap push with thread safety.Pushing to both heaps under a single lock ensures consistent state and avoids partial updates.
134-152: LGTM:pop_divingcorrectly handles consumed entries.The loop skips entries where
node_ptrisnullptr(already consumed viapop_best_first), and extracts bounds before returning the detached copy to avoid races with node fathoming.
166-170: LGTM: Safe lower bound retrieval.Returns
inffor an empty heap, which is the correct semantics (no nodes means no finite lower bound).cpp/src/dual_simplex/branch_and_bound.hpp (3)
70-81: LGTM on the newbnb_stats_tstructure.The use of
omp_atomic_tfor thread-safe counters (total_lp_solve_time,nodes_explored,nodes_unexplored,total_lp_iters) is appropriate for the multi-threaded B&B context. The non-atomicstart_timeis acceptable since it's initialized once before the parallel region.Minor observation:
last_logandnodes_since_last_logare marked atomic but the comment states they're only used by the main thread. This is safe but slightly over-synchronized.
214-239: Approve updated traversal method signatures.The new
plunge_fromanddive_fromsignatures properly separate shallow plunging (best-first with bounded depth) from deep diving. Thedive_fromtaking explicitstart_lower/start_upperbounds is consistent with the memory optimization goal of not storing bounds per-node in the diving queue.
176-177: Thread-safety verification complete:node_queue_tproperly synchronizes concurrent access.The
node_queue_timplementation correctly provides internal synchronization viaomp_mutex_tmutex. All public methods that may be called concurrently (push,pop_best_first,pop_diving, and size/bound queries) are protected withstd::lock_guard, ensuring thread-safe access to the underlying heaps and shared state.cpp/src/dual_simplex/branch_and_bound.cpp (10)
247-255: LGTM onget_lower_bound()refactoring.The aggregation from
lower_bound_ceiling_,node_queue.get_lower_bound(), andlocal_lower_bounds_correctly computes the global lower bound. The final check returning-inffor non-finite values is a good defensive practice.
680-681: LGTM on stats accumulation.The LP solve time and iteration counts are correctly accumulated into the stats structure passed to
solve_node.
725-727: Approve new variable selection API usage.The structured binding correctly captures both
branch_varandobj_estimatefrom the newvariable_selection_and_obj_estimateAPI, andobjective_estimateis properly stored on the node for use in diving queue ordering.
751-752: LGTM on ITERATION_LIMIT propagation.The new
ITERATION_LIMITstatus is correctly propagated from the LP solver throughsolve_node.
881-882: LGTM on node_queue.push() usage in ramp-up.Nodes are correctly pushed to the unified queue once the ramp-up phase has generated enough nodes to keep threads busy.
924-924: Good fix: Reset basis when node is fathomed.Setting
recompute_bounds_and_basis = truewhen a node is fathomed ensures the next node processed will have its bounds and basis correctly recomputed rather than relying on incremental updates from a now-invalid parent path.
1039-1063: LGTM on best_first_thread refactoring.The transition from direct heap access to
node_queue.pop_best_first()is clean. The optional-based return correctly handles the case when no node is available while other subtrees are still active.
1163-1168: LGTM on ITERATION_LIMIT handling in diving.Breaking out of the dive loop on
ITERATION_LIMITis correct behavior - it allows the diving thread to abandon this dive and potentially pick up a new starting node rather than continuing to consume iterations on a potentially unproductive path.
1404-1405: LGTM on root variable selection with new API.The structured binding correctly uses the new
variable_selection_and_obj_estimateAPI at the root node, consistent with usage insolve_node.
1469-1470: LGTM on final lower bound calculation.The conditional correctly uses
node_queue.get_lower_bound()when the queue is non-empty, falling back to the root's lower bound when the tree has been fully explored.
|
🔔 Hi @anandhkb @nguidotti, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you! If this is an "epic" issue, then please add the "epic" label to this issue. |
2 participants
This PR introduces the following changes:
node_queue) to allow the sharing of information between them. This also greatly reduces memory consumption (33GBvs48GBforneos-848589after250s) since the lower and upper variable no longer needs to be stored for diving.The performance remains the same as the main branch:
226feasible solutions with~12.8%primal gap on a GH200 for the MIPLIB2017 dataset.This PR was split from #697 to be easier to review.
Reference:
[1] T. Achterberg, “Constraint Integer Programming,” PhD, Technischen Universität Berlin,
Berlin, 2007. doi: 10.14279/depositonce-1634.
Checklist
Summary by CodeRabbit
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.