Skip to content
This repository was archived by the owner on Feb 23, 2026. It is now read-only.

feat: add type annotations to wrapped grpc calls - #554

Merged
daniel-sanche merged 16 commits into
mainfrom
type-annotate-grpc-wrappers
Nov 17, 2023
Merged

feat: add type annotations to wrapped grpc calls#554
daniel-sanche merged 16 commits into
mainfrom
type-annotate-grpc-wrappers

Conversation

@daniel-sanche

@daniel-sanchedaniel-sanche commented Nov 17, 2023

Copy link
Copy Markdown
Contributor

Our gapic libraries declare that they return Iterable[SomeProto] or Awaitable[SomeProto] or Awaitable[AsyncIterable[SomeProto], but that doesn't tell the whole story. The objects returned are actuallygrpc.Call subclasses, which expose the ability to retrieve metadata from the rpc, or call cancel, or other trigger useful functionality.

Unfortunately, there is currently no way to use these grpc.Call methods without mypy errors, because of the restrictive return type annotation used

This PR is the first step in addressing this, by giving us a more powerful return type in api-core.

On the sync side, I made _StreamingResponseIterator into a Generic container, and gave it a better public facing name. This way, we we can return GrpcStream[SomeProto] instead of Iterable[SomeProto], with all grpc.Call methods accessible

On the Async side, I made every _WrappedXYResponse class into a Generic container, and declared new GrpcAsyncStream[SomeProto] and AwaitableGrpcCall[SomeProto] types that can be used in place of AsyncIterable[SomeProto] and Awaitable[SomeProto] respectively.

For more context on the motivating problem, see googleapis/gapic-generator-python#1856. This PR lays some of the groundwork for resolving that issue in the future. But the type improvements here should also stand alone

@daniel-sanche
daniel-sanche requested review from a teamNovember 17, 2023 01:28
@product-auto-labelproduct-auto-labelBot added the size: s Pull request size is small. label Nov 17, 2023
@product-auto-labelproduct-auto-labelBot added size: m Pull request size is medium. and removed size: s Pull request size is small. labels Nov 17, 2023
@parthea

Copy link
Copy Markdown
Contributor

@daniel-sanche Please could you address the lint failure?

@daniel-sanche

Copy link
Copy Markdown
ContributorAuthor

@parthea done

# public type alias denoting the return type of streaming gapic calls
GrpcAsyncStream = _WrappedStreamResponseMixin[S]
# public type alias denoting the return type of unary gapic calls
AwaitableGrpcCall = _WrappedUnaryResponseMixin[U]

@daniel-sanchedaniel-sancheNov 17, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Let me know if you have other naming suggestions for these (AsyncGrpcCall? GrpcAsyncIterable?)

I liked Awaitable because it's clear how to interact with it, and Stream instead of Iterable because it can do more than just iterate. But names are hard and I'm open to alternatives

Comment threadtests/asyncio/test_grpc_helpers_async.py Outdated
Comment threadgoogle/api_core/grpc_helpers.py Outdated


class _StreamingResponseIterator(grpc.Call):
class GrpcStream(grpc.Call, Generic[S]):

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Do you think this could be a breaking change? Could we keep the name as _StreamingResponseIterator ? There are many hits for _StreamingResponseIterator in Google search. I'm worried that changing _StreamingResponseIterator could cause issues downstream in user code as it is a response we provide.

@daniel-sanchedaniel-sancheNov 17, 2023

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Makes sense, I'll create a type alias instead, like I did on the async side

I thought it would be safe to change since it's a private class, but better to be on the safe side

@daniel-sanche
daniel-sanche merged commit fc12b40 into mainNov 17, 2023
@daniel-sanche
daniel-sanche deleted the type-annotate-grpc-wrappers branch November 17, 2023 18:29
@release-pleaserelease-pleaseBot mentioned this pull request Nov 17, 2023
Sign up for freeto subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

size: mPull request size is medium.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants

@daniel-sanche@parthea