Reduce concurrent LP host memory footprint - #1640
Conversation
Signed-off-by: Hugo Linsenmaier <hlinsenmaier@gmail.com>
📝 WalkthroughWalkthroughChangesThe PR updates GPU-backed MPS adoption, propagates RAFT handles through barrier and concurrent LP solving, manages presolved problem lifetime with ChangesSolver 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?
| 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) {
...
}
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
3 participants
On B200,
mcf_2500_100_500peak memory dropped from168.12 GiBon main to143.73 GiBwith this change, a24.39 GiB (14.5%)reduction