feat: add async rest transport call methods #2140
Conversation
| {# TODO (https://github.com/googleapis/gapic-generator-python/issues/2128): Update `rest_version` to include the transport dependency version. #} | ||
| DEFAULT_CLIENT_INFO = gapic_v1.client_info.ClientInfo( | ||
| gapic_version=BASE_DEFAULT_CLIENT_INFO.gapic_version, | ||
| grpc_version=None, | ||
| rest_version=None, | ||
| ) | ||
|
|
||
| {# TODO: Add an `_interceptor` property once implemented #} |
There was a problem hiding this comment.
Please include the TODO bug to follow up
There was a problem hiding this comment.
I think it's fine to ignore this one since this comment is addressed and removed in a follow up PR: #2151.
| @property | ||
| def {{method.transport_safe_name|snake_case}}(self) -> Callable[ | ||
| [{{method.input.ident}}], | ||
| {{method.output.ident}}]: | ||
|
|
||
| return # type: ignore | ||
| return self._{{method.name}}(self._session, self._host) # type: ignore |
There was a problem hiding this comment.
Do we still need the # type: ignore ?
There was a problem hiding this comment.
Yes. My initial thought was that it's probably a mismatch since I wasn't returning a Callable. However, I later realised that we're doing the same in rest.py.
I haven't really looked into why we've been ignoring the type in rest.py but I'm following whatever we're doing there. Probably because the Callables for client streaming which raise NotImplmentedError do not return anything so mypy isn't happy about it? Regardless, I've filed an issue to investigate and remove this as a follow up.
ohmayr
force-pushed
the
wrap-async-methods-with-error-handling-logic
branch
from
79cc77d
to
23cb93e
Compare
| return hash("Async{{service.name}}RestTransport.{{method.name}}") | ||
|
|
||
| async def __call__(self, | ||
| request: {{method.input.ident}}, *, |
There was a problem hiding this comment.
What does the asterisk mean here?
There was a problem hiding this comment.
* enforces that arguments following request must be passed in as keyword arguments.
There was a problem hiding this comment.
Oh, it's a Python thing. Because it was on the same line, I assumed it was part of the type hint and glossed over the comma.
I suggest having the * in its own line, like the other parameters. It gets lost here.
| @property | ||
| def {{method.transport_safe_name|snake_case}}(self) -> Callable[ | ||
| [{{method.input.ident}}], | ||
| {{method.output.ident}}]: | ||
|
|
||
| return # type: ignore | ||
| return self._{{method.name}}(self._session, self._host) # type: ignore |
There was a problem hiding this comment.
So each of these properties is a callable that invokes the appropriate inner class. When do the self._wrapped_methods get called? (And which of your PRs will ensure that?)
There was a problem hiding this comment.
wrapping happens during transport initialization / construction via self._prep_wrapped_messages(client_info). If you look at the implementation for this helper, it creates a dictionary called _wrapped_methods which is something like {method : wrapped_method, ... }.
At the client level instead of calling method(), we call self._wrapped_methods[method](). However, we're not doing this for the following methods:
set_iam_policyget_iam_policytest_iam_permissions
FYI these are mixin methods. We don't pre-wrap mixin methods which is why this logic isn't added. We probably should do that or at least pass down the kind property to avoid gRPC code path in a rest call at the very least.
There was a problem hiding this comment.
I think this is important. I've filed #2156 to ensure that we do this as a follow up since this PR doesn't touch the client layer.
There was a problem hiding this comment.
At the client level instead of calling
method(), we callself._wrapped_methods[method]()
OK, this is what I was looking for.
Once we have self._wrapped_methods, do we ever call a method() directly? If we don't, then the most elegant thing would arguably be to overwrite the properties with their wrapped versions, and just called each (now-wrapped) method() directly. Does that make sense? If so, it would be a good code simplification to do later.
ohmayr
force-pushed
the
implement-async-rest-call-method-class
branch
from
6444826
to
c562932
Compare
ohmayr
force-pushed
the
implement-async-rest-call-method-class
branch
from
c562932
to
8fd829c
Compare
This PR adds asynchronous rest transport call methods which when called raise a
NotImplementedError. Follow up PRs will implement this method class.As of this PR, asynchronous rest transport does not support calling any type of methods.
This PR should be reviewed and merged after: #2139.