Add cooperative cancel for LP/MIP solves - #1693
Conversation
|
engine team: I did this with AI so please check carefully and treat this as a rough cut -- there very well may be improvements that can be made. I reviewed and kept the changes as minimal as possible. I will call out a few places specifically with comments where I'm uncertain what the best approach is. The basic idea is to test a cancel flag, settable from a thread in the same process as the solver, every place that a time limit or iteration limit is currently checked. Much of the code change is the modification of timer objects which now take a cancel flag at initialization. Also, please feel free to take over this PR and carry it across the goal line :) |
|
/ok to test 99d01ec |
| break; | ||
| } | ||
|
|
||
| if (settings_.cancel_requested != nullptr && |
There was a problem hiding this comment.
The code here matches the behavior in the timer limit case afaik. Maybe they can be combined?
| // Wait for the root relaxation solution to be sent by the diversity manager or dual simplex | ||
| while (!root_crossover_solution_set_.load(std::memory_order_acquire) && | ||
| *get_root_concurrent_halt() == 0) { | ||
| // Dual may early-return on time/cancel without crossover; also poll limits |
There was a problem hiding this comment.
In testing I hit a point where after a cancel, a MIP would never complete. It seemed to be the case that one of the threads in the OpenMP team was halted while the other wasn't, and so the join never completed. After drilling in to the failure with the agent, this seemed like the fix and no hangs were ever observed afterward.
| // Concurrent root MIP: dual and barrier/PDLP/crossover race. On any phase-2 | ||
| // exit (optimal, time limit, cancel→TIME_LIMIT early return, etc.), raise | ||
| // concurrent_halt so waiters in solve_root_relaxation are not stuck forever | ||
| // when dual returns before the success-path epilogue below. |
There was a problem hiding this comment.
related to the same deadlock mentioned above
|
|
||
| int cancel_job(const std::string& job_id, JobStatus& job_status_out, std::string& message) | ||
| { | ||
| std::lock_guard<std::mutex> lock(tracker_mutex); |
There was a problem hiding this comment.
this was changed because we need the ability to unlock, since we return the cancel status immediately but launch a background thread to do a SIGKILL if the worker fails to cancel the job.
📝 WalkthroughWalkthroughThe change adds cooperative cancellation across public solver APIs, timers, LP and MIP algorithms, branch-and-bound, gRPC workers, and test suites. Running jobs now propagate cancellation flags and use graceful termination before process escalation. ChangesCooperative cancellation
Estimated code review effort: 5 (Critical) | ~120 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
cpp/src/pdlp/solve.cu (1)
691-730: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRemap cancellation in the distributed PDLP result.
pdlp_solver_t::check_limits()already readssettings_.cancel_requested, so the distributed solve exits without waiting fortime_limit. However, it returnsTimeLimitfor both cancellation and timeout. Before returning fromsolve_lp_distributed_from_mps, map the status topdlp_termination_status_t::Cancelledwhensettings_resolved.cancel_requestedis set. The FP32 path uses the caller’s cancellation-aware timer, so copyingcancel_requestedintofsdoes not change its current behavior.🤖 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/pdlp/solve.cu` around lines 691 - 730, Update solve_lp_distributed_from_mps to remap the distributed solver result to pdlp_termination_status_t::Cancelled when settings_resolved.cancel_requested is set and the solver reports the cancellation-as-TimeLimit status. Preserve TimeLimit for genuine timeouts, and do not alter the existing FP32 settings-copy behavior.cpp/src/branch_and_bound/branch_and_bound.cpp (2)
3918-4994: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftDeterministic B&B never polls the cancellation flag.
The non-deterministic path (
plunge_with, lines 1741-1748) and root-relaxation path (lines 3365-3378) explicitly pollsettings_.cancel_requestedand reportmip_status_t::CANCELLED. The deterministic path does not.deterministic_sync_callbackdecidesdeterministic_global_termination_status_fromtime_limit,work_limit, and gap closure only;run_deterministic_bfs_loop,solve_node_deterministic,run_deterministic_diving_loop, anddeterministic_divecontain no cancellation check either.If a caller sets
settings_.deterministic = trueand requests cancellation, the deterministic solve keeps running untiltime_limit/work_limit/completion. This defeats the PR's goal of cooperative cancellation for the deterministic solve mode.Add a cancellation check in
deterministic_sync_callback, ahead of the time-limit check, to respect the documented priority (Cancelled > ConcurrentHalt > TimeLimit > IterationLimit):🐛 Proposed fix location (deterministic_sync_callback)
+ if (cuopt::cancel_flag_set(settings_.cancel_requested)) { + deterministic_global_termination_status_ = mip_status_t::CANCELLED; + } if (toc(exploration_stats_.start_time) > settings_.time_limit) { deterministic_global_termination_status_ = mip_status_t::TIME_LIMIT; }Based on learnings from the review guide: "Check that cancellation polling does not introduce excessive synchronization or hot-path overhead" and "correct status remapping across LP/MIP phases and early-return paths"; the deterministic sync point is the natural, already-existing hot-path-safe location for this check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3918 - 4994, Update deterministic_sync_callback to check settings_.cancel_requested before the existing time-limit evaluation and set deterministic_global_termination_status_ to mip_status_t::CANCELLED when requested. Preserve the existing termination-priority ordering so cancellation takes precedence over time and work limits, while continuing to use the existing sync-point shutdown flow.
3487-3497: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReturn
CANCELLEDwhen cancellation occurs during cut generation.
dual_status_thas noCANCELLEDvalue, so phase 2 maps cancellation toTIME_LIMIT. Checkcuopt::cancel_flag_set(settings_.cancel_requested)in the cut-pass loop anddo_cut_passbefore limit-related early returns, and returnmip_status_t::CANCELLED, matching root handling.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cpp/src/branch_and_bound/branch_and_bound.cpp` around lines 3487 - 3497, Update the cut-generation flow around the cut-pass loop and do_cut_pass to check cuopt::cancel_flag_set(settings_.cancel_requested) before time-limit early returns. When cancellation is set, finalize the solution as needed and return mip_status_t::CANCELLED; preserve TIME_LIMIT for actual time-limit expiry and match the existing root cancellation handling.cpp/src/pdlp/pdlp.cu (1)
615-654: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRemap cancellation per batch entry
When
batch_mode_is true,check_limitsreturns one termination status per climber. The unconditional remap insolve_lpcallsset_termination_status(Cancelled), but that method asserts that the solution has exactly one termination entry. A cancelled batch therefore returns an error solution instead ofCancelledstatuses. Skip the scalar setter for batch results and remap the batch status vector element-wise.🤖 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/pdlp/pdlp.cu` around lines 615 - 654, Update the cancellation remapping in solve_lp so batch results do not pass through the scalar set_termination_status(Cancelled) path, which requires exactly one termination entry. For batch_mode_ results, remap every entry in the termination-status vector to Cancelled; retain the existing scalar setter for non-batch solutions.
🧹 Nitpick comments (7)
python/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.py (1)
269-274: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the new
workersparameter in the docstring.The docstring explains
server_log_pathbut notworkers. Add one line so callers know the default is a single worker.As per path instructions: "Docstring CONTENT on new public APIs — params, returns, raises".
🤖 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 `@python/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.py` around lines 269 - 274, Update the start_grpc_server docstring to document the workers parameter, stating that it controls the worker count and defaults to one worker; leave the existing parameter descriptions unchanged.Source: Path instructions
cpp/tests/mip/cooperative_cancel_test.cu (2)
33-33: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename the
clockalias.
clockis also the name of the C standard function<ctime>::clock. The alias is confined to an unnamed namespace, so it is legal, but it can create ambiguity for later unqualifiedclock()calls in this translation unit. A name such assteady_clock_tavoids that.🤖 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/tests/mip/cooperative_cancel_test.cu` at line 33, Rename the `clock` type alias to an unambiguous name such as `steady_clock_t`, and update all references to the alias in this translation unit while preserving its use of `std::chrono::steady_clock`.
96-104: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winHandle instances that finish before the cancel flag is set.
cancel_after_sis 2.0 seconds. Some listed instances (for examplegen-ip054andneos5) can reachOptimalin under 2 seconds on a fast GPU. In that caseEXPECT_GE(secs, 1.8)and theCancelledstatus check both fail, and the failure is environment-dependent rather than a real defect. Treat an early natural termination as a skip for that instance instead of a failure.Line 104 also hard-fails when fewer than three datasets are present. The LP test at Line 184 uses
GTEST_SKIP()for the same condition. Use the same behavior in both tests so a partial dataset download does not report a false failure.💚 Proposed change
ASSERT_TRUE(solution.has_value()) << rel << " produced no solution object"; - EXPECT_GE(secs, cancel_after_s * 0.9) << rel << " finished before cancel was set"; + if (secs < cancel_after_s * 0.9) { + GTEST_LOG_(WARNING) << rel << " finished in " << secs << "s, before cancel was set; skipping"; + --ran; + continue; + } EXPECT_LT(secs, max_total_s) << rel << " did not unwind promptly after cancel (" << secs << "s)"; EXPECT_EQ(solution->get_termination_status(), mip_termination_status_t::Cancelled) << rel << " status=" << static_cast<int>(solution->get_termination_status()) << " after " << secs << "s"; } - ASSERT_GE(ran, 3) << "Need at least a few MIP datasets under RAPIDS_DATASET_ROOT_DIR"; + if (ran == 0) { + GTEST_SKIP() << "No MIP datasets found under RAPIDS_DATASET_ROOT_DIR"; + }🤖 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/tests/mip/cooperative_cancel_test.cu` around lines 96 - 104, Update the cooperative-cancellation test loop around solution termination checks to skip an instance when it naturally finishes before the cancellation delay, rather than asserting cancellation timing and status; preserve the existing assertions for instances still running when cancellation is set. Replace the hard ASSERT_GE(ran, 3) dataset-count failure with the same GTEST_SKIP() behavior used by the LP test near line 184, while retaining the minimum-dataset condition.cpp/src/grpc/server/grpc_job_management.cpp (1)
322-353: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider making the cooperative grace period configurable.
kGraceis a compile-time constant of 120 seconds. Deployments with long GPU kernels or with tighter SLAs cannot tune it. The server already carries aconfigobject, so a config field would allow tuning without a rebuild. The current logic (value captures, slot re-validation,ESRCHchecks) is correct as written.🤖 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/grpc/server/grpc_job_management.cpp` around lines 322 - 353, Make the cooperative cancellation grace period configurable through the existing config object instead of the compile-time kGrace constant in the detached cancellation thread. Add or reuse a duration-valued configuration field, capture its value before launching the thread, and use it to compute the deadline while preserving the existing slot re-validation and ESRCH checks.python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py (2)
1370-1375: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unused intermediate-log path.
trial_shutdowndiscardsintermediate_timeoutandmin_intermediate_linesat Line 1446.wait_for_solver_intermediatesis defined but never called, and the--intermediate-timeoutand--min-intermediate-linesCLI flags now have no effect. Delete the function, the two parameters, and the two flags, or restore the pre-SIGINT intermediate check that they were written for.I can produce that cleanup patch if you want it.
Also applies to: 1445-1446
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around lines 1370 - 1375, Remove the unused intermediate-log path: delete wait_for_solver_intermediates, remove intermediate_timeout and min_intermediate_lines from trial_shutdown and related call sites, and remove the --intermediate-timeout and --min-intermediate-lines CLI flags and handling. Do not restore the obsolete pre-SIGINT intermediate check.
293-302: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valueAvoid
preexec_fninServerHandle.start().The loop can call
ServerHandle.start()whileTrafficGeneratoris running.preexec_fnis unsafe in multithreaded processes and can deadlock betweenfork()andexec(). Use parent-side cleanup, or move parent-death handling into the launched server or a wrapper.🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around lines 293 - 302, Remove preexec_fn=_set_pdeathsig from ServerHandle.start() and replace the parent-death handling with a safe alternative that does not execute Python between fork and exec, such as parent-side cleanup or server/wrapper-side handling. Preserve the existing subprocess output, environment, session, and text-buffering behavior.python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py (1)
41-52: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShare the server log-pattern definitions instead of duplicating them. Both files define the same cooperative-cancel, legacy-kill, fallback-kill, and worker-restart regexes against the same C++ log messages. The copies have already diverged, and a future change to a server log string will silently break only one of them.
python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py#L41-L52: move these four patterns into the sharedgrpc_server_fixturesmodule and import them here; this copy carries the extrare.DOTALLflag that the other lacks.python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py#L62-L79: import the same shared patterns instead of redefiningCANCEL_COOP_RE,CANCEL_KILL_RE,CANCEL_FALLBACK_KILL_RE, andRESTARTED_WORKER_RE.🤖 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 `@python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py` around lines 41 - 52, Move the four shared log regex definitions into the grpc_server_fixtures module, preserving the canonical patterns without the extra re.DOTALL flag. In python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py lines 41-52, remove the local definitions and import the shared symbols; in python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py lines 62-79, likewise remove the local CANCEL_COOP_RE, CANCEL_KILL_RE, CANCEL_FALLBACK_KILL_RE, and RESTARTED_WORKER_RE definitions and import them from grpc_server_fixtures.
🤖 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/include/cuopt/mathematical_optimization/mip/solver_settings.hpp`:
- Around line 187-190: Update the solve_mip cancellation handling so the pointee
behind settings.cancel_requested is cleared before solve_mip returns, including
normal completion, early-return paths, and exception exits. Keep
cancel_requested as a non-owning pointer in solver_settings.hpp, but make the
implementation reset the referenced atomic<bool> value rather than the pointer
itself, and update the related comment/documentation to state that the pointee
value is cleared.
In `@cpp/src/dual_simplex/solve.hpp`:
- Around line 36-38: Update the branch-and-bound node and cut-pass retry
handling to recognize lp_status_t::CANCELLED before fallback conversion, and
return mip_status_t::CANCELLED directly. Preserve existing mappings for all
other statuses and avoid treating cancellation as dual_status_t::NUMERICAL.
In `@cpp/src/mip_heuristics/presolve/bounds_presolve.cu`:
- Line 235: Update all three shared bound_update_loop implementations so each
checks timer.check_time_limit() immediately after every GPU bound update and
before classifying a false result as NO_UPDATE or CONVERGENCE. Apply this at
bounds_presolve.cu:235-235, 248-248, and 263-263;
load_balanced_bounds_presolve.cu:630-630, 642-642, and 671-671; and
multi_probe.cu:383-383 and 397-397, preserving cancellation propagation so
normal post-loop or caller work is skipped.
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp`:
- Around line 748-761: Thread the shared cancel_requested flag through the
Papilo presolve wrapper functions and assign
PresolveOptions::early_exit_callback to a callback that observes it. Ensure the
callback is configured before presolve begins while preserving the existing
probing badge limits, and add a regression test that requests cancellation
during Papilo presolve and verifies early termination.
In `@cpp/src/mip_heuristics/relaxed_lp/relaxed_lp.cuh`:
- Line 28: Initialize the relaxed-LP cancellation pointer in the
feasibility-pump, constraint-propagation, and lower-bound constraint-propagation
settings objects from the owning MIP context, reusing
context.settings.cancel_requested. Ensure every relaxed-LP invocation receives
these populated settings, and add a regression test covering cancellation during
relaxed-LP execution in feasibility pump.
In `@cpp/src/mip_heuristics/solve.cu`:
- Line 382: Apply cancellation precedence in cpp/src/mip_heuristics/solve.cu at
382-382 by checking the timer immediately after construction and returning
Cancelled before setup, heuristics, or presolve. At 663-670, ensure every
presolve terminal-status return checks cancellation first or uses a shared
cancellation-aware finalization path. At 804-810, remap the status to Cancelled
before log_detailed_summary() and write_to_sol_file() so the return value, logs,
and solution output agree.
In `@cpp/src/utilities/timer.hpp`:
- Line 23: Mark the single-argument timer_t constructor that delegates to
timer_t(double, nullptr) as explicit to prevent implicit double-to-timer_t
conversions, while preserving its existing delegation behavior.
In `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py`:
- Around line 641-658: Update the afiro dataset handling around
fetch_and_check_result so a missing afiro_original.mps is treated as skipped
rather than setting afiro_ok to False. Preserve result validation when the file
exists, and ensure the final ok calculation does not fail solely because the
optional dataset is absent.
In `@python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py`:
- Around line 47-51: Update the _CANCEL_FALLBACK_KILL_RE pattern by removing
re.DOTALL so its wildcard cannot cross newline boundaries. Keep the existing
message structure and capture groups, ensuring matches remain limited to a
single server log line and unrelated lines do not satisfy the pattern.
---
Outside diff comments:
In `@cpp/src/branch_and_bound/branch_and_bound.cpp`:
- Around line 3918-4994: Update deterministic_sync_callback to check
settings_.cancel_requested before the existing time-limit evaluation and set
deterministic_global_termination_status_ to mip_status_t::CANCELLED when
requested. Preserve the existing termination-priority ordering so cancellation
takes precedence over time and work limits, while continuing to use the existing
sync-point shutdown flow.
- Around line 3487-3497: Update the cut-generation flow around the cut-pass loop
and do_cut_pass to check cuopt::cancel_flag_set(settings_.cancel_requested)
before time-limit early returns. When cancellation is set, finalize the solution
as needed and return mip_status_t::CANCELLED; preserve TIME_LIMIT for actual
time-limit expiry and match the existing root cancellation handling.
In `@cpp/src/pdlp/pdlp.cu`:
- Around line 615-654: Update the cancellation remapping in solve_lp so batch
results do not pass through the scalar set_termination_status(Cancelled) path,
which requires exactly one termination entry. For batch_mode_ results, remap
every entry in the termination-status vector to Cancelled; retain the existing
scalar setter for non-batch solutions.
In `@cpp/src/pdlp/solve.cu`:
- Around line 691-730: Update solve_lp_distributed_from_mps to remap the
distributed solver result to pdlp_termination_status_t::Cancelled when
settings_resolved.cancel_requested is set and the solver reports the
cancellation-as-TimeLimit status. Preserve TimeLimit for genuine timeouts, and
do not alter the existing FP32 settings-copy behavior.
---
Nitpick comments:
In `@cpp/src/grpc/server/grpc_job_management.cpp`:
- Around line 322-353: Make the cooperative cancellation grace period
configurable through the existing config object instead of the compile-time
kGrace constant in the detached cancellation thread. Add or reuse a
duration-valued configuration field, capture its value before launching the
thread, and use it to compute the deadline while preserving the existing slot
re-validation and ESRCH checks.
In `@cpp/tests/mip/cooperative_cancel_test.cu`:
- Line 33: Rename the `clock` type alias to an unambiguous name such as
`steady_clock_t`, and update all references to the alias in this translation
unit while preserving its use of `std::chrono::steady_clock`.
- Around line 96-104: Update the cooperative-cancellation test loop around
solution termination checks to skip an instance when it naturally finishes
before the cancellation delay, rather than asserting cancellation timing and
status; preserve the existing assertions for instances still running when
cancellation is set. Replace the hard ASSERT_GE(ran, 3) dataset-count failure
with the same GTEST_SKIP() behavior used by the LP test near line 184, while
retaining the minimum-dataset condition.
In `@python/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.py`:
- Around line 269-274: Update the start_grpc_server docstring to document the
workers parameter, stating that it controls the worker count and defaults to one
worker; leave the existing parameter descriptions unchanged.
In `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py`:
- Around line 1370-1375: Remove the unused intermediate-log path: delete
wait_for_solver_intermediates, remove intermediate_timeout and
min_intermediate_lines from trial_shutdown and related call sites, and remove
the --intermediate-timeout and --min-intermediate-lines CLI flags and handling.
Do not restore the obsolete pre-SIGINT intermediate check.
- Around line 293-302: Remove preexec_fn=_set_pdeathsig from
ServerHandle.start() and replace the parent-death handling with a safe
alternative that does not execute Python between fork and exec, such as
parent-side cleanup or server/wrapper-side handling. Preserve the existing
subprocess output, environment, session, and text-buffering behavior.
In `@python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py`:
- Around line 41-52: Move the four shared log regex definitions into the
grpc_server_fixtures module, preserving the canonical patterns without the extra
re.DOTALL flag. In
python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py lines 41-52,
remove the local definitions and import the shared symbols; in
python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py lines
62-79, likewise remove the local CANCEL_COOP_RE, CANCEL_KILL_RE,
CANCEL_FALLBACK_KILL_RE, and RESTARTED_WORKER_RE definitions and import them
from grpc_server_fixtures.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 5d7c6f36-6678-48ff-9cb8-a5c0487c6ac6
📒 Files selected for processing (49)
cpp/include/cuopt/mathematical_optimization/constants.hcpp/include/cuopt/mathematical_optimization/mip/solver_settings.hppcpp/include/cuopt/mathematical_optimization/mip/solver_solution.hppcpp/include/cuopt/mathematical_optimization/pdlp/solver_settings.hppcpp/include/cuopt/mathematical_optimization/pdlp/solver_solution.hppcpp/src/barrier/barrier.cucpp/src/barrier/sparse_cholesky.cuhcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/phase2.cppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/right_looking_lu.cppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/solve.hppcpp/src/grpc/server/grpc_job_management.cppcpp/src/grpc/server/grpc_worker.cppcpp/src/grpc/server/grpc_worker_infra.cppcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/diversity/recombiners/bound_prop_recombiner.cuhcpp/src/mip_heuristics/diversity/recombiners/fp_recombiner.cuhcpp/src/mip_heuristics/feasibility_jump/early_cpufj.cucpp/src/mip_heuristics/feasibility_jump/early_cpufj.cuhcpp/src/mip_heuristics/feasibility_jump/feasibility_jump.cucpp/src/mip_heuristics/local_search/feasibility_pump/feasibility_pump.cucpp/src/mip_heuristics/local_search/local_search.cucpp/src/mip_heuristics/presolve/bounds_presolve.cucpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cucpp/src/mip_heuristics/presolve/load_balanced_bounds_presolve.cucpp/src/mip_heuristics/presolve/multi_probe.cucpp/src/mip_heuristics/presolve/third_party_presolve.cppcpp/src/mip_heuristics/relaxed_lp/relaxed_lp.cucpp/src/mip_heuristics/relaxed_lp/relaxed_lp.cuhcpp/src/mip_heuristics/solution/solution.cucpp/src/mip_heuristics/solution/solution.cuhcpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver.cucpp/src/mip_heuristics/solver_solution.cucpp/src/pdlp/pdlp.cucpp/src/pdlp/solve.cucpp/src/pdlp/solver_solution.cucpp/src/utilities/solve_limits.hppcpp/src/utilities/timer.hppcpp/tests/mip/CMakeLists.txtcpp/tests/mip/cooperative_cancel_test.cupython/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.pypython/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.pypython/cuopt/cuopt/tests/linear_programming/test_grpc_client.py
| // Optional cooperative cancel (level-triggered). Non-owning. Cleared by | ||
| // solve_mip on return. Set true from another thread to request early exit. | ||
| std::atomic<bool>* cancel_requested{nullptr}; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate solve_mip implementations and check for cancel_requested reset behavior.
set -euo pipefail
rg -n -C6 'cancel_requested' cpp/src/mip_heuristics/solve.cu cpp/src/branch_and_bound/branch_and_bound.cpp 2>/dev/null || true
rg -n 'cancel_requested->store|\*.*cancel_requested\s*=' cpp/src --type=cpp --type=cuda -g '*.cu' -g '*.cpp' -g '*.hpp'Repository: NVIDIA/cuopt
Length of output: 11252
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- solve_mip declarations and definitions ---'
rg -n -C8 'solve_mip|cancel_requested' cpp/include cpp/src --glob '*.{hpp,h,cpp,cu,cuh,inl}' | \
rg -n 'solve_mip|cancel_requested|catch|return' | head -n 500
printf '%s\n' '--- complete solve.cu control-flow regions ---'
sed -n '200,270p' cpp/src/mip_heuristics/solve.cu
sed -n '630,830p' cpp/src/mip_heuristics/solve.cu
sed -n '830,930p' cpp/src/mip_heuristics/solve.cu
printf '%s\n' '--- all writes or helper implementations involving the cancellation flag ---'
rg -n -C4 'cancel_requested|cancel_flag_set|solve_limit_reached' cpp/include cpp/src --glob '*.{hpp,h,cpp,cu,cuh,inl}' | \
rg -v 'load\(|get_cancel|cancel_flag_set|solve_limit_reached' | head -n 500Repository: NVIDIA/cuopt
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- solve_mip_helper and solve_mip implementations ---'
sed -n '320,355p' cpp/src/mip_heuristics/solve.cu
sed -n '800,885p' cpp/src/mip_heuristics/solve.cu
sed -n '885,980p' cpp/src/mip_heuristics/solve.cu
printf '%s\n' '--- cancellation writes across the repository ---'
rg -n 'cancel_requested[^;\n]*(store|exchange)|cancel_requested[^;\n]*=' cpp/include cpp/src --glob '*.{hpp,h,cpp,cu,cuh,inl}' | head -n 300
rg -n '\.(store|exchange)\([^)]*\)' cpp/src --glob '*.{cpp,cu}' | rg 'cancel|preempt|halt' | head -n 200
printf '%s\n' '--- relevant cancellation helper definitions ---'
rg -n -C8 'cancel_flag_set|atomic_flag_set|remap_limit_status_if_cancelled' cpp/include cpp/src --glob '*.{hpp,h,cpp,cu,cuh,inl}' | head -n 300Repository: NVIDIA/cuopt
Length of output: 37164
Reset the cancellation flag before returning from solve_mip
solve_mip does not reset *settings.cancel_requested, so reusing a true flag causes the next solve to exit immediately. Reset the pointee on normal, early-return, and exception paths. Document that the pointee value is cleared, not the pointer.
🤖 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/include/cuopt/mathematical_optimization/mip/solver_settings.hpp` around
lines 187 - 190, Update the solve_mip cancellation handling so the pointee
behind settings.cancel_requested is cleared before solve_mip returns, including
normal completion, early-return paths, and exception exits. Keep
cancel_requested as a non-owning pointer in solver_settings.hpp, but make the
implementation reset the referenced atomic<bool> value rather than the pointer
itself, and update the related comment/documentation to state that the pointee
value is cleared.
| { | ||
| auto& handle_ptr = pb.handle_ptr; | ||
| timer_t timer(settings.time_limit); | ||
| timer_t timer(settings.time_limit, context.settings.cancel_requested); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'bound_update_loop|check_time_limit|calculate_bounds_update|update_bounds_from_slack|termination_criterion_t::(TIME_LIMIT|NO_UPDATE|CONVERGENCE)' \
cpp/src/mip_heuristics/presolveRepository: NVIDIA/cuopt
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- bound_presolve loop ---'
sed -n '172,205p' cpp/src/mip_heuristics/presolve/bounds_presolve.cu
printf '%s\n' '--- load_balanced loop ---'
sed -n '529,558p' cpp/src/mip_heuristics/presolve/load_balanced_bounds_presolve.cu
printf '%s\n' '--- multi_probe loop ---'
sed -n '269,320p' cpp/src/mip_heuristics/presolve/multi_probe.cu
printf '%s\n' '--- timer definition and result mapping ---'
rg -n -C 5 'class timer_t|struct timer_t|check_time_limit|get_cancel_requested|termination_criterion_t::TIME_LIMIT|termination_criterion_t::CANCEL|Cancelled|cancel_requested' \
cpp/src | head -n 500Repository: NVIDIA/cuopt
Length of output: 41327
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- update implementations and synchronization ---'
sed -n '107,170p' cpp/src/mip_heuristics/presolve/bounds_presolve.cu
sed -n '129,268p' cpp/src/mip_heuristics/presolve/multi_probe.cu
sed -n '515,528p' cpp/src/mip_heuristics/presolve/load_balanced_bounds_presolve.cu
printf '%s\n' '--- callers that consume presolve termination criteria ---'
rg -n -C 8 \
'bound_presolve_result|bounds_presolve_result|presolve_result.*termination|termination_criterion_t::NO_UPDATE|termination_criterion_t::CONVERGENCE' \
cpp/src/mip_heuristics
printf '%s\n' '--- all relevant timer construction sites and loop call paths ---'
rg -n -C 3 \
'timer_t timer\(settings\.time_limit, context\.settings\.cancel_requested\)|return bound_update_loop' \
cpp/src/mip_heuristics/presolve/{bounds_presolve.cu,load_balanced_bounds_presolve.cu,multi_probe.cu}Repository: NVIDIA/cuopt
Length of output: 41697
Check cancellation after each bound update. In all three shared bound_update_loop() implementations, check timer.check_time_limit() before classifying a false update result as NO_UPDATE or CONVERGENCE. A cancellation during the GPU update can otherwise trigger normal post-loop and caller work before the outer solve remaps the status. This applies to all eight listed entry points.
📍 Affects 3 files
cpp/src/mip_heuristics/presolve/bounds_presolve.cu#L235-L235(this comment)cpp/src/mip_heuristics/presolve/bounds_presolve.cu#L248-L248cpp/src/mip_heuristics/presolve/bounds_presolve.cu#L263-L263cpp/src/mip_heuristics/presolve/load_balanced_bounds_presolve.cu#L630-L630cpp/src/mip_heuristics/presolve/load_balanced_bounds_presolve.cu#L642-L642cpp/src/mip_heuristics/presolve/load_balanced_bounds_presolve.cu#L671-L671cpp/src/mip_heuristics/presolve/multi_probe.cu#L383-L383cpp/src/mip_heuristics/presolve/multi_probe.cu#L397-L397
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/mip_heuristics/presolve/bounds_presolve.cu` at line 235, Update all
three shared bound_update_loop implementations so each checks
timer.check_time_limit() immediately after every GPU bound update and before
classifying a false result as NO_UPDATE or CONVERGENCE. Apply this at
bounds_presolve.cu:235-235, 248-248, and 263-263;
load_balanced_bounds_presolve.cu:630-630, 642-642, and 671-671; and
multi_probe.cu:383-383 and 397-397, preserving cancellation propagation so
normal post-loop or caller work is skipped.
Source: Path instructions
| // minimum badge size to a large value. The time limit makes sure that we exit if it takes too | ||
| // long. | ||
| // | ||
| // Also cap maxbadgesize so presolve.tlim / cancel can be observed between badges. Without a | ||
| // cap, one badge can be ~ncols/2 and run far past tlim (observed on seymour1.mps), blocking | ||
| // cooperative cancel until the badge finishes. The alternative would be a much longer | ||
| // watchdog grace before SIGKILL while Papilo ignores cancel mid-badge; capping badges is | ||
| // preferable. Keep minbadgesize <= maxbadgesize. Cap may slow probing on huge MIPs vs | ||
| // unlimited badges. | ||
| if (reduction_allowed("probing")) { | ||
| int min_badgesize = std::max(ncols / 2, 32); | ||
| constexpr int max_badgesize = 512; | ||
| int min_badgesize = std::min(std::max(ncols / 2, 32), max_badgesize); | ||
| params.setParameter("probing.minbadgesize", min_badgesize); | ||
| params.setParameter("probing.maxbadgesize", max_badgesize); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 8 \
'papilo_presolver\.apply|set_presolve_options|cancel_requested|interrupt|probing\.(min|max)badgesize' \
cpp/src/mip_heuristics/presolveRepository: NVIDIA/cuopt
Length of output: 25953
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- Papilo sources and declarations ---'
git ls-files | rg -i 'papilo|third_party_presolve|presolve.*test|test.*presolve' | head -200
printf '%s\n' '--- Relevant implementation context ---'
sed -n '680,910p' cpp/src/mip_heuristics/presolve/third_party_presolve.cpp
sed -n '1040,1145p' cpp/src/mip_heuristics/presolve/third_party_presolve.cpp
printf '%s\n' '--- Cancellation and interruption references ---'
rg -n -i -C 4 \
'cancel_requested|interrupt|interruption|stop|terminate|abort|time.?limit|tlim' \
--glob '*.{cpp,hpp,h,inl,cmake,txt,yml,yaml}' . | rg -i 'papilo|presolv|cancel|interrupt|tlim' | head -300Repository: NVIDIA/cuopt
Length of output: 50368
🌐 Web query:
Papilo Presolve API interruption cancellation tlim interrupt apply PresolveOptions
💡 Result:
To manage interruptions, cancellation, and time limits within the PaPILO (Parallel Presolve for Integer and Linear Optimization) API, you can utilize the following mechanisms: 1. Time Limit (tlim): You can control the duration of the presolve process using the presolve.tlim parameter [1][2]. This parameter accepts a double-precision floating-point value representing the time limit in seconds [3][4]. The time limit is actively checked during the application of presolve reductions [1][2]. 2. Forceful Cancellation/Early Exit: If you need to forcefully stop PaPILO from a calling program, you can use the early_exit_callback feature available within PresolveOptions (introduced in PAPILO_API_VERSION 8) [1][2]. This allows for a graceful, early termination of the presolve process. 3. Handling Stopped Results: When the process is interrupted or reaches a limit, PaPILO returns specific result codes. For instance, PAPILO_SOLVE_RESULT_STOPPED indicates that solving was stopped early without a solution due to limits or interrupts [5], while PAPILO_SOLVE_RESULT_FEASIBLE indicates that solving stopped early with a feasible solution [6]. 4. Parameter Access: You can modify basic settings, including time limits, by accessing the PresolveOptions instance associated with your papilo::Presolve object [7]. Note that while this method is convenient, it may not support parameters added by individual presolvers or perform extensive error checking for invalid values [8][7]. For command-line usage, the papilo executable also provides a --tlim argument to set the time limit for the solver [4]. Additionally, internal abort factors (e.g., presolve.abortfac) can be used to control the intensity and depth of the presolve reductions, which indirectly impacts the total time spent in presolve [9].
Citations:
- 1: https://github.com/scipopt/papilo/blob/v3.0.0/CHANGELOG
- 2: https://github.com/scipopt/papilo/blob/main/CHANGELOG
- 3: https://github.com/lgottwald/PaPILO/blob/master/parameters.txt
- 4: https://www.mankier.com/1/papilo
- 5: https://docs.rs/papilo-sys/latest/papilo_sys/constant.Papilo_SolveResult_PAPILO_SOLVE_RESULT_STOPPED.html
- 6: https://docs.rs/papilo-sys/latest/papilo_sys/constant.Papilo_SolveResult_PAPILO_SOLVE_RESULT_FEASIBLE.html
- 7: https://github.com/scipopt/papilo
- 8: https://github.com/scipopt/papilo/
- 9: https://zenodo.org/records/14047017
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- PaPILO dependency/version configuration ---'
rg -n -i -C 3 \
'papilo|PAPILO_GITHASH|PAPILO_API_VERSION|early_exit_callback|CPMAdd|FetchContent' \
--glob '*.{cmake,cmake.in,txt,yml,yaml,json,lock,sh,hpp,h,cpp}' . \
| head -250
printf '%s\n' '--- Presolve call sites and cancellation ownership ---'
rg -n -C 8 \
'apply_to_subproblem|apply_papilo|apply_host|cancel_requested|presolve_time' \
cpp/src cpp/include cpp/tests \
| rg -n -C 4 'third_party_presolve|apply_to_subproblem|apply_papilo|cancel_requested|presolve' \
| head -350
printf '%s\n' '--- Existing presolve tests ---'
sed -n '1,260p' cpp/tests/mip/presolve_test.cu
rg -n -C 8 -i 'papilo|cancel|time.?limit|presolv' cpp/tests/mip/presolve_test.cu cpp/tests/linear_programming/unit_tests/presolve_test.cuRepository: NVIDIA/cuopt
Length of output: 47850
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- PaPILO repository and tag ---'
sed -n '260,292p' cpp/CMakeLists.txt
printf '%s\n' '--- MIP presolve call and cancellation flow ---'
sed -n '2025,2165p' cpp/src/pdlp/solve.cu
rg -n -C 8 \
'apply_presolve_from_op_problem|cancel_requested|terminal_solution_from_presolve_status' \
cpp/src/mip_heuristics/presolve/third_party_presolve.cpp \
cpp/src/mip_heuristics/presolve/third_party_presolve.hpp \
cpp/src/pdlp/solve.cu \
cpp/include/cuopt/mathematical_optimization/mip/solver_settings.hpp
printf '%s\n' '--- Papilo API references from the pinned fork ---'
repo='akifcorduk/papilo'
for path in \
'src/papilo/core/Presolve.hpp' \
'src/papilo/core/PresolveOptions.hpp' \
'src/papilo/core/Timer.hpp' \
'src/papilo/core/PresolveMethod.hpp'; do
printf '\n### %s\n' "$path"
curl -LfsS "https://raw.githubusercontent.com/$repo/main/$path" \
| rg -n -C 5 -i \
'early_exit|callback|tlim|time.?limit|interrupt|stop|Timer|apply\(' \
| head -180 || true
doneRepository: NVIDIA/cuopt
Length of output: 41167
🏁 Script executed:
#!/bin/bash
set -euo pipefail
base='https://raw.githubusercontent.com/akifcorduk/papilo/32b3a87dbf4955d5a2803be74145c389ea31434d'
printf '%s\n' '--- PaPILO interruption checks at the pinned commit ---'
for path in \
'src/papilo/core/Presolve.hpp' \
'src/papilo/core/PresolveOptions.hpp' \
'src/papilo/core/PresolveMethod.hpp' \
'src/papilo/misc/Timer.hpp'; do
printf '\n### %s\n' "$path"
curl -LfsS "$base/$path" \
| nl -ba \
| sed -n '1,240p' \
| rg -n -C 8 \
'early_exit|is_interrupted|is_user_interrupted|is_time_exceeded|apply_all_presolver|run_presolvers|tlim|PresolveStatus::kAbort' \
|| true
done
printf '%s\n' '--- All cuOpt PaPILO wrapper declarations and definitions ---'
rg -n -C 12 \
'apply_presolve_from_mps_data|apply_presolve_from_op_problem|apply_papilo|apply_presolve_from' \
cpp/src/mip_heuristics/presolve/third_party_presolve.cpp \
cpp/src/mip_heuristics/presolve/third_party_presolve.hpp
printf '%s\n' '--- Cancellation result handling after presolve ---'
rg -n -C 10 \
'remap_limit_status_if_cancelled|cancel_flag_set|CUOPT_TERMINATION_STATUS_CANCELLED|Cancelled' \
cpp/src/pdlp/solve.cu cpp/src/mip_heuristics/presolveRepository: NVIDIA/cuopt
Length of output: 29965
🏁 Script executed:
#!/bin/bash
set -euo pipefail
base='https://raw.githubusercontent.com/akifcorduk/papilo/32b3a87dbf4955d5a2803be74145c389ea31434d'
printf '%s\n' '--- PaPILO Presolve interruption logic ---'
curl -LfsS "$base/src/papilo/core/Presolve.hpp" \
| rg -n -C 14 \
'early_exit|is_interrupted|is_user_interrupted|is_time_exceeded|kAbort|apply_all_presolver_reductions|run_presolvers' \
| head -260
printf '%s\n' '--- PaPILO method-level timer behavior ---'
curl -LfsS "$base/src/papilo/core/PresolveMethod.hpp" \
| rg -n -C 12 \
'early_exit|is_interrupted|is_user_interrupted|is_time_exceeded|timer\.|execute\(' \
| head -220
printf '%s\n' '--- PaPILO option declaration ---'
curl -LfsS "$base/src/papilo/core/PresolveOptions.hpp" \
| rg -n -C 8 'early_exit_callback|double tlim|addParameter.*tlim' \
| head -100
printf '%s\n' '--- PaPILO status enum ---'
for path in \
'src/papilo/core/PresolveStatus.hpp' \
'src/papilo/core/PresolveResult.hpp' \
'src/papilo/core/Presolve.hpp'; do
printf '\n### %s\n' "$path"
curl -LfsS "$base/$path" \
| rg -n -C 8 'enum class PresolveStatus|kAbort|kUnchanged|return result|result.status' \
| head -160 || true
doneRepository: NVIDIA/cuopt
Length of output: 19533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
base='https://raw.githubusercontent.com/akifcorduk/papilo/32b3a87dbf4955d5a2803be74145c389ea31434d'
src=$(curl -LfsS "$base/src/papilo/core/Presolve.hpp")
printf '%s\n' '--- Presolve callback use ---'
printf '%s\n' "$src" \
| rg -n -C 16 \
'Presolve<REAL>::is_interrupted|Presolve<REAL>::is_user_interrupted|Presolve<REAL>::is_time_exceeded|early_exit_callback|presolveOptions\.tlim|is_interrupted\(' \
| head -300
printf '%s\n' '--- Presolve method execution loop ---'
printf '%s\n' "$src" \
| sed -n '1010,1075p'
printf '%s\n' '--- Presolve result after an interruption ---'
printf '%s\n' "$src" \
| sed -n '520,635p'Repository: NVIDIA/cuopt
Length of output: 17851
Wire the shared cancellation flag into Papilo.
Set Papilo’s PresolveOptions::early_exit_callback from cancel_requested and thread it through the presolve wrappers. The callback is checked between presolve operations, while the badge cap limits long probing operations. Add a regression test that cancels during Papilo presolve.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/mip_heuristics/presolve/third_party_presolve.cpp` around lines 748 -
761, Thread the shared cancel_requested flag through the Papilo presolve wrapper
functions and assign PresolveOptions::early_exit_callback to a callback that
observes it. Ensure the callback is configured before presolve begins while
preserving the existing probing badge limits, and add a regression test that
requests cancellation during Papilo presolve and verifies early termination.
Source: Path instructions
| bool per_constraint_residual = true; | ||
| bool has_initial_primal = true; | ||
| std::atomic<int>* concurrent_halt = nullptr; | ||
| std::atomic<bool>* cancel_requested = nullptr; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -C 6 \
'relaxed_lp_settings_t|cancel_requested|get_relaxed_lp_solution|run_lp_with_vars_fixed' \
cpp/src/mip_heuristicsRepository: NVIDIA/cuopt
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- relaxed-LP settings construction and calls ---'
rg -n -C 8 \
'relaxed_lp_settings_t|run_lp_with_vars_fixed\(|get_relaxed_lp_solution\(' \
cpp/src/mip_heuristics/feasibility_pump cpp/src/mip_heuristics \
-g '*.cu' -g '*.cuh' | \
grep -E 'feasibility_pump|relaxed_lp_settings_t|run_lp_with_vars_fixed|get_relaxed_lp_solution|cancel_requested|bound_presolve' | \
head -n 240
printf '%s\n' '--- bound presolve initialization and cancellation sources ---'
rg -n -C 8 \
'bound_presolve_t|bound_presolve|cancel_requested|set_context|context.settings' \
cpp/src/mip_heuristics/presolve cpp/src/mip_heuristics/relaxed_lp \
-g '*.cu' -g '*.cuh' | head -n 300Repository: NVIDIA/cuopt
Length of output: 12224
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- feasibility-pump implementation context ---'
sed -n '1,120p' cpp/src/mip_heuristics/local_search/feasibility_pump/feasibility_pump.cu
sed -n '180,245p' cpp/src/mip_heuristics/local_search/feasibility_pump/feasibility_pump.cu
sed -n '500,545p' cpp/src/mip_heuristics/local_search/feasibility_pump/feasibility_pump.cu
printf '%s\n' '--- all relaxed-LP settings initializers ---'
rg -n -C 12 'relaxed_lp_settings_t lp_settings' cpp/src/mip_heuristics \
-g '*.cu' -g '*.cuh'
printf '%s\n' '--- bound presolve context definition and construction ---'
rg -n -C 12 \
'class bound_presolve_t|struct bound_presolve_t|bound_presolve_t\(|bound_presolve.*context|context.settings.cancel_requested' \
cpp/src/mip_heuristics/presolve cpp/src/mip_heuristics \
-g '*.cuh' -g '*.cu' | head -n 400Repository: NVIDIA/cuopt
Length of output: 50368
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
root = Path("cpp/src/mip_heuristics")
for path in sorted(root.rglob("*")):
if path.suffix not in {".cu", ".cuh"}:
continue
text = path.read_text()
for match in re.finditer(r"\brelaxed_lp_settings_t\s+(\w+)\s*;", text):
start = match.start()
end = text.find(";", text.find("run_lp_with_vars_fixed", start))
if end < 0:
end = min(len(text), start + 2500)
chunk = text[start:end + 1]
line = text.count("\n", 0, start) + 1
print(f"\n{path}:{line}")
print(chunk[:2200])
PY
printf '%s\n' '--- relaxed-LP cancellation forwarding ---'
sed -n '35,95p' cpp/src/mip_heuristics/relaxed_lp/relaxed_lp.cu
printf '%s\n' '--- bound presolve cancellation context ---'
sed -n '32,65p' cpp/src/mip_heuristics/presolve/bounds_presolve.cuh
sed -n '76,90p' cpp/src/mip_heuristics/presolve/bounds_presolve.cu
sed -n '229,270p' cpp/src/mip_heuristics/presolve/bounds_presolve.cu
printf '%s\n' '--- feasibility-pump bound-presolve construction ---'
rg -n -C 10 \
'constraint_prop_t|bounds_update|bound_presolve_t' \
cpp/src/mip_heuristics/local_search/feasibility_pump \
cpp/src/mip_heuristics/local_search \
-g '*.cu' -g '*.cuh' | head -n 220Repository: NVIDIA/cuopt
Length of output: 40136
Propagate cancel_requested to every MIP relaxed-LP call.
get_relaxed_lp_solution forwards this pointer to PDLP and its timer. Both settings objects in feasibility_pump.cu leave it null. The same omission exists in constraint_prop.cu and lb_constraint_prop.cu. Initialize each pointer from the owning MIP context. bound_presolve_t already reads context.settings.cancel_requested. Add a cancellation regression test for relaxed LP execution inside feasibility pump.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/mip_heuristics/relaxed_lp/relaxed_lp.cuh` at line 28, Initialize the
relaxed-LP cancellation pointer in the feasibility-pump, constraint-propagation,
and lower-bound constraint-propagation settings objects from the owning MIP
context, reusing context.settings.cancel_requested. Ensure every relaxed-LP
invocation receives these populated settings, and add a regression test covering
cancellation during relaxed-LP execution in feasibility pump.
Source: Path instructions
|
|
||
| raft::common::nvtx::range fun_scope("Running solver"); | ||
| auto timer = timer_t(time_limit); | ||
| auto timer = timer_t(time_limit, settings.cancel_requested); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
HIGH: Apply cancellation precedence before every observable terminal path.
A preset cancellation still runs preprocessing, starts early heuristics, and enters Papilo presolve before Line 663 checks the flag. A cancellation that coincides with a presolve INFEASIBLE, UNBOUNDED, or UNBNDORINFEAS result returns that status before Line 663. Lines 797-802 also log and write the old status before Line 809 changes the returned status to Cancelled.
cpp/src/mip_heuristics/solve.cu#L382-L382: Check the cancellation-aware timer immediately after construction. ReturnCancelledbefore expensive setup, early heuristics, or presolve.cpp/src/mip_heuristics/solve.cu#L663-L670: Check cancellation before every presolve terminal-status return, or route all presolve exits through one cancellation-aware finalization path.cpp/src/mip_heuristics/solve.cu#L804-L810: Remap toCancelledbeforelog_detailed_summary()andwrite_to_sol_file()so returned status, logs, and.soloutput agree.
📍 Affects 1 file
cpp/src/mip_heuristics/solve.cu#L382-L382(this comment)cpp/src/mip_heuristics/solve.cu#L663-L670cpp/src/mip_heuristics/solve.cu#L804-L810
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cpp/src/mip_heuristics/solve.cu` at line 382, Apply cancellation precedence
in cpp/src/mip_heuristics/solve.cu at 382-382 by checking the timer immediately
after construction and returning Cancelled before setup, heuristics, or
presolve. At 663-670, ensure every presolve terminal-status return checks
cancellation first or uses a shared cancellation-aware finalization path. At
804-810, remap the status to Cancelled before log_detailed_summary() and
write_to_sol_file() so the return value, logs, and solution output agree.
Source: Path instructions
| timer_t() = delete; | ||
| timer_t(const timer_t&) = default; | ||
| timer_t(double time_limit_) | ||
| timer_t(double time_limit_) : timer_t(time_limit_, nullptr) {} |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Mark the single-argument constructor explicit.
timer_t(double time_limit_) is a single-argument constructor and is not marked explicit. This allows an unintended implicit conversion from double to timer_t at any call site expecting a timer_t.
♻️ Proposed fix
- timer_t(double time_limit_) : timer_t(time_limit_, nullptr) {}
+ explicit timer_t(double time_limit_) : timer_t(time_limit_, nullptr) {}As per path instructions: "Use explicit for single-argument constructors, keep data members private, and mark virtual overrides with override/final."
📝 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.
| timer_t(double time_limit_) : timer_t(time_limit_, nullptr) {} | |
| explicit timer_t(double time_limit_) : timer_t(time_limit_, nullptr) {} |
🤖 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/timer.hpp` at line 23, Mark the single-argument timer_t
constructor that delegates to timer_t(double, nullptr) as explicit to prevent
implicit double-to-timer_t conversions, while preserving its existing delegation
behavior.
Source: Path instructions
| afiro_path = datasets / "linear_programming" / "afiro_original.mps" | ||
| if afiro_path.is_file(): | ||
| afiro_ok, afiro_detail = fetch_and_check_result( | ||
| client, | ||
| Read(str(afiro_path)), | ||
| make_settings(30.0), | ||
| [], | ||
| expected_obj=-464.753, | ||
| label="LP(afiro)", | ||
| timeout=60.0, | ||
| server=server, | ||
| ) | ||
| notes.append(afiro_detail) | ||
| else: | ||
| afiro_ok = False | ||
| notes.append("LP(afiro): mps not found") | ||
|
|
||
| ok = lp_ok and mip_ok and afiro_ok |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Do not fail the trial when the afiro dataset is absent.
afiro_ok stays False when the file is missing, so ok becomes False. The trial then reports a failure for a missing optional dataset instead of a real defect. Treat the missing dataset as skipped.
🐛 Proposed fix
else:
- afiro_ok = False
+ afiro_ok = True # optional dataset; absence is not a failure
notes.append("LP(afiro): mps not found")📝 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.
| afiro_path = datasets / "linear_programming" / "afiro_original.mps" | |
| if afiro_path.is_file(): | |
| afiro_ok, afiro_detail = fetch_and_check_result( | |
| client, | |
| Read(str(afiro_path)), | |
| make_settings(30.0), | |
| [], | |
| expected_obj=-464.753, | |
| label="LP(afiro)", | |
| timeout=60.0, | |
| server=server, | |
| ) | |
| notes.append(afiro_detail) | |
| else: | |
| afiro_ok = False | |
| notes.append("LP(afiro): mps not found") | |
| ok = lp_ok and mip_ok and afiro_ok | |
| afiro_path = datasets / "linear_programming" / "afiro_original.mps" | |
| if afiro_path.is_file(): | |
| afiro_ok, afiro_detail = fetch_and_check_result( | |
| client, | |
| Read(str(afiro_path)), | |
| make_settings(30.0), | |
| [], | |
| expected_obj=-464.753, | |
| label="LP(afiro)", | |
| timeout=60.0, | |
| server=server, | |
| ) | |
| notes.append(afiro_detail) | |
| else: | |
| afiro_ok = True # optional dataset; absence is not a failure | |
| notes.append("LP(afiro): mps not found") | |
| ok = lp_ok and mip_ok and afiro_ok |
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around
lines 641 - 658, Update the afiro dataset handling around fetch_and_check_result
so a missing afiro_original.mps is treated as skipped rather than setting
afiro_ok to False. Preserve result validation when the file exists, and ensure
the final ok calculation does not fail solely because the optional dataset is
absent.
| _CANCEL_FALLBACK_KILL_RE = re.compile( | ||
| r"Job\s+(\S+)\s+still running after cooperative cancel grace;.*" | ||
| r"SIGKILL to worker\s+(\d+)", | ||
| re.DOTALL, | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Remove re.DOTALL from the fallback-kill pattern.
The server writes the fallback message as a single log line. With re.DOTALL, .* spans newlines and matches across unrelated log lines. Any earlier "...cooperative cancel grace;" text followed anywhere later by "SIGKILL to worker N" then matches, and the is None assertion at Line 446 fails without a real fallback. Restrict the match to one line.
🐛 Proposed fix
_CANCEL_FALLBACK_KILL_RE = re.compile(
r"Job\s+(\S+)\s+still running after cooperative cancel grace;.*"
- r"SIGKILL to worker\s+(\d+)",
- re.DOTALL,
+ r"SIGKILL to worker\s+(\d+)"
)📝 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.
| _CANCEL_FALLBACK_KILL_RE = re.compile( | |
| r"Job\s+(\S+)\s+still running after cooperative cancel grace;.*" | |
| r"SIGKILL to worker\s+(\d+)", | |
| re.DOTALL, | |
| ) | |
| _CANCEL_FALLBACK_KILL_RE = re.compile( | |
| r"Job\s+(\S+)\s+still running after cooperative cancel grace;.*" | |
| r"SIGKILL to worker\s+(\d+)" | |
| ) |
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py` around lines
47 - 51, Update the _CANCEL_FALLBACK_KILL_RE pattern by removing re.DOTALL so
its wildcard cannot cross newline boundaries. Keep the existing message
structure and capture groups, ensuring matches remain limited to a single server
log line and unrelated lines do not satisfy the pattern.
| auto gpu_solution = cuopt::mathematical_optimization::solve_mip(*gpu_problem, dj.mip_settings); | ||
| SERVER_LOG_INFO("[Worker] solve_mip done"); | ||
|
|
||
| if (gpu_solution.get_termination_status() == |
There was a problem hiding this comment.
the point of this is, in the grpc server, always set the status as "cancelled" if we know it was cancelled regardless of the status that comes back. It may be hard to properly return "cancel" vs "time limit" throughout the solver.
| template <typename i_t, typename f_t> | ||
| bool diversity_manager_t<i_t, f_t>::check_b_b_preemption() | ||
| { | ||
| if (context.settings.cancel_requested != nullptr && |
There was a problem hiding this comment.
This is one of those places that needs close engine attention :)
| this->preemption_flag_.store(false); | ||
| this->start_time_ = std::chrono::steady_clock::now(); | ||
|
|
||
| fj_cpu_ = init_fj_cpu_standalone(*this->problem_ptr_, *this->solution_ptr_, preemption_flag_); |
There was a problem hiding this comment.
this should be checked closely as well. It substitutes the cancel flag for the local preempt flag if the cancel flag has been wired in. If the solve was not configured with a cancel flag, it's unchanged.
| // Climbers are null until the matching start_* fills them (and start_lptopt | ||
| // is independent of start_scratch). Skip unset slots so stop is safe if a | ||
| // start never ran. | ||
| for (size_t i = 0; i < scratch_cpu_fj.size(); ++i) { |
There was a problem hiding this comment.
This is just an extra safety. In one iteration of the code, it was possible that this might be null. Left the check because it seemed like a good one to have.
| @@ -745,11 +745,20 @@ void set_presolve_parameters( | |||
| }; | |||
| // Papilo has work unit measurements for probing. Because of this when the first batch fails to | |||
| // produce any reductions, the algorithm stops. To avoid stopping the algorithm, we set a | |||
There was a problem hiding this comment.
This is another place that needs close attention. Cursor suggested adding a max badge size for presolve so that the cancel check was sure to be polled more often. However, I don't know what affect this may have on performance. The alternative is to make the grace period for SIGKILL longer in threads that monitor cancel (as in the gRPC server).
| sol = dm.run_solver(); | ||
| } // implicit barrier for all tasks created in B&B and heuristics | ||
|
|
||
| const bool cancel_requested = cuopt::cancel_flag_set(context.settings.cancel_requested) || |
There was a problem hiding this comment.
another place to check correctness.
Poll a shared cancel flag like time limits so mid-solve cancel can unwind and avoid as much as possible the need for SIGKILL especially in the gRPC server, since SIGKILL can leave the GPU in an uninterruptible sleep in some cases. Remap limit statuses to Cancelled at solution finalization, preempt MIP heuristics on cancel. Signed-off-by: Trevor McKay <tmgithub1@gmail.com>
99d01ec to
d7a7b07
Compare
CI Test Summary15 failed · 16 passed · 0 skipped
|
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (3)
python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py (3)
1758-1768: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe traffic-burst guard never applies in the default random mode.
Line 1760 sets
next_actiontoNoneunless--fixed-orderis set. Random order is the default, sonext_action in ("result", "shutdown")is always false and the burst always runs. The stated protection against a SIGKILL into a poisoned GPU beforeresultandshutdownis inactive by default.Choose the next action before the burst decision and reuse it in the next iteration.
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around lines 1758 - 1768, Update the action scheduling around next_action so random mode selects the upcoming action before evaluating allow_burst, rather than leaving next_action as None. Reuse that selected action in the following iteration’s execution path, while preserving fixed-order behavior and skipping bursts before “result” or “shutdown”.
1472-1553: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove or wire up the dead intermediate-log path.
wait_for_solver_intermediateshas no caller.trial_shutdowndiscardsintermediate_timeoutandmin_intermediate_linesat lines 1550-1553.INTERMEDIATE_LOG_REand the--intermediate-timeoutand--min-intermediate-linesCLI options therefore have no effect. Either callwait_for_solver_intermediatesbeforesend_sigintso the SIGINT lands on an actively solving job, or delete the function, the regex, and the two CLI options.🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around lines 1472 - 1553, The intermediate-log functionality is currently dead because trial_shutdown discards its parameters and never calls wait_for_solver_intermediates. Wire this path into trial_shutdown before send_sigint, passing intermediate_timeout and min_intermediate_lines and requiring the helper to confirm sufficient solver progress, or remove wait_for_solver_intermediates, INTERMEDIATE_LOG_RE, and both CLI options consistently.
309-318: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAvoid
preexec_fnafter starting threads.The initial server start precedes the worker threads, but later
ServerHandle.start()calls run whiletrafficandserver-stdoutthreads exist.preexec_fncan deadlock afterfork()in this state. Replace it with a safer parent-death cleanup mechanism, or rely on explicit process-group cleanup.🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around lines 309 - 318, Update ServerHandle.start() to remove preexec_fn=_set_pdeathsig, since it forks while traffic and server-stdout threads may be running. Use a thread-safe parent-death cleanup mechanism or ensure explicit process-group cleanup reliably terminates the spawned server and descendants.
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py`:
- Around line 562-565: Guard every trial-helper gRPC call so transient GrpcError
instances become recorded trial failures rather than escaping the harness: in
python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py:562-565,
move client.submit inside the try in the result-check helper and return (False,
f"{label}: submit failed: {e}") on failure; in
python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py:741-767,
wrap client.status in wait_until_processing and client.submit in submit_spec,
returning the expected status or recorded failure to callers.
- Around line 1234-1244: Update the kill-log scans in the surrounding lifecycle
logic to use finditer for both CANCEL_KILL_RE and CANCEL_FALLBACK_KILL_RE,
selecting the first match whose group(1) equals target instead of repeatedly
searching only the first line. Preserve the existing None behavior when no
matching target job_id is found, consistent with cancel_log_names_job.
- Around line 95-99: Update the SIGSEGV note construction in the
WORKER_KILLED_RE loop to label m.group(1) as the worker index rather than a PID,
while preserving the existing signal check and note content.
- Around line 1066-1101: Fix the race in TrafficGenerator._run by clearing
self._idle before checking self.enabled, or by holding self._lock across the
enabled check and submission, so pause_and_drain cannot observe an idle
generator while a job submission is in flight. Preserve the existing idle
behavior when disabled and ensure pause_and_drain waits before
snapshot_outstanding.
---
Nitpick comments:
In `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py`:
- Around line 1758-1768: Update the action scheduling around next_action so
random mode selects the upcoming action before evaluating allow_burst, rather
than leaving next_action as None. Reuse that selected action in the following
iteration’s execution path, while preserving fixed-order behavior and skipping
bursts before “result” or “shutdown”.
- Around line 1472-1553: The intermediate-log functionality is currently dead
because trial_shutdown discards its parameters and never calls
wait_for_solver_intermediates. Wire this path into trial_shutdown before
send_sigint, passing intermediate_timeout and min_intermediate_lines and
requiring the helper to confirm sufficient solver progress, or remove
wait_for_solver_intermediates, INTERMEDIATE_LOG_RE, and both CLI options
consistently.
- Around line 309-318: Update ServerHandle.start() to remove
preexec_fn=_set_pdeathsig, since it forks while traffic and server-stdout
threads may be running. Use a thread-safe parent-death cleanup mechanism or
ensure explicit process-group cleanup reliably terminates the spawned server and
descendants.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 2cc870b9-38b0-454b-ac4f-6cec31e684a3
📒 Files selected for processing (24)
cpp/include/cuopt/mathematical_optimization/mip/solver_settings.hppcpp/src/barrier/barrier.cucpp/src/barrier/sparse_cholesky.cuhcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/phase2.cppcpp/src/dual_simplex/primal.cppcpp/src/dual_simplex/right_looking_lu.cppcpp/src/dual_simplex/simplex_solver_settings.hppcpp/src/grpc/server/grpc_job_management.cppcpp/src/grpc/server/grpc_worker.cppcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/diversity/recombiners/fp_recombiner.cuhcpp/src/mip_heuristics/local_search/local_search.cucpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cucpp/src/mip_heuristics/relaxed_lp/relaxed_lp.cuhcpp/src/mip_heuristics/solve.cucpp/src/mip_heuristics/solver.cucpp/src/mip_heuristics/solver_solution.cucpp/src/utilities/solve_limits.hppcpp/tests/mip/cooperative_cancel_test.cupython/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.pypython/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.pypython/cuopt/cuopt/tests/linear_programming/test_grpc_client.py
🚧 Files skipped from review as they are similar to previous changes (22)
- cpp/src/mip_heuristics/diversity/recombiners/fp_recombiner.cuh
- cpp/src/dual_simplex/right_looking_lu.cpp
- cpp/src/grpc/server/grpc_job_management.cpp
- cpp/src/barrier/sparse_cholesky.cuh
- cpp/tests/mip/cooperative_cancel_test.cu
- cpp/src/dual_simplex/primal.cpp
- cpp/src/utilities/solve_limits.hpp
- cpp/src/mip_heuristics/diversity/diversity_manager.cu
- cpp/src/dual_simplex/simplex_solver_settings.hpp
- cpp/src/dual_simplex/crossover.cpp
- cpp/src/mip_heuristics/solve.cu
- python/cuopt/cuopt/tests/fixtures/grpc_server_fixtures.py
- cpp/src/mip_heuristics/relaxed_lp/relaxed_lp.cuh
- cpp/src/branch_and_bound/branch_and_bound.cpp
- cpp/src/grpc/server/grpc_worker.cpp
- cpp/src/dual_simplex/phase2.cpp
- cpp/src/mip_heuristics/local_search/local_search.cu
- cpp/src/mip_heuristics/solver_solution.cu
- cpp/src/mip_heuristics/presolve/conflict_graph/clique_table.cu
- cpp/src/barrier/barrier.cu
- python/cuopt/cuopt/tests/linear_programming/test_grpc_client.py
- cpp/include/cuopt/mathematical_optimization/mip/solver_settings.hpp
| for m in WORKER_KILLED_RE.finditer(server_log_text): | ||
| sig = int(m.group(2)) | ||
| if sig == 11: | ||
| notes.append(f"unexpected worker SIGSEGV pid={m.group(1)}") | ||
| return "; ".join(notes) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the mislabeled field in the segfault note.
WORKER_KILLED_RE captures the worker index in group 1 and the signal number in group 2. Line 98 reports group 1 as pid. The note then shows a worker index labeled as a PID, which misleads diagnosis of cooperative-cancel failures.
🐛 Proposed fix
- notes.append(f"unexpected worker SIGSEGV pid={m.group(1)}")
+ notes.append(f"unexpected worker SIGSEGV worker={m.group(1)}")📝 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.
| for m in WORKER_KILLED_RE.finditer(server_log_text): | |
| sig = int(m.group(2)) | |
| if sig == 11: | |
| notes.append(f"unexpected worker SIGSEGV pid={m.group(1)}") | |
| return "; ".join(notes) | |
| for m in WORKER_KILLED_RE.finditer(server_log_text): | |
| sig = int(m.group(2)) | |
| if sig == 11: | |
| notes.append(f"unexpected worker SIGSEGV worker={m.group(1)}") | |
| return "; ".join(notes) |
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around
lines 95 - 99, Update the SIGSEGV note construction in the WORKER_KILLED_RE loop
to label m.group(1) as the worker index rather than a PID, while preserving the
existing signal check and note content.
| job_id = client.submit(problem, settings) | ||
| log(f"Result-check submit {label} job_id={job_id}") | ||
| try: | ||
| terminal = client.wait(job_id, timeout=int(timeout)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Unguarded gRPC calls in trial helpers abort the whole harness. client.submit and client.status can raise GrpcError. Several helpers call them outside any try, and main catches only KeyboardInterrupt. One transient error therefore ends the run and skips summary.print_report(), which loses every trial result collected so far. A stress harness must record such an error as a failed trial instead.
python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py#L562-L565: moveclient.submitinside atryand return(False, f"{label}: submit failed: {e}")onGrpcError.python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py#L741-L767: wrapclient.statusinwait_until_processingandclient.submitinsubmit_specso callers receive a status value or a recorded failure instead of a propagatingGrpcError.
📍 Affects 1 file
python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py#L562-L565(this comment)python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py#L741-L767
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around
lines 562 - 565, Guard every trial-helper gRPC call so transient GrpcError
instances become recorded trial failures rather than escaping the harness: in
python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py:562-565,
move client.submit inside the try in the result-check helper and return (False,
f"{label}: submit failed: {e}") on failure; in
python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py:741-767,
wrap client.status in wait_until_processing and client.submit in submit_spec,
returning the expected status or recorded failure to callers.
| def pause_and_drain(self, client: Client, settle_s: float = 1.0) -> None: | ||
| """Stop submitting, wait idle, prefer letting short jobs finish over SIGKILL.""" | ||
| self.enabled.clear() | ||
| if not self.wait_idle(timeout=15.0): | ||
| log("WARNING: traffic generator did not go idle within 15s") | ||
| leftover = self.snapshot_outstanding() | ||
| if leftover: | ||
| log(f"Draining {len(leftover)} traffic job(s) before trial") | ||
| for jid in leftover: | ||
| # Prefer waiting for completion so we do not SIGKILL mid-CUDA | ||
| # right before the next trial (especially result/shutdown). | ||
| try: | ||
| st = client.status(jid) | ||
| if st in (JobStatus.QUEUED, JobStatus.PROCESSING): | ||
| try: | ||
| client.wait(jid, timeout=8) | ||
| except GrpcError: | ||
| pass | ||
| except GrpcError: | ||
| pass | ||
| try: | ||
| client.delete(jid) | ||
| except GrpcError: | ||
| pass | ||
| with self._lock: | ||
| self._outstanding.discard(jid) | ||
| if leftover and settle_s > 0: | ||
| time.sleep(settle_s) | ||
|
|
||
| def _run(self) -> None: | ||
| while not self._stop.is_set(): | ||
| if not self.enabled.is_set(): | ||
| self._idle.set() | ||
| time.sleep(0.05) | ||
| continue | ||
| self._idle.clear() |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Close the _idle race so pause_and_drain cannot return while a submission is in flight.
_run checks self.enabled at line 1097 and clears _idle at line 1101. Between those two lines _idle is still set from the previous iteration. If pause_and_drain clears enabled in that window, wait_idle returns True immediately, and snapshot_outstanding runs before the new job_id is added at line 1114. The trial then starts with an undrained background job on a worker, which makes cancel and shutdown trials nondeterministic.
Clear _idle before the enabled check, or hold _lock across the enabled check and the submission.
🐛 Proposed fix
def _run(self) -> None:
while not self._stop.is_set():
+ self._idle.clear()
if not self.enabled.is_set():
self._idle.set()
time.sleep(0.05)
continue
- self._idle.clear()
try:📝 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.
| def pause_and_drain(self, client: Client, settle_s: float = 1.0) -> None: | |
| """Stop submitting, wait idle, prefer letting short jobs finish over SIGKILL.""" | |
| self.enabled.clear() | |
| if not self.wait_idle(timeout=15.0): | |
| log("WARNING: traffic generator did not go idle within 15s") | |
| leftover = self.snapshot_outstanding() | |
| if leftover: | |
| log(f"Draining {len(leftover)} traffic job(s) before trial") | |
| for jid in leftover: | |
| # Prefer waiting for completion so we do not SIGKILL mid-CUDA | |
| # right before the next trial (especially result/shutdown). | |
| try: | |
| st = client.status(jid) | |
| if st in (JobStatus.QUEUED, JobStatus.PROCESSING): | |
| try: | |
| client.wait(jid, timeout=8) | |
| except GrpcError: | |
| pass | |
| except GrpcError: | |
| pass | |
| try: | |
| client.delete(jid) | |
| except GrpcError: | |
| pass | |
| with self._lock: | |
| self._outstanding.discard(jid) | |
| if leftover and settle_s > 0: | |
| time.sleep(settle_s) | |
| def _run(self) -> None: | |
| while not self._stop.is_set(): | |
| if not self.enabled.is_set(): | |
| self._idle.set() | |
| time.sleep(0.05) | |
| continue | |
| self._idle.clear() | |
| def pause_and_drain(self, client: Client, settle_s: float = 1.0) -> None: | |
| """Stop submitting, wait idle, prefer letting short jobs finish over SIGKILL.""" | |
| self.enabled.clear() | |
| if not self.wait_idle(timeout=15.0): | |
| log("WARNING: traffic generator did not go idle within 15s") | |
| leftover = self.snapshot_outstanding() | |
| if leftover: | |
| log(f"Draining {len(leftover)} traffic job(s) before trial") | |
| for jid in leftover: | |
| # Prefer waiting for completion so we do not SIGKILL mid-CUDA | |
| # right before the next trial (especially result/shutdown). | |
| try: | |
| st = client.status(jid) | |
| if st in (JobStatus.QUEUED, JobStatus.PROCESSING): | |
| try: | |
| client.wait(jid, timeout=8) | |
| except GrpcError: | |
| pass | |
| except GrpcError: | |
| pass | |
| try: | |
| client.delete(jid) | |
| except GrpcError: | |
| pass | |
| with self._lock: | |
| self._outstanding.discard(jid) | |
| if leftover and settle_s > 0: | |
| time.sleep(settle_s) | |
| def _run(self) -> None: | |
| while not self._stop.is_set(): | |
| self._idle.clear() | |
| if not self.enabled.is_set(): | |
| self._idle.set() | |
| time.sleep(0.05) | |
| continue | |
| self._idle.clear() |
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around
lines 1066 - 1101, Fix the race in TrafficGenerator._run by clearing self._idle
before checking self.enabled, or by holding self._lock across the enabled check
and submission, so pause_and_drain cannot observe an idle generator while a job
submission is in flight. Preserve the existing idle behavior when disabled and
ensure pause_and_drain waits before snapshot_outstanding.
| if kill_match is None: | ||
| kill_match = CANCEL_KILL_RE.search(text) | ||
| if kill_match is not None and kill_match.group(1) != target: | ||
| kill_match = None | ||
| if fallback_match is None: | ||
| fallback_match = CANCEL_FALLBACK_KILL_RE.search(text) | ||
| if ( | ||
| fallback_match is not None | ||
| and fallback_match.group(1) != target | ||
| ): | ||
| fallback_match = None |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use finditer for the kill-log scans so a non-matching first line does not hide the target.
CANCEL_KILL_RE.search(text) and CANCEL_FALLBACK_KILL_RE.search(text) return only the first match in the delta. If that first line names a different job_id, the code resets the variable to None and repeats the same search on every iteration. A later line that names target is never found. mode then stays cooperative or unknown, and the trial reports the wrong cancel mode or fails. cancel_log_names_job already uses finditer for the same log lines.
🐛 Proposed fix
if kill_match is None:
- kill_match = CANCEL_KILL_RE.search(text)
- if kill_match is not None and kill_match.group(1) != target:
- kill_match = None
+ kill_match = next(
+ (
+ m
+ for m in CANCEL_KILL_RE.finditer(text)
+ if m.group(1) == target
+ ),
+ None,
+ )
if fallback_match is None:
- fallback_match = CANCEL_FALLBACK_KILL_RE.search(text)
- if (
- fallback_match is not None
- and fallback_match.group(1) != target
- ):
- fallback_match = None
+ fallback_match = next(
+ (
+ m
+ for m in CANCEL_FALLBACK_KILL_RE.finditer(text)
+ if m.group(1) == target
+ ),
+ None,
+ )📝 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.
| if kill_match is None: | |
| kill_match = CANCEL_KILL_RE.search(text) | |
| if kill_match is not None and kill_match.group(1) != target: | |
| kill_match = None | |
| if fallback_match is None: | |
| fallback_match = CANCEL_FALLBACK_KILL_RE.search(text) | |
| if ( | |
| fallback_match is not None | |
| and fallback_match.group(1) != target | |
| ): | |
| fallback_match = None | |
| if kill_match is None: | |
| kill_match = next( | |
| ( | |
| m | |
| for m in CANCEL_KILL_RE.finditer(text) | |
| if m.group(1) == target | |
| ), | |
| None, | |
| ) | |
| if fallback_match is None: | |
| fallback_match = next( | |
| ( | |
| m | |
| for m in CANCEL_FALLBACK_KILL_RE.finditer(text) | |
| if m.group(1) == target | |
| ), | |
| None, | |
| ) |
🤖 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 `@python/cuopt/cuopt/tests/linear_programming/grpc_lifecycle_stress.py` around
lines 1234 - 1244, Update the kill-log scans in the surrounding lifecycle logic
to use finditer for both CANCEL_KILL_RE and CANCEL_FALLBACK_KILL_RE, selecting
the first match whose group(1) equals target instead of repeatedly searching
only the first line. Preserve the existing None behavior when no matching target
job_id is found, consistent with cancel_log_names_job.
chris-maes
left a comment
chris-maes
left a comment
There was a problem hiding this comment.
I'm in favor of adding the ability to stop the solver. Thanks for looking into this.
An important use case is the user pressing Ctrl-C to stop a MIP solve and still getting back some solution info in this case.
But this is a giant PR. Somethings seems off it takes ~3K lines of code to stop the solver.
Could we take a step back and work out the basic design of how we want this to work? For example, what is the termination code for canceled solves etc? Do we want to piggy back off CONCURRENT_HALT or do we want to separate this?
Then lets figure out how to implement this. I don't think we want to put this in the timer class. I also don't think we want to have to remap status codes.
2 participants
Poll a shared cancel flag like time limits so mid-solve cancel can unwind and avoid as much as possible the need for SIGKILL especially in the gRPC server, since SIGKILL can leave the GPU in an uninterruptible sleep in some cases. Remap limit statuses to Cancelled at solution finalization, preempt MIP heuristics on cancel.