Add a synchronous grpc client #647
Conversation
| @@ -160,9 +227,9 @@ class {{ service.py_name }}Base(ServiceBase): | |||
| , {{ method.py_input_message_param }}: "{{ method.py_input_message_type }}" | |||
| {%- else -%} | |||
| {# Client streaming: need a request iterator instead #} | |||
| , {{ method.py_input_message_param }}_iterator: {{ output_file.typing_compiler.async_iterator(method.py_input_message_type) }} | |||
| , {{ method.py_input_message_param }}_iterator: "{{ output_file.typing_compiler.async_iterator(method.py_input_message_type) }}" | |||
There was a problem hiding this comment.
This and the next diff are in fact fixes for the existing stub generation. If wanted I could split it into a separate PR.
| we are going to mock the channel and simply test the client. | ||
|
|
||
| If we wanted to test without mocking we would need to use all the machinery here: | ||
| https://github.com/grpc/grpc/blob/master/src/python/grpcio_tests/tests/testing/_client_test.py |
There was a problem hiding this comment.
Or I could spawn an actual server. I would think that's the best testing, but it is not done so in the rest of the code (which uses the nice grpclib ChannelFor)
| DoThingResponse, | ||
| GetThingRequest, | ||
| GetThingResponse, | ||
| TestSyncStub as ThingServiceClient, |
There was a problem hiding this comment.
This is the new synchronous stub.
|
Hello, and thank you for the work! I agree that it would be a great idea to support this. However, I wouldn't merge the PR as it is now: indeed, I would say that adding an option to allow the user to choose between sync and async right at the beginning would be better. Plus, it should be possible to avoid duplicating all the template code. Then, this should be integrated in the github action so that these implementations are tested too... So it is not a small change. However, before starting working on that, I would like first to simplify the current implementation, since it will have an impact on the template file. |
|
Sure, it might be useful to provide ways to selectively enable one or the other. In particular I can imagine not wanting to pull in the grpclib dependency if I don't want the async version. Important to note: |
|
For testing, are you saying that a new test_*.py file is not automatically picked up and ran? Locally the |
@AdrienVannson If I understood this correctly, you propose to select which version of the client should be generated on the compilation stage? The following comment is based on that interpretation. In my opinion it is better to generate both sync and async versions. Common "pythonic" way is to just add an it would be more convenient knowing that both synchronous and asynchronous codebases could use the same betterproto generated package, guaranteeing consistency. Otherwise you would have to reinvent the wheel and distribute both versions. |
|
Hello! Due to the difficulty to have PR approved, the impossibility to become a maintainer of this repo as well as to have a better repo name, after discussing with the current maintainers, I did a fork of betterproto here: https://github.com/betterproto/ From now on, it is very likely that most of the development will be done there I've been improving various aspects of the project, I fixed a lot of bugs and improved the implementation to make it more easy to maintain. The project is now distributed in the For now, it is still a bit early, I still need time to fix a few remaining problems (already existing in betterproto) before making an stable release and making an announcement about it, but it should already be usable. I started adding your PR to the fork, see https://github.com/betterproto/python-betterproto2-compiler/pull/14/files . I used jinja blocks to avoid code duplication when possible. However, I haven't tested anything at all yet. Concerning the tests, I think that spawning a real server would indeed be a better way to test everything. For now, the new synchronous clients are just added to the generated files with a different name, but I would like to add an option to choose between no clients, only sync clients, only async clients or both clients, as well as an option concerning the naming. Also, I think it would be better to have a |
Summary
I find that often, we want a non async grpc client. This PR adds a
*SyncStubgenerated class using only the standard synchronous grpc library. It is pretty much exactly the same as the*Stubone except it is synchronous.I have not tested it extensively yet, but the code is pretty straightforward.
Let me know what you think and I can tweak/test more. Also you might not want something like this or would want it to be an option/extra instead of always generating it.
Checklist