fix: update return annotation for streaming calls

[parthea]

Towards b/347351441

GrpcStream[Response] exposes the underlying grpc.Call for streaming calls, there is currently no way to use these grpc.Call methods without mypy errors, because of the restrictive return type annotation used. See googleapis/python-api-core#554 (comment) for context.

Adding the GrpcStream[Response] and Awaitable[AsyncIterable[Response]] response types for streaming calls will satisfy type checkers.

BEGIN_COMMIT_OVERRIDE
fix(deps): require google-api-core >= 2.17.1
END_COMMIT_OVERRIDE

google-api-core>=2.15.0 is the minimum version which supports types GrpcAsyncStream and GrpcStream
google-api-core>=2.17.1 is needed which fixes an issue with REST Streaming which can be seen in this build log

[daniel-sanche]

This should be redundant, since GrpcStream is just an alias for _StreamingResponseIterator, which is already Iterable. So we should just be able to use GrpcStream here

returning a union type can be ugly, since any attempt to use the returned instance will cause typing issues, since mypy won't know which of the types it is

[parthea]

For REST client, GrpcStream would be the response type. My initial thought is that we want a type which doesn't have Grpc in the name. Perhaps we could use a new StreamResponse instead of GrpcStream?

[parthea]

Given that this response type is also used for REST streaming, I'm not sure that we can avoid the union here. _StreamingResponseIterator has a dependency on gRPC. We would need to update _StreamingResponseIterator if we want to use it for REST

[parthea]

As an alternative to updating the response type, we could add a callback similar to what we have for unary requests in #2243 as a callback could solve the feature request in #2256 (comment)

[daniel-sanche]

That's a good point, I wasn't considering the rest side. We could make a StreamResponse as a mixin class, that we could add to both _StreamingResponseIterator and rest_streaming.ResponseIterator?

Yeah, using a callback in all cases could be a good way to resolve some of this complexity. It seems like we'd have some of the same typing issues there though, where we still need a way to represent results for either grpc or rest, right? I might have to dig deeper into the rest side

[daniel-sanche]

same comment here: it would be great if we could avoid the Union return

[parthea]

I don't see any way to avoid this as this is also the response type for REST. Let's decide on the next steps for #2256 (comment) and the same solution could be applied to async.

[daniel-sanche]

Did you find a solution for how to deal with api_core.retry.Retry in this?

The retry object wraps the rpc iterator, and it doesn't currently expose grc.Call methods, so anything involving retires won't return a GrpcStream. It's not exactly clear to me how we should expose metadata at the retry level (do we want to give metadata for each failed call, or only the final successful one? I think each failed request could be needed?)

I don't think this is relevant for bigtable, since we extend the retry logic. But it's probably needed in other contexts

[parthea]

We could consider adding this as future work, given that it's more than just changing the response type, and isn't required in the short term. In the first iteration, we will only be able to provide metadata when the request is successful. We can follow up with more enhancements. We may consider something similar to unary (prototype in #2243) where there is a callback.

[daniel-sanche]

Ok, sounds good

FWIW, I think it would be a good idea to still design a solution for failed attempts, so we don't paint ourselves into a corner here. (maybe we can use the retry.on_error callback for this?) But yeah, I'll assume we just want one set of metadata for now

[parthea]

Closing as obsolete. The proposed solution in b/347351441 is to use gRPC/REST interceptors. #2191 tracks improving documentation to clarify how gRPC/REST interceptors can be used to obtain metadata.