Strong branching fixing - TEST PR TO TRIGGER CODERABBIT#2
Open
aliceb-nv wants to merge 22 commits into
Open
Conversation
aliceb-nv
changed the title
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds prune_fixed_fractional_variables helper and integrates post-strong-branching bound-tightening (ceil/floor, bounds_strengthening, pruning, and early root LP re-solve). Threads an atomic upper_bound through strong/trial branching for cutoff-aware solves. Sets a default reduced_cost_strengthening value in heuristics. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2437-2462: The shared original_lp_.lower and original_lp_.upper
are accessed and modified without synchronization; wrap reads and writes around
those sections (the blocks that compute and assign new_lb/new_ub and the
settings_.log.debug calls that read original_lp_.lower/upper) with the
mutex_original_lp_ (e.g., create a std::lock_guard or std::unique_lock on
mutex_original_lp_ before accessing original_lp_) so that all mutations and
reads in the SB tighten logic (references: original_lp_.lower,
original_lp_.upper, root_relax_soln_.x[j], and the surrounding
num_tightened/num_cutoff/num_infeasible increments) are protected from data
races.
- Around line 2431-2434: The calls to set_solution_at_root that copy
root_relax_soln_ and then return mip_status_t::OPTIMAL are unsafe because
root_relax_soln_ can be fractional or stale after bound tightening; replace
these exits (the set_solution_at_root + OPTIMAL return sites) with logic that
first validates the incumbent and, if invalid, re-solves the problem with the
tightened bounds (i.e., call the routine that re-solves the root relaxation /
MIP under the new bounds) and only call set_solution_at_root and return OPTIMAL
if the re-solve produces a valid integer incumbent (or otherwise finalize from
an already-validated incumbent); alternatively, guard the existing
set_solution_at_root call with an incumbent_is_valid() check and fall back to
resolving when false so you never return OPTIMAL from a stale/fractional
root_relax_soln_.
- Around line 2524-2525: After calling
prune_fixed_fractional_variables(original_lp_, settings_, fractional) the vector
fractional may become empty; add an immediate guard after updating
num_fractional (i_t num_fractional = fractional.size()) that checks if
(num_fractional == 0) and exits the current branch-processing path (return or
continue consistent with the surrounding function control flow) to avoid
downstream code that accesses fractional[0] during variable selection; make the
guard right after the existing num_fractional update so subsequent logic like
variable selection/branching is skipped when there are no fractional variables.
In `@cpp/src/branch_and_bound/pseudo_costs.cpp`:
- Around line 85-87: The current branch treats dual::status_t::ITERATION_LIMIT
as a certifiable solution and stores compute_objective(child_problem,
solution.x) into obj; change the condition so ITERATION_LIMIT is not grouped
with OPTIMAL or CUTOFF (i.e., only treat OPTIMAL and CUTOFF as certifiable), and
handle ITERATION_LIMIT separately so you do not assign the estimated objective
to obj used by strong-branching/fathoming; instead mark the child as
non-certifiable (e.g., leave obj unset, set a flag, or assign a
sentinel/infinite bound) so downstream SB logic won’t use the estimate for bound
tightening. Ensure you update the code paths around compute_objective, obj, and
any flags/logic that rely on certifiable status (references:
dual::status_t::ITERATION_LIMIT, dual::status_t::OPTIMAL,
dual::status_t::CUTOFF, compute_objective(child_problem, solution.x), obj,
solution.x).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/branch_and_bound/pseudo_costs.hppcpp/src/mip_heuristics/solver.cu
There was a problem hiding this comment.
♻️ Duplicate comments (2)
cpp/src/branch_and_bound/branch_and_bound.cpp (2)
2488-2493:⚠️ Potential issue | 🔴 CriticalProtect SB propagation bound updates with
mutex_original_lp_.
sb_presolve.bounds_strengthening(...)is mutatingoriginal_lp_.lower/upperat Line [2491]-Line [2492] without synchronization, while other paths access those bounds undermutex_original_lp_. This reintroduces a shared-state race in the root phase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2488 - 2493, The bounds_strengthening call mutates shared original_lp_.lower/upper without locking; protect the mutation by acquiring mutex_original_lp_ (e.g., std::lock_guard<std::mutex> guard(mutex_original_lp_)) around the call to sb_presolve.bounds_strengthening(...) and any direct updates to original_lp_.lower or original_lp_.upper so that sb_presolve.bounds_strengthening, which may write those vectors, executes under the same mutex used elsewhere when accessing original_lp_.
2497-2507:⚠️ Potential issue | 🔴 CriticalGuard the empty-fractional case immediately after pruning.
After
prune_fixed_fractional_variables(...),num_fractionalcan become zero in both blocks. Without an immediate exit/finalization path, downstream branch selection at Line [2540] assumes non-emptyfractionaland can fail.Also applies to: 2531-2535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2497 - 2507, After calling prune_fixed_fractional_variables(original_lp_, settings_, fractional) and recomputing num_fractional, guard the case where num_fractional == 0 by taking the appropriate early exit/finalization (e.g., return, continue, or call the same finalization path used when branching is complete) instead of proceeding to the later branch-selection logic; update the block containing num_fixed, num_tightened and the similar block around the other prune call (the 2531-2535 equivalent) to check num_fractional immediately after pruning and handle the empty-fractional case so the subsequent code that assumes fractional non-empty (and the assert) cannot fail.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2488-2493: The bounds_strengthening call mutates shared
original_lp_.lower/upper without locking; protect the mutation by acquiring
mutex_original_lp_ (e.g., std::lock_guard<std::mutex> guard(mutex_original_lp_))
around the call to sb_presolve.bounds_strengthening(...) and any direct updates
to original_lp_.lower or original_lp_.upper so that
sb_presolve.bounds_strengthening, which may write those vectors, executes under
the same mutex used elsewhere when accessing original_lp_.
- Around line 2497-2507: After calling
prune_fixed_fractional_variables(original_lp_, settings_, fractional) and
recomputing num_fractional, guard the case where num_fractional == 0 by taking
the appropriate early exit/finalization (e.g., return, continue, or call the
same finalization path used when branching is complete) instead of proceeding to
the later branch-selection logic; update the block containing num_fixed,
num_tightened and the similar block around the other prune call (the 2531-2535
equivalent) to check num_fractional immediately after pruning and handle the
empty-fractional case so the subsequent code that assumes fractional non-empty
(and the assert) cannot fail.
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
2497-2507:⚠️ Potential issue | 🔴 CriticalReplace debug-only empty checks with runtime handling.
At Line [2506] and Line [2534],
assert(num_fractional > 0)is not sufficient. In release builds this disappears, and the later branch selection path can still index an emptyfractional(via Line [2540]), causing UB/crash. Add a realif (num_fractional == 0)exit path (finalize from a validated incumbent or re-solve before returning).Suggested minimal guard pattern
- assert(num_fractional > 0); + if (num_fractional == 0) { + bool has_incumbent = false; + mutex_upper_.lock(); + has_incumbent = incumbent_.has_incumbent; + mutex_upper_.unlock(); + if (has_incumbent) { + solver_status_ = mip_status_t::OPTIMAL; + set_final_solution(solution, upper_bound_.load()); + return solver_status_; + } + // No validated incumbent: re-solve with tightened bounds before branching. + return mip_status_t::NUMERICAL; + }Also applies to: 2531-2535
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2497 - 2507, The assert on num_fractional (after calling prune_fixed_fractional_variables) must be replaced with a real runtime guard: check if num_fractional == 0 and handle it instead of asserting. In the function that calls prune_fixed_fractional_variables (where symbols original_lp_, settings_, fractional, num_fractional are used and later code indexes fractional for branching), add an if (num_fractional == 0) branch that either finalizes the node using a validated incumbent (call the existing routine you have for accepting/recording a feasible incumbent or node completion) and returns, or if no incumbent is available trigger a safe re-solve/refresh of the LP and then return/continue; do not access fractional when it is empty. Replace both assert(num_fractional > 0) occurrences with this runtime handling so the branch-selection code that indexes fractional is never reached with an empty vector.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2497-2507: The assert on num_fractional (after calling
prune_fixed_fractional_variables) must be replaced with a real runtime guard:
check if num_fractional == 0 and handle it instead of asserting. In the function
that calls prune_fixed_fractional_variables (where symbols original_lp_,
settings_, fractional, num_fractional are used and later code indexes fractional
for branching), add an if (num_fractional == 0) branch that either finalizes the
node using a validated incumbent (call the existing routine you have for
accepting/recording a feasible incumbent or node completion) and returns, or if
no incumbent is available trigger a safe re-solve/refresh of the LP and then
return/continue; do not access fractional when it is empty. Replace both
assert(num_fractional > 0) occurrences with this runtime handling so the
branch-selection code that indexes fractional is never reached with an empty
vector.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/pseudo_costs.cppcpp/src/branch_and_bound/pseudo_costs.hpp
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2498-2503: The log message uses num_fixed and num_tightened
incorrectly (num_fixed counts variables removed from fractional, num_tightened
counts bound improvements), so update the printf in the branch-and-bound
reporting to avoid the misleading subtraction: either track actual variable
fixings separately and log that new counter, or simplify the message to only
report num_fixed (and remove the "- %d" format/argument). Locate the printf call
that references settings_.log.printf("Strong branching bounds tightening: %d
variables fixed (%d from propagation)\n", num_fixed, num_fixed - num_tightened)
and change it to one of the two options above so the reported values (num_fixed,
num_tightened, fractional) are semantically correct.
Comment on lines
+2498
to
+2503
| if (num_fixed > 0) { | ||
| settings_.log.printf( | ||
| "Strong branching bounds tightening: %d variables fixed (%d from propagation)\n", | ||
| num_fixed, | ||
| num_fixed - num_tightened); | ||
| num_fractional = fractional.size(); |
There was a problem hiding this comment.
Minor: Log message calculation is semantically incorrect.
num_fixed counts variables pruned from the fractional list (those with tight bounds), while num_tightened counts individual bound improvements from SB. A variable may have its bound tightened without becoming fixed, so num_fixed - num_tightened doesn't accurately represent "variables fixed from propagation."
Consider either:
- Tracking actual fixings separately, or
- Simplifying the message to just report
num_fixed
Suggested simpler log message
settings_.log.printf(
- "Strong branching bounds tightening: %d variables fixed (%d from propagation)\n",
- num_fixed,
- num_fixed - num_tightened);
+ "Strong branching bounds tightening: %d variables now fixed\n",
+ num_fixed);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2498 - 2503, The
log message uses num_fixed and num_tightened incorrectly (num_fixed counts
variables removed from fractional, num_tightened counts bound improvements), so
update the printf in the branch-and-bound reporting to avoid the misleading
subtraction: either track actual variable fixings separately and log that new
counter, or simplify the message to only report num_fixed (and remove the "- %d"
format/argument). Locate the printf call that references
settings_.log.printf("Strong branching bounds tightening: %d variables fixed (%d
from propagation)\n", num_fixed, num_fixed - num_tightened) and change it to one
of the two options above so the reported values (num_fixed, num_tightened,
fractional) are semantically correct.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
2499-2502:⚠️ Potential issue | 🟡 MinorThe “from propagation” count is still semantically off.
num_fixed(fractional vars removed) andnum_tightened(bound updates) are different counters, sonum_fixed - num_tightenedcan misreport. This log should be simplified or backed by a dedicated propagation-fixing counter.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2499 - 2502, The log message using settings_.log.printf is incorrect because num_fixed and num_tightened are different metrics; either remove the misleading subtraction or introduce a dedicated counter for propagation-driven fixes. Update the code around settings_.log.printf to either (A) print the two counters separately: num_fixed and num_tightened, or (B) add and maintain a new variable (e.g., num_fixed_by_propagation) that is incremented when a variable is fixed due to propagation and use that in the message instead of num_fixed - num_tightened; ensure the chosen counter(s) are updated in the strong branching tightening code paths where variables are fixed and where bounds are tightened.
🧹 Nitpick comments (1)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
2506-2541: Consider extracting the duplicated LP re-solve/finalization block.The SB and RC zero-fractional paths are nearly identical. A shared helper would reduce drift risk and keep status handling consistent.
Also applies to: 2569-2604
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 2506 - 2541, Extract the duplicated LP re-solve and finalization logic into a helper (e.g., resolve_root_lp_and_handle_result) that encapsulates the call to dual_phase2_with_advanced_basis, updates exploration_stats_.total_lp_iters, computes root_objective_ via compute_objective(original_lp_, root_relax_soln_.x), recomputes fractional via fractional_variables(settings_, root_relax_soln_.x, var_types_, fractional), and handles the three outcomes: on OPTIMAL clear fractional and call set_solution_at_root and return mip_status_t::OPTIMAL if still integer-feasible; on TIME_LIMIT set solver_status_ = mip_status_t::TIME_LIMIT, call set_final_solution(solution, root_objective_) and return; otherwise log the error with settings_.log.printf and return mip_status_t::NUMERICAL; then replace the duplicated blocks (including the one around dual_phase2_with_advanced_basis and the block at the other site) with a single call to this helper, passing lp_settings, original_lp_, root_relax_soln_, basis_update, basic_list, nonbasic_list, exploration_stats_.start_time and any required out-params.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2488-2492: The SB/RC bound-pruning path accesses
original_lp_.lower and original_lp_.upper (and calls
prune_fixed_fractional_variables) without locking and can race with other
producers that use mutex_original_lp_; wrap all reads/writes to original_lp_
used by bounds_strengthening_t (the sb_presolve.bounds_strengthening call and
any subsequent accesses at the nearby sites referenced) inside a scoped lock on
mutex_original_lp_ (e.g., std::lock_guard<std::mutex> guard(mutex_original_lp_))
so that sb_presolve and any calls that read/modify original_lp_.lower/upper are
protected; apply the same locking pattern to the other occurrences noted (the
nearby lines around the second sb_presolve use and the site at 2566) to
eliminate races.
---
Duplicate comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2499-2502: The log message using settings_.log.printf is incorrect
because num_fixed and num_tightened are different metrics; either remove the
misleading subtraction or introduce a dedicated counter for propagation-driven
fixes. Update the code around settings_.log.printf to either (A) print the two
counters separately: num_fixed and num_tightened, or (B) add and maintain a new
variable (e.g., num_fixed_by_propagation) that is incremented when a variable is
fixed due to propagation and use that in the message instead of num_fixed -
num_tightened; ensure the chosen counter(s) are updated in the strong branching
tightening code paths where variables are fixed and where bounds are tightened.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2506-2541: Extract the duplicated LP re-solve and finalization
logic into a helper (e.g., resolve_root_lp_and_handle_result) that encapsulates
the call to dual_phase2_with_advanced_basis, updates
exploration_stats_.total_lp_iters, computes root_objective_ via
compute_objective(original_lp_, root_relax_soln_.x), recomputes fractional via
fractional_variables(settings_, root_relax_soln_.x, var_types_, fractional), and
handles the three outcomes: on OPTIMAL clear fractional and call
set_solution_at_root and return mip_status_t::OPTIMAL if still integer-feasible;
on TIME_LIMIT set solver_status_ = mip_status_t::TIME_LIMIT, call
set_final_solution(solution, root_objective_) and return; otherwise log the
error with settings_.log.printf and return mip_status_t::NUMERICAL; then replace
the duplicated blocks (including the one around dual_phase2_with_advanced_basis
and the block at the other site) with a single call to this helper, passing
lp_settings, original_lp_, root_relax_soln_, basis_update, basic_list,
nonbasic_list, exploration_stats_.start_time and any required out-params.
## Issue Closes NVIDIA#932 Authors: - Ishika Roy (https://github.com/Iroy30) - Ramakrishnap (https://github.com/rgsl888prabhu) Approvers: - Ramakrishnap (https://github.com/rgsl888prabhu) URL: NVIDIA#947
apply -UNDEBUG to coupt flags instead of global settings
NVIDIA#973) Contributes to rapidsai/build-planning#257 * builds CUDA 13 wheels with the 13.0 CTK * ensures wheels ship with a runtime dependency of `nvidia-nvjitlink>={whatever-minor-version-they-were-built-against}` Contributes to rapidsai/build-planning#256 * updates wheel tests to cover a range of CTK versions (we previously, accidentally, were only testing the latest 12.x and 13.x) Contributes to rapidsai/build-planning#205 * switches `rapidsai/shared-workflows` branches, `@python-3.14 -> @main` (rapidsai/shared-workflows#508 was merged) ## Issue N/A Authors: - James Lamb (https://github.com/jameslamb) Approvers: - Bradley Dice (https://github.com/bdice) URL: NVIDIA#973
Disable running notebooks which are added examples while building docs Authors: - Ramakrishnap (https://github.com/rgsl888prabhu) Approvers: - Trevor McKay (https://github.com/tmckayus) URL: NVIDIA#980
Fixes NVIDIA#295 ## Issue Authors: - Ishika Roy (https://github.com/Iroy30) Approvers: - Gil Forsyth (https://github.com/gforsyth) URL: NVIDIA#945
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
6 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Issue
Checklist