Update lp testing#1110
Open
Iroy30 wants to merge 4 commits intoNVIDIA:mainfrom
Open
Conversation
Member
Author
|
/ok to test f2791df |
📝 WalkthroughWalkthroughExtracts solver-parameter handling into a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 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 | 🟠 MajorWrite the generated
.mpsand.solfiles undertmp_path.This test writes
afiro_out.mpsandafiro.solinto the process working directory, so parallel runs can collide and stale artifacts can mask failures. Please build both paths undertmp_pathand pass those full paths intoCUOPT_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 livesolver_paramslist.
solver_settings.pyxuses 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
📒 Files selected for processing (12)
python/cuopt/cuopt/CMakeLists.txtpython/cuopt/cuopt/linear_programming/solver/CMakeLists.txtpython/cuopt/cuopt/linear_programming/solver/solver.pxdpython/cuopt/cuopt/linear_programming/solver/solver_parameters.pypython/cuopt/cuopt/linear_programming/solver/solver_parameters.pyxpython/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyxpython/cuopt/cuopt/linear_programming/solver_settings/CMakeLists.txtpython/cuopt/cuopt/linear_programming/solver_settings/__init__.pypython/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pxdpython/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyxpython/cuopt/cuopt/tests/linear_programming/test_lp_solver.pypython/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) |
There was a problem hiding this comment.
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.
| @@ -1,4 +1,22 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2024-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
| # SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
Collaborator
There was a problem hiding this comment.
Suggested change
| # SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |
| # SPDX-FileCopyrightText: Copyright (c) 2024-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. |
| @@ -0,0 +1,10 @@ | |||
| # cmake-format: off | |||
| # SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |||
Collaborator
There was a problem hiding this comment.
Suggested change
| # SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. | |
| # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. |
| @@ -0,0 +1,98 @@ | |||
| # SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. # noqa | |||
Collaborator
There was a problem hiding this comment.
Suggested change
| # SPDX-FileCopyrightText: Copyright (c) 2023-2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. # noqa | |
| # SPDX-FileCopyrightText: Copyright (c) 2026, NVIDIA CORPORATION & AFFILIATES. All rights reserved. # noqa |
Collaborator
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
PR looks good, have few suggestions.
Collaborator
rgsl888prabhu
left a comment
rgsl888prabhu
left a comment
There was a problem hiding this comment.
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): |
Collaborator
There was a problem hiding this comment.
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_callbackson 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_filepreserves the invariant by mirroring every loaded value back intosettings_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.
|
|
||
| if self.get_pdlp_warm_start_data() is not None: | ||
| from cuopt.linear_programming.solver.solver_wrapper import ( | ||
| get_data_ptr, |
Collaborator
There was a problem hiding this comment.
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.
Iroy30
added
non-breaking
Member
Author
|
/ok to test 53bbc68 |
There was a problem hiding this comment.
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 | 🟠 MajorUse
tmp_pathfor generated solver artifacts.
afiro_out.mpsandafiro.solare 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 throughtmp_pathand 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
📒 Files selected for processing (5)
python/cuopt/cuopt/linear_programming/solver/solver_parameters.pypython/cuopt/cuopt/linear_programming/solver_settings/__init__.pypython/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyxpython/cuopt/cuopt/tests/linear_programming/test_lp_solver.pypython/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 | ||
| ) |
There was a problem hiding this comment.
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.
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
3 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
Issue
Checklist