change semantics of problem construction in cuOptReadProblem#1559
change semantics of problem construction in cuOptReadProblem#1559tmckayus wants to merge 1 commit into
Conversation
since the mps data model is constructed internally, use move semantics when building a CPU problem to avoid a copy
tmckayus
added
non-breaking
📝 WalkthroughWalkthroughCPU-backed optimization problems now adopt parsed MPS/QPS storage without copying, while device-backed problems retain population fallback. Reader call sites use the new path, solution parsing uses problem-owned variable names, and tests compare adoption with population. ChangesMPS Data Model Adoption
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.
🧹 Nitpick comments (4)
cpp/tests/linear_programming/unit_tests/solution_interface_test.cu (1)
495-517: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo test case exercises a non-empty quadratic objective through the adopt path.
expect_adopt_matches_populatealready asserts onget_quadratic_objective_values/indices/offsets()(lines 471-473), but neitheradopt_matches_populate_lp_filenoradopt_matches_populate_quadratic_constraintssupplies a model with a quadratic objective — only quadratic constraints are covered. TheQ_values_/Q_indices_/Q_offsets_move branch incpu_optimization_problem_t::adopt_from_mps_data_model(cpp_optimization_problem.cpp lines 1165-1167) is therefore untested against the populate baseline.As per path instructions for
cpp/tests/**, tests should validate numerical correctness for the specific paths under test rather than just "runs without error." Want me to draft a QP-objective test case (e.g.read_lp_from_stringwith a quadratic objective term) reusingexpect_adopt_matches_populate?🤖 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 495 - 517, Add a test in SolutionInterfaceTest that builds a model with a non-empty quadratic objective using read_lp_from_string, then passes it to expect_adopt_matches_populate. Keep the existing quadratic-constraint coverage and ensure the new case exercises the quadratic objective values, indices, and offsets comparison through the adopt path.Source: Path instructions
cpp/src/pdlp/cpu_optimization_problem.cpp (2)
1139-1183: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdopt path bypasses the validating setters used by the populate path.
adopt_from_mps_data_modelassignsA_,Q_values_/Q_indices_/Q_offsets_, etc. directly viastd::move, skipping thecuopt_expectsnull/size checks thatset_csr_constraint_matrix/set_quadratic_objective_matrix(andpopulate_from_mps_data_model) perform. This is presumably fine sincemps_data_model_tis already internally consistent after parsing, but it means a parser defect would no longer be caught here and could surface later as an out-of-bounds read (e.g. indexingQ_offsets_[n_vars]downstream).Consider adding lightweight invariant checks (e.g.
cuopt_expectson offsets/size consistency) mirroring what the setters enforce, so a parser bug fails fast instead of propagating silently.🤖 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/cpu_optimization_problem.cpp` around lines 1139 - 1183, adopt_from_mps_data_model bypasses validation performed by the matrix setters, allowing malformed CSR data to propagate. Add lightweight cuopt_expects invariant checks in this method for A_ and Q_values_/Q_indices_/Q_offsets_ nullability, offset lengths, and index/value size consistency, mirroring set_csr_constraint_matrix and set_quadratic_objective_matrix before downstream use.
1114-1135: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winUse explicit field assignment here. The two
quadratic_constraint_tlayouts match today, but this positional aggregate init is coupled to both declarations and can break silently if either is reordered.add_quadratic_constraintalready uses named assignments, so mirroring that pattern here would be safer.🤖 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/cpu_optimization_problem.cpp` around lines 1114 - 1135, Replace the positional aggregate initialization in move_quadratic_constraints_from_model with explicit field-by-field assignments, mirroring the named-assignment pattern used by add_quadratic_constraint. Populate each destination quadratic_constraint_t member from qc, preserving move semantics for strings and vectors, then append the fully initialized object to converted.cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp (1)
85-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment missing
@paramtag.The brief and moved-from note are good, but there's no
@paramdescribingdata_model, unlike the free-function counterpart inoptimization_problem_utils.hppwhich documents@tparam/@paramfully.As per path instructions, "New public functions/classes need Doxygen-style documentation" and "Verify parameter descriptions match actual types/behavior" for public C++ headers under
cpp/include/cuopt/**/*.📝 Proposed doc addition
/** * `@brief` Transfer parsed MPS/QPS storage into this CPU problem without copying array/string data. * * The model is left in a moved-from state and must not be used afterward. + * + * `@param`[in,out] data_model Parsed MPS/QPS model to adopt; left moved-from on return. */ void adopt_from_mps_data_model(io::mps_data_model_t<i_t, f_t>&& data_model);🤖 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/cpu_optimization_problem.hpp` around lines 85 - 91, Add a Doxygen `@param` description for the rvalue-reference parameter data_model in adopt_from_mps_data_model, stating that it is the parsed MPS/QPS storage being adopted without copying and is left moved-from afterward; keep the documentation consistent with the corresponding free function.Source: Path instructions
🤖 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.
Nitpick comments:
In `@cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp`:
- Around line 85-91: Add a Doxygen `@param` description for the rvalue-reference
parameter data_model in adopt_from_mps_data_model, stating that it is the parsed
MPS/QPS storage being adopted without copying and is left moved-from afterward;
keep the documentation consistent with the corresponding free function.
In `@cpp/src/pdlp/cpu_optimization_problem.cpp`:
- Around line 1139-1183: adopt_from_mps_data_model bypasses validation performed
by the matrix setters, allowing malformed CSR data to propagate. Add lightweight
cuopt_expects invariant checks in this method for A_ and
Q_values_/Q_indices_/Q_offsets_ nullability, offset lengths, and index/value
size consistency, mirroring set_csr_constraint_matrix and
set_quadratic_objective_matrix before downstream use.
- Around line 1114-1135: Replace the positional aggregate initialization in
move_quadratic_constraints_from_model with explicit field-by-field assignments,
mirroring the named-assignment pattern used by add_quadratic_constraint.
Populate each destination quadratic_constraint_t member from qc, preserving move
semantics for strings and vectors, then append the fully initialized object to
converted.
In `@cpp/tests/linear_programming/unit_tests/solution_interface_test.cu`:
- Around line 495-517: Add a test in SolutionInterfaceTest that builds a model
with a non-empty quadratic objective using read_lp_from_string, then passes it
to expect_adopt_matches_populate. Keep the existing quadratic-constraint
coverage and ensure the new case exercises the quadratic objective values,
indices, and offsets comparison through the adopt path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: d3d99f78-2913-436c-9b2b-fed8f8d760fc
📒 Files selected for processing (6)
cpp/cuopt_cli.cppcpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hppcpp/include/cuopt/mathematical_optimization/optimization_problem_utils.hppcpp/src/pdlp/cpu_optimization_problem.cppcpp/src/pdlp/cuopt_c.cppcpp/tests/linear_programming/unit_tests/solution_interface_test.cu
CI Test Summary✅ All 31 test job(s) passed. |
| #include <string> | ||
| #include <vector> | ||
|
|
||
| namespace cuopt::mathematical_optimization::io { |
There was a problem hiding this comment.
Nit: move this inside the cuopt::mathematical_optimization block below with a namespace io.
|
|
||
| cuopt::mathematical_optimization::populate_from_mps_data_model(problem_interface.get(), | ||
| mps_data_model); | ||
| // adopt_from_mps_data_model moves the parsed storage into a HOST-backed problem and falls back to |
There was a problem hiding this comment.
Why is this comment needed? It looks like it just says what the function does; any relevant details should be on the function docstring.
| // adopt_from_mps_data_model must produce a problem identical to populate_from_mps_data_model. | ||
| TEST_F(SolutionInterfaceTest, adopt_matches_populate_lp_file) | ||
| { | ||
| const auto model = cuopt::mathematical_optimization::io::read_mps<int, double>(lp_file_); |
There was a problem hiding this comment.
| const auto model = cuopt::mathematical_optimization::io::read_mps<int, double>(lp_file_); | |
| const auto model = io::read_mps<int, double>(lp_file_); |
|
|
||
| TEST_F(SolutionInterfaceTest, adopt_matches_populate_quadratic_constraints) | ||
| { | ||
| const auto model = cuopt::mathematical_optimization::io::read_lp_from_string<int, double>(R"LP( |
There was a problem hiding this comment.
| const auto model = cuopt::mathematical_optimization::io::read_lp_from_string<int, double>(R"LP( | |
| const auto model = io::read_lp_from_string<int, double>(R"LP( |
We should check why code is generated in this style and if there's anything we can do to prevent it.
3 participants
Since the mps data model is constructed internally, use move semantics when building a CPU problem to avoid an unnecessary copy. This is a small optimization but on a large problem with 10M variables and 200k constraints, it can save half a second in problem creation.