Follow-up for LP format#1251
Merged
rapids-bot[bot] merged 35 commits intoMay 22, 2026
Merged
Conversation
cuOptReadProblem, cuopt_cli, the Python ParseLp() wrapper, and the self-hosted client now dispatch on the input filename: a case-insensitive ".lp" suffix routes to a new LP parser; everything else (including .mps, .mps.gz, .mps.bz2, and extensionless inputs) continues to use parse_mps. The LP parser supports LP, MIP, and QP problems in the conventional LP dialect used by most commercial solvers (not the lpsolve variant). SOS, PWL, semi-continuous, user-cut, and general-constraint sections raise a ValidationError rather than silently mis-parsing. Quadratic-constraint support (QCMATRIX) remains MPS-only. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Signed-off-by: Miles Lubin <mlubin@nvidia.com>
The error message in cuopt_self_host_client.py was updated to say "MPS/LP data" alongside the LP reader changes, but the matching assertion in test_self_hosted_service.sh was not, causing CLI test 7 to fail. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
parse_linear_expression silently accepted two malformed inputs:
- '2 [ x^2 ] / 2' parsed as objective_offset=2 plus quadratic x^2,
rather than rejecting the unsupported scalar-multiplication-of-bracket
form. Same shape occurs in quadratic constraint rows.
- '<number> *' followed by a relation, section header, or EOL silently
dropped the '*' and turned the number into a bare constant.
Both now raise a ValidationError pointing at the offending line. Added
tests for both malformed forms plus the legitimate forms ('5 + [...]/2'
and '3 * x') to pin the boundary.
Signed-off-by: Miles Lubin <mlubin@nvidia.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
parser_finalize.hpp was designed as a shared finalization template but only the LP parser ever called it; MPS keeps its own fill_problem. Move the implementation into lp_parser.cpp's anonymous namespace alongside flush_quadratic_constraints, since both serve the same caller and run consecutively. While inlining, drop the dormant requires-expression branches for objective_scaling_factor_value, ranges_values, and qmatrix_entries — none of these fields exist on lp_parser_t. The Parser template parameter becomes a concrete lp_parser_t<i_t, f_t>&. No behavior change for any caller. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Two block-level comments were framed in terms of how this PR rearranged
the code ("MPS-only tests preserved from mps_parser_test.cpp", "Extracted
from ParseMps"). Replace with descriptions of what the code actually does
today, independent of the refactor that produced it.
The MPS-only test header also incorrectly claimed the LP parser doesn't
support semi-continuous variables or quadratic constraints — it does;
those tests are preserved because they exercise MPS-specific syntax
(bound codes, QCMATRIX blocks), not because LP lacks the semantics.
Signed-off-by: Miles Lubin <mlubin@nvidia.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The 3-token 'Semi - Continuous' form was the only spelling recognized,
but both Gurobi and CPLEX LP-format references explicitly list bare
'Semi' and 'Semis' as documented synonyms:
Gurobi: "Valid keywords for variable type headers are: binary,
binaries, bin, general, generals, gen, semi-continuous,
semis, or semi."
CPLEX: "The SEMI-CONTINUOUS section is preceded by the keyword
SEMI-CONTINUOUS, SEMI, or SEMIS."
Accepted spellings (case-insensitive) are now: 'Semi-Continuous',
'Semi', and 'Semis'.
Signed-off-by: Miles Lubin <mlubin@nvidia.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
cuOpt's set_quadratic_objective_matrix symmetrizes internally via H = Q + Q^T before solving (1/2) x^T H x, so the parser does not need to materialize the full symmetric Q. Switch the quadratic-objective path to: parse_quadratic_bracket: apply LP's '/ 2' uniformly to diagonal and off-diagonal entries (instead of only halving off-diagonals). finalize_problem: emit the upper-triangular quadobj_entries as CSR directly (drop the mirror pass and the post-scale by 0.5). End-to-end PDLP tests (cpp/tests/qp/unit_tests/lp_parser_solve_test.cu) parse three small QP files (diagonal, positive cross term, negative cross term) and verify objective values and primal solutions against hand-computed optima. Quadratic constraints are NOT changed: cuOpt's set_quadratic_constraints does not symmetrize, so per-constraint Q must remain fully symmetric. The MPS parser is also unchanged. LP and MPS now store the quadratic objective in different equivalent forms (upper-tri vs full-sym × 0.5) in mps_data_model_t; both produce the same H after cuOpt's symmetrize step. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Per Gurobi's LP-format reference, the bracket reserved for the quadratic portion of an objective or constraint expression accepts only squared terms (e.g., '2 x ^ 2') and product terms (e.g., '3 x * y'). HiGHS's LP reader (highs/io/filereaderlp/reader.cpp) likewise rejects anything other than those four forms. The parser previously accepted bare linear terms like '2 x' inside the bracket and folded them into the row's linear part — an extension not supported by any LP reader I could find documentation or source for. Tighten the parser to reject this and prune the now-dead out_linear plumbing from parse_quadratic_bracket and its two call sites. One existing test (qc_outer_minus_sign_flips_quadratic_and_linear) used the rejected form to verify outer-sign propagation. Rewrite it to use the conventional spelling with the linear term outside the bracket, and add two negative tests that pin the new rejection (one in objective, one in constraint). Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Inline source comments and public docstrings shouldn't single out specific solver vendors when describing LP-format conventions. Rephrase each spot to describe the rule itself instead — the conventions are the same whether or not a particular product is named. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The argparse help string for the positional filename argument said only "input MPS or LP file (dispatched by .lp / .mps extension)", which both underspecified the supported set and omitted .qps and the compressed variants. Update it to enumerate every extension parse_problem() accepts. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The test wrote a temp .lp file and called cuOptReadProblem; cleanup (cuOptDestroyProblem and std::filesystem::remove) ran only at the end of the test body, so an assertion failure mid-test would leak both the problem handle and the temp file. Introduce a small RAII guard whose destructor unconditionally destroys the handle (if non-null) and removes the temp file, so cleanup runs on every exit path. std::filesystem::remove is called with std::error_code to avoid throwing during stack unwinding. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
dispatch_parse() writes content to a temp file, then calls parse_problem(tmp.string()), then removes the temp file. If parse_problem throws (the unrecognized-extension test exercises exactly this path), the std::filesystem::remove call is skipped and the file leaks. Replace the manual post-parse remove with an RAII guard whose destructor removes the file on every scope exit — success, return, or exception. std::error_code is passed to remove so the destructor never throws during stack unwinding while a parse exception is propagating. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
…ariants The two extension-routing blocks in cuopt_self_host_client.py only recognized literal '.lp' and '.mps' suffixes — '.qps' and the compressed variants (.lp.gz, .lp.bz2, .mps.gz, .mps.bz2, .qps.gz, .qps.bz2) fell through to the unparsed-upload path even though the underlying mps_parser.ParseMps / ParseLp accept all of them via the same C++ file_to_string dispatch as the CLI. Factor the extension check into a small helper that lowercases the path and strips a single .gz / .bz2 compression suffix before matching, then use it in both _parse_file_to_data_model (which picks ParseLp vs ParseMps) and read_cuopt_problem_data (which decides whether to parse client-side or ship to the server). QPS routes to ParseMps; that matches the C++ parse_problem() dispatch table. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The cuopt.linear_programming.mps_parser package now exposes both ParseMps and ParseLp, so 'mps_parser' is no longer accurate. Rename the directory to 'io' (mirroring the cpp/include/cuopt/linear_programming/io/ layout on the C++ side) and rename the decorator catch_mps_parser_exception → catch_io_exception. This package was added on this branch in 72ba054 (post-26.04) and has not appeared in any release, so the rename is safe without a compatibility shim. Importing files that previously did 'from cuopt.linear_programming import mps_parser' now import 'io as mps_parser' to keep the local binding stable; user-facing docs/examples and the public top-level import in cuopt/linear_programming/__init__.py use the new module name directly. The C++ error tag MPS_PARSER_ERROR_TYPE that the decorator parses out of RuntimeError messages is intentionally unchanged — that tag is produced by the mps_parser_expects() macro, which is shared between both parsers on the C++ side and would have a much larger blast radius to rename. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Annotate ParseLp with `lp_file_path: str` and `-> DataModel`, importing DataModel from cuopt.linear_programming.data_model so the annotation resolves at runtime. The old docstring claimed unsupported-section input raises ValueError; in practice the catch_io_exception decorator translates the C++ parser's tagged RuntimeError into typed Python exceptions (InputValidationError / InputRuntimeError / OutOfMemoryError). Replace the misleading sentence with a proper Raises section listing all three typed exceptions and the conditions that trigger them. Also expand the lp_file_path parameter description to mention the compressed-input variants the parser accepts, and trim the Returns phrasing so it points at lp_file_path explicitly. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The mps_roundtrip and lp_roundtrip tests each used a fixed shared
/tmp path (e.g. /tmp/mps_roundtrip_lp_test.mps) and removed it
manually at the end of the test. Two problems:
1. Parallel test runs writing to the same path race each other.
2. An assertion failure or exception between write and remove
leaks the file.
Add a small temp_file_t RAII helper that picks a unique path under
std::filesystem::temp_directory_path() using pid + an atomic counter,
and removes the file on every scope exit (std::error_code so the
destructor never throws). Adopt it in every test that previously
spelled a literal /tmp/*.mps path (the six mps/lp roundtrip tests and
the QCQP-QC1 roundtrip), and use it inside dispatch_parse too,
replacing the existing inline cleanup_t scope guard.
Signed-off-by: Miles Lubin <mlubin@nvidia.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The "This example demonstrates how to" bullet pointed at the package path cuopt.linear_programming.io after the recent rename, but the example code calls ParseMps directly (imported from the public top-level). Update the bullet to name the actual function so the prose matches the sample. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The C API entry point previously dereferenced filename via std::string(filename) without a null check and stored into *problem_ptr without a null check on the out-pointer. A null filename would segfault inside libstdc++'s string constructor, and a null problem_ptr would segfault on assignment; an empty filename would advance into the generic file-not-found / parse-error paths. Validate filename != nullptr && filename[0] != '\0' && problem_ptr != nullptr up front, return CUOPT_INVALID_ARGUMENT immediately on failure, and (importantly) skip the `new problem_and_stream_view_t` allocation that would otherwise leak when the validation fails. The existing parser exception path already handles cleanup for non-null inputs. Document the new return code on the public header docstring and add a test (c_api.read_problem_null_or_empty_inputs_rejected) covering all three invalid combinations. Signed-off-by: Miles Lubin <mlubin@nvidia.com> Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Expose parse_problem via Cython with fixed_mps_format support, add Problem.read(), deprecate readMPS, and migrate callers/tests/docs from ParseMps/ParseLp. Extend self-hosted CI and server examples for LP and compressed inputs. Co-authored-by: Cursor <cursoragent@cursor.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:
📝 WalkthroughWalkthroughAdds LP parsing and a unified ParseProblem dispatcher (case-insensitive .mps/.qps/.lp, including .gz/.bz2) across C++ headers, Cython bindings, Python APIs (ParseProblem, Problem.read), self-hosted client parsing, tests, and documentation/examples. ChangesUnified Problem File Parser with Extension-Based Dispatch
🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
chris-maes
reviewed
May 21, 2026
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
python/cuopt/cuopt/tests/linear_programming/test_python_API.py (1)
342-355: ⚡ Quick winAdd compressed-input coverage for
Problem.read.This test validates
.mpsand.lp, but the new dispatch path also includes compressed extensions. Adding one.lp.gz/.lp.bz2assertion here would better protect the new behavior.As per coding guidelines,
"python/**/tests/**: Focus on ... Edge cases"and this new path includes compressed-extension variants.🤖 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 `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py` around lines 342 - 355, The test test_problem_read_mps_and_lp only checks .mps and .lp but misses compressed-extension handling; update this test to also call Problem.read on a compressed variant (e.g., change lp_path to lp_path + ".gz" or use a separate lp_gz_path with ".lp.gz" or ".lp.bz2"), read it via Problem.read, and assert the same properties (NumVariables == 2 and the VariableName set equals {"VAR1", "VAR2"}) as done for lp_problem and mps_problem so the compressed dispatch path is covered; reference the test name test_problem_read_mps_and_lp and the Problem.read call to locate where to add the extra assertions.
🤖 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 `@python/cuopt/cuopt/linear_programming/io/parser.py`:
- Line 49: The new public function toDict(model, json=False) is missing type
hints and a complete API docstring; add annotations (e.g., def toDict(model:
Any, json: bool = False) -> Dict[str, Any]) using typing imports and document
params, returns and raises in the docstring for toDict, describing the expected
type/shape of model, the json flag behavior, the returned dictionary (or
JSON-serializable structure), and any exceptions raised (TypeError/ValueError if
input is invalid); update imports to include typing symbols used and ensure the
docstring follows project docstyle for public APIs.
---
Nitpick comments:
In `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py`:
- Around line 342-355: The test test_problem_read_mps_and_lp only checks .mps
and .lp but misses compressed-extension handling; update this test to also call
Problem.read on a compressed variant (e.g., change lp_path to lp_path + ".gz" or
use a separate lp_gz_path with ".lp.gz" or ".lp.bz2"), read it via Problem.read,
and assert the same properties (NumVariables == 2 and the VariableName set
equals {"VAR1", "VAR2"}) as done for lp_problem and mps_problem so the
compressed dispatch path is covered; reference the test name
test_problem_read_mps_and_lp and the Problem.read call to locate where to add
the extra assertions.
🪄 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: c4a58e33-4aac-48b7-9fef-fc05a3b5c445
📒 Files selected for processing (10)
cpp/src/io/utilities/cython_parser.cppdocs/cuopt/source/cuopt-cli/cli-examples.rstdocs/cuopt/source/cuopt-python/lp-qp-milp/lp-qp-milp-api.rstdocs/cuopt/source/cuopt-server/examples/lp-examples.rstdocs/cuopt/source/cuopt-server/examples/lp/examples/mps_datamodel_example.pypython/cuopt/cuopt/linear_programming/io/parser.pypython/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.pypython/cuopt/cuopt/tests/linear_programming/test_lp_solver.pypython/cuopt/cuopt/tests/linear_programming/test_python_API.pypython/libcuopt/libcuopt/tests/test_cli.sh
💤 Files with no reviewable changes (2)
- python/cuopt/cuopt/tests/linear_programming/test_cpu_only_execution.py
- docs/cuopt/source/cuopt-server/examples/lp/examples/mps_datamodel_example.py
✅ Files skipped from review due to trivial changes (1)
- docs/cuopt/source/cuopt-server/examples/lp-examples.rst
| return parser_wrapper.ParseProblem(file_path, fixed_mps_format) | ||
|
|
||
|
|
||
| def toDict(model, json=False): |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Add type hints and full API docstring content for toDict.
toDict is a new public Python API and currently lacks type hints and docstring content (params/returns/raises).
Proposed patch
+from typing import Any
+
-def toDict(model, json=False):
+def toDict(model: parser_wrapper.DataModel, json: bool = False) -> dict[str, Any]:
+ """Convert a parser DataModel to a Python dictionary.
+
+ Parameters
+ ----------
+ model : parser_wrapper.DataModel
+ Parsed optimization model to serialize.
+ json : bool, default=False
+ If True, converts infinities to `"inf"`/`"ninf"` in list fields for JSON friendliness.
+
+ Returns
+ -------
+ dict[str, Any]
+ Dictionary containing matrix, bounds, objective, and variable metadata.
+
+ Raises
+ ------
+ ValueError
+ If `model` is not a `parser_wrapper.DataModel`.
+ """As per coding guidelines, "**/*.py: Add type hints on all new public API functions and classes in Python" and "Include docstring content (params, returns, raises) on all new public Python APIs".
🤖 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 `@python/cuopt/cuopt/linear_programming/io/parser.py` at line 49, The new
public function toDict(model, json=False) is missing type hints and a complete
API docstring; add annotations (e.g., def toDict(model: Any, json: bool = False)
-> Dict[str, Any]) using typing imports and document params, returns and raises
in the docstring for toDict, describing the expected type/shape of model, the
json flag behavior, the returned dictionary (or JSON-serializable structure),
and any exceptions raised (TypeError/ValueError if input is invalid); update
imports to include typing symbols used and ensure the docstring follows project
docstyle for public APIs.
Member
Author
|
/ok to test 7d802b1 |
Member
Author
|
/ok to test 1384e7b |
mlubin
reviewed
May 21, 2026
Contributor
|
You may want to rebase and squash; the commit history on this PR is confusing. |
mlubin
reviewed
May 22, 2026
Member
Author
|
/ok to test ca97a74 |
mlubin
approved these changes
May 22, 2026
rgsl888prabhu
requested changes
May 22, 2026
Collaborator
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
Few minor comments,rest looks good.
…el_example.py Co-authored-by: Ramakrishnap <42624703+rgsl888prabhu@users.noreply.github.com>
Member
Author
|
/ok to test 89c6280 |
rgsl888prabhu
approved these changes
May 22, 2026
Member
Author
|
/merge |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Deprecates Problem.readMPS and adds unified Problem.read for MPS and LP file formats.
Renaming
Follows after #1120
Issue
Checklist