Conversation
## Summary by CodeRabbit ## Release Notes * **Performance** * Optimized branch-and-bound algorithm with improved search termination conditions * **Improvements** * Enhanced concurrency control mechanisms across solver components * Improved logger initialization and lifecycle management for better resource handling <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> Authors: - Alice Boucher (https://github.com/aliceb-nv) Approvers: - Nicolas Blin (https://github.com/Kh4ster) URL: NVIDIA#691
|
/ok to test f341e34 |
📝 WalkthroughWalkthroughThe PR extends the basis repair and basis update mechanisms in the dual simplex solver to accept variable bounds vectors, enabling bounds-aware repair decisions. Function signatures for Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
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 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
cpp/src/dual_simplex/basis_solves.cpp (1)
614-671: Handle fixed variables inbasis_repair(and assert bounds sizes). The new bound-aware status assignment (Lines 663-671) is the right direction, but it misclassifies fixed variables (lower == upper) asNONBASIC_LOWER. That can cascade into incorrect bound placement / infeasibility tracking. Also consider guarding against bounds vector size mismatches.template <typename i_t, typename f_t> i_t basis_repair(const csc_matrix_t<i_t, f_t>& A, const simplex_solver_settings_t<i_t, f_t>& settings, const std::vector<f_t>& lower, const std::vector<f_t>& upper, const std::vector<i_t>& deficient, const std::vector<i_t>& slacks_needed, std::vector<i_t>& basis_list, std::vector<i_t>& nonbasic_list, std::vector<variable_status_t>& vstatus) { const i_t m = A.m; const i_t n = A.n; assert(basis_list.size() == m); assert(nonbasic_list.size() == n - m); + assert(lower.size() == static_cast<size_t>(n)); + assert(upper.size() == static_cast<size_t>(n)); ... vstatus[replace_j] = variable_status_t::BASIC; - // This is the main issue. What value should bad_j take on. - if (lower[bad_j] == -inf && upper[bad_j] == inf) { + // Set removed basic variable to an appropriate nonbasic bound status. + if (std::abs(upper[bad_j] - lower[bad_j]) < settings.fixed_tol) { + vstatus[bad_j] = variable_status_t::NONBASIC_FIXED; + } else if (lower[bad_j] == -inf && upper[bad_j] == inf) { vstatus[bad_j] = variable_status_t::NONBASIC_FREE; } else if (lower[bad_j] > -inf) { vstatus[bad_j] = variable_status_t::NONBASIC_LOWER; } else if (upper[bad_j] < inf) { vstatus[bad_j] = variable_status_t::NONBASIC_UPPER; } else { assert(1 == 0); } }cpp/src/dual_simplex/phase2.cpp (1)
653-691: Bug:update_single_primal_infeasibilityupdates a squared accumulator but is called with the linear accumulator.
update_single_primal_infeasibilitycomputesold_val/new_valas squared infeasibilities and applies(new_val - old_val)to itsprimal_infreference, but the call at Line 2832+ passesprimal_infeasibility(linear sum). This will corrupt the linear metric (and can go negative/oscillate despitemax(0, ...)).Minimal fix (keep
update_single_primal_infeasibilityas “squared” updater, consistent with the other two calls):@@ phase2::update_single_primal_infeasibility(lp.lower, lp.upper, x, settings.primal_tol, squared_infeasibilities, infeasibility_indices, entering_index, - primal_infeasibility); + primal_infeasibility_squared);Follow-up (recommended): rename the parameter to
primal_inf_squaredinupdate_single_primal_infeasibility/update_primal_infeasibilitiesto prevent future mixups.Also applies to: 2809-2840
🧹 Nitpick comments (1)
cpp/src/dual_simplex/basis_updates.cpp (1)
2046-2072: Add defensive size checks (and consider checking thebasis_repairreturn) before using bounds.
Right nowlower/upperare assumed consistent withA.n/vstatus, andbasis_repair(...)’s return is ignored—fine if guaranteed, but brittle if upstream ever violates invariants. As per coding guidelines/learnings, validating bounds/state at phase transitions is important.@@ int basis_update_mpf_t<i_t, f_t>::refactor_basis( const csc_matrix_t<i_t, f_t>& A, const simplex_solver_settings_t<i_t, f_t>& settings, const std::vector<f_t>& lower, const std::vector<f_t>& upper, std::vector<i_t>& basic_list, std::vector<i_t>& nonbasic_list, std::vector<variable_status_t>& vstatus) { + assert(lower.size() == static_cast<size_t>(A.n)); + assert(upper.size() == static_cast<size_t>(A.n)); + assert(vstatus.size() == static_cast<size_t>(A.n)); + @@ if (factorize_basis(...) == -1) { settings.log.debug("Initial factorization failed\n"); - basis_repair(A, settings, lower, upper, deficient, slacks_needed, basic_list, nonbasic_list, vstatus); + const auto repair_rc = + basis_repair(A, settings, lower, upper, deficient, slacks_needed, basic_list, nonbasic_list, vstatus); + (void)repair_rc; // or handle if non-zero is meaningful
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cpp/src/dual_simplex/basis_solves.cpp(3 hunks)cpp/src/dual_simplex/basis_solves.hpp(1 hunks)cpp/src/dual_simplex/basis_updates.cpp(2 hunks)cpp/src/dual_simplex/basis_updates.hpp(1 hunks)cpp/src/dual_simplex/crossover.cpp(3 hunks)cpp/src/dual_simplex/phase2.cpp(12 hunks)cpp/src/dual_simplex/primal.cpp(1 hunks)
🧰 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/basis_updates.hppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/basis_solves.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/dual_simplex/basis_solves.hppcpp/src/dual_simplex/phase2.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/basis_updates.hppcpp/src/dual_simplex/basis_solves.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/basis_updates.hppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/basis_solves.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/dual_simplex/basis_solves.hppcpp/src/dual_simplex/phase2.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/basis_updates.hppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/basis_solves.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/dual_simplex/basis_solves.hppcpp/src/dual_simplex/phase2.cpp
🧠 Learnings (9)
📓 Common learnings
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
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)
📚 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/basis_updates.hppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/basis_solves.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/dual_simplex/basis_solves.hppcpp/src/dual_simplex/phase2.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/primal.cppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/basis_solves.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/dual_simplex/basis_solves.hppcpp/src/dual_simplex/phase2.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} : 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/primal.cppcpp/src/dual_simplex/basis_solves.cppcpp/src/dual_simplex/phase2.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/crossover.cppcpp/src/dual_simplex/basis_solves.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/dual_simplex/phase2.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 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/crossover.cppcpp/src/dual_simplex/basis_solves.cppcpp/src/dual_simplex/basis_updates.cppcpp/src/dual_simplex/basis_solves.hppcpp/src/dual_simplex/phase2.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/basis_solves.cppcpp/src/dual_simplex/phase2.cpp
📚 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/basis_solves.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} : 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/basis_solves.cpp
🧬 Code graph analysis (5)
cpp/src/dual_simplex/basis_updates.hpp (1)
cpp/src/dual_simplex/presolve.cpp (3)
lower(240-240)upper(85-85)upper(241-241)
cpp/src/dual_simplex/crossover.cpp (2)
cpp/src/dual_simplex/basis_solves.cpp (3)
basis_repair(614-675)basis_repair(614-622)basis_repair(860-868)cpp/src/dual_simplex/basis_solves.hpp (1)
basis_repair(43-51)
cpp/src/dual_simplex/basis_solves.cpp (1)
cpp/src/dual_simplex/presolve.cpp (3)
lower(240-240)upper(85-85)upper(241-241)
cpp/src/dual_simplex/basis_updates.cpp (3)
cpp/src/dual_simplex/basis_solves.cpp (3)
basis_repair(614-675)basis_repair(614-622)basis_repair(860-868)cpp/src/dual_simplex/basis_solves.hpp (1)
basis_repair(43-51)cpp/src/dual_simplex/basis_updates.hpp (1)
A(374-380)
cpp/src/dual_simplex/phase2.cpp (1)
cpp/src/dual_simplex/primal.cpp (2)
primal_infeasibility(202-240)primal_infeasibility(202-205)
🔇 Additional comments (7)
cpp/src/dual_simplex/primal.cpp (1)
301-301: Updatedbasis_repaircall looks correct (bounds propagated). Line 301 now passeslp.lower/lp.upper, aligning with the new bound-aware repair contract.cpp/src/dual_simplex/crossover.cpp (1)
788-790: Allbasis_repaircall sites updated consistently. These hunks correctly passlp.lower/lp.upper, matching the updatedbasis_repairsignature.Also applies to: 1135-1136, 1326-1327
cpp/src/dual_simplex/basis_solves.cpp (1)
860-868: Instantiation updated consistently. Template instantiation now matches the newbasis_repair(..., lower, upper, ...)signature.cpp/src/dual_simplex/basis_solves.hpp (1)
43-51: Header signature matches the new bound-aware repair API. Addslower/upperin the right position to align with implementations and updated call sites.cpp/src/dual_simplex/basis_updates.hpp (1)
374-380: Signature update is correctly implemented across all call sites. All threerefactor_basiscalls in cpp/src/dual_simplex/phase2.cpp (lines 2248, 2891, 2898) correctly passlp.lowerandlp.upperafter bounds validation. The assertion checks at lines 2222–2223 ensurelower.size() == upper.size() == nbefore invocation, satisfying the contract requirements.cpp/src/dual_simplex/phase2.cpp (2)
621-651: Good fix:squared_infeasibilitiesis now cleared on recomputation.
This addresses the “stale infeasible list” failure mode and aligns with the “reset algorithm state between phases/recomputations” guidance.
2248-2250: Bounds-awarerefactor_basis(...)wiring looks consistent.
Passinglp.lower/lp.upperthrough refactor/repair is the right direction for correct post-repairvstatusclassification (removed variable becomes NONBASIC_{LOWER,UPPER,FREE}).Also applies to: 2891-2902
nguidotti
left a comment
There was a problem hiding this comment.
LGTM. I can confirm that this fixes the bound violations in neos-2657525-crna. It has similar results for netlib for simplex, barrier + crossover and pdlp + crossover as the main branch.
|
🔔 Hi @anandhkb @chris-maes, 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. |
1 similar comment
|
🔔 Hi @anandhkb @chris-maes, 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. |
|
🔔 Hi @anandhkb @chris-maes, 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. |
The goal of this PR is to improve the diving framework in the MIP solver based on [1, Section 9.2]. It introduces the following changes: - Modify diving implementation to use diving heuristics, such as coefficient diving (9.2.1), line search diving (9.2.4), guided diving (9.2.3) and pseudocost diving (9.2.5). - Impose a node, iteration and backtrack limit to force the exploration of different branches of the tree (#718) - Unified the diving and best-first heap into a single object (`node_queue`) to allow the sharing of information between them. This also greatly reduces memory consumption (`33GB` vs `48GB` for `neos-848589` after `250s`) since the lower and upper variable no longer needs to be stored for diving (#718). - Order the node in the diving heap based on the pseudocost estimate [1, Section 6.4] (#718) - Adjusted B&B logs to allow 2 letter symbols in the table (#714) - Includes #694 MIPLIB results (GH200): main (#718): 226 feasible solutions with 12.8% average primal gap | | **All** | **No Coefficient Diving** | **No Pseudocost Diving** | **No Guided Diving** | **No Line Search Diving** | | :-------------------------: | :-----: | :-----------------------: | :----------------------: | :------------------: | :-----------------------: | | **Feasible Solutions** | 225 | 224 | 225 | 224 | 225 | | **Optimal** | 44 | 43 | 45 | 43 | 44 | | **Average Primal Gap** | 12.083 | 12.486 | 12.658 | 12.431 | 13.151 | | **Average Primal Integral** | 8233 | 8508 | 8524 | 8406 | 8789 | Reference: [1] T. Achterberg, “Constraint Integer Programming,” PhD, Technischen Universität Berlin, Berlin, 2007. doi: 10.14279/depositonce-1634. Authors: - Nicolas L. Guidotti (https://github.com/nguidotti) - Chris Maes (https://github.com/chris-maes) Approvers: - Chris Maes (https://github.com/chris-maes) - Ramakrishnap (https://github.com/rgsl888prabhu) URL: #697
|
This fix is included in #697. |
This fixes two issues that could cause dual simplex infeasible list to incorrect:
squared_infeasibilitesSummary by CodeRabbit
Release Notes
Bug Fixes
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.