Conversation
|
/ok to test e59c9cd |
nguidotti
left a comment
There was a problem hiding this comment.
Looks good to me. Thanks for the fix, Alice.
📝 WalkthroughWalkthroughTwo files are modified: one refactors RMM async memory resource handling by removing shared pointer wrapper and updating call sites to use direct object passing instead of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
benchmarks/linear_programming/cuopt/run_mip.cpp (1)
535-560:⚠️ Potential issue | 🟠 MajorMove
run_single_file()into the adaptor scope.When
--memory-limitor--track-allocationsis enabled,limiting_adaptor/tracking_adaptoris destroyed at the end of its branch, butrun_single_file()executes afterward. Sinceset_current_device_resource()stores a non-owning pointer, the solver will run against a dangling memory resource in those modes, causing undefined behavior.Move the solve call into each branch to keep the adaptor alive throughout execution.
Suggested fix
auto memory_resource = make_async(); + auto run = [&] { + run_single_file(path, + 0, + 0, + n_gpus, + out_dir, + initial_solution_file, + heuristics_only, + num_cpu_threads, + write_log_file, + log_to_console, + reliability_branching, + time_limit, + work_limit, + deterministic); + }; if (memory_limit > 0) { auto limiting_adaptor = rmm::mr::limiting_resource_adaptor(memory_resource, memory_limit * 1024ULL * 1024ULL); rmm::mr::set_current_device_resource(limiting_adaptor); + run(); } else if (track_allocations) { rmm::mr::tracking_resource_adaptor tracking_adaptor(memory_resource, /*capture_stacks=*/true); rmm::mr::set_current_device_resource(tracking_adaptor); + run(); } else { rmm::mr::set_current_device_resource(memory_resource); + run(); } - run_single_file(path, - 0, - 0, - n_gpus, - out_dir, - initial_solution_file, - heuristics_only, - num_cpu_threads, - write_log_file, - log_to_console, - reliability_branching, - time_limit, - work_limit, - deterministic);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@benchmarks/linear_programming/cuopt/run_mip.cpp` around lines 535 - 560, The call to run_single_file() uses the current device resource after limiting_adaptor or tracking_adaptor have been destroyed; keep the adaptor alive by moving the run_single_file(...) invocation into each branch that creates an adaptor (the branch that builds limiting_adaptor and the branch that builds tracking_adaptor) and also call it in the else branch where you set_current_device_resource(memory_resource), so that the lifetime of limiting_adaptor/tracking_adaptor encloses the call; reference memory_resource, limiting_adaptor, tracking_adaptor, rmm::mr::set_current_device_resource, and run_single_file when making the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@benchmarks/linear_programming/cuopt/run_mip.cpp`:
- Around line 535-560: The call to run_single_file() uses the current device
resource after limiting_adaptor or tracking_adaptor have been destroyed; keep
the adaptor alive by moving the run_single_file(...) invocation into each branch
that creates an adaptor (the branch that builds limiting_adaptor and the branch
that builds tracking_adaptor) and also call it in the else branch where you
set_current_device_resource(memory_resource), so that the lifetime of
limiting_adaptor/tracking_adaptor encloses the call; reference memory_resource,
limiting_adaptor, tracking_adaptor, rmm::mr::set_current_device_resource, and
run_single_file when making the change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b330eefd-5406-4df4-8df0-0b7c58596057
📒 Files selected for processing (2)
benchmarks/linear_programming/cuopt/run_mip.cppcpp/tests/mip/determinism_test.cu
This PR fixes the B&B determinism test flakiness by replacing seymour1.mps with another instance which completes faster.
run_mip.cpp was also fixed to build following the recent CCCL MR changes.
Closes #1134
Description
Issue
Checklist