Main merge release/26.02 #801
Conversation
- Fixed early termination in `CMS750_4` due to an incorrect root objective from the PDLP. It provides an objective in user space and B&B expects the objective in the solver space. - Fixed setting the number of threads to a fixed value when running `cuopt_cli`. Now the number of threads in the probing cache can be controlled by the `bounds_update::settings_t`. - Inverted loop order in probing cache to avoid creating and deleting the OpenMP thread pool each iteration. - PDLP/Barrier no longer set the root objective and solution when the B&B is already running. - Improved the logs when solving the root relaxation using the concurrent mode. Authors: - Nicolas L. Guidotti (https://github.com/nguidotti) Approvers: - Akif ÇÖRDÜK (https://github.com/akifcorduk) URL: NVIDIA#786
rgsl888prabhu
added
non-breaking
📝 WalkthroughWalkthroughThis PR refactors root relaxation logging and bookkeeping in branch-and-bound, removes duplicate logging, adds conditional logging when inside MIP solve, introduces objective space transformation helpers, enables dynamic threading in presolve with configurable thread counts, and updates integration points to use the new transformations. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/mip/presolve/probing_cache.cu (1)
939-939: Revert the blanket settings reset or save and restore only the modified fields.The line
bound_presolve.settings = {};resets all settings to defaults, includingnum_threadsandparallel_bounds_updatethat may have been configured by the caller. Sincebound_presolveis passed by reference, this permanently modifies the caller's object. The function only temporarily modifiesiteration_limitandtime_limit(lines 856–857), so save and restore only those fields instead of resetting all settings. Alternatively, document this as an intentional side effect with clear expectations for callers.
🤖 Fix all issues with AI agents
In `@cpp/src/dual_simplex/branch_and_bound.cpp`:
- Around line 1371-1386: Replace the plain bool is_root_solution_set with an
atomic to prevent a data race: change the declaration to std::atomic<bool>
is_root_solution_set{false}; (mirroring root_crossover_solution_set_). In the
writer in branch_and_bound::solve() (where is_root_solution_set is set) store
using std::memory_order_release, and in readers such as
set_root_relaxation_solution() load using std::memory_order_acquire. Update any
direct reads/writes of is_root_solution_set to use atomic load/store with those
memory orders to ensure safe synchronization.
In `@cpp/src/dual_simplex/branch_and_bound.hpp`:
- Around line 87-96: Replace the plain bool guard is_root_solution_set with a
std::atomic<bool> and change the check to an atomic exchange so only one thread
proceeds to write the root solution; specifically, in
set_root_relaxation_solution use something like if
(!is_root_solution_set.exchange(true, std::memory_order_acq_rel)) { ... } (and
retain root_crossover_solution_set_.store as before). Add `#include` <atomic> and
apply the same atomic-guard fix to the other occurrence referenced (around the
second use at the other location).
In `@cpp/src/dual_simplex/solve.hpp`:
- Around line 33-47: Add a direct include for the string header to make the
header self-contained: include <string> at the top of the file that defines
lp_status_to_string so the return type std::string is available without relying
on transitive includes (update cpp/src/dual_simplex/solve.hpp to add the include
and keep the existing lp_status_to_string function as-is).
In `@cpp/src/mip/problem/problem.cu`:
- Around line 1992-1996: The division in
problem_t<i_t,f_t>::get_solver_obj_from_user_obj uses
presolve_data.objective_scaling_factor without checking for zero/near-zero; add
a guard in get_solver_obj_from_user_obj that checks if
std::abs(presolve_data.objective_scaling_factor) is below a small threshold
(e.g. relative epsilon using std::numeric_limits<f_t>::epsilon()) and if so
either assert or throw a descriptive std::runtime_error (include the offending
scaling value in the message) to prevent returning inf/NaN; otherwise perform
the division as before.
🧹 Nitpick comments (2)
cpp/src/mip/presolve/bounds_presolve.cuh (1)
24-25: Consider moving<omp.h>include to implementation files.The
<omp.h>header is included in this header file, but OpenMP APIs don't appear to be used directly in this header. Moving this include to the.cuimplementation files (likeprobing_cache.cu) that actually use OpenMP functions (omp_get_thread_num(),omp_get_max_threads()) would reduce unnecessary header dependencies.cpp/src/linear_programming/solve.cu (1)
45-47: Make the conditional logging macro statement-safe.Without a
do { ... } while (0), this macro can cause dangling-elseissues when used in control flow. Wrap it or replace with an inline function.♻️ Suggested fix
-#define CUOPT_LOG_CONDITIONAL_INFO(condition, ...) \ - if ((condition)) { CUOPT_LOG_INFO(__VA_ARGS__); } +#define CUOPT_LOG_CONDITIONAL_INFO(condition, ...) \ + do { \ + if ((condition)) { CUOPT_LOG_INFO(__VA_ARGS__); } \ + } while (0)
|
|
||
| settings_.log.printf("\n"); | ||
| if (root_status == lp_status_t::OPTIMAL) { | ||
| settings_.log.printf("Root relaxation solution found in %d iterations and %.2fs by %s\n", | ||
| iter, | ||
| toc(start_time), | ||
| solver_name.c_str()); | ||
| settings_.log.printf("Root relaxation objective %+.8e\n", user_objective); | ||
| } else { | ||
| settings_.log.printf("Root relaxation returned status: %s\n", | ||
| lp_status_to_string(root_status).c_str()); | ||
| } | ||
|
|
||
| settings_.log.printf("\n"); | ||
| is_root_solution_set = true; | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for the declaration of is_root_solution_set in header files
echo "=== Searching for is_root_solution_set declaration ==="
rg -n -C3 'is_root_solution_set' --type=cpp --type=hpp
echo ""
echo "=== Checking for std::atomic usage ==="
rg -n 'std::atomic.*is_root_solution|is_root_solution.*std::atomic' --type=cpp --type=hpp
echo ""
echo "=== Looking for class definition containing this member ==="
rg -n 'class.*BranchAndBound|class.*DualSimplex' --type=hpp -A 50 | head -100Repository: NVIDIA/cuopt
Length of output: 312
🏁 Script executed:
#!/bin/bash
# First, find header files related to branch_and_bound
echo "=== Finding branch_and_bound related header files ==="
find . -name "*branch_and_bound*" -type f | grep -E '\.(h|hpp)$'
echo ""
echo "=== Searching for is_root_solution_set in all files ==="
grep -r "is_root_solution_set" --include="*.cpp" --include="*.h" --include="*.hpp" --include="*.cuh" --include="*.cu" -n
echo ""
echo "=== Looking at the branch_and_bound.cpp file structure ==="
head -50 cpp/src/dual_simplex/branch_and_bound.cpp | grep -E '#include|class|struct'
echo ""
echo "=== Checking if it's a member variable by looking at class definition ==="
grep -r "class.*BranchAndBound" --include="*.h" --include="*.hpp" -A 100 | head -200Repository: NVIDIA/cuopt
Length of output: 1255
🏁 Script executed:
#!/bin/bash
# Check the context around line 87 in the header where it's read
echo "=== Context of is_root_solution_set check at line 87 ==="
sed -n '80,95p' cpp/src/dual_simplex/branch_and_bound.hpp
echo ""
echo "=== Full declaration context around line 167 ==="
sed -n '160,175p' cpp/src/dual_simplex/branch_and_bound.hpp
echo ""
echo "=== Check the full function where line 1385 assignment is ==="
sed -n '1350,1390p' cpp/src/dual_simplex/branch_and_bound.cppRepository: NVIDIA/cuopt
Length of output: 3037
Replace plain bool is_root_solution_set with std::atomic<bool> to prevent race condition.
The flag guards against concurrent PDLP/Barrier solvers overwriting the root solution when B&B is already running, as confirmed by the PR context. Currently, is_root_solution_set is declared as a plain bool at line 167 of the header without synchronization, while being read by set_root_relaxation_solution() (external solvers) and written by the B&B solve function at line 1385. This creates a data race between concurrent threads.
Change the declaration to std::atomic<bool> is_root_solution_set{false}; and use appropriate memory ordering (e.g., std::memory_order_acquire for read, std::memory_order_release for write), consistent with the nearby root_crossover_solution_set_ pattern already used in the class.
🤖 Prompt for AI Agents
In `@cpp/src/dual_simplex/branch_and_bound.cpp` around lines 1371 - 1386, Replace
the plain bool is_root_solution_set with an atomic to prevent a data race:
change the declaration to std::atomic<bool> is_root_solution_set{false};
(mirroring root_crossover_solution_set_). In the writer in
branch_and_bound::solve() (where is_root_solution_set is set) store using
std::memory_order_release, and in readers such as set_root_relaxation_solution()
load using std::memory_order_acquire. Update any direct reads/writes of
is_root_solution_set to use atomic load/store with those memory orders to ensure
safe synchronization.
| if (!is_root_solution_set) { | ||
| root_crossover_soln_.x = primal; | ||
| root_crossover_soln_.y = dual; | ||
| root_crossover_soln_.z = reduced_costs; | ||
| root_objective_ = objective; | ||
| root_crossover_soln_.objective = objective; | ||
| root_crossover_soln_.user_objective = user_objective; | ||
| root_crossover_soln_.iterations = iterations; | ||
| root_crossover_solution_set_.store(true, std::memory_order_release); | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -path "*/dual_simplex/branch_and_bound.hpp" -type fRepository: NVIDIA/cuopt
Length of output: 99
🏁 Script executed:
cat -n cpp/src/dual_simplex/branch_and_bound.hpp | sed -n '80,105p'Repository: NVIDIA/cuopt
Length of output: 1469
🏁 Script executed:
cat -n cpp/src/dual_simplex/branch_and_bound.hpp | sed -n '160,175p'Repository: NVIDIA/cuopt
Length of output: 659
🏁 Script executed:
rg -n "is_root_solution_set" cpp/src/dual_simplex/Repository: NVIDIA/cuopt
Length of output: 293
🏁 Script executed:
rg -n "set_root_relaxation_solution" cpp/src/dual_simplex/ -A 20Repository: NVIDIA/cuopt
Length of output: 2015
🏁 Script executed:
cat -n cpp/src/dual_simplex/branch_and_bound.cpp | sed -n '1375,1395p'Repository: NVIDIA/cuopt
Length of output: 892
🏁 Script executed:
rg -n "is_root_solution_set\s*=" cpp/src/dual_simplex/ -B 5 -A 2Repository: NVIDIA/cuopt
Length of output: 654
🏁 Script executed:
rg -n "set_root_relaxation_solution" cpp/src/dual_simplex/branch_and_bound.cpp -B 5 -A 15Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
rg -n "set_root_relaxation_solution\(" cpp/src/dual_simplex/ -B 3 -A 3Repository: NVIDIA/cuopt
Length of output: 758
🏁 Script executed:
cat -n cpp/src/dual_simplex/branch_and_bound.cpp | sed -n '1320,1390p'Repository: NVIDIA/cuopt
Length of output: 3543
🏁 Script executed:
rg -n "enable_concurrent_lp_root_solve" cpp/src/dual_simplex/ -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 1974
🏁 Script executed:
rg -n "PDLP\|pdlp" cpp/src/dual_simplex/branch_and_bound.cpp -B 2 -A 2 | head -40Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
cat -n cpp/src/dual_simplex/branch_and_bound.cpp | sed -n '1250,1290p'Repository: NVIDIA/cuopt
Length of output: 1804
🏁 Script executed:
rg -n "set_root_relaxation_solution" cpp/src/ -B 2 -A 2Repository: NVIDIA/cuopt
Length of output: 3247
🏁 Script executed:
cat -n cpp/src/dual_simplex/branch_and_bound.cpp | sed -n '1430,1470p'Repository: NVIDIA/cuopt
Length of output: 2151
Fix data race on is_root_solution_set guard in concurrent root relaxation.
is_root_solution_set is a plain bool without atomic protection. When concurrent LP root solve is enabled (line 1450-1461), both PDLP (diversity_manager.cu:442) and dual simplex can invoke set_root_relaxation_solution concurrently, creating a data race on the guard check at line 87 and the write that occurs elsewhere. Use std::atomic<bool> with atomic exchange to ensure thread-safe guarding.
🐛 Proposed fix
- bool is_root_solution_set{false};
+ std::atomic<bool> is_root_solution_set{false};
void set_root_relaxation_solution(...){
- if (!is_root_solution_set) {
+ if (!is_root_solution_set.exchange(true, std::memory_order_acq_rel)) {
root_crossover_soln_.x = primal;
root_crossover_soln_.y = dual;
root_crossover_soln_.z = reduced_costs;
root_objective_ = objective;
root_crossover_soln_.objective = objective;
root_crossover_soln_.user_objective = user_objective;
root_crossover_soln_.iterations = iterations;
root_crossover_solution_set_.store(true, std::memory_order_release);
}
}As per coding guidelines, prevent thread-unsafe use of shared solver state.
Also applies to: 167
🤖 Prompt for AI Agents
In `@cpp/src/dual_simplex/branch_and_bound.hpp` around lines 87 - 96, Replace the
plain bool guard is_root_solution_set with a std::atomic<bool> and change the
check to an atomic exchange so only one thread proceeds to write the root
solution; specifically, in set_root_relaxation_solution use something like if
(!is_root_solution_set.exchange(true, std::memory_order_acq_rel)) { ... } (and
retain root_crossover_solution_set_.store as before). Add `#include` <atomic> and
apply the same atomic-guard fix to the other occurrence referenced (around the
second use at the other location).
| static std::string lp_status_to_string(lp_status_t status) | ||
| { | ||
| switch (status) { | ||
| case lp_status_t::OPTIMAL: return "OPTIMAL"; | ||
| case lp_status_t::INFEASIBLE: return "INFEASIBLE"; | ||
| case lp_status_t::UNBOUNDED: return "UNBOUNDED"; | ||
| case lp_status_t::ITERATION_LIMIT: return "ITERATION_LIMIT"; | ||
| case lp_status_t::TIME_LIMIT: return "TIME_LIMIT"; | ||
| case lp_status_t::NUMERICAL_ISSUES: return "NUMERICAL_ISSUES"; | ||
| case lp_status_t::CUTOFF: return "CUTOFF"; | ||
| case lp_status_t::CONCURRENT_LIMIT: return "CONCURRENT_LIMIT"; | ||
| case lp_status_t::UNSET: return "UNSET"; | ||
| } | ||
| return "UNKNOWN"; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "solve.hpp" -type f | head -20Repository: NVIDIA/cuopt
Length of output: 175
🏁 Script executed:
cat -n cpp/src/dual_simplex/solve.hpp | head -60Repository: NVIDIA/cuopt
Length of output: 2449
🏁 Script executed:
# Check if <string> is already included directly
grep -n "^#include <string>" cpp/src/dual_simplex/solve.hppRepository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
# Check what headers are transitively including string
grep -r "include <string>" cpp/src/dual_simplex/*.hpp | head -20Repository: NVIDIA/cuopt
Length of output: 218
🏁 Script executed:
# Check what types.hpp includes to see if it brings in <string>
cat cpp/src/dual_simplex/types.hpp | head -50Repository: NVIDIA/cuopt
Length of output: 573
🏁 Script executed:
# Also check if any included header brings in <string>
for header in basis_updates initial_basis presolve simplex_solver_settings types; do
echo "=== $header.hpp ==="
grep -n "#include <string>" cpp/src/dual_simplex/${header}.hpp
doneRepository: NVIDIA/cuopt
Length of output: 182
🏁 Script executed:
# Check if std::vector is used and where it comes from
grep -n "std::vector" cpp/src/dual_simplex/solve.hppRepository: NVIDIA/cuopt
Length of output: 665
🏁 Script executed:
# Check if any of the included headers transitively includes <string> or <vector>
for header in basis_updates initial_basis presolve simplex_solver_settings; do
echo "=== Checking $header.hpp ==="
head -30 cpp/src/dual_simplex/${header}.hpp | grep -E "^#include"
doneRepository: NVIDIA/cuopt
Length of output: 1002
🏁 Script executed:
# Check if there's a common precompiled header or config header
grep -r "^#pragma once" cpp/src/dual_simplex/*.hpp | wc -lRepository: NVIDIA/cuopt
Length of output: 58
🏁 Script executed:
# Let's check one of the included headers more thoroughly to see all its includes
head -40 cpp/src/dual_simplex/basis_updates.hppRepository: NVIDIA/cuopt
Length of output: 1262
Consider adding #include <string> for header self-containment, though it's currently available transitively.
lp_status_to_string returns std::string, but this header doesn't directly include <string>. While the type is available through the transitive include chain (presolve.hpp → user_problem.hpp → <string>), adding a direct include would make the header self-contained and more resilient to future refactoring of that chain.
🤖 Prompt for AI Agents
In `@cpp/src/dual_simplex/solve.hpp` around lines 33 - 47, Add a direct include
for the string header to make the header self-contained: include <string> at the
top of the file that defines lp_status_to_string so the return type std::string
is available without relying on transitive includes (update
cpp/src/dual_simplex/solve.hpp to add the include and keep the existing
lp_status_to_string function as-is).
| template <typename i_t, typename f_t> | ||
| f_t problem_t<i_t, f_t>::get_solver_obj_from_user_obj(f_t user_obj) const | ||
| { | ||
| return (user_obj / presolve_data.objective_scaling_factor) - presolve_data.objective_offset; | ||
| } |
There was a problem hiding this comment.
Guard against zero/near-zero objective scaling.
objective_scaling_factor is used as a divisor. If it is 0 (or effectively 0), this returns inf/NaN and corrupts downstream objective handling. Add a guard/assert before division.
🐛 Proposed fix
template <typename i_t, typename f_t>
f_t problem_t<i_t, f_t>::get_solver_obj_from_user_obj(f_t user_obj) const
{
- return (user_obj / presolve_data.objective_scaling_factor) - presolve_data.objective_offset;
+ const f_t scale = presolve_data.objective_scaling_factor;
+ cuopt_assert(scale != f_t(0), "objective_scaling_factor must be non-zero");
+ return (user_obj / scale) - presolve_data.objective_offset;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| template <typename i_t, typename f_t> | |
| f_t problem_t<i_t, f_t>::get_solver_obj_from_user_obj(f_t user_obj) const | |
| { | |
| return (user_obj / presolve_data.objective_scaling_factor) - presolve_data.objective_offset; | |
| } | |
| template <typename i_t, typename f_t> | |
| f_t problem_t<i_t, f_t>::get_solver_obj_from_user_obj(f_t user_obj) const | |
| { | |
| const f_t scale = presolve_data.objective_scaling_factor; | |
| cuopt_assert(scale != f_t(0), "objective_scaling_factor must be non-zero"); | |
| return (user_obj / scale) - presolve_data.objective_offset; | |
| } |
🤖 Prompt for AI Agents
In `@cpp/src/mip/problem/problem.cu` around lines 1992 - 1996, The division in
problem_t<i_t,f_t>::get_solver_obj_from_user_obj uses
presolve_data.objective_scaling_factor without checking for zero/near-zero; add
a guard in get_solver_obj_from_user_obj that checks if
std::abs(presolve_data.objective_scaling_factor) is below a small threshold
(e.g. relative epsilon using std::numeric_limits<f_t>::epsilon()) and if so
either assert or throw a descriptive std::runtime_error (include the offending
scaling value in the message) to prevent returning inf/NaN; otherwise perform
the division as before.
2 participants
Description
For #796
Summary by CodeRabbit
Release Notes
New Features
num_threadssetting.Improvements
✏️ Tip: You can customize this high-level summary in your review settings.