Fix adj_list check bug and improve clique cuts - #1386
Conversation
Brings in upstream's unified OpenMP threading model (PR NVIDIA#1099) and other fixes (NVIDIA#1206 concurrent LP exception cleanup, NVIDIA#1214/NVIDIA#1216 destruction/ capture fixes) while preserving local work on the cut and clique stack. Conflict resolution highlights: - Drop std::future/std::async clique flow; adopt upstream's omp task + omp_atomic_t<bool> signal_extend pattern. - Drop modify_problem parameter from find_initial_cliques (we already removed the code that consumed it); adapt the omp-task call site in branch_and_bound::solve accordingly. - Take upstream's [this, &population] capture for the root-LP CPUFJ improvement callback; the new omp taskwait-before-destruction guarantee makes the prior context-lifetime fix unnecessary. - Take upstream's do_cut_pass refactor of the per-pass LP resolve loop; move our per-pass root_lp_with_cuts publish into do_cut_pass so the benchmark metric is still updated on early exits. - Keep our out-of-line omp_mutex_t definitions in omp_helpers.cpp; the enhanced omp_atomic_t with std::memory_order is taken from upstream.
generate_clique_cuts() used a hand-rolled emptiness test on only first + addtl_cliques, so it early-exited and skipped separation when only small_clique_adj (the small/adjacency-list cliques) was populated. Use the canonical clique_table_t::empty(), which also accounts for small_clique_adj, so those smaller clique cuts are generated. Signed-off-by: akif <akifcorduk@gmail.com>
|
/ok to test 98b1ee2 |
| f_t cut_generation_start_time = tic(); | ||
| i_t cut_pool_size = 0; | ||
| f_t cut_generation_start_time = tic(); | ||
| auto publish_cut_generation_time = [&](bool force_time_limit_value = false) { |
There was a problem hiding this comment.
I think it is useful to be able to track cut generation time, but i am commenting on this to discuss.
There was a problem hiding this comment.
Couldn't we get away with just storing the starting time and ending time of cuts? If anyone queries the current cut time while we're in the middle of the cut loop, it's just a matter of doing return toc(start_time) instead of return end_time
But agreed, definitely useful!
There was a problem hiding this comment.
Most of these information is available in the logs, no? You only need the benchmark_info struct to pass to the run_mip file, right?
📝 WalkthroughWalkthroughThis PR adds MIPLIB gap-statistic reporting with root-LP benchmarking instrumentation and refactors clique adjacency from hash-based to CSR-backed storage. The solver now tracks root LP objectives before and after cut generation; branch-and-bound publishes these metrics alongside cut-generation timing. A new MIPLIB header provides instance lookup, optimum catalogs, and formatted gap-stat output. The clique table switches to compact CSR maps for adjacency, refactors extension as a work-budget-driven scheduler, and updates cut generation to handle complement pairs in a dedicated first pass. Branch-and-bound signatures are updated to match. ChangesRoot LP benchmark metrics and MIPLIB reporting
Clique table storage and cut-generation refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Infer (1.2.0)cpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.cpp:8:10: fatal error: 'branch_and_bound/branch_and_bound.hpp' file not found ... [truncated 1187 characters] ... nclude" cpp/src/cuts/cuts.cppcpp/src/cuts/cuts.cpp:8:10: fatal error: 'cuts/cuts.hpp' file not found ... [truncated 1047 characters] ... clang/install/lib/clang/18/include" benchmarks/linear_programming/cuopt/run_mip.cppIn file included from benchmarks/linear_programming/cuopt/run_mip.cpp:9: ... [truncated 2200 characters] ... -50 Comment |
|
@coderabbitai help |
ChatThere are 3 ways to chat with CodeRabbit:
CodeRabbit commands
Other keywords and placeholders
Status, support, documentation and community
|
|
@coderabbitai full review |
✅ Action performedFull review finished. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/linear_programming/cuopt/miplib2017_optima.hpp`:
- Line 25: Replace the lone "`#pragma` once" in the header with a traditional
include guard: remove "`#pragma` once", add an `#ifndef` / `#define` pair using a
unique uppercase macro (e.g.
BENCHMARKS_LINEAR_PROGRAMMING_CUOPT_MIPLIB2017_OPTIMA_HPP) at the top of the
file and close with a matching `#endif` with a trailing comment at the end of the
file; ensure the macro name matches the header (miplib2017_optima.hpp) and is
unique across the repo.
In `@cpp/include/cuopt/linear_programming/mip/solver_settings.hpp`:
- Around line 33-40: The comment incorrectly says the stored LP values are in
the "solver objective sense" which encourages mixing objective spaces; update
the text to state that root_lp_no_cuts and root_lp_with_cuts are published via
compute_user_objective(...) (see branch_and_bound.cpp) and therefore represent
user-space/objective values compatible with MIPLIB comparisons; replace the
phrase "in B&B's solver objective sense" with wording that they are user-space
objectives (or already converted by compute_user_objective) and clarify
quiet_NaN() meaning remains the same.
In `@cpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cuh`:
- Around line 83-95: csr_var_map_t's slice helpers can compute pointers into an
empty indices vector causing UB; change slice_begin(i_t v) and slice_end(i_t v)
to first check if indices.empty() and return nullptr when empty, and update
slice_contains(i_t v, i_t value) to treat a nullptr range as empty (return
false) before calling std::binary_search; keep avg_slice_size() as-is. Also
ensure callers like fill_var_clique_maps / finalize_from_unsorted_pairs /
get_adj_set_of_var / check_adjacency rely on slice_contains or handle a nullptr
range as an empty slice.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9be46e1d-b244-43be-ae7b-4ad7221fb149
📒 Files selected for processing (12)
benchmarks/linear_programming/cuopt/miplib2017_optima.hppbenchmarks/linear_programming/cuopt/run_mip.cppcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cucpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cuhcpp/src/mip_heuristics/solver.cucpp/src/utilities/omp_helpers.hppcpp/tests/mip/cuts_test.cu
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/src/utilities/omp_helpers.hpp (1)
31-31: ⚡ Quick winRemove redundant include.
<utility>is already included unconditionally at line 11, so the guarded include here is redundant.♻️ Proposed fix
`#include` <omp.h> `#include` <memory> -#include <utility> namespace cuopt {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/utilities/omp_helpers.hpp` at line 31, Remove the redundant guarded include of <utility> in omp_helpers.hpp — the header is already included unconditionally earlier (the "`#include` <utility>" at the top), so delete the second "`#include` <utility>" occurrence to avoid duplication and keep includes clean.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 2680-2692: The current lambda publish_cut_generation_time
overwrites the measured cut_generation_time with settings_.time_limit whenever
the measured time exceeds the global solver time limit, which overreports
because cut_generation_start_time is taken after earlier solver work; change the
logic so you only force-set cut_generation_time to settings_.time_limit when
force_time_limit_value is true (remove or tighten the condition "||
cut_generation_time > settings_.time_limit"), leaving the actual measured
cut_generation_time otherwise; apply the same change to the analogous code at
the other location (the block around the lines referenced 2743-2745) and keep
references to publish_cut_generation_time, cut_generation_start_time,
force_time_limit_value, and settings_.time_limit.
---
Nitpick comments:
In `@cpp/src/utilities/omp_helpers.hpp`:
- Line 31: Remove the redundant guarded include of <utility> in omp_helpers.hpp
— the header is already included unconditionally earlier (the "`#include`
<utility>" at the top), so delete the second "`#include` <utility>" occurrence to
avoid duplication and keep includes clean.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 7f53375c-0094-41f7-8499-7263553edbda
📒 Files selected for processing (12)
benchmarks/linear_programming/cuopt/miplib2017_optima.hppbenchmarks/linear_programming/cuopt/run_mip.cppcpp/include/cuopt/linear_programming/mip/solver_settings.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/cuts/cuts.cppcpp/src/cuts/cuts.hppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cucpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cuhcpp/src/mip_heuristics/solver.cucpp/src/utilities/omp_helpers.hppcpp/tests/mip/cuts_test.cu
|
/ok to test |
@akifcorduk, there was an error processing your request: See the following link for more information: https://docs.gha-runners.nvidia.com/cpr/e/1/ |
|
/ok to test b3e0eef |
|
/ok to test bd4228d |
aliceb-nv
left a comment
aliceb-nv
left a comment
There was a problem hiding this comment.
LGTM, thanks a ton Akif! Only minor nits :)
| f_t cut_generation_start_time = tic(); | ||
| i_t cut_pool_size = 0; | ||
| f_t cut_generation_start_time = tic(); | ||
| auto publish_cut_generation_time = [&](bool force_time_limit_value = false) { |
There was a problem hiding this comment.
Couldn't we get away with just storing the starting time and ending time of cuts? If anyone queries the current cut time while we're in the middle of the cut loop, it's just a matter of doing return toc(start_time) instead of return end_time
But agreed, definitely useful!
| if (settings_.benchmark_info_ptr != nullptr) { | ||
| settings_.benchmark_info_ptr->root_lp_with_cuts = | ||
| static_cast<double>(compute_user_objective(original_lp_, root_objective_)); | ||
| } |
There was a problem hiding this comment.
Is there any scenario where benchmark_info_ptr != nullptr? We're just computing simple scalars, I think it'd make the code easier to read if it was just a regular reference / non-null ptr
There was a problem hiding this comment.
Also nit but I tend to prefer implicit casts / C-style casts for non-critical things :) static_cast(...) is kinda harder to read
There was a problem hiding this comment.
The lambda encapsulates the logic of cut time and writing to benchmark info together. But okay let me remove the lambda and handle it on return blocks.
There was a problem hiding this comment.
In production code, it benchmark_info_ptr != nullptr doesn't happen but it happens when we run benchmark with run_mip. I think it is irrelevant to the user and I wanted to separate it from the rest of the user settings.
For the cast, AI did the cast, I also prefer C style casts for simple static casts. Will corrrect it.
| for (const i_t* it = var_clique_first.slice_begin(var_idx); | ||
| it != var_clique_first.slice_end(var_idx); |
There was a problem hiding this comment.
Very minor nit: maybe we could have the returned slice be represented as std::span<> (or a custom type with begin() and end()0, so that we can use range-for loops directly
There was a problem hiding this comment.
Good idea thanks!
| f_t cut_generation_start_time = tic(); | ||
| i_t cut_pool_size = 0; | ||
| f_t cut_generation_start_time = tic(); | ||
| auto publish_cut_generation_time = [&](bool force_time_limit_value = false) { |
There was a problem hiding this comment.
Most of these information is available in the logs, no? You only need the benchmark_info struct to pass to the run_mip file, right?
|
Mostly nitpicks. Thanks, Akif! |
|
|
/ok to test dd69485 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@benchmarks/linear_programming/cuopt/miplib2017_bks.hpp`:
- Line 25: Replace the single-line "`#pragma` once" with a traditional include
guard: remove the pragma, add a unique macro guard name (e.g., use
BENCHMARKS_LINEAR_PROGRAMMING_CUOPT_MIPLIB2017_BKS_HPP or MIPLIB2017_BKS_HPP)
with an `#ifndef` / `#define` pair at the top of the header and a matching `#endif`
comment at the end of the file to close the guard; ensure the macro name is
all-caps and matches exactly in both places.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 8529a91c-32fb-483d-bb7c-5baf24f770e8
📒 Files selected for processing (7)
benchmarks/linear_programming/cuopt/miplib2017_bks.hppbenchmarks/linear_programming/cuopt/run_mip.cppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/cuts/cuts.cppcpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cucpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cuhcpp/src/utilities/omp_helpers.hpp
🚧 Files skipped from review as they are similar to previous changes (5)
- benchmarks/linear_programming/cuopt/run_mip.cpp
- cpp/src/utilities/omp_helpers.hpp
- cpp/src/cuts/cuts.cpp
- cpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cuh
- cpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cu
|
/merge |
3 participants
Improve clique cut generation by simplifing the conflict-graph clique table and fix complement-pair handling in clique cut construction drop the paired variable to form a valid fixing cut; detect contradictory variable/complement cliques as infeasible.
Fixes a bug of skipping the small adjacency-list cliques, now we use clique_table_t::empty() function to check all clique data structures.
The mip gap or gap glosed doesn't change significantly but we add on average 1.5 optimal solutions. Tested on 2 batches for main and this PR.