Supporting client-side gRPC cancellation #393

tanmayv25 · 2023-09-08T00:01:47Z

Enhances python client API to issue gRPC cancellation.

C++ client side grpc cancellation will be added at a later point.

Tests are added here: triton-inference-server/server#6278

src/python/library/tritonclient/grpc/_client.py

src/python/library/tritonclient/grpc/_infer_stream.py

rmccorm4 · 2023-09-13T00:52:48Z

src/python/library/tritonclient/grpc/_client.py

@@ -1502,6 +1522,7 @@ def wrapped_callback(call_future):
                if request_id != "":
                    verbose_message = verbose_message + " '{}'".format(request_id)
                print(verbose_message)
+            return self._call_future


I think @nnshah1 had some concerns about returning this grpc future directly, not wanting users to call future.result() on it, possibly conflicting with our own callback that blocks on the result. If we want to return a cancel-only future, we can wrap it and hide the underlying future/.result() method.

However, I think it's fine to return it directly and document they shouldn't call .result() on it for now.

I'll let Neelay comment on this.

I would like us to define a "WrappedFuture" class, where we override the result() method to return an InferResult() (similar in that sense to the wrapped_callback). Or hide the result as @rmccorm4 mentioned.

I think returning a future but having the result() be different than with the callback will be non-intuitive. - also don't like the idea of documenting 'not to use result()' or 'result()' is different that InferResult() - all seem more confusing than to wrap it.

Let me know if it's feasible.

Updated here: cdf8aea

nnshah1 · 2023-09-13T02:09:02Z

src/python/library/tritonclient/grpc/_infer_stream.py

        self._active = True
+        self._response_iterator = None

    def __del__(self):
        self.close()


I'd advocate for del to be non blocking and use self.close(cancel_requests=True) - unless I'm missing something / good reason to keep it blocking.

src/python/library/tritonclient/grpc/_client.py

Tabrizian · 2023-09-13T16:44:36Z

src/python/library/tritonclient/grpc/_client.py

@@ -1451,6 +1452,23 @@ def async_infer(
            Optional custom parameters to be included in the inference
            request.

+        Returns


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.

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.

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.

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.

Tabrizian · 2023-09-13T17:02:10Z

src/python/library/tritonclient/grpc/aio/__init__.py

+        get_call_obj : bool
+            If set True, then this function will yield
+            grpc.aio.call object first bfore the
+            InferResult.
+            This object can be used to issue request
+            cancellation if required. This can be attained
+            by following:
+            -------
+            generator = client.infer(..., get_call_obj=True)
+            grpc_call = await anext(generator)
+            grpc_call.cancel()
+            -------


Would it be possible to wrap the call object in another class (perhaps Request) that once awaited would provide and has a cancel method?

There are some new primitives that got added for async_generators in python 3.10. I am not very familiar with these right out of bat first trial was not successful.
Reverting cancellation support in asyncio for now. Will be addressing the issue later.

Co-authored-by: Iman Tabrizian <iman.tabrizian@gmail.com>

* Supporting client-side gRPC cancellation (#393) * Supporting client-side gRPC cancellation * Address review comments * Wrap future to expose only cancel * Update src/python/library/tritonclient/grpc/_client.py Co-authored-by: Iman Tabrizian <iman.tabrizian@gmail.com> * Reverting changes in asyncio library * Format * Make class variable real private --------- Co-authored-by: Iman Tabrizian <iman.tabrizian@gmail.com> * Fix L0_grpc (#400) * Improve the check --------- Co-authored-by: Iman Tabrizian <iman.tabrizian@gmail.com>

tanmayv25 requested review from Tabrizian, nnshah1 and jbkyang-nvi September 8, 2023 00:01

rmccorm4 reviewed Sep 8, 2023

View reviewed changes

src/python/library/tritonclient/grpc/_client.py Outdated Show resolved Hide resolved

rmccorm4 reviewed Sep 8, 2023

View reviewed changes

src/python/library/tritonclient/grpc/_infer_stream.py Show resolved Hide resolved

tanmayv25 requested a review from rmccorm4 September 13, 2023 00:45

rmccorm4 reviewed Sep 13, 2023

View reviewed changes

nnshah1 reviewed Sep 13, 2023

View reviewed changes

Tabrizian reviewed Sep 13, 2023

View reviewed changes

tanmayv25 force-pushed the tanmayv-grpc-client branch from b947d73 to 0308aa4 Compare September 13, 2023 23:40

tanmayv25 and others added 5 commits September 18, 2023 16:00

Supporting client-side gRPC cancellation

6c72805

Address review comments

8047f9e

Wrap future to expose only cancel

dcc9e27

Update src/python/library/tritonclient/grpc/_client.py

4b3774a

Co-authored-by: Iman Tabrizian <iman.tabrizian@gmail.com>

Reverting changes in asyncio library

e568b72

tanmayv25 force-pushed the tanmayv-grpc-client branch from 70b590d to e568b72 Compare September 18, 2023 23:00

tanmayv25 added 2 commits September 18, 2023 16:20

Format

6a18b6c

Make class variable real private

fbd5035

kthui approved these changes Sep 19, 2023

View reviewed changes

tanmayv25 merged commit a147986 into request-cancellation Sep 19, 2023

tanmayv25 deleted the tanmayv-grpc-client branch September 19, 2023 01:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Supporting client-side gRPC cancellation #393

Supporting client-side gRPC cancellation #393

tanmayv25 commented Sep 8, 2023 •

edited

Loading

rmccorm4 Sep 13, 2023 •

edited

Loading

nnshah1 Sep 13, 2023 •

edited

Loading

tanmayv25 Sep 13, 2023 •

edited

Loading

nnshah1 Sep 13, 2023

Tabrizian Sep 13, 2023

tanmayv25 Sep 13, 2023 •

edited

Loading

Tabrizian Sep 13, 2023

tanmayv25 Sep 13, 2023

Tabrizian Sep 13, 2023

tanmayv25 Sep 18, 2023

Supporting client-side gRPC cancellation #393

Supporting client-side gRPC cancellation #393

Conversation

tanmayv25 commented Sep 8, 2023 • edited Loading

rmccorm4 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

nnshah1 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

tanmayv25 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

nnshah1 Sep 13, 2023

Choose a reason for hiding this comment

Tabrizian Sep 13, 2023

Choose a reason for hiding this comment

tanmayv25 Sep 13, 2023 • edited Loading

Choose a reason for hiding this comment

Tabrizian Sep 13, 2023

Choose a reason for hiding this comment

tanmayv25 Sep 13, 2023

Choose a reason for hiding this comment

Tabrizian Sep 13, 2023

Choose a reason for hiding this comment

tanmayv25 Sep 18, 2023

Choose a reason for hiding this comment

tanmayv25 commented Sep 8, 2023 •

edited

Loading

rmccorm4 Sep 13, 2023 •

edited

Loading

nnshah1 Sep 13, 2023 •

edited

Loading

tanmayv25 Sep 13, 2023 •

edited

Loading

tanmayv25 Sep 13, 2023 •

edited

Loading