Reduce concurrent LP host memory footprint - #1640
Conversation
Signed-off-by: Hugo Linsenmaier <hlinsenmaier@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe PR updates GPU-backed MPS adoption, propagates RAFT handles through barrier and concurrent LP solving, controls translation and problem lifetime, and reports available and total host memory. Solver and runtime updates
Estimated code review effort: 4 (Complex) | ~45 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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cpp/src/pdlp/solve.cu (1)
1612-1804: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftAdd a concurrent-solve regression test.
This task-local handle and winner-only conversion path has no supplied coverage. Add a small deterministic Concurrent LP test that validates feasibility/objective and returned primal/dual vector shapes regardless of which solver wins.
As per coding guidelines, “Contributors must add unit tests for code changes”.
🤖 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 1612 - 1804, Add a small deterministic Concurrent LP regression test covering the concurrent solve flow around dispatch_concurrent_solvers and the winner-only conversion branches. Configure the test so either solver may win, then assert the returned solution is feasible, has the expected objective, and has primal and dual vectors sized to the problem’s variables and constraints.Source: Coding guidelines
🤖 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/tests/linear_programming/unit_tests/solution_interface_test.cu`:
- Around line 518-534: Extend TEST_F(SolutionInterfaceTest,
gpu_adopt_consumes_mps_data_model) to snapshot representative objective and CSR
payload vectors from model before std::move, then retrieve the corresponding
vectors from problem after adopt_from_mps_data_model and compare their values,
while preserving the existing dimension, nnz, and moved-from assertions.
In `@cpp/tests/utilities/test_cli.cpp`:
- Around line 111-112: Strengthen the RAM assertions in the CLI test by parsing
the numeric values from the “RAM: ...” output, validating the expected GiB
format, and asserting total is positive while available is within 0 through
total. Keep the existing label checks, and use the parsed values rather than
only checking string presence.
---
Outside diff comments:
In `@cpp/src/pdlp/solve.cu`:
- Around line 1612-1804: Add a small deterministic Concurrent LP regression test
covering the concurrent solve flow around dispatch_concurrent_solvers and the
winner-only conversion branches. Configure the test so either solver may win,
then assert the returned solution is feasible, has the expected objective, and
has primal and dual vectors sized to the problem’s variables and constraints.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fff71895-38a7-4921-9ced-edd9b14c57cb
📒 Files selected for processing (8)
cpp/include/cuopt/mathematical_optimization/optimization_problem_utils.hppcpp/src/dual_simplex/solve.cppcpp/src/dual_simplex/solve.hppcpp/src/pdlp/solve.cucpp/src/pdlp/translate.hppcpp/src/utilities/version_info.cppcpp/tests/linear_programming/unit_tests/solution_interface_test.cucpp/tests/utilities/test_cli.cpp
| TEST_F(SolutionInterfaceTest, gpu_adopt_consumes_mps_data_model) | ||
| { | ||
| auto model = io::read_mps<int, double>(lp_file_); | ||
| const auto n_variables = model.get_n_variables(); | ||
| const auto n_constraints = model.get_n_constraints(); | ||
| const auto nnz = model.get_nnz(); | ||
|
|
||
| raft::handle_t handle; | ||
| optimization_problem_t<int, double> problem(&handle); | ||
| adopt_from_mps_data_model(&problem, std::move(model)); | ||
|
|
||
| EXPECT_EQ(model.get_n_variables(), 0); | ||
| EXPECT_EQ(model.get_n_constraints(), 0); | ||
| EXPECT_EQ(problem.get_n_variables(), n_variables); | ||
| EXPECT_EQ(problem.get_n_constraints(), n_constraints); | ||
| EXPECT_EQ(problem.get_nnz(), nnz); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert copied payload values as well.
This passes even if objective or CSR values are corrupted after model is cleared. Snapshot representative vectors before the move, then copy them back from problem and compare them after adoption.
As per path instructions, C++ tests should focus on “Numerical correctness validation (not just ‘runs without error’)”.
🤖 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/linear_programming/unit_tests/solution_interface_test.cu` around
lines 518 - 534, Extend TEST_F(SolutionInterfaceTest,
gpu_adopt_consumes_mps_data_model) to snapshot representative objective and CSR
payload vectors from model before std::move, then retrieve the corresponding
vectors from problem after adopt_from_mps_data_model and compare their values,
while preserving the existing dimension, nnz, and moved-from assertions.
Source: Path instructions
| EXPECT_TRUE(output.find("GiB total") != std::string::npos); | ||
| EXPECT_TRUE(output.find("GiB available") != std::string::npos); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the reported RAM values, not only the labels.
These checks pass even if the parser emits 0.00 GiB values. Parse the RAM: ... fields and verify the format plus sane bounds such as total > 0 and 0 <= available <= total.
As per path instructions, C++ utility tests should validate numerical correctness, not only that the command runs.
Suggested assertion shape
- EXPECT_TRUE(output.find("GiB total") != std::string::npos);
- EXPECT_TRUE(output.find("GiB available") != std::string::npos);
+ std::smatch match;
+ ASSERT_TRUE(std::regex_search(
+ output,
+ match,
+ std::regex(R"(RAM: ([0-9]+\.[0-9]{2}) GiB total, ([0-9]+\.[0-9]{2}) GiB available)")));
+ const auto total = std::stod(match[1].str());
+ const auto available = std::stod(match[2].str());
+ EXPECT_GT(total, 0.0);
+ EXPECT_GE(available, 0.0);
+ EXPECT_LE(available, total);📝 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.
| EXPECT_TRUE(output.find("GiB total") != std::string::npos); | |
| EXPECT_TRUE(output.find("GiB available") != std::string::npos); | |
| std::smatch match; | |
| ASSERT_TRUE(std::regex_search( | |
| output, | |
| match, | |
| std::regex(R"(RAM: ([0-9]+\.[0-9]{2}) GiB total, ([0-9]+\.[0-9]{2}) GiB available)"))); | |
| const auto total = std::stod(match[1].str()); | |
| const auto available = std::stod(match[2].str()); | |
| EXPECT_GT(total, 0.0); | |
| EXPECT_GE(available, 0.0); | |
| EXPECT_LE(available, total); |
🤖 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/utilities/test_cli.cpp` around lines 111 - 112, Strengthen the RAM
assertions in the CLI test by parsing the numeric values from the “RAM: ...”
output, validating the expected GiB format, and asserting total is positive
while available is within 0 through total. Keep the existing label checks, and
use the parsed values rather than only checking string presence.
Source: Path instructions
| f_t start_time, | ||
| lp_solution_t<i_t, f_t>& solution) | ||
| lp_solution_t<i_t, f_t>& solution, | ||
| const raft::handle_t* handle_ptr) |
There was a problem hiding this comment.
Why does passing in the handle_ptr (rather than getting it from the user_problem) help reduce memory?
There was a problem hiding this comment.
This is refactoring that allows us to get rid of this copy:
auto barrier_problem = dual_simplex_problem;
barrier_problem.handle_ptr = barrier_handle_ptr.get();
There was a problem hiding this comment.
where is that in the code? I see barrier_lp but that is after column scaling?
There was a problem hiding this comment.
| return; | ||
| } | ||
| populate_from_mps_data_model(problem, data_model); | ||
| if (auto* gpu_problem = dynamic_cast<optimization_problem_t<i_t, f_t>*>(problem)) { |
There was a problem hiding this comment.
I assume the = is intentional and that you didn't want == but you might want move that out of the if to be clear.
auto* gpu_problem = dynamic_cast<optimization_problem_t<i_t, f_t>*>(problem);
if (gpu_problem) {
...
}
There was a problem hiding this comment.
You can do this as well
if (auto* gpu_problem = dynamic_cast<optimization_problem_t<i_t, f_t>*>(problem); gpu_problem) {
...
}
CI Test Summary✅ All 31 test job(s) passed. |
Signed-off-by: Hugo Linsenmaier <hlinsenmaier@gmail.com>
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)
cpp/src/pdlp/solve.cu (1)
526-538: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSize the Barrier result before uncrush.
solutionstarts as(0, 0), but the concurrent external-handle path skips the only resize. The Barrier solve then writes throughuncrush_primal_solution/uncrush_dual_solution; unlikesolve_linear_programatcpp/src/dual_simplex/solve.cppLine 702, this path does not size the output first. Resize immediately before uncrushing in the Barrier implementation, and add a GoogleTest regression for this path.As per coding guidelines, contributors must add unit tests for code changes using GoogleTest examples under
cpp/src/tests.🤖 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 526 - 538, Resize the Barrier solver’s output solution immediately before uncrushing, covering the concurrent external-handle path where the existing handle_ptr == nullptr branch does not run; update the relevant Barrier implementation around uncrush_primal_solution/uncrush_dual_solution while preserving existing sizing behavior. Add a GoogleTest regression under cpp/src/tests that exercises this external-handle path and verifies the result is sized and completes successfully.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@cpp/src/pdlp/solve.cu`:
- Around line 526-538: Resize the Barrier solver’s output solution immediately
before uncrushing, covering the concurrent external-handle path where the
existing handle_ptr == nullptr branch does not run; update the relevant Barrier
implementation around uncrush_primal_solution/uncrush_dual_solution while
preserving existing sizing behavior. Add a GoogleTest regression under
cpp/src/tests that exercises this external-handle path and verifies the result
is sized and completes successfully.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: cc938589-351b-425e-bf36-b88c3f7813ed
📒 Files selected for processing (3)
cpp/src/dual_simplex/solve.cppcpp/src/pdlp/solve.cucpp/src/utilities/version_info.cpp
🚧 Files skipped from review as they are similar to previous changes (1)
- cpp/src/utilities/version_info.cpp
Signed-off-by: Hugo Linsenmaier <hlinsenmaier@gmail.com>
Signed-off-by: Hugo Linsenmaier <hlinsenmaier@gmail.com>
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)
cpp/src/pdlp/solve.cu (1)
1604-1609: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore device 1 before destroying
barrier_handle_ptr.
barrier_handle_ptrcreates CUDA library resources on device 1, butreset()runs on the dispatching thread after the device scope ends. Wrap the reset inraft::device_setter(1)for multi-GPU mode and add a regression test.🤖 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 1604 - 1609, Update the cleanup following run_barrier_thread so barrier_handle_ptr.reset() executes within raft::device_setter(1) when multi-GPU mode is active, ensuring device 1 is restored before destroying its CUDA resources. Preserve the existing cleanup behavior for non-multi-GPU mode and add a regression test covering destruction after the device scope ends.Source: MCP tools
🧹 Nitpick comments (1)
cpp/src/pdlp/solve.cu (1)
1693-1718: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd regression coverage for concurrent result selection.
Existing tests cover the dual-simplex winner only. Add deterministic GoogleTest coverage for the Barrier winner, PDLP winner,
ConcurrentLimitfallback, andinside_mipwith Barrier disabled.🤖 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 1693 - 1718, Add deterministic GoogleTest regression cases for concurrent result selection, extending the existing dual-simplex winner coverage to verify Barrier selection, PDLP selection, ConcurrentLimit fallback, and inside_mip behavior when Barrier is disabled. Reuse the existing concurrent-solve test fixtures and assert each case returns the expected solver result without introducing unrelated test changes.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@cpp/src/pdlp/solve.cu`:
- Around line 1604-1609: Update the cleanup following run_barrier_thread so
barrier_handle_ptr.reset() executes within raft::device_setter(1) when multi-GPU
mode is active, ensuring device 1 is restored before destroying its CUDA
resources. Preserve the existing cleanup behavior for non-multi-GPU mode and add
a regression test covering destruction after the device scope ends.
---
Nitpick comments:
In `@cpp/src/pdlp/solve.cu`:
- Around line 1693-1718: Add deterministic GoogleTest regression cases for
concurrent result selection, extending the existing dual-simplex winner coverage
to verify Barrier selection, PDLP selection, ConcurrentLimit fallback, and
inside_mip behavior when Barrier is disabled. Reuse the existing
concurrent-solve test fixtures and assert each case returns the expected solver
result without introducing unrelated test changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 4ea47c27-05ce-477b-9f2b-5715a89772b5
📒 Files selected for processing (1)
cpp/src/pdlp/solve.cu
Signed-off-by: Hugo Linsenmaier <hlinsenmaier@gmail.com>
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 (3)
cpp/src/pdlp/solve.cu (3)
1696-1702: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPropagate all terminal solver statuses.
run_barriersetsconcurrent_haltforINFEASIBLE,UNBOUNDED, andUNBOUNDED_OR_INFEASIBLEat Line 538-544. The selection logic accepts only BarrierOPTIMAL, anddual_simplex_solvedalso excludesUNBOUNDED_OR_INFEASIBLE. In MIP, the dual-simplex task is skipped. If Barrier proves infeasibility or unboundedness, PDLP can returnConcurrentLimit, and the final branches can return that status instead of the Barrier proof. Preserve all terminal Barrier statuses. Use a status-only solution when no valid vectors exist.As per coding guidelines, add GoogleTest coverage under
cpp/src/testsfor these concurrent terminal-status cases.Also applies to: 1737-1746, 1769-1789
🤖 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 1696 - 1702, Update the concurrent solver status selection around dual_simplex_status, barrier_status, dual_simplex_solved, and the final branches to propagate Barrier INFEASIBLE, UNBOUNDED, and UNBOUNDED_OR_INFEASIBLE results, including MIP cases where dual simplex is skipped, instead of returning ConcurrentLimit. Ensure status-only solutions are used when no valid solution vectors exist, and add GoogleTest coverage under cpp/src/tests for these terminal-status scenarios.Source: Coding guidelines
1704-1715: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winRelease PDLP buffers before converting a CPU winner.
sol_pdlpcan still own three GPU solution vectors whenconvert_dual_simplex_solallocates three more vectors. The converted winner remains alive whilesol_pdlp.copy_fromor the direct return executes. Reset or move the losing PDLP solution before conversion. Return or move the converted winner instead of copying it into the old PDLP result where possible. Apply this ordering to all three branches.Also applies to: 1737-1746, 1773-1784
🤖 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 1704 - 1715, In the three solution-selection branches around convert_dual_simplex_sol, release or move the losing sol_pdlp GPU solution before conversion so its buffers are not live alongside the conversion allocations. After conversion, return or move the converted winner directly where possible instead of copying it into the old PDLP result; preserve the existing winner-selection behavior across all three branches.
529-533: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winProject non-optimal SOC-expanded Barrier results before conversion.
For QCMATRIX/SOC problems, non-optimal Barrier results retain the expanded
user_problem.num_colsshape.convert_dual_simplex_solthen attaches original problem metadata without changing vector sizes. Project valid non-optimal vectors before conversion, or return a status-only solution with original dimensions. Add GoogleTest coverage for TIME_LIMIT and ITERATION_LIMIT results.🤖 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 529 - 533, Update the solve flow around simplex::lp_status_t::OPTIMAL so valid non-optimal Barrier results, especially TIME_LIMIT and ITERATION_LIMIT, are projected from the SOC-expanded user_problem shape to model-variable dimensions before convert_dual_simplex_sol attaches original metadata; alternatively return a status-only solution with original dimensions. Add GoogleTest coverage for both limit statuses and verify the resulting vector sizes.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@cpp/src/pdlp/solve.cu`:
- Around line 1696-1702: Update the concurrent solver status selection around
dual_simplex_status, barrier_status, dual_simplex_solved, and the final branches
to propagate Barrier INFEASIBLE, UNBOUNDED, and UNBOUNDED_OR_INFEASIBLE results,
including MIP cases where dual simplex is skipped, instead of returning
ConcurrentLimit. Ensure status-only solutions are used when no valid solution
vectors exist, and add GoogleTest coverage under cpp/src/tests for these
terminal-status scenarios.
- Around line 1704-1715: In the three solution-selection branches around
convert_dual_simplex_sol, release or move the losing sol_pdlp GPU solution
before conversion so its buffers are not live alongside the conversion
allocations. After conversion, return or move the converted winner directly
where possible instead of copying it into the old PDLP result; preserve the
existing winner-selection behavior across all three branches.
- Around line 529-533: Update the solve flow around
simplex::lp_status_t::OPTIMAL so valid non-optimal Barrier results, especially
TIME_LIMIT and ITERATION_LIMIT, are projected from the SOC-expanded user_problem
shape to model-variable dimensions before convert_dual_simplex_sol attaches
original metadata; alternatively return a status-only solution with original
dimensions. Add GoogleTest coverage for both limit statuses and verify the
resulting vector sizes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 3fa61f2a-04c7-42e0-aea5-19a8f91a8f57
📒 Files selected for processing (3)
cpp/src/dual_simplex/solve.cppcpp/src/pdlp/solve.cucpp/src/pdlp/translate.hpp
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/pdlp/translate.hpp
- cpp/src/dual_simplex/solve.cpp
| return; | ||
| } | ||
| populate_from_mps_data_model(problem, data_model); | ||
| if (auto* gpu_problem = dynamic_cast<optimization_problem_t<i_t, f_t>*>(problem)) { |
There was a problem hiding this comment.
You can do this as well
if (auto* gpu_problem = dynamic_cast<optimization_problem_t<i_t, f_t>*>(problem); gpu_problem) {
...
}
| barrier_settings, | ||
| timer.get_tic_start(), | ||
| solution, | ||
| handle_ptr == nullptr ? user_problem.handle_ptr : handle_ptr); |
| template <typename i_t, typename f_t> | ||
| static simplex::user_problem_t<i_t, f_t> cuopt_problem_to_user_problem( | ||
| raft::handle_t const* handle_ptr, mip::problem_t<i_t, f_t>& model) | ||
| raft::handle_t const* handle_ptr, mip::problem_t<i_t, f_t>& model, bool copy_names = true) |
There was a problem hiding this comment.
I think we should just remove names from problem_t class, its internal to the solver and not exposed to public APIs.
4 participants
On B200,
mcf_2500_100_500peak memory dropped from168.12 GiBon main to143.73 GiBwith this change, a24.39 GiB (14.5%)reduction