remove extra or unnecessary parameters in Python gRPC api - #1654
remove extra or unnecessary parameters in Python gRPC api#1654tmckayus wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughThe C++ and Python gRPC clients now use server-reported result types for unified LP/MIP retrieval. Tests cover unary and chunked downloads. Python incumbent streaming now requires settings-based callbacks. ChangesUnified result handling
Python client stream API
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 1
🧹 Nitpick comments (1)
python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx (1)
322-332: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTwo public
Clientmethods drop parameters without a deprecation shim.Both
result()andstart_incumbent_stream()remove a previously accepted keyword argument outright instead of deprecating it. The shared root cause is the same: the review guide requires that breaking Cython/Python signature changes normally preserve compatibility with aDeprecationWarning.
python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx#L322-L332: add a deprecated, ignoredis_mip=Nonekeyword toresult()that emits aDeprecationWarningwhen explicitly set, instead of raisingTypeErroron old call sites.python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx#L511-L517: add a deprecatedcallback=Nonekeyword tostart_incumbent_stream()that emits aDeprecationWarningdirecting callers to register callbacks throughsettings, instead of raisingTypeErroron old call sites.As per path instructions: "Cython/public Python signature changes are breaking API changes and should normally preserve compatibility with a DeprecationWarning."
🤖 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/grpc/linear_programming/grpc_client.pyx` around lines 322 - 332, Update python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx:322-332 in Client.result to accept an ignored is_mip=None keyword and emit a DeprecationWarning only when it is explicitly provided; update python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx:511-517 in Client.start_incumbent_stream to accept callback=None and emit a DeprecationWarning directing callers to register callbacks through settings. Preserve the current behavior otherwise.Source: Path instructions
🤖 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/grpc/linear_programming/grpc_client.pyx`:
- Around line 535-543: Update the settings validation around get_mip_callbacks
so it requires at least one callback that is an instance of GetSolutionCallback,
rather than accepting any non-null callback. Revise the GrpcError message to
state that a GetSolutionCallback is required for incumbent forwarding, while
preserving the existing settings-required validation.
---
Nitpick comments:
In `@python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx`:
- Around line 322-332: Update
python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx:322-332 in
Client.result to accept an ignored is_mip=None keyword and emit a
DeprecationWarning only when it is explicitly provided; update
python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx:511-517 in
Client.start_incumbent_stream to accept callback=None and emit a
DeprecationWarning directing callers to register callbacks through settings.
Preserve the current behavior otherwise.
🪄 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: bb84c960-d402-4e64-9308-9486803cd95b
📒 Files selected for processing (8)
cpp/include/cuopt/grpc/cython_grpc_client.hppcpp/src/grpc/client/cython_grpc_client.cppcpp/src/grpc/client/grpc_client.cppcpp/src/grpc/client/grpc_client.hppcpp/tests/linear_programming/grpc/grpc_client_test.cppcpp/tests/linear_programming/grpc/grpc_integration_test.cpppython/cuopt/cuopt/grpc/linear_programming/grpc_client.pxdpython/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx
…cumbents Add grpc_client_t::get_result alongside the existing get_lp_result / get_mip_result APIs (internals unchanged for now). Wire the Python async client result() path through get_result so LP vs MIP comes from the server, and require SolverSettings mip callbacks for start_incumbent_stream.
Add mock client tests so the unified get_result API is exercised for both problem types and both download modes.
d61440b to
dc593dc
Compare
CI Test Summary✅ All 31 test job(s) passed. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cpp/tests/linear_programming/grpc/grpc_client_test.cpp (1)
943-947: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAssert the decoded solution vector, not only the objective. All four new tests feed known solution values through the mock but check only the objective value and the pointer identity. The shared root cause is that no test validates the decoded vector, so a decoding defect in the unary or chunked path passes undetected while the objective still matches.
cpp/tests/linear_programming/grpc/grpc_client_test.cpp#L943-L947: assert the LP primal solution equals{1.5, 2.5}.cpp/tests/linear_programming/grpc/grpc_client_test.cpp#L979-L983: assert the MIP solution equals{1.0, 0.0}.cpp/tests/linear_programming/grpc/grpc_client_test.cpp#L1047-L1051: assert the chunk-decoded LP primal solution equals{1.5, 2.5}.cpp/tests/linear_programming/grpc/grpc_client_test.cpp#L1115-L1119: assert the chunk-decoded MIP solution equals{1.0, 0.0}.The chunked sites matter most. They exercise the raw byte reinterpretation in
download_chunked_result, where an element-size or offset defect changes the vector but leaves the header objective intact.As per path instructions: "Numerical correctness validation (not just 'runs without error')".
🤖 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 `@cpp/tests/linear_programming/grpc/grpc_client_test.cpp` around lines 943 - 947, Extend the assertions in cpp/tests/linear_programming/grpc/grpc_client_test.cpp at lines 943-947, 979-983, 1047-1051, and 1115-1119 to validate the decoded solution vectors: LP cases must equal {1.5, 2.5} and MIP cases must equal {1.0, 0.0}; retain the existing objective and pointer assertions, including for the chunk-decoded paths.Source: Path instructions
🤖 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 `@cpp/tests/linear_programming/grpc/grpc_client_test.cpp`:
- Around line 941-948: Add a regression test near the existing unified unary
get_result tests, named GetResultUnified_UnaryMissingSolution, that mocks a
completed CheckStatus response and a successful GetResult response with neither
solution field populated. Call client_->get_result for the missing-solution job
and assert success is false, both lp_solution and mip_solution are null, and
error_message is non-empty.
---
Nitpick comments:
In `@cpp/tests/linear_programming/grpc/grpc_client_test.cpp`:
- Around line 943-947: Extend the assertions in
cpp/tests/linear_programming/grpc/grpc_client_test.cpp at lines 943-947,
979-983, 1047-1051, and 1115-1119 to validate the decoded solution vectors: LP
cases must equal {1.5, 2.5} and MIP cases must equal {1.0, 0.0}; retain the
existing objective and pointer assertions, including for the chunk-decoded
paths.
🪄 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: 1e13f231-83cf-4798-a5b4-dff43c68fc7c
📒 Files selected for processing (7)
cpp/include/cuopt/grpc/cython_grpc_client.hppcpp/src/grpc/client/cython_grpc_client.cppcpp/src/grpc/client/grpc_client.cppcpp/src/grpc/client/grpc_client.hppcpp/tests/linear_programming/grpc/grpc_client_test.cpppython/cuopt/cuopt/grpc/linear_programming/grpc_client.pxdpython/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx
🚧 Files skipped from review as they are similar to previous changes (5)
- cpp/include/cuopt/grpc/cython_grpc_client.hpp
- python/cuopt/cuopt/grpc/linear_programming/grpc_client.pxd
- cpp/src/grpc/client/cython_grpc_client.cpp
- python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx
- cpp/src/grpc/client/grpc_client.cpp
| auto result = client_->get_result<int32_t, double>("unified-lp-unary"); | ||
|
|
||
| EXPECT_TRUE(result.success) << result.error_message; | ||
| EXPECT_FALSE(result.is_mip); | ||
| ASSERT_NE(result.lp_solution, nullptr); | ||
| EXPECT_EQ(result.mip_solution, nullptr); | ||
| EXPECT_NEAR(result.lp_solution->get_objective_value(), -464.753, 0.01); | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Add a test for a unary response that carries no solution.
The new tests cover only success paths. get_result has a reachable error path: a unary ResultResponse with neither lp_solution nor mip_solution set. That input falls through the is_mip check into the LP branch and returns "GetResult succeeded but no LP solution in response". No test pins this behavior. Without it, a later refactor can return success == true with both solution pointers null, and grpc_python_client_t::result in cpp/src/grpc/client/cython_grpc_client.cpp then dereferences a null lp_solution.
Add one test that returns an empty ResultResponse and asserts success == false plus both pointers null.
💚 Proposed test
TEST_F(GrpcClientTest, GetResultUnified_UnaryMissingSolution)
{
EXPECT_CALL(*mock_stub_, CheckStatus(_, _, _))
.WillOnce([](grpc::ClientContext*,
const cuopt::remote::StatusRequest&,
cuopt::remote::StatusResponse* resp) {
resp->set_job_status(cuopt::remote::COMPLETED);
resp->set_result_size_bytes(64);
resp->set_max_message_bytes(256 * 1024 * 1024);
return grpc::Status::OK;
});
EXPECT_CALL(*mock_stub_, GetResult(_, _, _))
.WillOnce([](grpc::ClientContext*,
const cuopt::remote::GetResultRequest&,
cuopt::remote::ResultResponse* resp) {
resp->set_status(cuopt::remote::SUCCESS);
return grpc::Status::OK;
});
auto result = client_->get_result<int32_t, double>("unified-missing");
EXPECT_FALSE(result.success);
EXPECT_EQ(result.lp_solution, nullptr);
EXPECT_EQ(result.mip_solution, nullptr);
EXPECT_FALSE(result.error_message.empty());
}As per path instructions: "Edge cases: empty, infeasible, unbounded, degenerate, singleton problems" and "When a bug fix lands, a regression test should cover the specific case".
🤖 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 `@cpp/tests/linear_programming/grpc/grpc_client_test.cpp` around lines 941 -
948, Add a regression test near the existing unified unary get_result tests,
named GetResultUnified_UnaryMissingSolution, that mocks a completed CheckStatus
response and a successful GetResult response with neither solution field
populated. Call client_->get_result for the missing-solution job and assert
success is false, both lp_solution and mip_solution are null, and error_message
is non-empty.
Source: Path instructions
This limits the changes. Internals will be cleaned up later
There was a problem hiding this comment.
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/grpc/linear_programming/grpc_client.pyx (1)
167-171: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDo not use
TypeErrorto detect callback arity.If the callback raises
TypeErrorinside its four-argument body, this code invokes it again with three arguments. The retry can duplicate side effects and replace the original error with an arity error. Keep the temporary compatibility path, but select the callback arity before invocation or use separate adapters.🤖 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/grpc/linear_programming/grpc_client.pyx` around lines 167 - 171, Update _call_incumbent_callback so it no longer catches TypeError from callback execution to determine arity. Preserve the temporary support for both callback signatures by determining the callable’s accepted arity before invocation or by using separate adapters, and ensure errors raised inside the callback propagate without retrying or duplicating side effects.
🤖 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.
Outside diff comments:
In `@python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx`:
- Around line 167-171: Update _call_incumbent_callback so it no longer catches
TypeError from callback execution to determine arity. Preserve the temporary
support for both callback signatures by determining the callable’s accepted
arity before invocation or by using separate adapters, and ensure errors raised
inside the callback propagate without retrying or duplicating side effects.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c5fe0b83-ac1e-4aef-a686-b7f14ef94752
📒 Files selected for processing (1)
python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx
1 participant
On review of the Python gRPC api, we have a few unnecessary parameters that can be removed:
"is_mip" on result is not necessary, can and should be learned from the server. Not only is it unnecessary, actually, but the implementation is seriously flawed. The is_mip value is currently being tracked in the client itself, not by data downloaded, and this parameter is being set by the client. This means that the same client can't be used for mixed submission of LPs and MIPs in a series of submissions and then retrieved later. Also, the same job can't be submitted by one client and retrieved by another unless the job type is known. This change adds a new get_result API to support the Python client which determines the job type from the download.
the "callback" parameter on start_incumbent_stream is not needed because incumbent callbacks are registered through "settings" as with a local solve, and in fact at least one registration in "settings" is needed for the server to send incumbent solutions at all. So "callback" is unnecessary and confusing. Remove it from the API, the internal plumbing to support it will be removed later.