docs: add Python async gRPC client guide under gRPC remote execution - #1653
Open
tmckayus wants to merge 2 commits into
Open
docs: add Python async gRPC client guide under gRPC remote execution#1653tmckayus wants to merge 2 commits into
tmckayus wants to merge 2 commits into
Conversation
Document remote execution vs explicit gRPC clients, add quick-start and streaming examples, and clarify integrated-client environment variables. Also clarify some existing docs on API and server behavior.
📝 WalkthroughWalkthroughThe PR reorganizes gRPC documentation around integrated remote execution and explicit Python async clients. It adds client guides, API references, streaming examples, TLS guidance, updated server capacity details, and Python client stream behavior documentation. ChangesgRPC documentation and client guidance
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 11
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)
576-580: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winKeep a timed-out incumbent stream registered.
join_incumbent_stream()removes the job from_incumbent_threadsbeforethread.join(timeout). If the timeout expires, the thread remains alive butdelete()no longer sees it and can delete server state while_poll_incumbents()is still running.Keep the entry until the thread finishes.
Proposed fix
- thread = self._incumbent_threads.pop(job_id, None) + thread = self._incumbent_threads.get(job_id) if thread is not None: thread.join(timeout) + if thread.is_alive(): + return + self._incumbent_threads.pop(job_id, None)🤖 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 576 - 580, Update join_incumbent_stream() so _incumbent_threads retains the job entry while thread.join(timeout) returns with the thread still alive; remove the entry only after the thread has finished. Ensure delete() can still observe the active incumbent-stream thread, while preserving the existing error retrieval from _incumbent_thread_errors.
🧹 Nitpick comments (1)
python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx (1)
534-538: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a runtime warning and removal version for the deprecated callback.
The docstring marks
callbackas deprecated, but the method does not emitDeprecationWarningand does not state a removal version. Add both, or remove the deprecation wording until the removal policy is defined.As per coding guidelines, a public Python API signature change must emit
DeprecationWarningwith a removal version before the old signature is broken.🤖 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 534 - 538, Update the public method containing the deprecated callback documentation to emit a DeprecationWarning whenever the plain callback argument is used, and specify the planned removal version in its deprecation documentation. Keep the callback behavior unchanged until removal; if no removal version can be established, remove the deprecation wording instead.Source: Coding guidelines
🤖 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 `@docs/cuopt/source/cuopt-grpc/api.rst`:
- Around line 104-107: Expand the Errors section in the API documentation to
specify each RPC’s status contract: CheckStatus uses Status::OK with
job_status=NOT_FOUND for unknown jobs; GetResult uses transport NOT_FOUND for
unknown jobs, UNAVAILABLE when results are not ready, and Status::OK with
status=ERROR_SOLVE_FAILED for failed jobs; DeleteResult and CancelJob use
Status::OK with outcomes reported in response fields. Keep the existing general
transport-versus-response-field distinction and proto reference.
- Around line 100-103: Update the “Problem types” documentation in the SubmitJob
section to identify the supported wire categories as LP/QP or MILP. Explicitly
state that QP is submitted through lp_request using the SolveLPRequest payload,
with quadratic fields in OptimizationProblem, while preserving the existing
routing availability note.
In `@docs/cuopt/source/cuopt-grpc/examples.rst`:
- Around line 9-18: Revise the opening remote-execution statement in the
examples documentation to limit the “run unchanged” claim to the integrated
Python, C API, and cuopt_cli examples. Exclude the separately documented Python
async client, which requires explicit Client(host, port) configuration and does
not use CUOPT_REMOTE_* variables.
In `@docs/cuopt/source/cuopt-grpc/examples/incumbent_stream_demo.py`:
- Around line 11-17: Update the default port used by the incumbent_stream_demo
example and its surrounding server/client commands to 5001, matching
cuopt_grpc_server and the linked guides; ensure running the script without
--port connects to the documented server port consistently.
- Around line 31-55: Add explicit type annotations to
IncumbentPrinter.get_solution, build_problem, and main, using Any for
extension-provided types that cannot be named; annotate main’s optional argv and
return values appropriately. Add meaningful docstrings to build_problem and main
documenting parameters, returns, and raises, and document get_solution as needed
for the public callback API while preserving its behavior.
- Around line 45-52: Update main() so every non-COMPLETED result from
client.wait() calls client.delete(job_id) and joins the incumbent-stream thread
before returning. Add pytest coverage for main() using a fake Client that
verifies cleanup for both COMPLETED and non-completed statuses, while retaining
test_mip_incumbent_stream as integration coverage.
In `@docs/cuopt/source/cuopt-grpc/python-async-client.rst`:
- Around line 54-67: Wrap the job lifecycle in all four examples with
try/finally so delete() executes on every exit path, including timeout, status,
transport, callback, result, and stream-join failures. In
docs/cuopt/source/cuopt-grpc/python-async-client.rst (54-67),
docs/cuopt/source/cuopt-grpc/python-async-client-examples.rst (17-34 and 76-86),
and docs/cuopt/source/cuopt-grpc/examples/incumbent_stream_demo.py (69-86),
explicitly join any log streams before cleanup while keeping delete() in the
finally block so it still runs if joining raises.
In `@docs/cuopt/source/cuopt-grpc/quick-start.rst`:
- Line 74: Update the quick-start server instructions to use the same default
port as the incumbent_stream_demo.py example, or explicitly instruct users to
pass --port 5001 when running that example; ensure the documented commands are
consistent so the examples connect without additional troubleshooting.
- Around line 48-52: Update the quick-start installation instructions around the
GPU server/client selector and the subsequent cuopt_grpc_server verification
command to state that the server-binary check runs only on the GPU server using
the C/libcuopt bundle; keep Python-only client guidance from implying that
command is available, and verify the documented examples and instructions
execute correctly.
- Around line 145-169: Update the async gRPC example around Client.submit,
Client.wait, and Client.result to avoid assert-based runtime validation and wrap
all post-submission operations in try/finally. Validate that wait returns
JobStatus.COMPLETED, allow failures or result errors to propagate, and always
call client.delete(job_id) whenever submission succeeds, including every exit
path.
In `@python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx`:
- Around line 287-291: Update the docstring for the job-waiting method
containing this documentation to describe that non-None timeouts are converted
with int(timeout), including the resulting indefinite wait for values such as
0.5, and that positive timeouts poll every second and raise GrpcError when they
expire instead of returning JobStatus; alternatively, validate timeout values to
prevent this behavior and document the enforced contract.
---
Outside diff comments:
In `@python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx`:
- Around line 576-580: Update join_incumbent_stream() so _incumbent_threads
retains the job entry while thread.join(timeout) returns with the thread still
alive; remove the entry only after the thread has finished. Ensure delete() can
still observe the active incumbent-stream thread, while preserving the existing
error retrieval from _incumbent_thread_errors.
---
Nitpick comments:
In `@python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx`:
- Around line 534-538: Update the public method containing the deprecated
callback documentation to emit a DeprecationWarning whenever the plain callback
argument is used, and specify the planned removal version in its deprecation
documentation. Keep the callback behavior unchanged until removal; if no removal
version can be established, remove the deprecation wording instead.
🪄 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: c0d71359-29d7-4f47-bc73-f2eee2d3d38c
📒 Files selected for processing (15)
docs/cuopt/source/_static/large-rubric.cssdocs/cuopt/source/conf.pydocs/cuopt/source/cuopt-grpc/advanced.rstdocs/cuopt/source/cuopt-grpc/api.rstdocs/cuopt/source/cuopt-grpc/examples.rstdocs/cuopt/source/cuopt-grpc/examples/incumbent_stream_demo.pydocs/cuopt/source/cuopt-grpc/grpc-server-architecture.mddocs/cuopt/source/cuopt-grpc/index.rstdocs/cuopt/source/cuopt-grpc/python-async-client-api.rstdocs/cuopt/source/cuopt-grpc/python-async-client-examples.rstdocs/cuopt/source/cuopt-grpc/python-async-client.rstdocs/cuopt/source/cuopt-grpc/quick-start.rstdocs/cuopt/source/cuopt-python/index.rstdocs/cuopt/source/introduction.rstpython/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx
Comment on lines
+31
to
+55
| class IncumbentPrinter(GetSolutionCallback): | ||
| """Same callback type used for local ``problem.solve(settings)``.""" | ||
|
|
||
| def __init__(self): | ||
| super().__init__() | ||
| self.entries = [] | ||
|
|
||
| def get_solution(self, solution, solution_cost, solution_bound, user_data): | ||
| cost = float(solution_cost[0]) | ||
| values = solution.tolist() | ||
| self.entries.append({"cost": cost, "solution": values}) | ||
| print(f"incumbent cost={cost:.4f} values={values}", flush=True) | ||
|
|
||
|
|
||
| def build_problem(): | ||
| problem = Problem("incumbent_stream_demo") | ||
| x = problem.addVariable(lb=0, ub=10, vtype=INTEGER, name="x") | ||
| y = problem.addVariable(lb=0, ub=10, vtype=INTEGER, name="y") | ||
| problem.addConstraint(x + y <= 10, name="c1") | ||
| problem.addConstraint(x - y >= 0, name="c2") | ||
| problem.setObjective(x + 2 * y, sense=MAXIMIZE) | ||
| return problem | ||
|
|
||
|
|
||
| def main(argv=None): |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Add the required type hints and API docstrings.
build_problem, main, and IncumbentPrinter.get_solution are new public Python functions or methods without annotations. build_problem and main also lack meaningful docstrings covering parameters, returns, and raises. Add explicit annotations, using Any where extension types cannot be named.
As per coding guidelines, new public Python functions and classes require type hints and meaningful docstrings covering parameters, returns, and raises.
🤖 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 `@docs/cuopt/source/cuopt-grpc/examples/incumbent_stream_demo.py` around lines
31 - 55, Add explicit type annotations to IncumbentPrinter.get_solution,
build_problem, and main, using Any for extension-provided types that cannot be
named; annotate main’s optional argv and return values appropriately. Add
meaningful docstrings to build_problem and main documenting parameters, returns,
and raises, and document get_solution as needed for the public callback API
while preserving its behavior.
Source: Coding guidelines
CI Test Summary✅ All 31 test job(s) passed. |
includes one code fix on log streaming
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 (3)
python/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx (3)
421-433: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winHonor
Falseduring log backfill.
start_log_streamdocumentsFalseas an early-stop signal. The fallback path calls_call_log_callback(...)but ignores its return value, so it continues invoking the callback after the callback returnsFalse.Stop the backfill loop when
_call_log_callback(...) is False.Suggested fix
for line in bulk: state["lines"].append(line) - _call_log_callback(state["callback"], line, True) + if _call_log_callback(state["callback"], line, True) is False: + break🤖 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 421 - 433, Update the log backfill loop in the method documenting start_log_stream behavior to inspect the return value of _call_log_callback(...). Stop iterating immediately when it returns False, while preserving the existing backfill and callback behavior for other return values.
308-322: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winCancel the job before joining the active incumbent stream.
delete()joinsjoin_incumbent_stream(job_id)before it calls the server’sdelete_job. The join has no timeout here, and_poll_incumbents()normally exits only after completion or cancellation. Deleting a running job with an active incumbent stream can therefore block indefinitely and never reach server cleanup. A stored stream error can also raise before deletion.Cancel queued or processing jobs before joining. Ensure the server deletion still runs when joining reports a stream error.
Suggested ordering
if job_id in self._incumbent_threads: + if self.status(job_id) in ( + JobStatus.QUEUED, + JobStatus.PROCESSING, + ): + self.cancel(job_id) self.join_incumbent_stream(job_id)Also applies to: 590-599
🤖 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 308 - 322, Update delete() to cancel queued or processing jobs before calling join_incumbent_stream(job_id), allowing _poll_incumbents() to exit before the join. Preserve any stream error while ensuring the server delete_job operation always runs, then propagate the stored error after deletion if appropriate.
549-559: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winCancel the job before joining an active incumbent stream.
Client.delete()joins the stream before deleting the job and never requests cancellation. An active stream can hang indefinitely. Cancel the job before joining it, and add tests for active-stream deletion, callback cancellation,DeprecationWarning, timed joins, and stored worker errors.🤖 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 549 - 559, Update Client.delete() to request job cancellation before joining any active incumbent stream, preventing an indefinite wait; preserve deletion after the stream exits. Add coverage for active-stream deletion, callback-triggered cancellation, DeprecationWarning behavior in start_incumbent_stream(), timed joins, and propagation of stored worker errors.Source: Coding guidelines
🤖 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 421-433: Update the log backfill loop in the method documenting
start_log_stream behavior to inspect the return value of
_call_log_callback(...). Stop iterating immediately when it returns False, while
preserving the existing backfill and callback behavior for other return values.
- Around line 308-322: Update delete() to cancel queued or processing jobs
before calling join_incumbent_stream(job_id), allowing _poll_incumbents() to
exit before the join. Preserve any stream error while ensuring the server
delete_job operation always runs, then propagate the stored error after deletion
if appropriate.
- Around line 549-559: Update Client.delete() to request job cancellation before
joining any active incumbent stream, preventing an indefinite wait; preserve
deletion after the stream exits. Add coverage for active-stream deletion,
callback-triggered cancellation, DeprecationWarning behavior in
start_incumbent_stream(), timed joins, and propagation of stored worker errors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: c5a91ae3-44b9-4959-a97a-19c399186b71
📒 Files selected for processing (7)
docs/cuopt/source/cuopt-grpc/api.rstdocs/cuopt/source/cuopt-grpc/examples.rstdocs/cuopt/source/cuopt-grpc/examples/incumbent_stream_demo.pydocs/cuopt/source/cuopt-grpc/python-async-client-examples.rstdocs/cuopt/source/cuopt-grpc/python-async-client.rstdocs/cuopt/source/cuopt-grpc/quick-start.rstpython/cuopt/cuopt/grpc/linear_programming/grpc_client.pyx
🚧 Files skipped from review as they are similar to previous changes (6)
- docs/cuopt/source/cuopt-grpc/python-async-client-examples.rst
- docs/cuopt/source/cuopt-grpc/python-async-client.rst
- docs/cuopt/source/cuopt-grpc/quick-start.rst
- docs/cuopt/source/cuopt-grpc/api.rst
- docs/cuopt/source/cuopt-grpc/examples/incumbent_stream_demo.py
- docs/cuopt/source/cuopt-grpc/examples.rst
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
1 participant
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.
Document remote execution vs explicit gRPC clients, add quick-start and streaming examples, and clarify integrated-client environment variables. Also clarify some existing docs on API and server behavior.