Conversation
andystaples
requested review from
berndverst,
Copilot and
halspang
and removed request for
Copilot
There was a problem hiding this comment.
Pull request overview
Adds an asyncio-friendly gRPC client to the Durable Task Python SDK (AsyncTaskHubGrpcClient) built on grpc.aio, and refactors shared request/interceptor/channel logic into internal helpers to keep sync and async clients aligned.
Changes:
- Introduces
AsyncTaskHubGrpcClientwith async equivalents of orchestration/entity operations. - Adds async gRPC channel creation (
get_async_grpc_channel) and async metadata interceptor support. - Adds async E2E coverage and updates test dependencies/configuration to run async pytest tests.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
durabletask/client.py |
Adds AsyncTaskHubGrpcClient and refactors shared logic usage for sync client. |
durabletask/internal/client_helpers.py |
New shared request/interceptor helper functions used by both clients. |
durabletask/internal/shared.py |
Adds get_async_grpc_channel and async interceptor typing. |
durabletask/internal/grpc_interceptor.py |
Adds DefaultAsyncClientInterceptorImpl and shared metadata application helper. |
tests/durabletask/test_orchestration_async_e2e.py |
New async E2E tests validating async client behavior with a sidecar. |
tests/durabletask/test_client.py |
Adds unit tests for async channel creation and async client construction. |
requirements.txt |
Adds pytest-asyncio for async testing. |
pyproject.toml |
Configures pytest-asyncio asyncio_mode = "auto". |
CHANGELOG.md |
Documents the new async client and related helpers. |
halspang
left a comment
halspang
left a comment
There was a problem hiding this comment.
Overall, I think this looks good! Just a few quick questions.
| ['method', 'timeout', 'metadata', 'credentials', 'wait_for_ready']), | ||
| grpc.aio.ClientCallDetails): | ||
| """This is an implementation of the aio ClientCallDetails interface needed for async interceptors. | ||
| This class takes six named values and inherits the ClientCallDetails from grpc.aio package. |
There was a problem hiding this comment.
It looks like we take five not six values.
| reuse_id_policy=reuse_id_policy, tags=tags, | ||
| version=version if version else self.default_version) | ||
|
|
||
| self._logger.info(f"Starting new '{req.name}' instance with ID = '{req.instanceId}'.") |
There was a problem hiding this comment.
In these logs that will be common with the sync client, I wonder if it would be worth differentiating them? Or will we be able to see that based on the logger? From the instantiation, I think it would just be client?
| ) | ||
| self._channel = channel | ||
| self._stub = stubs.TaskHubSidecarServiceStub(channel) | ||
| self._logger = shared.get_logger("client", log_handler, log_formatter) |
| ) | ||
| self._channel = channel | ||
| self._stub = stubs.TaskHubSidecarServiceStub(channel) | ||
| self._logger = shared.get_logger("client", log_handler, log_formatter) |
There was a problem hiding this comment.
Should client be async_client to distinguish between the clients?
| reuse_id_policy=reuse_id_policy, tags=tags, | ||
| version=version if version else self.default_version) | ||
|
|
||
| self._logger.info(f"Starting new '{req.name}' instance with ID = '{req.instanceId}'.") |
There was a problem hiding this comment.
If my above suggestion about client->async_client isn't valid, do we want to make the logs different? I think it'd be better if we can do it with a class style naming, but I think there could be some debug merit in having the two clients be different.
Adds an async version of TaskHubGrpcClient (AsyncTaskHubGrpcClient) that offers async-aware client methods using grpc.aio's async gRPC calls. Refactors code shared by both clients to /internal/client_helpers.py.
Compare to earlier open-source contrib #65 / dapr#17
Resolves #64