danielgtaylor
Add a synchronous grpc client
Summary
I find that often, we want a non async grpc client. This PR adds a *SyncStub generated class using only the standard synchronous grpc library. It is pretty much exactly the same as the *Stub one 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
- [X] If code changes were made then they have been tested.
- [ ] I have updated the documentation to reflect the changes.
- [ ] This PR fixes an issue.
- [X] This PR adds something new (e.g. new method or parameters).
- [X] This change has an associated test.
- [ ] This PR is a breaking change (e.g. methods or parameters removed/renamed)
- [ ] This PR is not a code change (e.g. documentation, README, ...)
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: In my current workflow, I need both the asynchronous and synchronous generated code. This means that they can't share the same class name.
For testing, are you saying that a new test_*.py file is not automatically picked up and ran? Locally the poe test command did work as expected. On that note, I've also improved the README since the section on testing appears to be out-dated.
I would say that adding an option to allow the user to choose between sync and async right at the beginning would be better
@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 a prefix before the method name, e.g. Python has dunder methods __aiter__, __anext__, __aenter__, LLama-Index has a lot of methods like stream_chat/astream_chat, DJango has Model.objects.create/Model.objects.acreate.
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 betterproto2 and betterproto2_compiler packages, and I started writing some documentation for it ( https://betterproto.github.io/python-betterproto2/ )
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 SyncServiceStub base class for these clients, and to keep supporting metadata, timeout etc.