Improve QC COO canonicalization performance#1516
Conversation
Store quadratic-constraint Q in canonical COO (one cross coefficient per variable pair) and run canonicalization at ingest boundaries (MPS/LP, C API, Python→solver, gRPC, MPS export). The RSOC fast path now accepts a single cross term (e.g. -2*x0*x1 for ||tail||^2 <= 2*x0*x1) instead of requiring symmetric MPS-style halves, which previously routed natural API forms to the general QC path and produced wrong optima. MPS QCMATRIX still accepts symmetric halves; the writer expands canonical cross terms back to MPS form on export. Adds tests and doc updates for C API, parser, and examples. Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
…im redundant canonicalization comments Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
Replace qc_coo options/wrappers with canonicalize_coo_matrix(rows,cols,vals,bool). Use machine epsilon for zero checks; sum both orientations into one (lo,hi) entry. Test via append_quadratic_constraint; update gRPC codegen call sites. Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
…out only for mps files Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
…atic cost in quadratic constraints Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
…icalization with sort-based algorithms (adding strict duplicate-entry rejection and fail-fast validation during parsing), and update the parser tests accordingly. Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
…st the grpc test data avoiding the effect from dropped 0 entry Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
Signed-off-by: YUWEN Chen <yuwchen@nvidia.com>
Signed-off-by: YUWEN Chen <yuwchen@nvidia.com>
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com> # Conflicts: # cpp/include/cuopt/mathematical_optimization/io/mps_data_model.hpp # cpp/src/grpc/grpc_problem_mapper.cpp # cpp/src/io/data_model_view.cpp # cpp/src/io/mps_parser.cpp # cpp/src/io/mps_writer.cpp
Signed-off-by: yuwenchen95 <yuwchen@nvidia.com>
Signed-off-by: YUWEN Chen <yuwchen@nvidia.com>
Signed-off-by: YUWEN Chen <yuwchen@nvidia.com>
Signed-off-by: YUWEN Chen <yuwchen@nvidia.com>
|
/ok to test fc12d4a |
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds a reusable workspace for quadratic COO canonicalization, updates canonicalization and append APIs to take it, and threads the new parameter through parsing, writing, optimization problem code, generated conversions, and tests. ChangesQuadratic constraint COO canonicalization workspace
Estimated code review effort: 3 (Moderate) | ~30 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
CI Test Summary✅ All 31 test job(s) passed. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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/include/cuopt/mathematical_optimization/io/mps_data_model.hpp (1)
328-348: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMissing
@param workspacedoc for the new parameter.Every other parameter of
append_quadratic_constraintis documented, but the newly-addedworkspaceparameter has no@paramentry.As per path instructions, "New public functions/classes need Doxygen-style documentation" and "Verify parameter descriptions match actual types/behavior" for
cpp/include/cuopt/**/*.📝 Proposed doc fix
* `@param` constraint_row_type MPS ROWS type: 'L' (<=) or 'G' (>=). Stored as given; 'G' rows are * converted to '<=' form when building the SOCP for the barrier solver. Equality ('E') is * not supported. + * `@param` workspace Reusable scratch workspace for COO canonicalization. Safe to reuse across + * successive calls (even for different constraints) but must not be shared across + * concurrent invocations. */🤖 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/io/mps_data_model.hpp` around lines 328 - 348, The append_quadratic_constraint documentation is missing a Doxygen `@param` entry for the new workspace argument. Update the comment above append_quadratic_constraint to document workspace with its actual type coo_canonicalization_workspace_t<i_t, f_t>& and describe that it is the reusable canonicalization scratch space used when ingesting the quadratic COO triplets.Source: Path instructions
🧹 Nitpick comments (2)
cpp/include/cuopt/mathematical_optimization/optimization_problem.hpp (1)
427-429: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winSame thread-safety documentation suggestion as
cpu_optimization_problem_t.
qc_coo_workspace_is reused acrossadd_quadratic_constraintcalls; concurrent use on the same instance is unsafe. Worth a short doc note, consistent with the equivalent member incpu_optimization_problem_t.As per path instructions,
cpp/include/cuopt/**/*: "Suggest documenting thread-safety, GPU requirements, numerical 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/include/cuopt/mathematical_optimization/optimization_problem.hpp` around lines 427 - 429, Add a short thread-safety note for qc_coo_workspace_ in optimization_problem_t, matching the documentation style used in cpu_optimization_problem_t. Clarify that this reusable workspace is mutated during add_quadratic_constraint calls, so concurrent use on the same optimization_problem_t instance is unsafe and callers should serialize access or use separate instances.Source: Path instructions
cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp (1)
228-230: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winConsider documenting thread-safety of the reused workspace.
qc_coo_workspace_is shared mutable state reused acrossadd_quadratic_constraintcalls on the same instance. Concurrent calls toadd_quadratic_constraintfrom multiple threads on the same object would race on this workspace (in addition to the existingquadratic_constraints_.push_back). A short doc comment noting the class/method is not thread-safe for concurrent mutation would help users avoid misuse.Note:
mutableis unnecessary here sinceadd_quadratic_constraintis notconst.As per path instructions,
cpp/include/cuopt/**/*: "Suggest documenting thread-safety, GPU requirements, numerical 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/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp` around lines 228 - 230, Document that the reused workspace in CPUOptimizationProblem is not safe for concurrent mutation, since qc_coo_workspace_ is shared state used by add_quadratic_constraint alongside quadratic_constraints_.push_back. Add a short comment near qc_coo_workspace_ or add_quadratic_constraint stating the class/method is not thread-safe for simultaneous calls on the same instance, and remove mutable from qc_coo_workspace_ because add_quadratic_constraint is already non-const.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.
Outside diff comments:
In `@cpp/include/cuopt/mathematical_optimization/io/mps_data_model.hpp`:
- Around line 328-348: The append_quadratic_constraint documentation is missing
a Doxygen `@param` entry for the new workspace argument. Update the comment above
append_quadratic_constraint to document workspace with its actual type
coo_canonicalization_workspace_t<i_t, f_t>& and describe that it is the reusable
canonicalization scratch space used when ingesting the quadratic COO triplets.
---
Nitpick comments:
In `@cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp`:
- Around line 228-230: Document that the reused workspace in
CPUOptimizationProblem is not safe for concurrent mutation, since
qc_coo_workspace_ is shared state used by add_quadratic_constraint alongside
quadratic_constraints_.push_back. Add a short comment near qc_coo_workspace_ or
add_quadratic_constraint stating the class/method is not thread-safe for
simultaneous calls on the same instance, and remove mutable from
qc_coo_workspace_ because add_quadratic_constraint is already non-const.
In `@cpp/include/cuopt/mathematical_optimization/optimization_problem.hpp`:
- Around line 427-429: Add a short thread-safety note for qc_coo_workspace_ in
optimization_problem_t, matching the documentation style used in
cpu_optimization_problem_t. Clarify that this reusable workspace is mutated
during add_quadratic_constraint calls, so concurrent use on the same
optimization_problem_t instance is unsafe and callers should serialize access or
use separate instances.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: fc032444-fa4b-4a7d-ad4a-26c040f89054
⛔ Files ignored due to path filters (2)
cpp/src/grpc/codegen/generated/generated_chunked_arrays_to_problem.incis excluded by!**/generated/**cpp/src/grpc/codegen/generated/generated_proto_to_problem.incis excluded by!**/generated/**
📒 Files selected for processing (13)
cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hppcpp/include/cuopt/mathematical_optimization/io/mps_data_model.hppcpp/include/cuopt/mathematical_optimization/optimization_problem.hppcpp/include/cuopt/mathematical_optimization/optimization_problem_utils.hppcpp/src/grpc/codegen/generate_conversions.pycpp/src/io/lp_parser.cppcpp/src/io/mps_data_model.cppcpp/src/io/mps_parser.cppcpp/src/io/mps_parser_internal.hppcpp/src/io/mps_writer.cppcpp/src/pdlp/cpu_optimization_problem.cppcpp/src/pdlp/optimization_problem.cucpp/tests/linear_programming/parser_test.cpp
💤 Files with no reviewable changes (1)
- cpp/src/io/mps_parser_internal.hpp
| for (size_t p = row_output_start; p < row_output_end; ++p) { | ||
| workspace.col_position_[entries[p].col] = workspace_t::invalid_index; | ||
| } | ||
| std::sort(entries.begin() + row_output_start, |
There was a problem hiding this comment.
Do we actually need columns to be sorted within rows?
There was a problem hiding this comment.
Same reason as for the sorting on rows.
| } else { | ||
| entries[static_cast<size_t>(marker[static_cast<size_t>(c)] - 1)].val += | ||
| vals[static_cast<size_t>(k)]; | ||
| std::sort(active_rows.begin(), active_rows.end(), [](const auto& lhs, const auto& rhs) { |
There was a problem hiding this comment.
Do we actually need rows to be sorted?
There was a problem hiding this comment.
I'm afraid we can't avoid it because the original bucket sorting relies on the assumption of sorted indices that we can do prefix sum and nnz entries are implicitly row-ordered by putting them into the ordered buckets 1 to n. Otherwise, we have to use unordered_map for counting.
| try { | ||
| // Discover active canonical rows and count entries in first-appearance order. | ||
| for (size_t k = 0; k < n; ++k) { | ||
| const i_t row = std::min(rows[k], cols[k]); |
There was a problem hiding this comment.
Consider doing a pass at the beginning to put all the entries in the correct triangle. This would reduce the context needed to understand the code and reduce the incidence of max and min in various spots.
There was a problem hiding this comment.
This is a tradeoff between a cleaner code design and avoiding an extra for loop.
| * entries that were touched. A workspace must not be shared across concurrent calls. | ||
| */ | ||
| template <typename i_t, typename f_t> | ||
| class coo_canonicalization_workspace_t { |
There was a problem hiding this comment.
This class is an implementation detail and doesn't really belong in a header file. The use of "friend" below hints at this. Most of the call sites are using canonicalize_quadratic_constraints so they don't need to see coo_canonicalization_workspace_t. Perhaps non-performance-sensitive sites can call a version that allocates a new coo_canonicalization_workspace_t?
Alternatively find a way to make this helper opaque so the body can be moved to a cpp file.
There was a problem hiding this comment.
The class is exposed due to the existence of append_quadratic_constraint where we add quadratic constraint incrementally while want to keep reusing workspace. Dropping reusing workspace will slow the speed a lot when using add_quadratic_constraint.
I made a new commit trying to keep reusing workspace without exposing the class details in mps_data_model.hpp.
Signed-off-by: YUWEN Chen <yuwchen@nvidia.com>
Signed-off-by: YUWEN Chen <yuwchen@nvidia.com>
Signed-off-by: YUWEN Chen <yuwchen@nvidia.com>
Signed-off-by: YUWEN Chen <yuwchen@nvidia.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/include/cuopt/mathematical_optimization/io/mps_data_model.hpp (1)
275-295: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winMissing
@param workspacedocumentation onappend_quadratic_constraint.The Doxygen block documents
linear_values,linear_indices,vals/rows/cols, andconstraint_row_type, but not the newly addedworkspaceparameter.As per path instructions, "New public functions/classes need Doxygen-style documentation" and reviewers should "Verify parameter descriptions match actual types/behavior" for
cpp/include/cuopt/**/*.📝 Proposed doc fix
* `@param` constraint_row_type MPS ROWS type: 'L' (<=) or 'G' (>=). Stored as given; 'G' rows are * converted to '<=' form when building the SOCP for the barrier solver. Equality ('E') is * not supported. + * `@param` workspace Reusable scratch storage for COO canonicalization; must not be shared across + * concurrent calls. */ void append_quadratic_constraint(i_t constraint_row_index,🤖 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/io/mps_data_model.hpp` around lines 275 - 295, The Doxygen comment for append_quadratic_constraint is missing documentation for the new workspace parameter, so update the existing block to add an `@param` entry for coo_canonicalization_workspace_t<i_t, f_t>& workspace and describe its role in canonicalizing the quadratic COO input; keep the rest of the parameter descriptions consistent with the actual types and behavior of the 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.
Outside diff comments:
In `@cpp/include/cuopt/mathematical_optimization/io/mps_data_model.hpp`:
- Around line 275-295: The Doxygen comment for append_quadratic_constraint is
missing documentation for the new workspace parameter, so update the existing
block to add an `@param` entry for coo_canonicalization_workspace_t<i_t, f_t>&
workspace and describe its role in canonicalizing the quadratic COO input; keep
the rest of the parameter descriptions consistent with the actual types and
behavior of the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 0208052b-8614-4a8d-b674-7cbddab7e56e
📒 Files selected for processing (8)
cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hppcpp/include/cuopt/mathematical_optimization/io/mps_data_model.hppcpp/include/cuopt/mathematical_optimization/optimization_problem.hppcpp/src/io/lp_parser.cppcpp/src/io/mps_data_model.cppcpp/src/io/mps_parser_internal.hppcpp/src/pdlp/cpu_optimization_problem.cppcpp/src/pdlp/optimization_problem.cu
🚧 Files skipped from review as they are similar to previous changes (2)
- cpp/src/io/lp_parser.cpp
- cpp/include/cuopt/mathematical_optimization/cpu_optimization_problem.hpp
2 participants
Description
Improve QC COO canonicalization performance by reusing a scratch workspace across constraints, instead of allocating fresh lookup buffers on every call. Canonicalization output is unchanged; this is a performance optimization for batch ingest paths (MPS/LP parse, gRPC, incremental
add_quadratic_constraint).Checklist