Update solution object to include the LP solver#822
Update solution object to include the LP solver#822nguidotti wants to merge 21 commits intoNVIDIA:release/26.04from
Conversation
|
Auto-sync is disabled for draft pull requests in this repository. Workflows must be run manually. Contributors can view more details about this message here. |
|
/ok to test 2758461 |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
|
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:
📝 WalkthroughWalkthroughThis PR fixes a spelling error in public termination-status macros (renaming Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
python/cuopt_self_hosted/cuopt_sh_client/cuopt_self_host_client.py (1)
189-204:⚠️ Potential issue | 🟡 MinorAdd backward-compatible fallback for
solved_by.If a client connects to an older server that still returns
solved_by_pdlp, this will throw and the function will silently return a raw dict instead of aThinClientSolution. Consider a fallback to preserve compatibility.🔧 Suggested fallback
- solved_by=sol["solved_by"], + solved_by=sol.get("solved_by", sol.get("solved_by_pdlp", None)),As per coding guidelines, maintain backward compatibility in Python and server APIs with deprecation warnings.
python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.py (1)
698-703:⚠️ Potential issue | 🟠 MajorType mismatch:
solved_byshould beint, notbool.The PR objective is to replace the boolean
solved_by_pdlpwith an enum-basedsolved_byfield that can represent multiple solvers (DualSimplex=1, PDLP=2, Barrier=3). However, the type here remainsbool.Based on the solver wrapper changes in this PR (
solver_wrapper.pyxline 362),solved_byis now an integer value fromlp_solver_type_t. The field type and description should be updated to reflect this.🐛 Proposed fix
- solved_by: bool = Field( + solved_by: int = Field( default=None, description=( - "Returns whether problem was solved by PDLP or Dual Simplex" + "Returns which solver solved the LP: 0=Unset, 1=DualSimplex, 2=PDLP, 3=Barrier" ), )
🤖 Fix all issues with AI agents
In `@cpp/include/cuopt/linear_programming/pdlp/solver_solution.hpp`:
- Around line 94-95: Update the comment for the member solved_by of type
lp_solver_type_t to accurately reflect all enum options (PDLP, Dual Simplex, and
Barrier); locate the declaration of solved_by (lp_solver_type_t solved_by =
lp_solver_type_t::Unset;) and change the comment text from "Whether the problem
was solved by PDLP or Dual Simplex" to include "Barrier" as well (e.g., "Whether
the problem was solved by PDLP, Dual Simplex, or Barrier").
In `@cpp/src/dual_simplex/branch_and_bound.cpp`:
- Around line 1357-1358: The current assignment to solver_name treats any
non-Barrier value as "PDLP", causing misleading logs when root_relax_solved_by
is Unset or DualSimplex; change the logic in the block that assigns solver_name
(referencing solver_name and root_relax_solved_by) to use a switch/if chain over
lp_solver_type_t (check lp_solver_type_t::Barrier, lp_solver_type_t::PDLP,
lp_solver_type_t::DualSimplex and lp_solver_type_t::Unset or a default case) and
set solver_name to the exact labels ("Barrier", "PDLP", "DualSimplex", "Unset"
or "Unknown") so the logged value accurately reflects the enum.
In `@cpp/src/linear_programming/pdlp.cu`:
- Around line 768-773: The termination-info entry can retain stale solver
metadata when the status is ConcurrentLimit; before the existing conditional in
the PDLP termination handling, explicitly reset
batch_solution_to_return_.get_additional_termination_informations()[climber_strategies_[i].original_index].solved_by
to lp_solver_type_t::Unset, then keep the existing if
(current_termination_strategy_.get_termination_status(i) !=
pdlp_termination_status_t::ConcurrentLimit) assignment to set solved_by =
lp_solver_type_t::PDLP; do the same reset at the other location covering lines
839–844 to ensure no stale values remain.
In `@python/cuopt_server/cuopt_server/utils/linear_programming/solver.py`:
- Line 493: The change replaces the public key "solved_by_pdlp" with "solved_by"
and also mismatches types: keep backward compatibility by preserving the old key
while adding the new key and emitting a DeprecationWarning; set both values from
sol.get_solved_by() (an int) and update the data model annotation in
data_definition.py to solved_by: int; specifically, in solver.py where you
assign solution["solved_by"] = sol.get_solved_by(), also set
solution["solved_by_pdlp"] = sol.get_solved_by() but wrap a DeprecationWarning
(or warnings.warn) indicating the key is deprecated, and change the type
declaration for solved_by in data_definition.py from bool to int.
In `@python/cuopt/cuopt/linear_programming/solution/solution.py`:
- Around line 119-121: Add a deprecated alias for the renamed API: implement a
solved_by_pdlp shim that maps to the new solved_by attribute and emits a
deprecation warning; specifically, in the Solution class add a `@property` def
solved_by_pdlp(self) that returns self.solved_by and a setter that assigns to
self.solved_by while calling warnings.warn("solved_by_pdlp is deprecated; use
solved_by", DeprecationWarning, stacklevel=2); also accept and translate an
incoming constructor/initializer keyword/parameter solved_by_pdlp to solved_by
(and log the same deprecation) and update any other places that previously
referenced solved_by_pdlp to use the shim so callers remain compatible.
🧹 Nitpick comments (3)
python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py (1)
588-588: LGTM - DualSimplex solver attribution verified.The assertion correctly verifies that DualSimplex solver returns
solved_by == 1.Consider defining named constants for solver type codes to improve readability and reduce magic numbers:
# At module level or in a constants file SOLVER_DUAL_SIMPLEX = 1 SOLVER_PDLP = 2 SOLVER_BARRIER = 3 # Then in tests: assert solution.get_solved_by() == SOLVER_DUAL_SIMPLEXThis would make tests more self-documenting and easier to maintain if enum values change.
cpp/tests/linear_programming/pdlp_test.cu (1)
977-978: Consider adding concurrent-mode coverage for Barrier/PDLP winners.Right now the solved_by assertion only covers DualSimplex for the empty-matrix concurrent case; adding one case where Barrier or PDLP wins would exercise the new enum more fully.
python/cuopt_self_hosted/cuopt_sh_client/thin_client_solution.py (1)
69-70: Consider backward compatibility for this breaking API change.The method
get_solved_by_pdlp()has been renamed toget_solved_by()without a deprecation path. While the PR is labeled as "breaking," consider whether a deprecation wrapper would ease migration for existing users:def get_solved_by_pdlp(self): """Deprecated: Use get_solved_by() instead.""" import warnings warnings.warn("get_solved_by_pdlp is deprecated, use get_solved_by instead", DeprecationWarning) return self.solved_by == 2 # PDLPIf full removal is intentional for this release, disregard this suggestion. As per coding guidelines, Python APIs should maintain backward compatibility with deprecation warnings.
Also applies to: 194-198
python/cuopt_server/cuopt_server/utils/linear_programming/solver.py
Outdated
Show resolved
Hide resolved
Signed-off-by: Nicolas Guidotti <224634272+nguidotti@users.noreply.github.com>
Signed-off-by: Nicolas Guidotti <224634272+nguidotti@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cpp/include/cuopt/linear_programming/utilities/cython_types.hpp`:
- Line 86: The C++ field was renamed/retyped to linear_programming::method_t
solved_by_, but Cython still declares and accesses the old bool solved_by_pdlp_;
update the Cython declarations and accessors to match the new symbol and type:
change the pxd declaration from bool solved_by_pdlp_ to the appropriate mapped
enum/typedef for linear_programming::method_t (or an int/enum proxy) and replace
all uses of lp_ptr.solved_by_pdlp_ in the wrapper with lp_ptr.solved_by_ (and
adjust any enum/value conversions in solver_wrapper.pyx accordingly).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7cfc763e-b131-45cf-baf6-c813ba5a01b1
📒 Files selected for processing (16)
cpp/include/cuopt/linear_programming/constants.hcpp/include/cuopt/linear_programming/cpu_optimization_problem_solution.hppcpp/include/cuopt/linear_programming/mip/solver_solution.hppcpp/include/cuopt/linear_programming/optimization_problem_solution.hppcpp/include/cuopt/linear_programming/optimization_problem_solution_interface.hppcpp/include/cuopt/linear_programming/pdlp/solver_settings.hppcpp/include/cuopt/linear_programming/pdlp/solver_solution.hppcpp/include/cuopt/linear_programming/utilities/cython_types.hppcpp/src/branch_and_bound/branch_and_bound.cppcpp/src/branch_and_bound/branch_and_bound.hppcpp/src/dual_simplex/crossover.cppcpp/src/dual_simplex/right_looking_lu.cppcpp/src/mip_heuristics/diversity/diversity_manager.cucpp/src/mip_heuristics/problem/problem.cuhcpp/src/mip_heuristics/solver.cucpp/src/pdlp/pdlp.cu
🚧 Files skipped from review as they are similar to previous changes (4)
- cpp/src/dual_simplex/right_looking_lu.cpp
- cpp/include/cuopt/linear_programming/pdlp/solver_solution.hpp
- cpp/include/cuopt/linear_programming/mip/solver_solution.hpp
- cpp/include/cuopt/linear_programming/constants.h
| int nb_iterations_{}; | ||
| double solve_time_{}; | ||
| bool solved_by_pdlp_{}; | ||
| linear_programming::method_t solved_by_{}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd 'cython_types\.hpp$|solver\.pxd$|solver_wrapper\.pyx$'
rg -n "solved_by_pdlp_|solved_by_" \
cpp/include/cuopt/linear_programming/utilities/cython_types.hpp \
python/cuopt/cuopt/linear_programming/solver/solver.pxd \
python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyxRepository: NVIDIA/cuopt
Length of output: 671
Cython struct field mismatch: solved_by_pdlp_ declaration and accessors not updated to match C++ field rename/retype.
The C++ struct field at line 86 changed to linear_programming::method_t solved_by_{}, but Cython declarations and wrapper code still reference the old field:
solver.pxd:178declaresbool solved_by_pdlp_solver_wrapper.pyx:480, 499accesslp_ptr.solved_by_pdlp_
Cython will attempt to access a non-existent field, causing a compile or runtime error. Update solver.pxd to declare the new field with correct type, and update all accessors in solver_wrapper.pyx to use the new name.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cpp/include/cuopt/linear_programming/utilities/cython_types.hpp` at line 86,
The C++ field was renamed/retyped to linear_programming::method_t solved_by_,
but Cython still declares and accesses the old bool solved_by_pdlp_; update the
Cython declarations and accessors to match the new symbol and type: change the
pxd declaration from bool solved_by_pdlp_ to the appropriate mapped enum/typedef
for linear_programming::method_t (or an int/enum proxy) and replace all uses of
lp_ptr.solved_by_pdlp_ in the wrapper with lp_ptr.solved_by_ (and adjust any
enum/value conversions in solver_wrapper.pyx accordingly).
Signed-off-by: Nicolas Guidotti <224634272+nguidotti@users.noreply.github.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
python/cuopt/cuopt/linear_programming/solution/solution.py (1)
200-200:⚠️ Potential issue | 🔴 CriticalFix the undefined
solved_bypath and keep the old API as a shim.Line 200 calls
SolverMethod(solved_by), but__init__still only acceptssolved_by_pdlp, so constructingSolutionnow raisesNameError. Downstream, the legacysolved_by_pdlpattribute/getter is no longer backed by state either. Please add asolved_byparameter, keepsolved_by_pdlpas a deprecated alias, and derive the legacy bool fromself.solved_byso existing callers keep working. As per coding guidelines, 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/solution/solution.py` at line 200, The constructor for the Solution class calls SolverMethod(solved_by) but __init__ only accepts solved_by_pdlp, causing NameError and breaking the legacy API; update Solution.__init__ to accept a new solved_by parameter (optional), set self.solved_by = SolverMethod(solved_by) (fall back to interpreting solved_by_pdlp if solved_by is None), keep solved_by_pdlp as a deprecated alias (e.g., set self.solved_by_pdlp = bool(self.solved_by == SolverMethod.PDLP) or compute it from self.solved_by) and emit a DeprecationWarning when solved_by_pdlp is used so existing callers work while guiding them to use solved_by instead; reference Solution.__init__, solved_by, solved_by_pdlp, and SolverMethod when making changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@python/cuopt/cuopt/linear_programming/solution/solution.py`:
- Line 200: The constructor for the Solution class calls SolverMethod(solved_by)
but __init__ only accepts solved_by_pdlp, causing NameError and breaking the
legacy API; update Solution.__init__ to accept a new solved_by parameter
(optional), set self.solved_by = SolverMethod(solved_by) (fall back to
interpreting solved_by_pdlp if solved_by is None), keep solved_by_pdlp as a
deprecated alias (e.g., set self.solved_by_pdlp = bool(self.solved_by ==
SolverMethod.PDLP) or compute it from self.solved_by) and emit a
DeprecationWarning when solved_by_pdlp is used so existing callers work while
guiding them to use solved_by instead; reference Solution.__init__, solved_by,
solved_by_pdlp, and SolverMethod when making changes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6f6bacd5-bfe2-4bdf-8a51-4699b0aa980f
📒 Files selected for processing (4)
python/cuopt/cuopt/linear_programming/solution/solution.pypython/cuopt/cuopt/linear_programming/solver/solver.pxdpython/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyxpython/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py
✅ Files skipped from review due to trivial changes (2)
- python/cuopt/cuopt/linear_programming/solver_settings/solver_settings.py
- python/cuopt/cuopt/linear_programming/solver/solver_wrapper.pyx
|
/ok to test fec19e1 |
|
/ok to test fc0a39d |
|
/ok to test 9c00324 |
| Time used for pre-solve | ||
| solve_time: Float64 | ||
| Solve time in seconds | ||
| solved_by_pdlp: bool |
There was a problem hiding this comment.
This would be a breaking change since we are replacing this with something else. May for this time, we can cleanly remove it, but for next time lets plan a support for 2 releases with deprecation for major elements before completely removing it.
There was a problem hiding this comment.
If you prefer, we can keep the solved_by_pdlp for now, set as solved_by_pdlp = solved_by == SolverMethod.PDLP and add a deprecation warning.
Signed-off-by: Nicolas Guidotti <224634272+nguidotti@users.noreply.github.com>
Signed-off-by: Nicolas Guidotti <224634272+nguidotti@users.noreply.github.com>
Signed-off-by: Nicolas Guidotti <224634272+nguidotti@users.noreply.github.com>
|
/ok to test dd24b5a |
5 participants
This PR replaced
solved_by_pdlpwithsolved_byinoptimization_problem_solution_tand all associated objects, such that now it is possible to retrieve which method was used for solving the LP when running in concurrent mode. This also fix a typo in theCUOPT_TERMINATION_STATUSand updates the B&B logs to display the method used for solving the root relaxation.Issue
Closes #787
Checklist