python: add solve_batch aliases for LP and routing#1260
Closed
Pritiks23 wants to merge 5 commits into
Closed
Conversation
Signed-off-by: Pritika Vipin <65793273+Pritiks23@users.noreply.github.com>
…nto feat/python-solve-batch-alias
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR adds snake_case ChangesBatch solving snake_case aliases
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 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)
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: 2
🧹 Nitpick comments (2)
python/cuopt/cuopt/tests/routing/test_batch_solve.py (1)
86-91: ⚡ Quick winAdd objective/route parity checks in alias test.
Status + vehicle-count parity is good, but it can still miss behavioral drift; include objective (and ideally route) equality checks to verify real output equivalence.
As per coding guidelines, "Flag 'runs without error' tests that don't validate numerical correctness".Proposed assertion additions
for batch_solution, alias_solution in zip(batch_solutions, alias_solutions): assert batch_solution.get_status() == alias_solution.get_status() assert ( batch_solution.get_vehicle_count() == alias_solution.get_vehicle_count() ) + assert batch_solution.get_total_objective() == pytest.approx( + alias_solution.get_total_objective() + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuopt/cuopt/tests/routing/test_batch_solve.py` around lines 86 - 91, Add assertions to verify numerical and route parity between batch_solution and alias_solution in addition to status and vehicle count: assert equality on the objective value (e.g., batch_solution.get_objective() == alias_solution.get_objective() or get_total_cost()/get_cost() if that API exists) and assert route-level equality by comparing route lists or per-vehicle sequences (e.g., batch_solution.get_routes() == alias_solution.get_routes() or iterate vehicles and compare each vehicle's route sequence). Use the existing symbols batch_solution and alias_solution and keep checks consistent with the solution object's API.python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py (1)
534-538: ⚡ Quick winStrengthen alias-parity assertions beyond termination status.
This test currently validates mostly control-path parity; add at least objective/value parity checks to enforce numerical equivalence between
BatchSolveandsolve_batch.As per coding guidelines, "Flag 'runs without error' tests that don't validate numerical correctness".Proposed assertion additions
for i in range(nb_solves): assert ( batch_solutions[i].get_termination_status() == alias_solutions[i].get_termination_status() ) + assert batch_solutions[i].get_primal_objective() == pytest.approx( + alias_solutions[i].get_primal_objective() + ) + assert batch_solutions[i].get_dual_objective() == pytest.approx( + alias_solutions[i].get_dual_objective() + )🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py` around lines 534 - 538, The loop currently only asserts termination parity; expand it to also assert numerical parity by comparing objective values (e.g., call the solution objective getter such as get_objective_value() or the concrete API method used in this test suite) for batch_solutions[i] vs alias_solutions[i] for each i in range(nb_solves), and additionally assert any available numeric metrics (e.g., primal/dual values or gap if the Solution API exposes get_primal_value()/get_dual_value()/get_gap()) to ensure BatchSolve and solve_batch produce numerically equivalent results as well as the same termination status.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@python/cuopt/cuopt/linear_programming/solver/solver.py`:
- Around line 192-194: The public function solve_batch currently lacks type
annotations and a full docstring; update the function signature for solve_batch
to include precise type hints for data_model_list (e.g., Sequence[DataModel] or
List[YourDataModelType]) and solver_settings (e.g.,
Optional[SolverSettingsType]) and the return type (e.g., BatchSolveResult or
appropriate type returned by BatchSolve), and add a complete pydocstyle
docstring documenting Parameters (types and meaning), Returns (type and
description), and Raises (exceptions that BatchSolve may propagate); ensure you
reference the existing BatchSolve wrapper behavior in the docstring and
import/forward any type names needed for annotations so the public API is fully
typed and documented.
In `@python/cuopt/cuopt/routing/vehicle_routing.py`:
- Around line 1604-1606: The public alias solve_batch currently lacks type hints
and a complete API docstring; update the signature to include proper type
annotations (e.g., typing.List[YourDataModelType],
Optional[YourSolverSettingsType] -> ReturnType) and expand its docstring to
include params, returns, and raises sections mirroring BatchSolve's contract.
Specifically, modify solve_batch to annotate the parameters and return value,
document the parameters (data_model_list, solver_settings), the return value
(what BatchSolve returns), and any exceptions that can be raised when calling
BatchSolve so callers have full API and type information. Ensure the docstring
style matches project pydocstyle conventions and reference BatchSolve in the
description.
---
Nitpick comments:
In `@python/cuopt/cuopt/tests/linear_programming/test_lp_solver.py`:
- Around line 534-538: The loop currently only asserts termination parity;
expand it to also assert numerical parity by comparing objective values (e.g.,
call the solution objective getter such as get_objective_value() or the concrete
API method used in this test suite) for batch_solutions[i] vs alias_solutions[i]
for each i in range(nb_solves), and additionally assert any available numeric
metrics (e.g., primal/dual values or gap if the Solution API exposes
get_primal_value()/get_dual_value()/get_gap()) to ensure BatchSolve and
solve_batch produce numerically equivalent results as well as the same
termination status.
In `@python/cuopt/cuopt/tests/routing/test_batch_solve.py`:
- Around line 86-91: Add assertions to verify numerical and route parity between
batch_solution and alias_solution in addition to status and vehicle count:
assert equality on the objective value (e.g., batch_solution.get_objective() ==
alias_solution.get_objective() or get_total_cost()/get_cost() if that API
exists) and assert route-level equality by comparing route lists or per-vehicle
sequences (e.g., batch_solution.get_routes() == alias_solution.get_routes() or
iterate vehicles and compare each vehicle's route sequence). Use the existing
symbols batch_solution and alias_solution and keep checks consistent with the
solution object's API.
🪄 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: 8ac3bf12-0774-42b7-aa2a-4a2fcf877822
📒 Files selected for processing (8)
python/cuopt/cuopt/linear_programming/__init__.pypython/cuopt/cuopt/linear_programming/solver/__init__.pypython/cuopt/cuopt/linear_programming/solver/solver.pypython/cuopt/cuopt/routing/__init__.pypython/cuopt/cuopt/routing/vehicle_routing.pypython/cuopt/cuopt/tests/linear_programming/test_lp_solver.pypython/cuopt/cuopt/tests/routing/API_COVERAGE.mdpython/cuopt/cuopt/tests/routing/test_batch_solve.py
Signed-off-by: Pritika Vipin <65793273+Pritiks23@users.noreply.github.com>
Contributor
|
The |
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.
Why this change
Users naturally expect a snake_case batch API in Python (
solve_batch([...])) that mirrors common ML serving patterns (single-call batch inference).cuOpt already has batch solving support through
BatchSolve(...), but discoverability is lower and naming is inconsistent with common Python style.This PR improves API ergonomics without changing solver behavior.
What we changed
solve_batch(...)aliases for:BatchSolve(...)fully intact for backward compatibility.solve_batch(...)returns the same outcomes asBatchSolve(...).What we did NOT change
BatchSolve(...).User impact
Before:
BatchSolve(...).After:
BatchSolve(...)or the clearer Python-stylesolve_batch(...).Compatibility and risk
Testing
cuopt/cudf) were unavailable in the shell; tests are included in the PR for CI validation.