Fix thrust tuple includes + more timer checks#902
Fix thrust tuple includes + more timer checks#902rapids-bot[bot] merged 4 commits intoNVIDIA:mainfrom
Conversation
aliceb-nv
added
bug
rgsl888prabhu
added
non-breaking
📝 WalkthroughWalkthroughAdds early time-limit checks in multiple MIP solve stages (diversity presolve, solve init, solver presolve and B&B setup) to short-circuit and return partial solutions when time is exhausted. Also adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
akifcorduk
added
non-breaking
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/src/mip_heuristics/solve.cu`:
- Around line 156-160: When returning early for time limit in
mip_heuristics::solve, the code calls solver.get_solver_stats() which still has
total_solve_time == 0; fix by capturing the stats object, set its
total_solve_time to the timer's elapsed time, and pass that updated stats to
detail::solution_t<i_t,f_t>::get_solution. Concretely: replace the direct
solver.get_solver_stats() call with auto stats = solver.get_solver_stats();
stats.total_solve_time = timer.elapsed_time(); then return
sol.get_solution(false, stats, false) while keeping the existing
timer.check_time_limit(), CUOPT_LOG_INFO and solution construction
(detail::solution_t<i_t, f_t> sol(problem)).
In `@cpp/src/mip_heuristics/solver.cu`:
- Around line 137-143: The early-return on time limit creates a default
solution_t<i_t, f_t> and calls context.problem_ptr->post_process_solution(sol)
without marking it as fully reduced; update both time-limit branches (the
timer_.check_time_limit() blocks that log via CUOPT_LOG_INFO) to call
sol.set_problem_fully_reduced() on the freshly constructed solution before
calling post_process_solution(sol), so post_process_assignment() does not read
an uninitialized assignment and matches the other early-return paths.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver.cucpp/src/pdlp/swap_and_resize_helper.cuhcpp/src/pdlp/utils.cuhcpp/src/utilities/copy_helpers.hppcpp/src/utilities/cuda_helpers.cuh
| if (timer_.check_time_limit()) { | ||
| CUOPT_LOG_INFO("Time limit reached after presolve"); | ||
| solution_t<i_t, f_t> sol(*context.problem_ptr); | ||
| context.stats.total_solve_time = timer_.elapsed_time(); | ||
| context.problem_ptr->post_process_solution(sol); | ||
| return sol; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, find the post_process_solution implementation
rg -n "post_process_solution" --include="*.{cu,cuh,cpp,hpp,h}" -A 5 | head -100Repository: NVIDIA/cuopt
Length of output: 497
🏁 Script executed:
#!/bin/bash
# Find all call sites of post_process_solution and the context around them
rg -n "post_process_solution" --include="*.{cu,cuh,cpp,hpp,h}" -B 2 -A 1Repository: NVIDIA/cuopt
Length of output: 497
🏁 Script executed:
#!/bin/bash
# Check the specific file and lines mentioned in the review
cat -n cpp/src/mip_heuristics/solver.cu | sed -n '100,150p'Repository: NVIDIA/cuopt
Length of output: 2752
🏁 Script executed:
#!/bin/bash
# Also check the other time-limit path mentioned
cat -n cpp/src/mip_heuristics/solver.cu | sed -n '290,310p'Repository: NVIDIA/cuopt
Length of output: 1207
🏁 Script executed:
#!/bin/bash
# Search for post_process_solution method definition using ast-grep
ast-grep --pattern 'post_process_solution($_) {
$$$
}'Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Search for post_process_solution using rg with correct syntax (without --include)
rg "post_process_solution" -t cpp -t cuda -B 2 -A 5Repository: NVIDIA/cuopt
Length of output: 84
🏁 Script executed:
#!/bin/bash
# Find the definition in the problem class
fd -e cu -e cuh -e cpp -e hpp -e h | xargs grep -l "post_process_solution" | head -5Repository: NVIDIA/cuopt
Length of output: 269
🏁 Script executed:
#!/bin/bash
# Count occurrences of the 4-line pattern: sol construction + elapsed_time + post_process + return
# Look for the exact pattern in the file
rg "solution_t.*sol.*context\.problem_ptr" cpp/src/mip_heuristics/solver.cu -A 3 | grep -c "post_process_solution"Repository: NVIDIA/cuopt
Length of output: 57
🏁 Script executed:
#!/bin/bash
# Look for post_process_solution definition in problem files
cat cpp/src/mip_heuristics/problem/problem.cuh | grep -n "post_process_solution" -A 10 | head -30Repository: NVIDIA/cuopt
Length of output: 736
🏁 Script executed:
#!/bin/bash
# Check problem.cu for the implementation
cat cpp/src/mip_heuristics/problem/problem.cu | grep -n "post_process_solution" -A 15 | head -50Repository: NVIDIA/cuopt
Length of output: 816
🏁 Script executed:
#!/bin/bash
# Find all instances of sol construction + post_process_solution pattern in solver.cu
grep -n "solution_t.*sol.*context\.problem_ptr" cpp/src/mip_heuristics/solver.cu -A 3Repository: NVIDIA/cuopt
Length of output: 1440
🏁 Script executed:
#!/bin/bash
# Find the presolve_data.post_process_solution implementation
cat cpp/src/mip_heuristics/problem/presolve_data.cu | grep -n "post_process_solution" -A 20 | head -80Repository: NVIDIA/cuopt
Length of output: 1146
🏁 Script executed:
#!/bin/bash
# Also check the header for the declaration
cat cpp/src/mip_heuristics/problem/presolve_data.cuh | grep -n "post_process_solution" -A 5Repository: NVIDIA/cuopt
Length of output: 454
🏁 Script executed:
#!/bin/bash
# Count the actual occurrences of the 4-line duplication pattern more precisely
# Pattern: solution_t creation + elapsed_time + post_process + return on consecutive lines
awk '/solution_t.*sol.*context\.problem_ptr/ {
getline; line1=$0;
getline; line2=$0;
getline; line3=$0;
getline; line4=$0;
if (line1 ~ /elapsed_time/ && line2 ~ /post_process_solution/ && line3 ~ /return/) {
print NR-4 ": Found pattern"
print $0; print line1; print line2; print line3
}
}' cpp/src/mip_heuristics/solver.cuRepository: NVIDIA/cuopt
Length of output: 367
post_process_solution called on default-constructed solution without set_problem_fully_reduced()
Both time-limit early returns (lines 137–143 and 298–302) call post_process_solution(sol) on a solution_t without calling set_problem_fully_reduced() first, unlike all other early-return paths (lines 100, 119, 126). The post_process_solution implementation accesses solution.assignment in post_process_assignment() on an uninitialized assignment. While this won't break functional correctness (the solution is marked infeasible downstream), this inconsistency should be resolved for clarity and consistency with other code paths.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/src/mip_heuristics/solver.cu` around lines 137 - 143, The early-return on
time limit creates a default solution_t<i_t, f_t> and calls
context.problem_ptr->post_process_solution(sol) without marking it as fully
reduced; update both time-limit branches (the timer_.check_time_limit() blocks
that log via CUOPT_LOG_INFO) to call sol.set_problem_fully_reduced() on the
freshly constructed solution before calling post_process_solution(sol), so
post_process_assignment() does not read an uninitialized assignment and matches
the other early-return paths.
|
/ok to test a15424f |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cpp/src/routing/utilities/check_input.cu (1)
20-20: LGTM — explicitthrust/tuple.hinclude correctly fixes the transitive dependency.The
.hextension is consistent with all other Thrust includes in this file (thrust/equal.h,thrust/extrema.h, etc.), and directly providesthrust::make_tuple/thrust::getused at lines 66, 71, 94, 99, 113, and 118.As an optional forward-looking note: Thrust provides an implementation of
std::tuplepulled in from libcu++, and the CCCL documentation recommends replacingthrust::tuplewithcuda::std::tuple. Migrating the existingthrust::make_tuple/thrust::getusages is out of scope here but worth tracking as a future refactor.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cpp/src/routing/utilities/check_input.cu` at line 20, The transitive dependency on thrust::make_tuple / thrust::get was missing; add an explicit include of thrust/tuple.h to cpp/src/routing/utilities/check_input.cu so the file directly provides thrust::make_tuple and thrust::get (used at the sites around lines referencing uses at 66, 71, 94, 99, 113, 118); no further changes to uses of thrust::tuple are required now (migration to cuda::std::tuple can be considered later).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@cpp/src/routing/utilities/check_input.cu`:
- Line 20: The transitive dependency on thrust::make_tuple / thrust::get was
missing; add an explicit include of thrust/tuple.h to
cpp/src/routing/utilities/check_input.cu so the file directly provides
thrust::make_tuple and thrust::get (used at the sites around lines referencing
uses at 66, 71, 94, 99, 113, 118); no further changes to uses of thrust::tuple
are required now (migration to cuda::std::tuple can be considered later).
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cpp/src/mip_heuristics/solve.cucpp/src/routing/utilities/check_input.cu
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/mip_heuristics/solve.cu
|
/merge |
4 participants
This PR fixes compilation by adding thrust/tuple.hpp includes to files that previously relied on transitive includes for the thrust::tuple typedef.
Timer checks are also added to ensure timeouts are respected more closely when they occur within the presolve phases.
Description
Issue
Checklist
Summary by CodeRabbit
Bug Fixes
Chores