Root heuristics - #1700
Conversation
…rom submip. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds DFS backtracking control, objective conversion, optional postsolve validation, and halt tracking. It refactors RINS and sub-MIP execution around prepared workers and adds persistent root-heuristic orchestration across root cut passes. ChangesRoot heuristics and sub-MIP execution
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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: 2
🧹 Nitpick comments (3)
cpp/src/branch_and_bound/branch_and_bound.cpp (1)
3549-3551: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the declaration-order dependency between
root_worker_countandroot_heuristics.The root heuristic tasks decrement
*worker_countwhen they finish.root_heuristicsis destroyed beforeroot_worker_countbecause it is declared later, and its element destructors callstop(), which blocks ontaskwaituntil those tasks complete. The ordering is correct as written.If a future change moves
root_heuristicsaboveroot_worker_count, the counter is destroyed while tasks can still decrement it, which is a use-after-free across threads. Add a short comment so the ordering is not reversed by accident.♻️ Proposed comment
+ // Declaration order matters: `root_heuristics` must outlive nothing and be destroyed + // FIRST, because its destructors taskwait for the workers that decrement + // `root_worker_count`. Do not move `root_worker_count` below `root_heuristics`. omp_atomic_t<i_t> root_worker_count = 0; std::list<root_heuristics_t<i_t, f_t>> root_heuristics; launch_root_heuristics(original_lp_, root_relax_soln_.x, root_heuristics, &root_worker_count);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3549 - 3551, Add a concise lifetime-ordering comment immediately before the declarations of root_worker_count and root_heuristics, stating that root_worker_count must be declared first because root_heuristics destruction waits for heuristic tasks that decrement the counter. Preserve the current declaration order.cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu (1)
1817-1829: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMake the worker-count decrement exception safe.
The counter is incremented before the task is created and decremented at the end of the task body. If
cpufj_solveexits by an exception, the decrement is skipped andworker_countstays high for the rest of the solve.launch_root_heuristicsthen refuses to start new root heuristics once the leaked count reachessettings_.num_threads - 1, so root heuristics stop permanently for that run.The same pattern exists in the RINS task in
cpp/src/branch_and_bound/branch_and_bound.cppat lines 2846-2852, and that path is more likely to throw:rinscallssolve_submip, which runs a nestedbranch_and_bound_t::solve()containingcuopt_expectschecks.Use a small RAII guard so the decrement always runs.
♻️ Proposed RAII guard
void fj_cpu_worker_t<i_t, f_t>::run_async(f_t time_limit, double work_unit_limit, omp_atomic_t<i_t>* worker_count) { if (!fj_cpu) return; if (worker_count) ++(*worker_count); `#pragma` omp task shared(fj_cpu) firstprivate(time_limit, work_unit_limit, worker_count) \ priority(CUOPT_DEFAULT_TASK_PRIORITY) default(none) depend(out : *fj_cpu) { - cpufj_solve(fj_cpu.get(), time_limit, work_unit_limit); - if (worker_count) --(*worker_count); + struct count_guard_t { + omp_atomic_t<i_t>* c; + ~count_guard_t() { if (c) --(*c); } + } guard{worker_count}; + cpufj_solve(fj_cpu.get(), time_limit, work_unit_limit); } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu` around lines 1817 - 1829, Make the worker-count decrement exception-safe in fj_cpu_worker_t::run_async and the analogous RINS task in branch_and_bound_t. Add a small RAII guard immediately after incrementing the counter, capturing worker_count and decrementing it on scope exit, then remove the manual task-body decrement while preserving the null-pointer check.cpp/src/mip_heuristics/root_heuristics.hpp (1)
29-37: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSignal both workers before waiting, and document the sibling-task requirement.
Two points about
stop():
fj_cpu_worker_.stop()blocks on its owntaskwaitbeforesubmip_worker_->haltis set. The RINS task therefore keeps running for the full duration of the CPU FJ join. Set both stop signals first, then wait. This shortens teardown without changing behavior.
#pragma omp taskwait depend(in : *worker)only joins the RINS task if that task is a sibling, because OpenMP task dependences apply within one task region. The matching#pragma omp task ... depend(out : *worker)is created inbranch_and_bound_t::launch_root_heuristics. Both call sites currently run directly insolve(), so the relationship holds. If aroot_heuristics_tis ever destroyed from inside another task, thistaskwaitdoes not join the RINS task andsubmip_worker_is freed while the task still uses it. State the requirement in a comment so the invariant is visible to callers.♻️ Proposed reordering and comment
+ // Requirement: destroy this object from the same OpenMP task region that called + // `branch_and_bound_t::launch_root_heuristics`. The `taskwait depend(...)` below only + // joins the RINS task when the two are sibling tasks. void stop() { - fj_cpu_worker_.stop(); + // Signal both workers first so they wind down in parallel, then join. + if (submip_worker_) { submip_worker_->halt = true; } + fj_cpu_worker_.stop(); if (submip_worker_) { - submip_worker_->halt = true; diving_worker_t<i_t, f_t>* worker = submip_worker_.get(); `#pragma` omp taskwait depend(in : *worker) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/mip_heuristics/root_heuristics.hpp` around lines 29 - 37, Update root_heuristics_t::stop() to signal submip_worker_->halt before calling fj_cpu_worker_.stop(), then retain the existing taskwait. Add a comment documenting that the taskwait joins the RINS task only when stop() is called from the same task region as the sibling task launched by branch_and_bound_t::launch_root_heuristics, so callers must preserve that requirement.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2759-2765: Replace the mutable var_types_ member with the captured
var_types snapshot in submip_fj_cpu_worker.create_worker at
cpp/src/branch_and_bound/branch_and_bound.cpp:2759-2765. Update solve_submip at
cpp/src/branch_and_bound/branch_and_bound.cpp:2286-2290 to accept a const
variable-type vector reference, pass rins’s var_types snapshot at its call site
around line 2779, and use that parameter in convert_lp_to_user_problem.
- Around line 2828-2829: Update the root heuristic lifecycle around
root_heuristics_t and the RINS/CPU FJ task completion paths to track completion
independently for both tasks, rather than relying on is_active. Remove each
heuristic entry only after both RINS and CPU FJ have finished, including
asynchronous CPU FJ cases where fj_cpu remains non-null, while preserving access
needed by still-running tasks.
---
Nitpick comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3549-3551: Add a concise lifetime-ordering comment immediately
before the declarations of root_worker_count and root_heuristics, stating that
root_worker_count must be declared first because root_heuristics destruction
waits for heuristic tasks that decrement the counter. Preserve the current
declaration order.
In `@cpp/src/mip_heuristics/feasibility_jump/fj_cpu.cu`:
- Around line 1817-1829: Make the worker-count decrement exception-safe in
fj_cpu_worker_t::run_async and the analogous RINS task in branch_and_bound_t.
Add a small RAII guard immediately after incrementing the counter, capturing
worker_count and decrementing it on scope exit, then remove the manual task-body
decrement while preserving the null-pointer check.
In `@cpp/src/mip_heuristics/root_heuristics.hpp`:
- Around line 29-37: Update root_heuristics_t::stop() to signal
submip_worker_->halt before calling fj_cpu_worker_.stop(), then retain the
existing taskwait. Add a comment documenting that the taskwait joins the RINS
task only when stop() is called from the same task region as the sibling task
launched by branch_and_bound_t::launch_root_heuristics, so callers must preserve
that requirement.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c81605c6-069a-4e91-88a2-143578b6b897
📒 Files selected for processing (12)
cpp/include/cuopt/mathematical_optimization/mip/submip_hyper_params.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/branch_and_bound/worker.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/solve.hppcpp/src/mip_heuristics/feasibility_jump/feasibility_jump.cucpp/src/mip_heuristics/feasibility_jump/fj_cpu.cucpp/src/mip_heuristics/feasibility_jump/fj_cpu_worker.cuhcpp/src/mip_heuristics/presolve/third_party_presolve.cppcpp/src/mip_heuristics/presolve/third_party_presolve.hppcpp/src/mip_heuristics/root_heuristics.hpp
CI Test Summary✅ All 31 test job(s) passed. |
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
aliceb-nv
left a comment
aliceb-nv
left a comment
There was a problem hiding this comment.
LGTM, minor nits, thanks Nicolas! As always, let's get Chris' eyes on this before the merge
| #ifdef DEBUG_SUBMIP | ||
| settings_.log.print_format("{} Running a quick DFS for the submip!", log_prefix); | ||
| #endif |
There was a problem hiding this comment.
Any reason why we're not using debug_format here? Reducing #ifdef clutter is good for readability IMO
There was a problem hiding this comment.
This is mostly to enable/disable the logs from sub-MIP quickly. With log set to debug, things can get pretty chaotic hahah
(maybe we could have a way to enable/disable the debug logs from each part of the solver separately)
There was a problem hiding this comment.
Maybe not for this PR, but perhaps we could later add a scope/type argument to the debug logs to allow for more fine-grained filtering :)
There was a problem hiding this comment.
Wrapped the submip logs calls with a macro
| #ifdef DEBUG_SUBMIP | ||
| submip_settings.log.log = true; | ||
| #else | ||
| submip_settings.log.log = false; | ||
| #endif |
There was a problem hiding this comment.
Seems like leftover debug code right? If possible let's ensure such code is out of major function bodies
There was a problem hiding this comment.
Same as above. Without the DEBUG_SUBMIP, the sub-MIP logs should be silent.
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
…t heuristics before B&B tree exploration. Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
Signed-off-by: Nicolas L. Guidotti <nguidotti@nvidia.com>
akifcorduk
left a comment
akifcorduk
left a comment
There was a problem hiding this comment.
Thanks Nicolas, just a few nitpicks.
| case search_strategy_t::FARKAS_DIVING: return 'F'; | ||
| case search_strategy_t::VECTOR_LENGTH_DIVING: return 'V'; | ||
| default: return 'U'; | ||
| case search_strategy_t::RINS: return 'S'; |
There was a problem hiding this comment.
I used S for SUBMIP, but I guess R is also valid
| } else { | ||
| #pragma omp task priority(CUOPT_DEFAULT_TASK_PRIORITY) affinity(worker) firstprivate(worker, sol) | ||
| rins(worker, sol); | ||
| #pragma omp task priority(CUOPT_DEFAULT_TASK_PRIORITY) affinity(worker) \ |
There was a problem hiding this comment.
Shouldn't we check if there are enough threads available at this point?
There was a problem hiding this comment.
This is controlled by the submip worker pool. If no worker is available, then we exit early (see the if some lines prior). Although we may need a better way to control the total number of threads, each level of the sub-MIP recursion can use more than one thread
|
|
||
| // If we already exhausted all threads for the root heuristics, stop workers for the | ||
| // oldest set of heuristics launched. Leave 2 threads for the cut passes and the clique | ||
| // table generation. Add the number of workers that will be launched (1 submip worker + |
There was a problem hiding this comment.
Clique table generation only happens in the first cut pass. So later it is freed. It complicates the logic, but in case you want to use that thread too :)
| mutex_upper_.unlock(); | ||
|
|
||
| if (settings_.inside_submip) { | ||
| // LLVM libomp's GOMP compatibility path skips GCC's firstprivate copy |
There was a problem hiding this comment.
Is this comment still necessary?
There was a problem hiding this comment.
Iirc this is because LLVM has an upstream bug where they don't honor the firstprivate copy constructor in their GCC OpenMP compat layer. It caused issues a few weeks back because some wheel builds had to move to LLVM's openmp lib due to the GCC openmp in their environment being outdated
There was a problem hiding this comment.
Should we switching to LLVM's openmp lib completely? Or even use clang?
There was a problem hiding this comment.
Probably a P2 task for much later :) Although it may be worth benchmarking GOMP vs llvmomp
3 participants
set_solution_from_submip(the objective was on the submip space, which is not comparable with the solution space of the B&B).Checklist