Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Supporting client-side gRPC cancellation #393

Merged
merged 7 commits into from
Sep 19, 2023
Merged
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
Wrap future to expose only cancel
  • Loading branch information
tanmayv25 committed Sep 18, 2023
commit dcc9e2730ec906f45f14d545b1e3a6daa653892a
26 changes: 21 additions & 5 deletions src/python/library/tritonclient/grpc/_client.py
Original file line number Diff line number Diff line change
@@ -98,6 +98,24 @@ def __init__(
self.http2_max_pings_without_data = http2_max_pings_without_data


class CallContext():
""" This is a wrapper over grpc future call which can be used to
issue cancellation on an ongoing RPC call.

Parameters
----------
grpc_future : gRPC.Future
The future tracking gRPC call.
"""
def __init__(self, grpc_future):
self._grpc_future = grpc_future

def cancel(self):
"""Issues cancellation on the underlying request.
"""
self._grpc_future.cancel()


class InferenceServerClient(InferenceServerClientBase):
"""An InferenceServerClient object is used to perform any kind of
communication with the InferenceServer using gRPC protocol. Most
@@ -1454,7 +1472,7 @@ def async_infer(

Returns
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think returning a future should replace the callback method. AFAIK, futures (or async/await) were added to replace the need for callbacks and we need to support one of them.

We could perhaps add another argument to async_infer named return_future with the default value of False. If it is set to True, this method would return a future and would never call the callback.

Copy link
Contributor Author

@tanmayv25 tanmayv25 Sep 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This makes sense. IMO we should also support cancelling request for callback based cancellation policy.
However, that is not possible without sending back the future. Maybe we can enforce that for accessing request cancellation feature, the user can not use callback based mode.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can enforce that for accessing request cancellation feature, the user can not use callback based mode.

Exactly, that's what I was thinking.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of returning Future with an additional method for retrieving the message. But I think it goes beyond the scope and can be addressed separately.
Additionally, we should be thinking of moving to aio clients at some point. Maintaining two library version is becoming heavy.

-------
grpc.future
CallContext
A representation of a computation in another control flow.
Computations represented by a Future may be yet to be begun,
may be ongoing, or may have already completed.
tanmayv25 marked this conversation as resolved.
Show resolved Hide resolved
@@ -1465,8 +1483,6 @@ def async_infer(
future = async_infer(...)
ret = future.cancel()
----------
See here for more details of future object:
https://grpc.github.io/grpc/python/grpc.html#grpc.Future


Raises
@@ -1516,13 +1532,13 @@ def wrapped_callback(call_future):
timeout=client_timeout,
compression=_grpc_compression_type(compression_algorithm),
)
self._call_future.add_done_callback(wrapped_callback)
if self._verbose:
verbose_message = "Sent request"
if request_id != "":
verbose_message = verbose_message + " '{}'".format(request_id)
print(verbose_message)
return self._call_future
self._call_future.add_done_callback(wrapped_callback)
return CallContext(self._call_future)
except grpc.RpcError as rpc_error:
raise_error_grpc(rpc_error)

2 changes: 1 addition & 1 deletion src/python/library/tritonclient/grpc/_infer_stream.py
Original file line number Diff line number Diff line change
@@ -62,7 +62,7 @@ def __init__(self, callback, verbose):
self._response_iterator = None

def __del__(self):
self.close()
self.close(cancel_requests=True)

def close(self, cancel_requests=False):
"""Gracefully close underlying gRPC streams.