Objective cut in recombiners #628
Conversation
This reverts commit d1413c8.
With this PR, the child node can reuse the basis factorisation from the parent, reducing the time required for solving the LP relaxation of the child. The solver recomputes the basis factorisation when the branch reaches the end. This is a follow-up of NVIDIA#383 and NVIDIA#473. Average (Primal) Gap over the MIPLIB2017 dataset: `main` (10f116b): `14.472358` with `224` feasible solutions This PR: `~14.26` with `225` feasible solutions On average, the `main` branch explored `1012412` nodes. This PR increases to `1850797` nodes (i.e., `82%` more nodes) for the same time frame. Compared to NVIDIA#473, it explores `15%` more nodes. This also fixes an incorrect report of an infeasible solution after a timeout and there are no nodes in the heap. Authors: - Nicolas L. Guidotti (https://github.com/nguidotti) - Chris Maes (https://github.com/chris-maes) - Alice Boucher (https://github.com/aliceb-nv) Approvers: - Chris Maes (https://github.com/chris-maes) URL: NVIDIA#492
📝 WalkthroughWalkthroughIntroduces a dual-pointer problem mechanism for solutions to handle cutting planes, propagates problem/weight context across population and local search, adds pointer swaps in recombiners, threads basis data through branch-and-bound, adjusts restart logic, refactors omp atomics, and adds CPU thread initialization guards in the benchmark runner. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
aliceb-nv
left a comment
aliceb-nv
left a comment
There was a problem hiding this comment.
Thanks for the PR Akif!
I have some concerns regarding code structure/refactoring, I just want to make sure we have robust code since we're going to be manipulating problem_t instances much more with the involvement of cuts soon
Plus I am not sure I fully understand and track the swapping of problem pointers as it stands 😅
| offspring.swap_problem_pointers(); | ||
| // find same values and populate it to offspring | ||
| i_t n_different_vars = | ||
| this->assign_same_integer_values(guiding_solution, other_solution, offspring); | ||
| CUOPT_LOG_DEBUG("FP rec: Number of different variables %d MAX_VARS %d", | ||
| n_different_vars, | ||
| fp_recombiner_config_t::max_n_of_vars_from_other); | ||
| i_t n_vars_from_other = n_different_vars; | ||
| if (n_vars_from_other > (i_t)fp_recombiner_config_t::max_n_of_vars_from_other) { | ||
| n_vars_from_other = fp_recombiner_config_t::max_n_of_vars_from_other; | ||
| thrust::default_random_engine g{(unsigned int)cuopt::seed_generator::get_seed()}; | ||
| thrust::shuffle(a.handle_ptr->get_thrust_policy(), | ||
| this->remaining_indices.data(), | ||
| this->remaining_indices.data() + n_different_vars, | ||
| g); | ||
| } | ||
| i_t n_vars_from_guiding = a.problem_ptr->n_integer_vars - n_vars_from_other; | ||
| if (n_vars_from_other == 0 || n_vars_from_guiding == 0) { | ||
| CUOPT_LOG_DEBUG("Returning false because all vars are common or different"); | ||
| return std::make_pair(offspring, false); | ||
| } | ||
| CUOPT_LOG_DEBUG( | ||
| "n_vars_from_guiding %d n_vars_from_other %d", n_vars_from_guiding, n_vars_from_other); | ||
| this->compute_vars_to_fix(offspring, vars_to_fix, n_vars_from_other, n_vars_from_guiding); | ||
| auto [fixed_problem, fixed_assignment, variable_map] = offspring.fix_variables(vars_to_fix); | ||
| fixed_problem.check_problem_representation(true); | ||
| if (!guiding_solution.get_feasible() && !other_solution.get_feasible()) { | ||
| relaxed_lp_settings_t lp_settings; | ||
| lp_settings.time_limit = fp_recombiner_config_t::infeasibility_detection_time_limit; | ||
| lp_settings.tolerance = fixed_problem.tolerances.absolute_tolerance; | ||
| lp_settings.return_first_feasible = true; | ||
| lp_settings.save_state = true; | ||
| lp_settings.check_infeasibility = true; | ||
| // run lp with infeasibility detection on | ||
| auto lp_response = | ||
| get_relaxed_lp_solution(fixed_problem, fixed_assignment, offspring.lp_state, lp_settings); | ||
| if (lp_response.get_termination_status() == pdlp_termination_status_t::PrimalInfeasible || | ||
| lp_response.get_termination_status() == pdlp_termination_status_t::DualInfeasible || | ||
| lp_response.get_termination_status() == pdlp_termination_status_t::TimeLimit) { | ||
| CUOPT_LOG_DEBUG("FP recombiner failed because LP found infeasible!"); | ||
| return std::make_pair(offspring, false); | ||
| } | ||
| } | ||
| // brute force rounding threshold is 8 | ||
| const bool run_fp = fixed_problem.n_integer_vars > 8; | ||
| if (run_fp) { | ||
| problem_t<i_t, f_t>* orig_problem_ptr = offspring.problem_ptr; | ||
| offspring.problem_ptr = &fixed_problem; | ||
| rmm::device_uvector<f_t> old_assignment(offspring.assignment, | ||
| offspring.handle_ptr->get_stream()); | ||
| offspring.handle_ptr->sync_stream(); | ||
| offspring.assignment = std::move(fixed_assignment); | ||
| cuopt_func_call(offspring.test_variable_bounds(false)); | ||
| timer_t timer(fp_recombiner_config_t::fp_time_limit); | ||
| fp.timer = timer; | ||
| fp.cycle_queue.reset(offspring); | ||
| fp.reset(); | ||
| fp.resize_vectors(*offspring.problem_ptr, offspring.handle_ptr); | ||
| fp.config.alpha = fp_recombiner_config_t::alpha; | ||
| fp.config.alpha_decrease_factor = fp_recombiner_config_t::alpha_decrease_factor; | ||
| bool is_feasible = fp.run_single_fp_descent(offspring); | ||
| if (is_feasible) { CUOPT_LOG_DEBUG("FP recombiner found feasible!"); } | ||
| CUOPT_LOG_DEBUG("FP completed after recombiner!"); | ||
| offspring.handle_ptr->sync_stream(); | ||
| offspring.problem_ptr = orig_problem_ptr; | ||
| fixed_assignment = std::move(offspring.assignment); | ||
| offspring.assignment = std::move(old_assignment); | ||
| offspring.handle_ptr->sync_stream(); | ||
| } | ||
| // unfix the assignment on given result no matter if it is feasible | ||
| offspring.unfix_variables(fixed_assignment, variable_map); | ||
| if (!run_fp) { offspring.round_nearest(); } | ||
| cuopt_assert(offspring.test_number_all_integer(), "All must be integers after offspring"); | ||
| offspring.compute_feasibility(); | ||
| bool same_as_parents = this->check_if_offspring_is_same_as_parents(offspring, a, b); | ||
| // adjust the max_n_of_vars_from_other | ||
| if (n_different_vars > (i_t)fp_recombiner_config_t::max_n_of_vars_from_other) { | ||
| if (same_as_parents) { | ||
| fp_recombiner_config_t::increase_max_n_of_vars_from_other(); | ||
| } else { | ||
| fp_recombiner_config_t::decrease_max_n_of_vars_from_other(); | ||
| } | ||
| } | ||
| offspring.swap_problem_pointers(); |
There was a problem hiding this comment.
Could we guard this kind of problem pointer swapping with RAII, say, a utility class like what scoped_guard does with mutexes?
I think it will be very easy to get confused on which problem is currently the active one
There was a problem hiding this comment.
I think it will just prevent the forgetting to swap back. It will probably be a better coding practice to do that. But it won't help with which problem is active, because we don't always revert.
There was a problem hiding this comment.
I tried RAII swaps but I think it complicates the code. I think we should do more robust arch as we will add more cuts soon. Probably whole heuristics also needs to see the same problem, we should probably think about population/infeasibility logic then.
| population.weights_with_cuts.cstr_weights.resize(offspring.problem_ptr->n_constraints, | ||
| offspring.handle_ptr->get_stream()); | ||
| raft::copy(population.weights_with_cuts.cstr_weights.data(), | ||
| population.weights.cstr_weights.data(), | ||
| population.weights.cstr_weights.size(), | ||
| offspring.handle_ptr->get_stream()); |
There was a problem hiding this comment.
I am a little worried about maintaining the two weights arrays as well. I fear it would be easy to end up with broken invariants
Could we maybe imagine a double_buffer_t or flip_buffer_t or ping_pong_t class that would take, say,
thrust::pair<problem_t*, rmm:device_vector> as its type, and allow for swapping between the two sets with the guarantee of no mismatched state?
There was a problem hiding this comment.
I think it makes sense. We should make it robust as more cuts are coming.
There was a problem hiding this comment.
The issue here is that problem is member of the solution and weights is the member of population. I think this might cause more invariants to break. I think the solution to all the problems you pointed out is to use uniform problem for all population and diversity management. Do you think we should tackle this now?
| void population_t<i_t, f_t>::apply_problem_ptr_to_all_solutions() | ||
| { | ||
| for (size_t i = 0; i < indices.size(); i++) { | ||
| solutions[indices[i].first].second.problem_with_cuts_ptr = problem_ptr_with_cuts; |
There was a problem hiding this comment.
I think we could benefit from adding a flags field to problem_t, one of which could be HasCuts or HasObjectiveCuts. Otherwise I think it will be difficult to check for invariants and ensure we didn't mess up pointer assignments
There was a problem hiding this comment.
There is already a flag for the objective cut. We can use that.
| solution.assignment.size(), | ||
| solution.handle_ptr->get_stream()); | ||
| save_solution_and_add_cutting_plane(population_ptr->best_feasible(), solution, best_objective); | ||
| population_ptr->apply_problem_ptr_to_all_solutions(); |
There was a problem hiding this comment.
I think updating all problem_with_cuts pointers should be the responsibility of the function adding the objective cut (unless I misundertand)
I am worried about the decoupling between the problem modifications and the updating of pointers; it may be all too easy to forget once, and have a lot of trouble tracking down the issue
There was a problem hiding this comment.
The issue here is that a cut might be added/modified multiple times. This is a lazy initialization which applies the problem and recomputes the qualities. If we recompute qualities everytime a cut is applied, we would lose perf.
There was a problem hiding this comment.
That makes sense :)
I could imagine a 'dirty' flag that would be set everytime a cut is added, and reset once apply_problem_ptr() is called and invariants are restored. That way we could assert(!dirty) when we need qualities to be up to date
Sorry for all the nitpicks 😅 (I just know I would, sooner or later, mess this up, so thinking defensively helps me in my code)
There was a problem hiding this comment.
What apply_problem_ptr_to_all_solutions really does is that it adds problem_ptr_with_cuts to the newly added solutions from add_external_solution. Otherwise, we could do it once in the beginning with the first cut.
| solution_copy.problem_ptr = old_problem_ptr; | ||
| solution_copy.resize_to_problem(); |
There was a problem hiding this comment.
We should probably have a update_problem_pointer method or similar for solution_t, to avoid having to call resize_to_problem and/or ensure any current or future invariant is maintained
With the addition of cuts soon I think we need to be very robust there
cpp/CMakeLists.txt
Outdated
|
|
||
|
|
||
| option(BUILD_MIP_BENCHMARKS "Build MIP benchmarks" OFF) | ||
| option(BUILD_MIP_BENCHMARKS "Build MIP benchmarks" ON) |
There was a problem hiding this comment.
We should probably leave this CMake option as OFF by default, and set it to ON in our own workflows
e.g. typically I call build.sh with --cmake-args=\\\"-DBUILD_MIP_BENCHMARKS=ON -DBUILD_LP_BENCHMARKS=ON ... as the build task in vscode/cursor
|
@akifcorduk is this going for 25.12? |
|
This says "Not Critical for 25.12" so I'm going to move it out of the 25.12 release. If you want to bring it into 25.12, just set the milestone back. Thanks. |
This PR changes how the explored node counter is updated, such that it is only incremented after solving a node in the B&B. Authors: - Nicolas L. Guidotti (https://github.com/nguidotti) Approvers: - Chris Maes (https://github.com/chris-maes) URL: NVIDIA#570
Calrify naming on a restart helper function and avoid potentially picking the wrong restart strategy ## Summary by CodeRabbit * **Refactor** * Revised restart-mode logic in the linear programming solver: restart checks were inverted to base initialization on the trust-region mode rather than the previous KKT-based condition. This changes which internal buffers and views are allocated/initialized for different restart strategies. No changes to public interfaces or exported signatures. <sub>✏️ Tip: You can customize this high-level summary in your review settings.</sub> Authors: - Nicolas Blin (https://github.com/Kh4ster) Approvers: - Alice Boucher (https://github.com/aliceb-nv) URL: NVIDIA#629
|
@aliceb-nv the point in keeping track of problem pointers is for two reasons:
|
|
Thanks Akif! |
![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
♻️ Duplicate comments (1)
cpp/src/mip/diversity/population.cuh (1)
174-174: Dual-pointer and dual-weights state management requires careful synchronization.As noted in past review comments, maintaining
problem_ptr_with_cuts/weights_with_cutsalongsideproblem_ptr/weightsintroduces risk of broken invariants. Consider documenting the ownership semantics and lifecycle of these members. The planned robust architecture for managing cuts (mentioned in past discussion) should address this.Also applies to: 184-184
🧹 Nitpick comments (7)
cpp/src/mip/diversity/diversity_config.hpp (1)
31-31: Good change for consistency.Increasing
lp_run_time_if_infeasiblefrom 1.0 to 2.0 makes it consistent withlp_run_time_if_feasible(line 30). This symmetry is sensible, especially when extending objective cuts to recombiners (per the PR objective), which may increase encounters with infeasible intermediate solutions.Consider adding a brief comment documenting the rationale for these time limits (e.g., "// LP time limits balanced to handle both feasible and infeasible cases during recombination"), especially since the PR description notes that benchmark results are pending.
conda/environments/all_cuda-129_arch-x86_64.yaml (1)
22-22: Simplify redundant version constraint pattern.All updated RAPIDS packages use the pattern
package==26.2.*,>=0.0.0a0. The>=0.0.0a0constraint is redundant;26.2.*already implies a lower bound of26.2.0. Simplify these topackage==26.2.*unless there's a specific reason to include pre-release versions.Apply this diff to remove redundant constraints (example shown for one package; apply to all affected lines):
- cudf==26.2.*,>=0.0.0a0 + cudf==26.2.* - libraft-headers==26.2.*,>=0.0.0a0 + libraft-headers==26.2.* - librmm==26.2.*,>=0.0.0a0 + librmm==26.2.* - pylibraft==26.2.*,>=0.0.0a0 + pylibraft==26.2.* - rapids-dask-dependency==26.2.*,>=0.0.0a0 + rapids-dask-dependency==26.2.* - rmm==26.2.*,>=0.0.0a0 + rmm==26.2.*Also applies to: 38-39, 56-56, 62-62, 65-65
docs/cuopt/source/project.json (1)
1-1: Version metadata is correct; optional wording tweakThe version field is correctly updated to
26.02.00and matches the rest of the release metadata.Optionally, you might clean up the description grammar:
-{"name": "cuopt", "version": "26.02.00", "url": "https://github.com/nvidia/cuopt", "description": "NVIDIA cuOpt is a optimization engine"} +{"name": "cuopt", "version": "26.02.00", "url": "https://github.com/nvidia/cuopt", "description": "NVIDIA cuOpt is an optimization engine"}README.md (1)
68-108: Install instructions now consistently target 26.02.*The pip and conda examples for CUDA 12.x / 13.x have been updated to use
cuopt-server[-cu12/-cu13]==26.02.*andcuopt-sh-client==26.02.*, which aligns the user-facing docs with the new release line.
- Please confirm that the 26.02.* wheels and conda packages for these names are actually published for all supported CUDA/Python combos, so the copy‑paste commands work as written.
- If you want the README to always highlight the latest release, you might optionally update the container example tag (currently 25.10.0) to a 26.02.00-based example in a follow-up.
benchmarks/linear_programming/cuopt/run_mip.cpp (1)
416-416: Edge case: division may yield zero threads.If
n_gpusexceedsomp_get_max_threads(), integer division will result innum_cpu_threads = 0, which may cause issues downstream. Consider adding a floor of 1:- if (num_cpu_threads < 0) { num_cpu_threads = omp_get_max_threads() / n_gpus; } + if (num_cpu_threads < 0) { num_cpu_threads = std::max(1, omp_get_max_threads() / n_gpus); }cpp/src/mip/local_search/local_search.cu (2)
258-270: Remove or document commented-out code.This block of commented-out time limit adjustment logic should either be removed or have a clear TODO/FIXME explaining why it's being kept. Leaving dead code in comments can cause maintenance confusion.
- // if (!solution.get_feasible()) { - // if (ls_config.at_least_one_parent_feasible) { - // fj_settings.time_limit = 0.5; - // timer = timer_t(fj_settings.time_limit); - // } else { - // fj_settings.time_limit = 0.25; - // timer = timer_t(fj_settings.time_limit); - // } - // } else { - // fj_settings.time_limit = std::min(1., timer.remaining_time()); - // } fj_settings.time_limit = std::min(1., timer.remaining_time()); timer = timer_t(fj_settings.time_limit);
601-618: Duplicated cutting plane logic - consider reusing handle_cutting_plane_and_weights.Lines 601-618 in
run_recombinersduplicate most of the logic fromhandle_cutting_plane_and_weights(lines 548-569). The only difference is thathandle_cutting_plane_and_weightsalso callsapply_problem_ptr_to_all_solutions()at the end. Consider refactoring to reuse the helper:population_ptr->add_external_solutions_to_population(); if (population_ptr->is_feasible()) { - if (!cutting_plane_added_for_active_run) { - solution.set_problem_ptr(&problem_with_objective_cut); - population_ptr->set_problem_ptr_with_cuts(&problem_with_objective_cut); - resize_to_new_problem(); - cutting_plane_added_for_active_run = true; - raft::copy(population_ptr->weights.cstr_weights.data(), - fj.cstr_weights.data(), - population_ptr->weights.cstr_weights.size(), - solution.handle_ptr->get_stream()); - raft::copy(population_ptr->weights_with_cuts.cstr_weights.data(), - fj.cstr_weights.data(), - fj.cstr_weights.size(), - solution.handle_ptr->get_stream()); - } - population_ptr->update_weights(); - save_solution_and_add_cutting_plane(population_ptr->best_feasible(), solution, best_objective); + handle_cutting_plane_and_weights(solution, population_ptr, best_objective); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (45)
.github/workflows/build.yaml(13 hunks).github/workflows/build_test_publish_images.yaml(1 hunks).github/workflows/nightly.yaml(1 hunks).github/workflows/pr.yaml(11 hunks).github/workflows/test.yaml(5 hunks).github/workflows/trigger-breaking-change-alert.yaml(1 hunks)RAPIDS_BRANCH(1 hunks)README.md(2 hunks)VERSION(1 hunks)benchmarks/linear_programming/cuopt/run_mip.cpp(2 hunks)ci/release/update-version.sh(1 hunks)ci/test_cpp.sh(1 hunks)ci/test_cpp_memcheck.sh(1 hunks)ci/test_python.sh(1 hunks)conda/environments/all_cuda-129_arch-aarch64.yaml(4 hunks)conda/environments/all_cuda-129_arch-x86_64.yaml(4 hunks)conda/environments/all_cuda-130_arch-aarch64.yaml(4 hunks)conda/environments/all_cuda-130_arch-x86_64.yaml(4 hunks)cpp/CMakeLists.txt(3 hunks)cpp/src/mip/diversity/diversity_config.hpp(1 hunks)cpp/src/mip/diversity/diversity_manager.cu(3 hunks)cpp/src/mip/diversity/population.cu(7 hunks)cpp/src/mip/diversity/population.cuh(4 hunks)cpp/src/mip/diversity/recombiners/bound_prop_recombiner.cuh(4 hunks)cpp/src/mip/diversity/recombiners/fp_recombiner.cuh(2 hunks)cpp/src/mip/diversity/recombiners/line_segment_recombiner.cuh(2 hunks)cpp/src/mip/diversity/recombiners/sub_mip.cuh(2 hunks)cpp/src/mip/local_search/local_search.cu(8 hunks)cpp/src/mip/local_search/local_search.cuh(1 hunks)cpp/src/mip/solution/solution.cu(4 hunks)cpp/src/mip/solution/solution.cuh(2 hunks)dependencies.yaml(17 hunks)docs/cuopt/source/cuopt-c/quick-start.rst(3 hunks)docs/cuopt/source/cuopt-python/quick-start.rst(3 hunks)docs/cuopt/source/cuopt-python/routing/routing-example.ipynb(1 hunks)docs/cuopt/source/cuopt-server/quick-start.rst(3 hunks)docs/cuopt/source/faq.rst(1 hunks)docs/cuopt/source/project.json(1 hunks)docs/cuopt/source/versions1.json(1 hunks)helmchart/cuopt-server/Chart.yaml(2 hunks)helmchart/cuopt-server/values.yaml(1 hunks)python/cuopt/pyproject.toml(2 hunks)python/cuopt_self_hosted/pyproject.toml(1 hunks)python/cuopt_server/pyproject.toml(1 hunks)python/libcuopt/pyproject.toml(2 hunks)
🧰 Additional context used
📓 Path-based instructions (8)
docs/**/*
⚙️ CodeRabbit configuration file
docs/**/*: For documentation changes, focus on:
- Accuracy: Verify code examples compile and run correctly
- Completeness: Check if API changes (parameters, return values, errors) are documented
- Clarity: Flag confusing explanations, missing prerequisites, or unclear examples
- Consistency: Version numbers, parameter types, and terminology match code
- Examples: Suggest adding examples for complex features or new APIs
- Missing docs: If PR changes public APIs without updating docs, flag as HIGH priority
When code changes affect docs:
- Suggest specific doc files that need updates (e.g., docs/cuopt/api.rst)
- Identify outdated information contradicting the code changes
- Recommend documenting performance characteristics, GPU requirements, or numerical tolerances
Files:
docs/cuopt/source/cuopt-python/routing/routing-example.ipynbdocs/cuopt/source/faq.rstdocs/cuopt/source/cuopt-python/quick-start.rstdocs/cuopt/source/cuopt-server/quick-start.rstdocs/cuopt/source/versions1.jsondocs/cuopt/source/cuopt-c/quick-start.rstdocs/cuopt/source/project.json
docs/**/*.{rst,md}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
docs/**/*.{rst,md}: Update documentation in docs/ for API changes (new parameters, return values, error codes), new public functions/classes, and changed algorithm behavior
Document numerical tolerances, parameter ranges, and algorithm phase descriptions in API documentation when solver behavior or tolerances change
Update version numbers and deprecation notices in documentation for breaking API changes; provide migration guidance for deprecated APIs
Files:
docs/cuopt/source/faq.rstdocs/cuopt/source/cuopt-python/quick-start.rstdocs/cuopt/source/cuopt-server/quick-start.rstdocs/cuopt/source/cuopt-c/quick-start.rst
**/*.{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:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/src/mip/solution/solution.cucpp/src/mip/diversity/recombiners/fp_recombiner.cuhcpp/src/mip/diversity/diversity_manager.cucpp/src/mip/diversity/recombiners/sub_mip.cuhcpp/src/mip/diversity/recombiners/line_segment_recombiner.cuhcpp/src/mip/diversity/diversity_config.hppcpp/src/mip/diversity/recombiners/bound_prop_recombiner.cuhcpp/src/mip/local_search/local_search.cucpp/src/mip/solution/solution.cuhcpp/src/mip/diversity/population.cucpp/src/mip/local_search/local_search.cuhcpp/src/mip/diversity/population.cuh
**/*.{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:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/src/mip/diversity/diversity_config.hpp
**/*.{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:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/src/mip/solution/solution.cucpp/src/mip/diversity/diversity_manager.cucpp/src/mip/diversity/diversity_config.hppcpp/src/mip/local_search/local_search.cucpp/src/mip/diversity/population.cu
**/*.{cu,cuh}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh}: Every CUDA kernel launch and memory operation must have error checking with CUDA_CHECK or equivalent verification
Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Files:
cpp/src/mip/solution/solution.cucpp/src/mip/diversity/recombiners/fp_recombiner.cuhcpp/src/mip/diversity/diversity_manager.cucpp/src/mip/diversity/recombiners/sub_mip.cuhcpp/src/mip/diversity/recombiners/line_segment_recombiner.cuhcpp/src/mip/diversity/recombiners/bound_prop_recombiner.cuhcpp/src/mip/local_search/local_search.cucpp/src/mip/solution/solution.cuhcpp/src/mip/diversity/population.cucpp/src/mip/local_search/local_search.cuhcpp/src/mip/diversity/population.cuh
**/*.cu
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.cu: Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations
Detect inefficient GPU kernel launches with low occupancy or poor memory access patterns; optimize for coalesced memory access and minimize warp divergence in hot paths
Files:
cpp/src/mip/solution/solution.cucpp/src/mip/diversity/diversity_manager.cucpp/src/mip/local_search/local_search.cucpp/src/mip/diversity/population.cu
**/*.{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/mip/diversity/diversity_config.hpp
🧠 Learnings (24)
📚 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 docs/**/*.{rst,md} : Update documentation in docs/ for API changes (new parameters, return values, error codes), new public functions/classes, and changed algorithm behavior
Applied to files:
docs/cuopt/source/faq.rst
📚 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} : Check that hard-coded GPU device IDs and resource limits are made configurable; abstract multi-backend support for different CUDA versions
Applied to files:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/CMakeLists.txt
📚 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} : Verify correct problem size checks before expensive GPU/CPU operations; prevent resource exhaustion on oversized problems
Applied to files:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/src/mip/diversity/population.cu
📚 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 race conditions are absent in multi-GPU code and multi-threaded server implementations; verify proper synchronization of shared state
Applied to files:
benchmarks/linear_programming/cuopt/run_mip.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 : Detect inefficient GPU kernel launches with low occupancy or poor memory access patterns; optimize for coalesced memory access and minimize warp divergence in hot paths
Applied to files:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/CMakeLists.txt
📚 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 **/*benchmark*.{cpp,cu,py} : Include performance benchmarks and regression detection for GPU operations; verify near real-time performance on million-variable problems
Applied to files:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/CMakeLists.txt
📚 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:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/src/mip/solution/solution.cucpp/src/mip/diversity/diversity_manager.cucpp/src/mip/local_search/local_search.cucpp/src/mip/solution/solution.cuhcpp/src/mip/diversity/population.cucpp/src/mip/local_search/local_search.cuh
📚 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:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/src/mip/solution/solution.cucpp/src/mip/diversity/recombiners/fp_recombiner.cuhcpp/src/mip/diversity/diversity_manager.cucpp/src/mip/diversity/recombiners/sub_mip.cuhcpp/src/mip/diversity/diversity_config.hppcpp/src/mip/diversity/recombiners/bound_prop_recombiner.cuhcpp/src/mip/local_search/local_search.cucpp/src/mip/solution/solution.cuhcpp/src/mip/diversity/population.cucpp/src/mip/local_search/local_search.cuhcpp/src/mip/diversity/population.cuh
📚 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:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/src/mip/solution/solution.cucpp/src/mip/diversity/recombiners/sub_mip.cuhcpp/src/mip/local_search/local_search.cucpp/src/mip/solution/solution.cuhcpp/src/mip/diversity/population.cucpp/src/mip/local_search/local_search.cuhcpp/src/mip/diversity/population.cuhcpp/CMakeLists.txt
📚 Learning: 2025-10-22T14:25:22.899Z
Learnt from: aliceb-nv
Repo: NVIDIA/cuopt PR: 527
File: cpp/src/mip/diversity/lns/rins.cu:167-175
Timestamp: 2025-10-22T14:25:22.899Z
Learning: In MIP (Mixed Integer Programming) problems in the cuOPT codebase, `n_integer_vars == 0` is impossible by definition—MIP problems must have at least one integer variable. If there are no integer variables, it would be a pure Linear Programming (LP) problem, not a MIP problem.
Applied to files:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/src/mip/diversity/population.cuh
📚 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:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/src/mip/solution/solution.cucpp/src/mip/diversity/recombiners/fp_recombiner.cuhcpp/src/mip/diversity/recombiners/sub_mip.cuhcpp/src/mip/diversity/recombiners/bound_prop_recombiner.cuhcpp/src/mip/local_search/local_search.cucpp/src/mip/solution/solution.cuhcpp/src/mip/diversity/population.cucpp/src/mip/local_search/local_search.cuhcpp/src/mip/diversity/population.cuh
📚 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} : Track GPU device memory allocations and deallocations to prevent memory leaks; ensure cudaMalloc/cudaFree balance and cleanup of streams/events
Applied to files:
benchmarks/linear_programming/cuopt/run_mip.cppcpp/CMakeLists.txt
📚 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 problem transformations: verify correctness of original→transformed→postsolve mappings and index consistency across problem representations
Applied to files:
cpp/src/mip/solution/solution.cucpp/src/mip/solution/solution.cuhcpp/src/mip/diversity/population.cucpp/src/mip/diversity/population.cuh
📚 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/mip/solution/solution.cucpp/src/mip/diversity/diversity_manager.cucpp/src/mip/diversity/recombiners/sub_mip.cuhcpp/src/mip/local_search/local_search.cucpp/src/mip/solution/solution.cuhcpp/src/mip/diversity/population.cucpp/src/mip/local_search/local_search.cuhcpp/src/mip/diversity/population.cuh
📚 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 **/*.{cpp,hpp,h} : Check for unclosed file handles when reading MPS/QPS problem files; ensure RAII patterns or proper cleanup in exception paths
Applied to files:
cpp/src/mip/diversity/recombiners/fp_recombiner.cuh
📚 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/mip/local_search/local_search.cucpp/src/mip/solution/solution.cuhcpp/src/mip/local_search/local_search.cuhcpp/src/mip/diversity/population.cuh
📚 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} : Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Applied to files:
cpp/src/mip/diversity/population.cucpp/CMakeLists.txt
📚 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} : Test with free variables, singleton problems, and extreme problem dimensions near resource limits to validate edge case handling
Applied to files:
cpp/src/mip/diversity/population.cucpp/src/mip/diversity/population.cuh
📚 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 **/*.{cpp,hpp,h} : Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state
Applied to files:
cpp/src/mip/diversity/population.cuh
📚 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/mip/diversity/population.cuh
📚 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/CMakeLists.txt
📚 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} : Every CUDA kernel launch and memory operation must have error checking with CUDA_CHECK or equivalent verification
Applied to files:
cpp/CMakeLists.txt
📚 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} : Verify error propagation from CUDA to user-facing APIs is complete; ensure CUDA errors are caught and mapped to meaningful user error codes
Applied to files:
cpp/CMakeLists.txt
📚 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,cpp,hpp,h} : Avoid inappropriate use of exceptions in performance-critical GPU operation paths; prefer error codes or CUDA error checking for latency-sensitive code
Applied to files:
cpp/CMakeLists.txt
🧬 Code graph analysis (1)
cpp/src/mip/solution/solution.cu (1)
cpp/include/cuopt/linear_programming/pdlp/solver_solution.hpp (1)
handle_ptr(282-283)
⏰ 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). (10)
- 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.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, amd64, 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.11, amd64, rockylinux8
- 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.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.0.2, 3.10, amd64, rockylinux8
- GitHub Check: checks / check-style
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
cpp/src/linear_programming/restart_strategy/localized_duality_gap_container.cu (1)
40-47: Minor:cudaMemsetAsyncsecond argument should be0, notf_t(0.0).
cudaMemsetAsynctakes a byte value (int) as the second parameter, not a floating-point value. While this works correctly since 0 andf_t(0.0)both result in zero-bytes being written, usingf_t(0.0)is misleading and relies on implicit conversion.- RAFT_CUDA_TRY(cudaMemsetAsync(primal_solution_.data(), - f_t(0.0), - sizeof(f_t) * primal_solution_.size(), - handle_ptr->get_stream())); - RAFT_CUDA_TRY(cudaMemsetAsync(dual_solution_.data(), - f_t(0.0), - sizeof(f_t) * dual_solution_.size(), - handle_ptr->get_stream())); + RAFT_CUDA_TRY(cudaMemsetAsync(primal_solution_.data(), + 0, + sizeof(f_t) * primal_solution_.size(), + handle_ptr->get_stream())); + RAFT_CUDA_TRY(cudaMemsetAsync(dual_solution_.data(), + 0, + sizeof(f_t) * dual_solution_.size(), + handle_ptr->get_stream()));cpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cu (2)
115-135: Stale comment on line 125.The comment says "If KKT restart, call the empty cusparse_view constructor" but the condition now checks
!is_trust_region_restart(). The comment should be updated to reflect the new semantics.- // If KKT restart, call the empty cusparse_view constructor + // If trust region restart is not used, call the empty cusparse_view constructor avg_duality_gap_cusparse_view_{
158-194: Consider caching or extracting the repeated pattern (optional).The pattern
(!is_trust_region_restart<i_t, f_t>()) ? 0 : static_cast<size_t>(size)appears many times. While member initializer lists don't allow local variables, a small helper function could improve readability:template <typename i_t, typename f_t> constexpr size_t trust_region_size_or_zero(i_t size) { return is_trust_region_restart<i_t, f_t>() ? static_cast<size_t>(size) : 0; }However, since
is_trust_region_restartreads a runtime hyperparameter, this optimization is minor and can be deferred.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
cpp/CMakeLists.txt(2 hunks)cpp/src/dual_simplex/branch_and_bound.cpp(23 hunks)cpp/src/dual_simplex/branch_and_bound.hpp(4 hunks)cpp/src/dual_simplex/phase2.cpp(1 hunks)cpp/src/linear_programming/restart_strategy/localized_duality_gap_container.cu(1 hunks)cpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cu(6 hunks)cpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cuh(1 hunks)cpp/src/utilities/omp_helpers.hpp(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/CMakeLists.txt
🧰 Additional context used
📓 Path-based instructions (6)
**/*.{cu,cuh}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh}: Every CUDA kernel launch and memory operation must have error checking with CUDA_CHECK or equivalent verification
Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Files:
cpp/src/linear_programming/restart_strategy/localized_duality_gap_container.cucpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cucpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cuh
**/*.cu
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.cu: Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations
Detect inefficient GPU kernel launches with low occupancy or poor memory access patterns; optimize for coalesced memory access and minimize warp divergence in hot paths
Files:
cpp/src/linear_programming/restart_strategy/localized_duality_gap_container.cucpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cu
**/*.{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/linear_programming/restart_strategy/localized_duality_gap_container.cucpp/src/dual_simplex/phase2.cppcpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cucpp/src/dual_simplex/branch_and_bound.cppcpp/src/utilities/omp_helpers.hppcpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cuhcpp/src/dual_simplex/branch_and_bound.hpp
**/*.{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/linear_programming/restart_strategy/localized_duality_gap_container.cucpp/src/dual_simplex/phase2.cppcpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cucpp/src/dual_simplex/branch_and_bound.cppcpp/src/utilities/omp_helpers.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/phase2.cppcpp/src/dual_simplex/branch_and_bound.cppcpp/src/utilities/omp_helpers.hppcpp/src/dual_simplex/branch_and_bound.hpp
**/*.{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/utilities/omp_helpers.hppcpp/src/dual_simplex/branch_and_bound.hpp
🧠 Learnings (12)
📓 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
📚 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/linear_programming/restart_strategy/localized_duality_gap_container.cucpp/src/dual_simplex/phase2.cppcpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cucpp/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} : 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/linear_programming/restart_strategy/localized_duality_gap_container.cucpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cucpp/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/linear_programming/restart_strategy/localized_duality_gap_container.cucpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cucpp/src/utilities/omp_helpers.hppcpp/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 **/*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/phase2.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} : Check that hard-coded GPU device IDs and resource limits are made configurable; abstract multi-backend support for different CUDA versions
Applied to files:
cpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cu
📚 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} : Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Applied to files:
cpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cucpp/src/utilities/omp_helpers.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/linear_programming/restart_strategy/pdlp_restart_strategy.cucpp/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: 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 : Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations
Applied to files:
cpp/src/utilities/omp_helpers.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 **/*.{cpp,hpp,h} : Prevent thread-unsafe use of global and static variables; use proper mutex/synchronization in server code accessing shared solver state
Applied to files:
cpp/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.hpp
🧬 Code graph analysis (2)
cpp/src/linear_programming/restart_strategy/localized_duality_gap_container.cu (1)
cpp/include/cuopt/linear_programming/pdlp/solver_solution.hpp (1)
handle_ptr(282-283)
cpp/src/dual_simplex/branch_and_bound.hpp (2)
cpp/src/dual_simplex/branch_and_bound.cpp (6)
node_presolver(837-837)node_presolver(1048-1048)node_presolver(1117-1117)basic_list(841-841)basic_list(1052-1052)basic_list(1121-1121)cpp/src/dual_simplex/phase2.cpp (1)
basic_list(2176-2176)
🔇 Additional comments (23)
cpp/src/utilities/omp_helpers.hpp (2)
94-101: I'm unable to proceed with verification due to repository access issues. The cloning of NVIDIA/cuopt failed, which prevents me from:
- Searching for existing member function calls to
fetch_minandfetch_max- Reading the full implementation context (lines 108-131)
- Identifying all usages across the codebase
- Confirming whether this is truly a breaking API change
Manual verification required: Cannot confirm if the API change is breaking without codebase access.
The review comment claims changing from member functions
fetch_min()andfetch_max()to free functions represents a breaking API change, contradicting the PR's "non-breaking" label. However, this cannot be validated automatically:
- No existing calls to
.fetch_min(or.fetch_max(could be found to confirm the claim- The full implementation context (lines 108-131) could not be reviewed
- The impact assessment depends on actual usage patterns in the codebase
Recommendation: Manually verify:
- Whether
fetch_minandfetch_maxwere previously public member functions- Whether any existing code calls them as member functions
- Whether the PR should be re-labeled or if the caller interface was intentionally changed
108-131: Based on the verification results, I found a critical issue with the review comment:The clang bug #127466 referenced in the code comment cannot be verified as a valid public LLVM bug report.
The web search found no public record of this bug in:
- bugs.llvm.org (LLVM's official bug tracker)
- LLVM/Clang documentation
- Any publicly accessible sources
Since the repository cloning failed, I cannot verify the codebase usage patterns directly. However, the unverifiable bug reference is itself a significant concern that should be raised.
Verify the clang 20 bug reference before accepting the workaround.
The code comment cites clang 20 bug #127466 as justification for converting templated member functions to non-templated free functions. However, this bug report does not appear in LLVM's public bug tracker (bugs.llvm.org) or official documentation.
Unverified bug reference: The clang bug #127466 is not publicly documented. Before accepting this architectural workaround (which loses template generality and introduces API inconsistency), confirm:
- The bug number and source (bugs.llvm.org, GitHub issue, or Phabricator URL)
- Whether it affects production-relevant clang versions
- The timeline for a fix
Loss of type generality: Specializing to
doubleonly narrows the API surface. Verify that no current or planned code usesfetch_min/fetch_maxwith other numeric types (e.g.,int,float,long).API inconsistency: These functions now break the pattern established by
fetch_add,fetch_sub,exchange,load, andstore(which are member functions). Document the expectation that this will be reverted once the compiler bug is resolved.cpp/src/linear_programming/restart_strategy/localized_duality_gap_container.cu (1)
31-38: Logic inversion looks correct for the trust-region-based approach.The change from
is_KKT_restart()to!is_trust_region_restart()correctly allocates these gradient and solution vectors only when trust-region restart is active. This is consistent with the header changes where the predicate now checks exclusively forTRUST_REGION_RESTART.cpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cu (1)
860-894: Explicit restart strategy dispatch is cleaner and more maintainable.The change from using the
is_KKT_restart()helper to explicit enum comparisons makes the restart logic clearer and easier to extend. The fallback error case handles unexpected values appropriately.cpp/src/linear_programming/restart_strategy/pdlp_restart_strategy.cuh (1)
348-354: I'm encountering persistent issues accessing the repository. Let me analyze what can be verified from the information provided in the review comment itself.Based on the review comment:
The code snippet provided (lines 348-354) clearly shows
is_trust_region_restart()checking forTRUST_REGION_RESTARTenum value.The review's claims state this is a semantic change from
is_KKT_restart()checking forKKT_RESTART.Evidence mentioned in the review: The
localized_duality_gap_container.cufile uses!is_trust_region_restart(), which the review says "aligns with the new semantics."However, without access to the repository, I cannot:
- Verify if
is_KKT_restart()has been completely removed- Confirm all call sites have been updated
- Validate that the inversion of semantics is correct across all usages
- Check for any orphaned references to the old function
The review comment raises a valid concern about semantic changes and call-site verification, but I cannot definitively confirm or refute the claim due to repository access limitations. Manual verification by inspecting:
- All occurrences of
is_KKT_restart(should be zero if fully migrated)- All occurrences of
is_trust_region_restartto confirm correct inverted logic at each call site- The enum definition to confirm KKT_RESTART and TRUST_REGION_RESTART are distinct values
is necessary to resolve this review comment.
cpp/src/dual_simplex/phase2.cpp (1)
2238-2240: Good defensive initialization of nonbasic_list.Clearing and reserving capacity before population ensures no stale data remains from prior calls when the vector is reused across multiple
dual_phase2_with_advanced_basisinvocations. This aligns with the broader PR goal of threading basis data through solve paths.cpp/src/dual_simplex/branch_and_bound.hpp (4)
130-132: Thread-safe logging state management.Converting
last_logtoomp_atomic_t<f_t>addresses potential data races when multiple threads access this field. Combined with the newshould_report_flag, this enables safe coordination of progress reporting across threads.
162-163: Atomic flag for coordinated reporting.The
should_report_flag enables a single thread to report progress while preventing duplicate logging from concurrent threads. The exchange-based pattern used in the implementation (seen in branch_and_bound.cpp) is a correct lock-free coordination mechanism.
189-196: Extended explore_subtree signature for basis reuse.Threading
basis_update_mpf_t,basic_list, andnonbasic_listthrough the exploration path allows reusing basis factorization data across sibling nodes in a subtree, reducing redundant factorization work. This aligns with the learning to reduce tight coupling by making solver components more modular and reusable.
209-220: Extended solve_node signature with basis-aware parameters.The addition of basis-related parameters enables warm-starting LP solves from a previous basis when exploring sibling nodes. The renamed
recompute_basis_and_boundsparameter clarifies that both bounds and basis may need recomputation, not just general recompute.cpp/src/dual_simplex/branch_and_bound.cpp (13)
627-663: Correct basis/bounds initialization based on recompute flag.When
recompute_bounds_and_basisis true, bounds are reset from root and the basis is reinitialized. When false (continuing down a path), only the branched variable bounds are updated. This optimization reduces redundant work when exploring sibling nodes.
665-678: Numerical recovery now uses advanced basis path.The fallback for numerical issues now uses
solve_linear_program_with_advanced_basisto maintain consistency with the primary solve path. This ensures the basis data structures remain in a consistent state after recovery.
733-733: Added missing tree update for branched nodes.Adding
search_tree.update(node_ptr, node_status_t::HAS_CHILDREN)ensures the search tree correctly tracks nodes that produced children.
803-826: Thread-safe progress reporting with exchange pattern.The
should_report_.exchange(false)pattern ensures only one thread reports progress at a time, preventing duplicate log lines. The flag is reset totrueafter reporting completes.
839-859: Consistent basis initialization in exploration_ramp_up.The basis structures are initialized with correct dimensions before solving nodes during the ramp-up phase. This matches the pattern used in other thread entry points.
896-980: Basis reuse logic in explore_subtree.The
recompute_bounds_and_basisflag is set to!has_children(status)after solving a node. This means:
- When a node produces children: the basis is reused for the next child (optimal case)
- When a node is pruned/fathomed: the basis must be recomputed for the next unrelated node
This correctly captures the warm-start optimization for subtree traversal.
1006-1014: Clear bounds_changed before computing node bounds.Resetting
bounds_changedbefore callingget_variable_boundsensures accurate tracking of which bounds were modified for this specific node, preventing stale markers from affecting bounds strengthening.
1050-1088: Basis initialization in best_first_thread.The basis structures are properly initialized before the exploration loop. The basis is then reused across nodes within the same subtree exploration via the
explore_subtreecall.
1119-1166: Basis initialization and reuse in diving_thread.The diving thread properly initializes basis structures once and reuses them across the dive. The
recompute_bounds_and_basisflag is correctly managed to allow warm-starting when continuing down a path and force recomputation when switching to a different branch.
1187-1198: Clear bounds_changed before detaching dive node.Same pattern as line 1008-1010: resetting
bounds_changedbefore computing bounds for the new diving root ensures accurate tracking.
1368-1368: Initialize should_report_ at solver start.Setting
should_report_ = trueat the start of the solve ensures the first progress report will be generated by one of the exploration threads.
754-761: Unable to complete verification — repository not accessible.The codebase cannot be accessed for direct verification. However, web search indicates that cuOpt's branch-and-bound solver runs on CPU with parallel threads, which makes atomic operations for thread-safe updates appropriate. The review comment's claim about
fetch_minensuring thread-safe updates tolower_bound_ceiling_is contextually plausible for a multi-threaded CPU-side implementation.However, critical details cannot be confirmed:
- Whether
fetch_minis properly defined and available- Type of
lower_bound_ceiling_(must be atomic type for correctness)- Actual implementation correctness at lines 754-761
- Whether the operation semantics match the stated intent
600-604: Based on my verification attempt, I found that:
Repository Access Issue: The codebase cannot be cloned to perform direct verification of the code context and surrounding logic.
Web Search Findings: cuOpt's documentation confirms that:
- Column scaling is enabled by default and improves numerical stability
- Scaling rescales the matrix and vectors (A, b, c) using scaling factors
- The
scale_columnssetting controls this featureReview Comment Assessment: The original comment correctly identifies that
scale_columns = falsedisables column scaling. However, without accessing the actual codebase, I cannot verify:
- Whether this change is intentional or documented with comments
- The context for why scaling is disabled specifically for node LP solves
- Whether there are conditions or guards around this setting
- If there are any numerical stability issues being addressed
Verify with maintainers whether
scale_columns = falsefor node LP solves is intentional.The setting correctly disables column scaling, which typically improves numerical stability. Confirm the rationale: Is this optimization for performance in node subproblems, or is there a specific numerical reason? If intentional, add a clarifying comment explaining the trade-off.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cpp/src/mip/local_search/local_search.cu (1)
549-571: Centralize problem-with-cuts pointer/weights handling to avoid drift and subtle invariantsYou now have two places that partially manage the “problem with objective cuts” state:
handle_cutting_plane_and_weights:
- Sets
population_ptr->set_problem_ptr_with_cuts.- Calls
solution.set_problem_ptr(&problem_with_objective_cut, /*is_cuts_problem=*/true).- Calls
resize_to_new_problem().- Copies weights from both
population_ptr->weightsandweights_with_cutsintofj.cstr_weights.- Calls
population_ptr->update_weights().- Calls
save_solution_and_add_cutting_planeand thenapply_problem_ptr_to_all_solutions().
run_recombiners(feasible branch):
- Re-implements a very similar
if (!cutting_plane_added_for_active_run)block (but callssolution.set_problem_ptr(&problem_with_objective_cut)without theis_cuts_problemflag and does not callapply_problem_ptr_to_all_solutions()in that branch).- Then calls
population_ptr->update_weights()andsave_solution_and_add_cutting_plane(...).- Only later, under stagnation, calls
apply_problem_ptr_to_all_solutions()and a secondsave_solution_and_add_cutting_plane(...).This duplication makes it easy for the two paths (FP vs. recombiners) to diverge in behavior, especially wrt:
- Whether
solutionand all population members have their problem pointer and “is cut” state updated consistently.- When
apply_problem_ptr_to_all_solutions()is invoked relative to external solutions being injected.- The exact semantics of copying
weightsandweights_with_cutsontofj.cstr_weights(the second copy currently overwrites the first; if this is intentional, it deserves to live in a single well‑documented helper).Given earlier concerns about coupling between problem modifications and pointer updates, it would be safer to funnel all “activate / update objective cut + weights” behavior through one helper instead of open‑coding it twice.
For example, in
run_recombinersyou could delegate to the existing helper:if (population_ptr->is_feasible()) { handle_cutting_plane_and_weights(solution, population_ptr, best_objective); }and drop the custom
if (!cutting_plane_added_for_active_run)block there, relying onhandle_cutting_plane_and_weightsto manage:
- First-time initialization of the cuts problem,
- Weight synchronization,
- And
apply_problem_ptr_to_all_solutions()after new solutions enter the population.This would align FP and recombiner semantics and keep the “problem_with_objective_cut invariants” in one place, which is especially important for maintaining correct problem context mappings across transformations.
Also applies to: 591-619, 656-659, 684-708, 711-713
🧹 Nitpick comments (1)
cpp/src/mip/local_search/local_search.cu (1)
268-273: Timer handling change is fine; consider removing commented legacy codeSpecial‑casing
fj_settings.time_limitand resettingtimeronly in the feasible branch improves clarity, and the logic is consistent with the intended behavior.The old, now‑redundant lines kept as comments:
// fj_settings.time_limit = std::min(1., timer.remaining_time()); // timer = timer_t(fj_settings.time_limit);can likely be dropped to avoid confusion once you are confident in the new behavior.
Also applies to: 270-271
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp/src/mip/diversity/diversity_manager.cu(2 hunks)cpp/src/mip/local_search/local_search.cu(8 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/mip/diversity/diversity_manager.cu
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cu,cuh}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh}: Every CUDA kernel launch and memory operation must have error checking with CUDA_CHECK or equivalent verification
Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Files:
cpp/src/mip/local_search/local_search.cu
**/*.cu
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.cu: Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations
Detect inefficient GPU kernel launches with low occupancy or poor memory access patterns; optimize for coalesced memory access and minimize warp divergence in hot paths
Files:
cpp/src/mip/local_search/local_search.cu
**/*.{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/mip/local_search/local_search.cu
**/*.{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/mip/local_search/local_search.cu
🧠 Learnings (8)
📓 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 algorithm correctness in optimization logic: simplex pivots, branch-and-bound decisions, routing heuristics, and constraint/objective handling must produce correct results
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
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)
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
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
⏰ 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). (10)
- GitHub Check: conda-cpp-build / compute-matrix
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.10, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.11, amd64, 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.10, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.12, amd64, rockylinux8
- 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.13, arm64, rockylinux8
- GitHub Check: wheel-build-cuopt-mps-parser / 13.0.2, 3.13, amd64, rockylinux8
- GitHub Check: wheel-build-cuopt-sh-client / 13.0.2, 3.10, amd64, rockylinux8
🔇 Additional comments (2)
cpp/src/mip/local_search/local_search.cu (2)
27-38: FP constructor wiring with LP optimal solution looks consistentPassing
lp_optimal_solution_intofp(context, fj, constraint_prop, line_segment_search, lp_optimal_solution_)keeps FP aligned with the LP reference solution used elsewhere in local search. No correctness or lifetime issues apparent here.
172-187: CPU fallback solution handling indo_fj_solveis soundIntroducing
solution_t<i_t, f_t> solution_cpu(solution);and using it as an isolated container for the best CPU FJ assignment keeps GPU and CPU paths cleanly separated and avoids mutatingsolutionuntil you know CPU is better. The copy pluscompute_feasibility()before comparing objectives is a reasonable pattern here.
| void local_search_t<i_t, f_t>::save_solution_and_add_cutting_plane( | ||
| solution_t<i_t, f_t>& solution, rmm::device_uvector<f_t>& best_solution, f_t& best_objective) | ||
| solution_t<i_t, f_t>& best_in_population, solution_t<i_t, f_t>& solution, f_t& best_objective) |
There was a problem hiding this comment.
best_objective type mismatch between helpers and run_fp is a blocker
save_solution_and_add_cutting_plane, handle_cutting_plane_and_weights, reset_alpha_and_save_solution, and run_recombiners now take f_t& best_objective, but in run_fp you declare:
double best_objective =
is_feasible ? solution.get_objective() : std::numeric_limits<double>::max();and pass this double into those helpers.
For f_t = float, this will not compile (cannot bind float& to a double) and, even for f_t = double, mixes template and concrete types unnecessarily.
Align everything on f_t to keep the template consistent and avoid hidden narrowing:
- double best_objective =
- is_feasible ? solution.get_objective() : std::numeric_limits<double>::max();
+ f_t best_objective =
+ is_feasible ? solution.get_objective() : std::numeric_limits<f_t>::max();The helper signatures can then remain as f_t& best_objective.
Optionally, you can also make best_in_population a const ref in save_solution_and_add_cutting_plane to document that it is not mutated:
-void local_search_t<i_t, f_t>::save_solution_and_add_cutting_plane(
- solution_t<i_t, f_t>& best_in_population, solution_t<i_t, f_t>& solution, f_t& best_objective)
+void local_search_t<i_t, f_t>::save_solution_and_add_cutting_plane(
+ const solution_t<i_t, f_t>& best_in_population,
+ solution_t<i_t, f_t>& solution,
+ f_t& best_objective)Also applies to: 572-579, 591-597, 640-641, 684-708
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
cpp/src/mip/local_search/local_search.cu (1)
627-628: Critical: Type mismatch forbest_objectivewill break float instantiation.Line 627 declares
best_objectiveasdouble, but the helper functionssave_solution_and_add_cutting_plane(line 506),reset_alpha_and_save_solution(line 565), andrun_recombiners(line 583) all expectf_t& best_objective. Forf_t = float, this will not compile (cannot bindfloat&todouble).This was flagged in a previous review. Apply this fix:
- double best_objective = - is_feasible ? solution.get_objective() : std::numeric_limits<double>::max(); + f_t best_objective = + is_feasible ? solution.get_objective() : std::numeric_limits<f_t>::max();Also applies to: lines 670-671, 689-690, 693-694 where best_objective is passed as a reference parameter.
🧹 Nitpick comments (1)
cpp/src/mip/local_search/local_search.cu (1)
505-519: Makebest_in_populationconst to clarify intent.The parameter
best_in_populationis not modified in this function. Making itconstdocuments that it's read-only and prevents accidental mutation.Apply this diff:
-void local_search_t<i_t, f_t>::save_solution_and_add_cutting_plane( - solution_t<i_t, f_t>& best_in_population, solution_t<i_t, f_t>& solution, f_t& best_objective) +void local_search_t<i_t, f_t>::save_solution_and_add_cutting_plane( + const solution_t<i_t, f_t>& best_in_population, solution_t<i_t, f_t>& solution, f_t& best_objective)
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cpp/src/mip/local_search/local_search.cu(8 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{cu,cuh}
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.{cu,cuh}: Every CUDA kernel launch and memory operation must have error checking with CUDA_CHECK or equivalent verification
Avoid reinventing functionality already available in Thrust, CCCL, or RMM libraries; prefer standard library utilities over custom implementations
Files:
cpp/src/mip/local_search/local_search.cu
**/*.cu
📄 CodeRabbit inference engine (.github/.coderabbit_review_guide.md)
**/*.cu: Verify race conditions and correctness of GPU kernel shared memory, atomics, and warp-level operations
Detect inefficient GPU kernel launches with low occupancy or poor memory access patterns; optimize for coalesced memory access and minimize warp divergence in hot paths
Files:
cpp/src/mip/local_search/local_search.cu
**/*.{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/mip/local_search/local_search.cu
**/*.{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/mip/local_search/local_search.cu
🧠 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: 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
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)
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
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
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
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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/mip/local_search/local_search.cu
📚 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} : Write tests validating numerical correctness of optimization results (not just 'runs without error'); test degenerate cases (infeasible, unbounded, empty, singleton problems)
Applied to files:
cpp/src/mip/local_search/local_search.cu
🔇 Additional comments (6)
cpp/src/mip/local_search/local_search.cu (6)
37-37: LGTM: Constructor initialization updated correctly.The addition of
lp_optimal_solution_to thefpinitialization aligns with the broader objective cut propagation changes.
172-172: LGTM: Solution copy for CPU/GPU comparison.The copy constructor usage is appropriate for comparing CPU and GPU solver results.
257-258: LGTM: Timer reset pattern is appropriate.The timer is constrained to the computed limit. This pattern ensures the local search respects the tighter of the two bounds (1 second or remaining time).
559-575: LGTM: Problem pointer management in solution save is correct.The function correctly:
- Resets FP state
- Creates a copy with the original problem pointer for population storage
- Handles cutting plane updates on the working solution
This maintains proper problem context separation.
643-645: LGTM: Problem pointer propagation is correct.The code properly updates both the working solution and the population with the cutting plane problem, then applies the pointer to all solutions. This maintains problem context consistency.
698-699: LGTM: Problem pointer cleanup is correct.The solution is properly restored to the original problem context at the end of
run_fp, with corresponding internal state resizing. This maintains proper resource management symmetry.
| raft::copy(population_ptr->weights.cstr_weights.data(), | ||
| fj.cstr_weights.data(), | ||
| population_ptr->weights.cstr_weights.size(), | ||
| solution.handle_ptr->get_stream()); | ||
| raft::copy(population_ptr->weights_with_cuts.cstr_weights.data(), | ||
| fj.cstr_weights.data(), | ||
| fj.cstr_weights.size(), | ||
| solution.handle_ptr->get_stream()); |
There was a problem hiding this comment.
Redundant weight copy operation.
Lines 545-548 copy population_ptr->weights.cstr_weights to fj.cstr_weights, but lines 549-552 immediately overwrite this with population_ptr->weights_with_cuts.cstr_weights. The first copy appears to be dead code.
If the first copy is intentional (e.g., for resizing), add a comment explaining why. Otherwise, remove it:
- raft::copy(population_ptr->weights.cstr_weights.data(),
- fj.cstr_weights.data(),
- population_ptr->weights.cstr_weights.size(),
- solution.handle_ptr->get_stream());
raft::copy(population_ptr->weights_with_cuts.cstr_weights.data(),
fj.cstr_weights.data(),
fj.cstr_weights.size(),
solution.handle_ptr->get_stream());📝 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.
| raft::copy(population_ptr->weights.cstr_weights.data(), | |
| fj.cstr_weights.data(), | |
| population_ptr->weights.cstr_weights.size(), | |
| solution.handle_ptr->get_stream()); | |
| raft::copy(population_ptr->weights_with_cuts.cstr_weights.data(), | |
| fj.cstr_weights.data(), | |
| fj.cstr_weights.size(), | |
| solution.handle_ptr->get_stream()); | |
| raft::copy(population_ptr->weights_with_cuts.cstr_weights.data(), | |
| fj.cstr_weights.data(), | |
| fj.cstr_weights.size(), | |
| solution.handle_ptr->get_stream()); |
🤖 Prompt for AI Agents
In cpp/src/mip/local_search/local_search.cu around lines 545 to 552, there is a
redundant copy: the first raft::copy writes population_ptr->weights.cstr_weights
into fj.cstr_weights and is immediately overwritten by the second copy from
population_ptr->weights_with_cuts.cstr_weights; remove the first raft::copy
unless it is intentionally used for resizing or side-effects — if it's
intentional, replace it with a short comment explaining the reason (e.g.,
"retain previous weights for X" or "ensure fj.cstr_weights capacity before
writing cuts"); otherwise delete lines 545–548 so only the correct
weights_with_cuts copy remains.
| if (population_ptr->is_feasible()) { | ||
| if (!cutting_plane_added_for_active_run) { | ||
| solution.set_problem_ptr(&problem_with_objective_cut); | ||
| population_ptr->set_problem_ptr_with_cuts(&problem_with_objective_cut); | ||
| resize_to_new_problem(); | ||
| cutting_plane_added_for_active_run = true; | ||
| raft::copy(population_ptr->weights.cstr_weights.data(), | ||
| fj.cstr_weights.data(), | ||
| population_ptr->weights.cstr_weights.size(), | ||
| solution.handle_ptr->get_stream()); | ||
| raft::copy(population_ptr->weights_with_cuts.cstr_weights.data(), | ||
| fj.cstr_weights.data(), | ||
| fj.cstr_weights.size(), | ||
| solution.handle_ptr->get_stream()); | ||
| } | ||
| population_ptr->update_weights(); | ||
| save_solution_and_add_cutting_plane(population_ptr->best_feasible(), solution, best_objective); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Duplicate cutting plane initialization logic.
Lines 590-603 duplicate the cutting plane setup from handle_cutting_plane_and_weights (lines 540-552). This same pattern also appears in run_fp (lines 635-647). This violates the DRY principle and makes maintenance harder.
As per coding guidelines: Refactor code duplication in solver components (3+ occurrences) into shared utilities.
Consider extracting this into a helper method that both functions can call, similar to how handle_cutting_plane_and_weights is used elsewhere in the file. This would centralize the cutting plane initialization logic and eliminate the redundant weight copies.
Note: This also includes the same double weight copy issue flagged in handle_cutting_plane_and_weights (lines 595-602).
|
🔔 Hi @anandhkb, 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 similar comments
|
🔔 Hi @anandhkb, 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, 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. |
6 participants
This PR extends the objective cut of the problem to the recombiners as well. There is also some cleanup of unused functions.
Pending: benchmark results/comparison.
Not critical for 25.12
Summary by CodeRabbit
New Features
Improvements
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.