Open
Conversation
Member
Author
|
/ok to test f2791df |
📝 WalkthroughWalkthroughThis pull request refactors solver settings handling by extracting it from the main solver module into a dedicated Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 🚥 Pre-merge checks | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✏️ 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
+365
to
+436
| 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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'get_data_ptr|set_pdlp_warm_start_data|current_primal_solution|current_dual_solution|ascontiguousarray|astype' \
python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyx \
python/cuopt/cuopt/linear_programming/solution/solution.py \
python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyxRepository: NVIDIA/cuopt
Length of output: 21611
🏁 Script executed:
# Search for dtype validation or normalization in PDLPWarmStartData class and related code
rg -n 'dtype|float64|astype|ascontiguousarray|contiguous' \
python/cuopt/cuopt/linear_programming/solution/solution.py \
--context 5
# Also check if there's validation in set_pdlp_warm_start_data setter
rg -n -A 10 'def set_pdlp_warm_start_data' \
python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyxRepository: NVIDIA/cuopt
Length of output: 6694
🏁 Script executed:
# Get the full PDLPWarmStartData class definition
sed -n '14,56p' python/cuopt/cuopt/linear_programming/solution/solution.py
# Get the full set_pdlp_warm_start_data method from solver_settings.pyx
sed -n '224,250p' python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.pyxRepository: NVIDIA/cuopt
Length of output: 2719
🏁 Script executed:
# Check if there's any validation in the C++ side or in any wrapper that calls set_c_solver_settings
rg -n 'set_c_solver_settings|astype.*float64|ensure.*dtype' \
python/cuopt/cuopt/linear_programming/ \
--type py --type pyxRepository: NVIDIA/cuopt
Length of output: 83
🏁 Script executed:
# Search for any validation or normalization around warm-start arrays
rg -n 'set_c_solver_settings|astype.*float64|ensure.*dtype|\.astype' \
python/cuopt/cuopt/linear_programming/
# Also check how warm start data flows from solution to solver
rg -n 'get_pdlp_warm_start_data.*astype|normalize.*warm|validate.*warm' \
python/cuopt/cuopt/Repository: NVIDIA/cuopt
Length of output: 935
🏁 Script executed:
# Check get_data_ptr implementation and how it's used with warm-start
sed -n '132,140p' python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx
# Verify there's no dtype check in get_data_ptr
rg -B 5 -A 15 'def get_data_ptr' python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyxRepository: NVIDIA/cuopt
Length of output: 1080
Normalize and validate warm-start buffers before casting to double* pointers.
PDLPWarmStartData accepts arrays without dtype validation, and get_data_ptr() extracts raw pointers without checking dtype or contiguity. The code then casts every warm-start buffer to const double* without validation, so float32, integer, or non-contiguous arrays will be misinterpreted by C++ code. This can silently corrupt warm-start data or cause out-of-bounds memory access.
Normalize each warm-start vector to contiguous float64 (e.g., via np.ascontiguousarray(arr, dtype=np.float64)) before passing to get_data_ptr(), or add early validation in PDLPWarmStartData.__init__() to reject incompatible inputs.
🤖 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 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.
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.
Comment on lines
+354
to
+375
| reloaded = solver_settings.SolverSettings() | ||
| reloaded.load_parameters_from_file(str(load_path)) | ||
| for name in solver_params: | ||
| _assert_solver_param_equal( | ||
| name, | ||
| expected_by_name[name], | ||
| reloaded.get_parameter(name), | ||
| ) | ||
|
|
||
| settings.set_c_solver_settings() | ||
| data_model_obj = data_model.DataModel() | ||
| A_values = np.array([1.0, 1.0]) | ||
| A_indices = np.array([0, 0]) | ||
| A_offsets = np.array([0, 1, 2]) | ||
| data_model_obj.set_csr_constraint_matrix(A_values, A_indices, A_offsets) | ||
| data_model_obj.set_constraint_bounds(np.array([1.0, 1.0])) | ||
| data_model_obj.set_objective_coefficients(np.array([1.0])) | ||
| data_model_obj.set_row_types(np.array(["L", "L"])) | ||
|
|
||
| solution = solver.Solve(data_model_obj, settings) | ||
| assert solution.get_termination_reason() == "Optimal" | ||
|
|
There was a problem hiding this comment.
Solve with reloaded here, not the original settings.
The round-trip check ends at reloaded.get_parameter(...), but the integration solve still uses settings. That leaves load_parameters_from_file() plus reloaded.set_c_solver_settings() unexercised in the actual solve path, and the test still only checks "Optimal". Please run the final solve with reloaded and assert the objective / solution values too.
As per coding guidelines, "Write tests validating numerical correctness of optimization results (not just 'runs without error'); test degenerate cases (infeasible, unbounded, empty, singleton problems)"
🤖 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
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.
| problem.solve(settings) | ||
| assert problem.Status.name == "Optimal" | ||
| assert problem.ObjValue == pytest.approx(-126, abs=1e-3) | ||
| assert problem.ObjValue == pytest.approx(3.715847, abs=1e-3) No newline at end of file |
There was a problem hiding this comment.
Add the trailing newline.
Ruff is already flagging W292 here, so this will keep the test file clean in CI.
🧰 Tools
🪛 Ruff (0.15.10)
[warning] 740-740: No newline at end of file
Add trailing newline
(W292)
🤖 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_python_API.py` at 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.
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
2 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