Skip to content

Update lp testing#1110

Merged
rapids-bot[bot] merged 19 commits into
NVIDIA:release/26.06from
Iroy30:update_lp_testing
May 20, 2026
Merged

Update lp testing#1110
rapids-bot[bot] merged 19 commits into
NVIDIA:release/26.06from
Iroy30:update_lp_testing

Conversation

@Iroy30
Copy link
Copy Markdown
Member

@Iroy30 Iroy30 commented Apr 16, 2026

Description

Issue

Checklist

  • I am familiar with the Contributing Guidelines.
  • Testing
    • New or existing tests cover these changes
    • Added tests
    • Created an issue to follow-up
    • NA
  • Documentation
    • The documentation is up to date with these changes
    • Added new documentation
    • NA

@Iroy30 Iroy30 requested review from a team as code owners April 16, 2026 04:00
@Iroy30 Iroy30 requested a review from rgsl888prabhu April 16, 2026 04:00
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented Apr 16, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented Apr 16, 2026

/ok to test f2791df

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Apr 16, 2026

Review Change Stack

Note

Reviews paused

It 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 reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Extracts solver-parameter handling into a new Cython/C++-backed solver_settings package, updates solver Cython bindings to accept a SolverSettings cdef class, removes the old solver_parameters.pyx, adds a Python shim for compatibility, and updates CMake and tests accordingly.

Changes

Solver settings migration

Layer / File(s) Summary
Build integration
python/cuopt/cuopt/CMakeLists.txt, python/cuopt/cuopt/linear_programming/solver_settings/CMakeLists.txt
Added linear_programming/solver_settings subdirectory and new Cython module build entry for solver_settings.pyx.
Solver Settings Core
python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pxd, python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyx, python/cuopt/cuopt/linear_programming/solver_settings/__init__.py
Introduced Cython/C++-backed solver settings API and cdef class SolverSettings with settings_dict, PDLP warm-start handling, serialization (dump_parameters_to_file, load_parameters_from_file), get_solver_parameter_names, get_solver_setting, and solver_params exported as a tuple.
Solver Interface
python/cuopt/cuopt/linear_programming/solver/solver.pxd, python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx
CImport solver settings declarations; updated Solve/BatchSolve signatures to accept SolverSettings; delegated parameter application and PDLP warm-start handling to SolverSettings; removed local get_data_ptr.
Utilities & Data Model
python/cuopt/cuopt/utilities/utils.py, python/cuopt/cuopt/utilities/__init__.py, python/cuopt/cuopt/linear_programming/data_model/data_model_wrapper.pyx
Added shared get_data_ptr (cudf.Series / numpy.ndarray) and export; replaced local implementations in data model and bindings; updated SPDX headers.
Tests
python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py, python/cuopt/cuopt/tests/linear_programming/test_python_API.py
Removed several legacy solver behavior tests; added parameter dump/load round-trip tests; tightened warm-start error messages; asserted solver method for barrier case; ensured solution file env var set for file output tests.

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Suggested labels

breaking

Suggested reviewers

  • tmckayus
🚥 Pre-merge checks | ✅ 2 | ❌ 3

