Update solution object to include the LP solver#822
Update solution object to include the LP solver#822nguidotti wants to merge 5 commits intoNVIDIA:mainfrom
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.
|
📝 WalkthroughWalkthroughThis pull request introduces a spelling correction to LP/MIP termination status constants (CUOPT_TERIMINATION_STATUS → CUOPT_TERMINATION_STATUS) and replaces the boolean Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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
| if (current_termination_strategy_.get_termination_status(i) != | ||
| pdlp_termination_status_t::ConcurrentLimit) { | ||
| batch_solution_to_return_ | ||
| .get_additional_termination_informations()[climber_strategies_[i].original_index] | ||
| .solved_by = lp_solver_type_t::PDLP; | ||
| } |
There was a problem hiding this comment.
Reset solved_by on ConcurrentLimit to avoid stale solver metadata.
When Line 768/839 skips assignment for ConcurrentLimit, the reused termination-info entry can retain a previous value and misreport the solver. Consider explicitly setting solved_by to lp_solver_type_t::Unset before the conditional.
🛠️ Suggested fix
- if (current_termination_strategy_.get_termination_status(i) !=
- pdlp_termination_status_t::ConcurrentLimit) {
- batch_solution_to_return_
- .get_additional_termination_informations()[climber_strategies_[i].original_index]
- .solved_by = lp_solver_type_t::PDLP;
- }
+ auto& term_info = batch_solution_to_return_
+ .get_additional_termination_informations()[climber_strategies_[i].original_index];
+ term_info.solved_by = lp_solver_type_t::Unset;
+ if (current_termination_strategy_.get_termination_status(i) !=
+ pdlp_termination_status_t::ConcurrentLimit) {
+ term_info.solved_by = lp_solver_type_t::PDLP;
+ }Also applies to: 839-844
🤖 Prompt for AI Agents
In `@cpp/src/linear_programming/pdlp.cu` around lines 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.
| ) | ||
| solution["solver_time"] = sol.get_solve_time() | ||
| solution["solved_by_pdlp"] = sol.get_solved_by_pdlp() | ||
| solution["solved_by"] = sol.get_solved_by() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check the solved_by field in data_definition.py
rg -n 'solved_by' python/cuopt_server/cuopt_server/utils/linear_programming/data_definition.pyRepository: NVIDIA/cuopt
Length of output: 88
🏁 Script executed:
#!/bin/bash
# Search for any deprecation warnings or backward compatibility handling around this change
rg -n 'solved_by' python/cuopt_server/cuopt_server/utils/linear_programming/solver.py -A 3 -B 3Repository: NVIDIA/cuopt
Length of output: 426
🏁 Script executed:
#!/bin/bash
# Check if there are any references to the old "solved_by_pdlp" key elsewhere in the codebase
rg 'solved_by_pdlp' python/cuopt_server/Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Find the get_solved_by() method definition
rg -n 'def get_solved_by|get_solved_by.*return' python/cuopt_server/ -A 2Repository: NVIDIA/cuopt
Length of output: 38
🏁 Script executed:
#!/bin/bash
# Check the method signature in the solver class
rg -n 'get_solved_by\(' python/cuopt_server/cuopt_server/utils/linear_programming/ -B 5 -A 5Repository: NVIDIA/cuopt
Length of output: 1402
Breaking API change requires backward compatibility measures.
The change from "solved_by_pdlp" to "solved_by" removes a public API key without backward compatibility support. Per the coding guidelines, Python server APIs must maintain backward compatibility with deprecation warnings.
Additionally, there is a type mismatch: data_definition.py declares solved_by: bool (line 698) but sol.get_solved_by() returns an integer (1=DualSimplex, 2=PDLP, 3=Barrier). The type should be int.
To comply with guidelines:
- Support the old
"solved_by_pdlp"key with a deprecation warning, or add a migration path - Fix the type annotation to
solved_by: int
🤖 Prompt for AI Agents
In `@python/cuopt_server/cuopt_server/utils/linear_programming/solver.py` at 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.
| solved_by: int | ||
| Whether the problem was solved by Dual Simplex(1), PDLP(2) or Barrier(3) | ||
| """ |
There was a problem hiding this comment.
Add a deprecation shim for solved_by_pdlp to preserve Python API compatibility.
This rename breaks existing callers. Please keep the old parameter/method as a deprecated alias that maps to solved_by.
🛠️ Suggested compatibility shim
@@
-from cuopt.linear_programming.solver.solver_wrapper import (
+import warnings
+from cuopt.linear_programming.solver.solver_wrapper import (
@@
- solved_by=None,
+ solved_by=None,
+ solved_by_pdlp=None,
@@
- self.solve_time = solve_time
- self.solved_by = solved_by
+ if solved_by is None and solved_by_pdlp is not None:
+ warnings.warn(
+ "solved_by_pdlp is deprecated; use solved_by instead.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ solved_by = 2 if solved_by_pdlp else 0 # 0 = Unset
+ self.solve_time = solve_time
+ self.solved_by = solved_by
@@
def get_solved_by(self):
"""
Returns whether the problem was solved by Dual Simplex(1), PDLP(2) or Barrier(3)
"""
return self.solved_by
+
+ def get_solved_by_pdlp(self):
+ """
+ Deprecated. Use get_solved_by().
+ """
+ warnings.warn(
+ "get_solved_by_pdlp is deprecated; use get_solved_by instead.",
+ DeprecationWarning,
+ stacklevel=2,
+ )
+ if self.solved_by is None:
+ return None
+ return self.solved_by == 2Also applies to: 157-157, 196-196, 292-296
🤖 Prompt for AI Agents
In `@python/cuopt/cuopt/linear_programming/solution/solution.py` around lines 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.
1 participant
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
Summary by CodeRabbit
Release Notes
Bug Fixes
New Features
API Changes
solved_by_pdlptosolved_byin solution objects; now returns solver type integer instead of boolean (1=Dual Simplex, 2=PDLP, 3=Barrier).