-
Notifications
You must be signed in to change notification settings - Fork 10.4k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Implement metadata in __call__ for aio unaryunary call #21336
Implement metadata in __call__ for aio unaryunary call #21336
Conversation
|
||
class _TestServiceServicer(test_pb2_grpc.TestServiceServicer): | ||
|
||
async def UnaryCall(self, request, context): | ||
# _maybe_echo_metadata(context) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can not get metadata from here until server-side implement it, the context
is a placeholder[1] and server-side just send fake metadata[2] now, so I comment this line.
[1] https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi#L78
[2] https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_cython/_cygrpc/aio/server.pyx.pxi#L88-L94
import grpc | ||
|
||
_EMPTY_FLAGS = 0 | ||
_EMPTY_MASK = 0 | ||
_EMPTY_METADATA = None | ||
|
||
UnaryUnaryOpsResult = namedtuple('UnaryUnaryOpsResult', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 you can make it in a more pythonic way [1]
@@ -182,11 +183,23 @@ def done(self) -> bool: | |||
""" | |||
return self._state is not _RpcState.ONGOING | |||
|
|||
async def initial_metadata(self): | |||
raise NotImplementedError() | |||
async def initial_metadata(self) -> Sequence[Tuple[Text, AnyStr]]: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Opening the can of worms, would make sense on changing this Sequence[Tuple[Text, AnyStr]
for a Dict[Text, AnyStr]
? Having the feeling that it would make life easier for the developers. Otherwise, they would be forced to wrap the sequence for implementing a dict view on their own.
Thoughts @ZHmao @gnossen @lidi? If so, let's do that this in this PR or another one?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Returning a plain old Dict
would lose the order of the metadata entries, which we currently preserve with Sequence[Tuple[Text, AnyStr]]
. Perhaps OrderedDict
instead?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a pity that we can not rely on default dict implementation that
preserves insertion order since Python 3.6, because we are giving support
to python 3.5+.
In any case, agree on using ordered dict.
response_deserializer=messages_pb2.SimpleResponse.FromString) | ||
call = hi(messages_pb2.SimpleRequest(), metadata=_INVOCATION_METADATA) | ||
self.assertIsNotNone(await call.initial_metadata()) | ||
self.assertIsNotNone(await call.trailing_metadata()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Correct me if I'm wrong, but we are basically testing the default value here? because the _INVOCATION_METADATA
is not finally wired in the server-side since _maybe_echo_metadata
does not work, amb I wrong?
If so, maybe for having a reliable coverage we would need to implement something that allows the code to test that the metadata is received. For that - one solution that does not require the context- is create another gRPC endpoint, for example, MetadataEcho
which would go through to all of hte metadata and will make an echo of the metadata found as metadata.
Besides that, and in the same way as being done in the cal_test.py
for the details, I would implement a couple of tests for testing that initial_metadata
and trailing_metadata
are awaitable, in these tests we could double-check that the value returned is the default value when there is no metada returned by the user.
Seems that YAPF is failing https://source.cloud.google.com/results/invocations/82d3f294-1878-4ddb-b22f-be6e94cab512/targets |
message=receive_message_operation.message(), | ||
code=receive_status_on_client_operation.code(), | ||
details=receive_status_on_client_operation.details(), | ||
trailing_metadata=receive_status_on_client_operation.trailing_metadata()) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest think twice about the UnaryUnaryOpsResult
design. Under streaming cases, the information is not going to arrive in one batch, and allows it to return just once. Otherwise, there will be two totally different designs of returning information from C-Core to application.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the PR for implementing the interceptors Im gonna segregate these
attributed as attributes belonging to the aio call object. Just for
unblocking the work here, if you agree, I would consider this namedtuple as
a temporary thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Personally, if this PR is blocking your work in interceptors, I would say they are probably better check in together? The amount of code is not huge, should be fine merging into what you have in your refactor version.
This temporary design here is on critical data path. My intention is that if you are sensing that this needs to be refactored soon, why not perform the refactor all together?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not really blocking me but if it gets merged first than mine the namedtuple
would be removed.
But true that the number of changes is not so big, If everybody agrees I can ask @ZHmao for opening the PR against my branch and deliver later a PR which will address both things, hte interceptor and support for metadata.
Do you kinda agree @ZHmao? If so, could you open the PR against this branch [1] and close this?
[1] https://github.com/Skyscanner/grpc/tree/client_unaryunary_interceptors
@pfreixes We are still planning to expose async generator in our surface. So, we are supporting 3.6+. For 3.5 users, it's our best effort to not actively break them. Generally, if you find 3.6 features are helpful here (reduce complexity, improve efficiency), we should use the features. |
Will open it against this branch: https://github.com/Skyscanner/grpc/tree/client_unaryunary_interceptors |
Implement
metadata
forAio UnaryUnary Call
.Fix #20144