-
Notifications
You must be signed in to change notification settings - Fork 1
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
[WIP] implement metadata for asynchronous unaryunary callable #10
[WIP] implement metadata for asynchronous unaryunary callable #10
Conversation
Implement the minimal stuff for making a unary call with the new experimental gRPC Python implementation for Asyncio, called Aio. What has been added: - Minimal iomgr code for performing the required network and timer calls. - Minimal Cython code implementing the channel, call and the callback context. - Minimal Python code that mimics the synchronous implementation but designed to be asynchronous. Not solved yet: - Tests have to be executed using the `GRPC_ENABLE_FORK_SUPPORT=0` environment variable for skipping the fork handles installed by the core library. Co-authored-by: Manuel Miranda <manuel.miranda@skyscanner.net> Co-authored-by: Mariano Anaya <mariano.anaya@skyscanner.net> Co-authored-by: Zhanghui Mao <zhanghui.mao@skyscanner.net>
3f6df55
to
799ced6
Compare
615b846
to
a44e6d7
Compare
) | ||
|
||
|
||
class TestMetadata(test_base.AioTestBase): |
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 put the test in the same file where we have the other tests, in the end we are testing the client with 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.
+1 just as a new test_unary_unary_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.
+1
DETAILS = 'test details' | ||
|
||
|
||
def metadata_transmitted(original_metadata, transmitted_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 don't understand this test, what is the purpose of it? It is not importing anything from gRPC
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 function used by TestMetadata
, this function help it check if the metadata of response is equal to original metadata.
|
||
return state.response | ||
|
||
async def with_state(self, |
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.
what is this method used for? Why did you add it here?
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 should use the name of sync version (with_call
) .
When we call this function instead of __call__
, we can return more detail information (like metadata), because __call__
only return response.
I copied it from sync version. This is with_call
in sync version: https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_channel.py#L606
And sync version use _end_unary_response_blocking
to check which parameters should be returned:
https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_channel.py#L615
https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_channel.py#L498-L502
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.
All of the code that is behind with_call
will be in some how refactorized for accommodating this proposal grpc#20001.
It's true that what you have implemented can be reused later for implementing that proposal, so IMO I would keep this method - please rename it - but I would put a disclaimer like.
# TODO(https://github.com/grpc/grpc/issues/20001) This method will be removed and its logic will be used for achieving the new unified version of __call__, future and with_call
``
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.
OK, I see 👍
self.trailing_metadata = trailing_metadata | ||
|
||
|
||
def _handle_call_result(operations, state, response_deserializer): |
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.
where did you get this logic from? Is it from the sync version? Can you link it? :) Gives the impression we are implementing more things than needed (for now)?
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 got it from here: https://github.com/grpc/grpc/blob/master/src/python/grpcio/grpc/_channel.py#L119
I think we may use this function construct more information in the future.
'/grpc.testing.TestService/UnaryCall', | ||
request_serializer=messages_pb2.SimpleRequest.SerializeToString, | ||
response_deserializer=messages_pb2.SimpleResponse.FromString) | ||
state = await hi.with_state(messages_pb2.SimpleRequest(), |
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.
why? Why not call hi
directly as we do in the other tests?
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.
If we want to get more information(like metadata) from __call__
, I need to change the return value of it. So I add a new method to return more details.
) | ||
|
||
|
||
class TestMetadata(test_base.AioTestBase): |
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.
+1 just as a new test_unary_unary_metadata
|
||
return state.response | ||
|
||
async def with_state(self, |
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.
All of the code that is behind with_call
will be in some how refactorized for accommodating this proposal grpc#20001.
It's true that what you have implemented can be reused later for implementing that proposal, so IMO I would keep this method - please rename it - but I would put a disclaimer like.
# TODO(https://github.com/grpc/grpc/issues/20001) This method will be removed and its logic will be used for achieving the new unified version of __call__, future and with_call
``
Implement
metadata
forAio UnaryUnary Call
.Fix grpc#20144
TODO