❌ Failed checks (1 warning, 2 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 25.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check ❓ Inconclusive The title 'Update lp testing' is vague and generic. It references 'testing' but does not convey specific information about what testing changes were made or the primary objective. Replace the vague title with a more descriptive one that specifies the key change, such as 'Refactor LP solver settings to separate module' or 'Migrate solver parameters to new solver_settings module'.
Description check ❓ Inconclusive The pull request description contains only a contribution template with no substantive content describing the actual changes. Provide a meaningful description explaining what changes are being made, why they are necessary, and how they address the referenced issue or objective.
✅ Passed checks (2 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Warning

Review ran into problems

🔥 Problems

Git: Failed to clone repository. Please run the @coderabbitai full review command to re-trigger a full review. If the issue persists, set path_filters to include or exclude specific files.


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py (1)

718-745: ⚠️ Potential issue | 🟠 Major

Write the generated .mps and .sol files under tmp_path.

This test writes afiro_out.mps and afiro.sol into the process working directory, so parallel runs can collide and stale artifacts can mask failures. Please build both paths under tmp_path and pass those full paths into CUOPT_USER_PROBLEM_FILE / CUOPT_SOLUTION_FILE.

As per coding guidelines, "Ensure test isolation: prevent GPU state, cached memory, and global variables from leaking between test cases; verify each test independently initializes its environment"

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py` around lines
718 - 745, The test currently writes afiro_out.mps and afiro.sol to the CWD
causing collisions; update the test to build full paths under the
pytest-provided tmp_path and use those paths when setting
CUOPT_USER_PROBLEM_FILE and CUOPT_SOLUTION_FILE in SolverSettings (referencing
SolverSettings.set_parameter, CUOPT_USER_PROBLEM_FILE, CUOPT_SOLUTION_FILE, and
solver.Solve), pass the tmp_path-based mps path into cuopt_mps_parser.ParseMps,
use those same tmp_path paths for the file existence asserts and os.remove
calls, and ensure any settings are reset (e.g., set CUOPT_USER_PROBLEM_FILE back
to "" if previously done) so the solver state remains isolated between tests.
🧹 Nitpick comments (1)
python/cuopt/cuopt/linear_programming/solver_settings/__init__.py (1)

6-22: Avoid exporting the live solver_params list.

solver_settings.pyx uses this module-global list for parameter validation, so re-exporting the same mutable object here lets callers accidentally corrupt validation state for the whole process. Export an immutable copy/tuple from the package layer instead.

Proposed hardening
 from .solver_settings import (
     PDLPSolverMode,
     SolverMethod,
     SolverSettings,
     get_solver_parameter_names,
     get_solver_setting,
-    solver_params,
+    solver_params as _solver_params,
 )
+
+solver_params = tuple(_solver_params)
 
 __all__ = [
     "PDLPSolverMode",
     "SolverMethod",
     "SolverSettings",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/cuopt/cuopt/linear_programming/solver_settings/__init__.py` around
lines 6 - 22, The module currently re-exports the live mutable list
solver_params which allows external code to mutate package-global validation
state; change the export to expose an immutable copy (e.g.,
tuple(solver_params)) instead of the original list and keep internal code using
the original list unchanged. Locate the names in this file (solver_params in the
from .solver_settings import (...) list and the "__all__" list) and replace the
exported symbol with an immutable copy while leaving the internal
solver_settings.pyx and the original solver_params in that module untouched so
validation continues to use the mutable list internally but callers only receive
an immutable tuple.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyx`:
- Around line 365-436: The warm-start buffers retrieved via
get_pdlp_warm_start_data() are passed raw to get_data_ptr() and then cast to
const double* (in the call to c_solver_settings.set_pdlp_warm_start_data), which
can misinterpret float32/integer or non-contiguous arrays; normalize each array
to contiguous float64 before extracting pointers (e.g., use
np.ascontiguousarray(arr, dtype=np.float64)) for current_primal_solution,
current_dual_solution, initial_primal_average, initial_dual_average,
current_ATY, sum_primal_solutions, sum_dual_solutions,
last_restart_duality_gap_primal_solution, and
last_restart_duality_gap_dual_solution, or alternatively add dtype/contiguity
checks in PDLPWarmStartData.__init__ to raise on incompatible inputs so
get_data_ptr() always receives a contiguous float64 buffer.

In `@python/cuopt/cuopt/linear_programming/solver/solver_parameters.py`:
- Around line 4-16: Add a one-time deprecation warning at module import in the
compatibility shim (solver_parameters.py): import the warnings module and call
warnings.warn("cuopt.linear_programming.solver_parameters is deprecated; use
cuopt.linear_programming.solver_settings instead", DeprecationWarning,
stacklevel=2) near the top of the file (before exporting CUOPT_ constants) so
callers using the old import path get a clear migration signal; place the
warning call alongside the existing imports and the for _name in
dir(_solver_settings_ext) loop to ensure it runs on import.

In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py`:
- Around line 354-375: The test currently loads parameters into reloaded but
continues to call solver.Solve(data_model_obj, settings) instead of exercising
reloaded; change the solve call to solver.Solve(data_model_obj, reloaded) and
ensure you invoke reloaded.set_c_solver_settings() before the solve (mirroring
settings.set_c_solver_settings()); after solving, assert not only
solution.get_termination_reason() == "Optimal" but also numerical correctness by
checking solution.get_objective_value() against the expected objective and
solution.get_primal_values() (or the appropriate accessor) against expected
variable values for this small LP to validate results.

In `@python/cuopt/cuopt/tests/linear_programming/test_python_API.py`:
- Line 740: Add a trailing newline at the end of the file so the final line
"assert problem.ObjValue == pytest.approx(3.715847, abs=1e-3)" is followed by a
newline character; this fixes the W292 warning flagged by Ruff and keeps
python/cuopt/cuopt/tests/linear_programming/test_python_API.py clean in CI.

---

Outside diff comments:
In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py`:
- Around line 718-745: The test currently writes afiro_out.mps and afiro.sol to
the CWD causing collisions; update the test to build full paths under the
pytest-provided tmp_path and use those paths when setting
CUOPT_USER_PROBLEM_FILE and CUOPT_SOLUTION_FILE in SolverSettings (referencing
SolverSettings.set_parameter, CUOPT_USER_PROBLEM_FILE, CUOPT_SOLUTION_FILE, and
solver.Solve), pass the tmp_path-based mps path into cuopt_mps_parser.ParseMps,
use those same tmp_path paths for the file existence asserts and os.remove
calls, and ensure any settings are reset (e.g., set CUOPT_USER_PROBLEM_FILE back
to "" if previously done) so the solver state remains isolated between tests.

---

Nitpick comments:
In `@python/cuopt/cuopt/linear_programming/solver_settings/__init__.py`:
- Around line 6-22: The module currently re-exports the live mutable list
solver_params which allows external code to mutate package-global validation
state; change the export to expose an immutable copy (e.g.,
tuple(solver_params)) instead of the original list and keep internal code using
the original list unchanged. Locate the names in this file (solver_params in the
from .solver_settings import (...) list and the "__all__" list) and replace the
exported symbol with an immutable copy while leaving the internal
solver_settings.pyx and the original solver_params in that module untouched so
validation continues to use the mutable list internally but callers only receive
an immutable tuple.
🪄 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: Pro Plus

Run ID: a7cb691a-cda0-439c-84dd-681333c096fb

📥 Commits

Reviewing files that changed from the base of the PR and between 1f2fb22 and f2791df.

📒 Files selected for processing (12)
  • python/cuopt/cuopt/CMakeLists.txt
  • python/cuopt/cuopt/linear_programming/solver/CMakeLists.txt
  • python/cuopt/cuopt/linear_programming/solver/solver.pxd
  • python/cuopt/cuopt/linear_programming/solver/solver_parameters.py
  • python/cuopt/cuopt/linear_programming/solver/solver_parameters.pyx
  • python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx
  • python/cuopt/cuopt/linear_programming/solver_settings/CMakeLists.txt
  • python/cuopt/cuopt/linear_programming/solver_settings/__init__.py
  • python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pxd
  • python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyx
  • python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py
  • python/cuopt/cuopt/tests/linear_programming/test_python_API.py
💤 Files with no reviewable changes (1)
  • python/cuopt/cuopt/linear_programming/solver/solver_parameters.pyx

Comment on lines +4 to +16
"""Backward-compatible module; LP parameter helpers live in solver_settings."""

from cuopt.linear_programming.solver_settings.solver_settings import (
get_solver_parameter_names,
get_solver_setting,
solver_params,
)

import cuopt.linear_programming.solver_settings.solver_settings as _solver_settings_ext

for _name in dir(_solver_settings_ext):
if _name.startswith("CUOPT_"):
globals()[_name] = getattr(_solver_settings_ext, _name)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Emit a deprecation warning from the compatibility shim.

This keeps the old import path working, but callers get no signal to migrate to cuopt.linear_programming.solver_settings. Please warn once at import with an explicit deprecation message and stacklevel=2.

Suggested change
+import warnings
+
 """Backward-compatible module; LP parameter helpers live in solver_settings."""
+
+warnings.warn(
+    "cuopt.linear_programming.solver.solver_parameters is deprecated; "
+    "use cuopt.linear_programming.solver_settings instead.",
+    DeprecationWarning,
+    stacklevel=2,
+)
 
 from cuopt.linear_programming.solver_settings.solver_settings import (
     get_solver_parameter_names,
     get_solver_setting,
     solver_params,

As per coding guidelines, **/*.{h,hpp,py}: maintain backward compatibility in Python and server APIs with deprecation warnings

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/cuopt/cuopt/linear_programming/solver/solver_parameters.py` around
lines 4 - 16, Add a one-time deprecation warning at module import in the
compatibility shim (solver_parameters.py): import the warnings module and call
warnings.warn("cuopt.linear_programming.solver_parameters is deprecated; use
cuopt.linear_programming.solver_settings instead", DeprecationWarning,
stacklevel=2) near the top of the file (before exporting CUOPT_ constants) so
callers using the old import path get a clear migration signal; place the
warning call alongside the existing imports and the for _name in
dir(_solver_settings_ext) loop to ensure it runs on import.

Comment thread python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py
Comment thread python/cuopt/cuopt/tests/linear_programming/test_python_API.py Outdated
@anandhkb anandhkb added this to the 26.06 milestone Apr 20, 2026
Comment thread python/cuopt/cuopt/linear_programming/solver/solver_parameters.py Outdated
Comment thread python/cuopt/cuopt/linear_programming/solver_settings/__init__.py Outdated
Comment thread python/cuopt/cuopt/linear_programming/solver_settings/CMakeLists.txt Outdated
Comment thread python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pxd Outdated
Copy link
Copy Markdown
Collaborator

@rgsl888prabhu rgsl888prabhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

PR looks good, have few suggestions.

Copy link
Copy Markdown
Collaborator

@rgsl888prabhu rgsl888prabhu left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code-level review — focusing on the Python/Cython layer. CI and PR metadata items are out of scope per request.

Net take: the refactor is clean and the new dump/load-parameters API is a good addition. Inline comments below are mostly readability and small structural improvements; none of them block on correctness.

"""
return self.pdlp_warm_start_data

def set_c_solver_settings(self):
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document the reset-replay invariant.

set_solver_setting in solver_wrapper.pyx calls settings.c_solver_settings.reset(new solver_settings_t[int, double]()) immediately before calling this method, so every Solve() throws away the C++ settings object and repopulates it from self.settings_dict + self.pdlp_warm_start_data.

That means:

  • settings_dict / pdlp_warm_start_data / mip_callbacks on the Python side are the source of truth.
  • Any direct mutation of self.c_solver_settings (e.g. through a future C++-only API) will be discarded on the next Solve.
  • load_parameters_from_file preserves the invariant by mirroring every loaded value back into settings_dict.

Worth a short docstring on this method making the contract explicit — otherwise a future reader can reasonably assume c_solver_settings is long-lived state.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed


if self.get_pdlp_warm_start_data() is not None:
from cuopt.linear_programming.solver.solver_wrapper import (
get_data_ptr,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lazy import inside the hot path is a code smell.

This from cuopt.linear_programming.solver.solver_wrapper import get_data_ptr is here only to break an import cycle (solver_wrapper.pyx imports SolverSettings from this module).

Suggest moving get_data_ptr to cuopt.utilities — where series_from_buf and InputValidationError already live — and importing it normally at the top of both files. That eliminates the cycle and the per-Solve import/attribute lookup.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

addressed

Comment thread python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyx Outdated
Comment thread python/cuopt/cuopt/linear_programming/solver/solver_parameters.py Outdated
Comment thread python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx
Comment thread python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py
Comment thread python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py
@Iroy30 Iroy30 added non-breaking Introduces a non-breaking change improvement Improves an existing functionality labels Apr 28, 2026
@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented Apr 28, 2026

/ok to test 53bbc68

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py (1)

719-728: ⚠️ Potential issue | 🟠 Major

Use tmp_path for generated solver artifacts.

afiro_out.mps and afiro.sol are still written to fixed names in the working directory. That makes this test race-prone under parallel pytest runs and leaves state behind whenever cleanup is skipped by an earlier failure. Please route both settings through tmp_path and read the files back from there.

🧪 Suggested test hardening
-def test_write_files():
+def test_write_files(tmp_path):
@@
-    settings.set_parameter(CUOPT_USER_PROBLEM_FILE, "afiro_out.mps")
-    settings.set_parameter(CUOPT_SOLUTION_FILE, "afiro.sol")
+    problem_path = tmp_path / "afiro_out.mps"
+    solution_path = tmp_path / "afiro.sol"
+    settings.set_parameter(CUOPT_USER_PROBLEM_FILE, str(problem_path))
+    settings.set_parameter(CUOPT_SOLUTION_FILE, str(solution_path))
@@
-    assert os.path.isfile("afiro_out.mps")
+    assert problem_path.is_file()
@@
-    afiro = cuopt_mps_parser.ParseMps("afiro_out.mps")
-    os.remove("afiro_out.mps")
+    afiro = cuopt_mps_parser.ParseMps(str(problem_path))
@@
-    settings.set_parameter(CUOPT_SOLUTION_FILE, "afiro.sol")
+    settings.set_parameter(CUOPT_SOLUTION_FILE, str(solution_path))
@@
-    assert os.path.isfile("afiro.sol")
+    assert solution_path.is_file()
@@
-    with open("afiro.sol") as f:
+    with open(solution_path, encoding="utf-8") as f:
         for line in f:
             if "X01" in line:
                 assert float(line.split()[-1]) == pytest.approx(80)
-
-    os.remove("afiro.sol")

As per coding guidelines, "Flag flaky tests due to GPU timing, uninitialized memory, or race conditions".

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py` around lines
719 - 728, The test_write_files test writes solver output to fixed filenames;
change it to create a temporary directory via the pytest-provided tmp_path and
set CUOPT_USER_PROBLEM_FILE and CUOPT_SOLUTION_FILE to paths inside that
tmp_path using settings.set_parameter on the SolverSettings instance (the same
settings used in test_write_files), then after running the solver read/verify
the generated files from tmp_path (e.g., tmp_path / "afiro_out.mps" and tmp_path
/ "afiro.sol") so artifacts are isolated and the test is race-safe.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyx`:
- Around line 377-448: The warm-start arrays from get_pdlp_warm_start_data() are
only partially size-checked: ensure every buffer
(warm_start_data.current_primal_solution, current_dual_solution,
initial_primal_average, initial_dual_average, current_ATY, sum_primal_solutions,
sum_dual_solutions, last_restart_duality_gap_primal_solution,
last_restart_duality_gap_dual_solution) is validated against the derived
primal_size and dual_size (use the shapes used for last_restart_duality_gap_* to
compute primal_size/dual_size) before calling get_data_ptr() and
c_solver_settings.set_pdlp_warm_start_data; if any shape mismatches,
raise/handle an error so set_pdlp_warm_start_data cannot receive inconsistent
pointers. Use the existing symbols get_pdlp_warm_start_data,
warm_start_data.<field>, get_data_ptr, and
c_solver_settings.set_pdlp_warm_start_data to locate and implement the checks.

---

Outside diff comments:
In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py`:
- Around line 719-728: The test_write_files test writes solver output to fixed
filenames; change it to create a temporary directory via the pytest-provided
tmp_path and set CUOPT_USER_PROBLEM_FILE and CUOPT_SOLUTION_FILE to paths inside
that tmp_path using settings.set_parameter on the SolverSettings instance (the
same settings used in test_write_files), then after running the solver
read/verify the generated files from tmp_path (e.g., tmp_path / "afiro_out.mps"
and tmp_path / "afiro.sol") so artifacts are isolated and the test is race-safe.
🪄 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: 1a2154f8-87a7-4768-be3b-5ac2bc5cfdce

📥 Commits

Reviewing files that changed from the base of the PR and between f2791df and 8d26500.

📒 Files selected for processing (5)
  • python/cuopt/cuopt/linear_programming/solver/solver_parameters.py
  • python/cuopt/cuopt/linear_programming/solver_settings/__init__.py
  • python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyx
  • python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py
  • python/cuopt/cuopt/tests/linear_programming/test_python_API.py
💤 Files with no reviewable changes (1)
  • python/cuopt/cuopt/tests/linear_programming/test_python_API.py
🚧 Files skipped from review as they are similar to previous changes (2)
  • python/cuopt/cuopt/linear_programming/solver/solver_parameters.py
  • python/cuopt/cuopt/linear_programming/solver_settings/init.py

Comment on lines +377 to +448
if self.get_pdlp_warm_start_data() is not None:
from cuopt.linear_programming.solver.solver_wrapper import (
get_data_ptr,
)

warm_start_data = self.get_pdlp_warm_start_data()
c_current_primal_solution = (
get_data_ptr(
warm_start_data.current_primal_solution # noqa
)
)
c_current_dual_solution = (
get_data_ptr(
warm_start_data.current_dual_solution
)
)
c_initial_primal_average = (
get_data_ptr(
warm_start_data.initial_primal_average # noqa
)
)
c_initial_dual_average = (
get_data_ptr(
warm_start_data.initial_dual_average
)
)
c_current_ATY = (
get_data_ptr(
warm_start_data.current_ATY
)
)
c_sum_primal_solutions = (
get_data_ptr(
warm_start_data.sum_primal_solutions
)
)
c_sum_dual_solutions = (
get_data_ptr(
warm_start_data.sum_dual_solutions
)
)
c_last_restart_duality_gap_primal_solution = (
get_data_ptr(
warm_start_data.last_restart_duality_gap_primal_solution # noqa
)
)
c_last_restart_duality_gap_dual_solution = (
get_data_ptr(
warm_start_data.last_restart_duality_gap_dual_solution # noqa
)
)
c_solver_settings.set_pdlp_warm_start_data(
<const double *> c_current_primal_solution,
<const double *> c_current_dual_solution,
<const double *> c_initial_primal_average,
<const double *> c_initial_dual_average,
<const double *> c_current_ATY,
<const double *> c_sum_primal_solutions,
<const double *> c_sum_dual_solutions,
<const double *> c_last_restart_duality_gap_primal_solution,
<const double *> c_last_restart_duality_gap_dual_solution,
warm_start_data.last_restart_duality_gap_primal_solution.shape[0], # Primal size # noqa
warm_start_data.last_restart_duality_gap_dual_solution.shape[0], # Dual size # noqa
warm_start_data.initial_primal_weight,
warm_start_data.initial_step_size,
warm_start_data.total_pdlp_iterations,
warm_start_data.total_pdhg_iterations,
warm_start_data.last_candidate_kkt_score,
warm_start_data.last_restart_kkt_score,
warm_start_data.sum_solution_weight,
warm_start_data.iterations_since_last_restart # noqa
)
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Validate every warm-start buffer against the same primal/dual sizes.

Lines 438-439 derive primal_size / dual_size from the two last_restart_duality_gap_* arrays, but the other seven buffers are passed as raw pointers without any shape check. solver_wrapper.pyx only validates current_primal_solution and current_dual_solution against the problem dimensions, so an inconsistent auxiliary buffer here can still make set_pdlp_warm_start_data() read past the end of one array during Solve() or dump_parameters_to_file().

🛡️ Suggested guard
         if self.get_pdlp_warm_start_data() is not None:
             from cuopt.linear_programming.solver.solver_wrapper import (
                 get_data_ptr,
             )

             warm_start_data = self.get_pdlp_warm_start_data()
+            primal_size = warm_start_data.current_primal_solution.shape[0]
+            dual_size = warm_start_data.current_dual_solution.shape[0]
+
+            primal_fields = (
+                "initial_primal_average",
+                "sum_primal_solutions",
+                "last_restart_duality_gap_primal_solution",
+            )
+            dual_fields = (
+                "initial_dual_average",
+                "current_ATY",
+                "sum_dual_solutions",
+                "last_restart_duality_gap_dual_solution",
+            )
+
+            for field in primal_fields:
+                if getattr(warm_start_data, field).shape[0] != primal_size:
+                    raise ValueError(
+                        f"Invalid PDLPWarmStart data: {field} has inconsistent primal size"
+                    )
+            for field in dual_fields:
+                if getattr(warm_start_data, field).shape[0] != dual_size:
+                    raise ValueError(
+                        f"Invalid PDLPWarmStart data: {field} has inconsistent dual size"
+                    )
+
             c_current_primal_solution = (
                 get_data_ptr(
                     warm_start_data.current_primal_solution # noqa
@@
             c_solver_settings.set_pdlp_warm_start_data(
                 <const double *> c_current_primal_solution,
                 <const double *> c_current_dual_solution,
@@
-                warm_start_data.last_restart_duality_gap_primal_solution.shape[0], # Primal size # noqa
-                warm_start_data.last_restart_duality_gap_dual_solution.shape[0], # Dual size # noqa
+                primal_size,
+                dual_size,
                 warm_start_data.initial_primal_weight,
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyx`
around lines 377 - 448, The warm-start arrays from get_pdlp_warm_start_data()
are only partially size-checked: ensure every buffer
(warm_start_data.current_primal_solution, current_dual_solution,
initial_primal_average, initial_dual_average, current_ATY, sum_primal_solutions,
sum_dual_solutions, last_restart_duality_gap_primal_solution,
last_restart_duality_gap_dual_solution) is validated against the derived
primal_size and dual_size (use the shapes used for last_restart_duality_gap_* to
compute primal_size/dual_size) before calling get_data_ptr() and
c_solver_settings.set_pdlp_warm_start_data; if any shape mismatches,
raise/handle an error so set_pdlp_warm_start_data cannot receive inconsistent
pointers. Use the existing symbols get_pdlp_warm_start_data,
warm_start_data.<field>, get_data_ptr, and
c_solver_settings.set_pdlp_warm_start_data to locate and implement the checks.

@github-actions
Copy link
Copy Markdown

github-actions Bot commented May 5, 2026

🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you!

If this is an "epic" issue, then please add the "epic" label to this issue.
If it is a PR and not ready for review, then please convert this to draft.
If you just want to switch off this notification, then use the "skip inactivity reminder" label.

1 similar comment
@github-actions
Copy link
Copy Markdown

🔔 Hi @anandhkb, this pull request has had no activity for 7 days. Please update or let us know if it can be closed. Thank you!

If this is an "epic" issue, then please add the "epic" label to this issue.
If it is a PR and not ready for review, then please convert this to draft.
If you just want to switch off this notification, then use the "skip inactivity reminder" label.

@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented May 18, 2026

/ok to test 5e61083

@rgsl888prabhu
Copy link
Copy Markdown
Collaborator

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 19, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

@anandhkb anandhkb added the P0 label May 19, 2026
@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented May 19, 2026

/ok to test 5e0a4b8

@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented May 19, 2026

/merge

@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented May 20, 2026

/ok to test 98fcdcc

@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented May 20, 2026

/ok to test 3007054

@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented May 20, 2026

/ok to test b45d214

@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented May 20, 2026

/merge

@rgsl888prabhu rgsl888prabhu changed the base branch from main to release/26.06 May 20, 2026 17:46
@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented May 20, 2026

/ok to test 50937b3

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

10 similar comments
@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@rapids-bot
Copy link
Copy Markdown
Contributor

rapids-bot Bot commented May 20, 2026

This PR's base branch has been changed since the last /merge command. Please issue the command again to confirm the merge with the new base branch.

@Iroy30
Copy link
Copy Markdown
Member Author

Iroy30 commented May 20, 2026

/merge

@rapids-bot rapids-bot Bot merged commit efbad52 into NVIDIA:release/26.06 May 20, 2026
89 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

improvement Improves an existing functionality non-breaking Introduces a non-breaking change P0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants