Skip to content

Grpcio support #201

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

Closed
wants to merge 4 commits into from
Closed

Conversation

roblabla
Copy link
Contributor

@roblabla roblabla commented Jan 21, 2021

Fixes #44

This PR does two thing: it introduces a grpc-kind flag that allows specifying which grpc backend we want, if any (none, grpcio, grpclib). It defaults to grpclib for backward compatibility.

Secondly, it adds a new gRPC codegen backend for grpcio. This codegen backend generates base classes that can be registered against an aio grpcio server.

It's fairly early, I literally just whipped this up in a couple hours, but wanted some feedback on the design. I'm not very familiar with python! The base class generated by the new backend looks like this:

class ServiceName(better_proto.grpcio.ServiceBase):
    async def method(
        self, request: ty, context: grpc.aio.ServicerContext
    ) -> ty:
    async def stream_stream_method(
        self, request: AsyncIterator[ty], context: grpc.aio.ServicerContext
    ) -> AsyncGenerator[ty, None]:

    __proto_path__ = ""

    @property
    def __rpc_methods__(self):

There are three significant differences compared to the grpclib backend:

  • The methods take the request object as a single argument. I find this easier to deal with - in my app, I have some fairly large objects going over the network, and getting a bunch of arguments can be bothersome.
  • The returned type of server-streaming requests is AsyncGenerator instead of AsyncIterator. This comes down to a difference between grpcio and grpclib: grpcio expects an AsyncGenerator from server-streaming requests, whereas grpclib expects an AsyncIterator. It might be possible to create a wrapper to unify both backends, but I question how wise this would be. I personally much prefer grpcio's way of returning an AsyncGenerator - yielding within the request is much more natural than returning an iterator, especially for stream_stream requests!
  • I added a method called register_to_server, which registers a given implementation against the grpcio server. Grpcio's official stubs instead have an add_{{typename}}_to_server standalone function. I figured making it a method would be cleaner, as it avoids the necessary dance to turn typename lowercase. We now have a betterproto.grpc.grpcio_server.register_servicers standalone function to register multiple servicers to a grpcio server.

TODO:

Copy link
Collaborator

@nat-n nat-n left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this. It looks great! I'd been meaning to do it myself but not gotten around to it...

Yes having some unit tests would be a good idea. I'd suggest looking at how the tests are done for the grpclib servicer, hopefully it's possible to avoid spinning up a server over TCP, though I'm not sure if is.

Having something like register_to_server is a good idea, though I wonder if an instance method is the best way to do it. How about instead defining a __rpc_handlers__ field on the generated class, and then providing a betterproto.register_servicers(service, "grpc.aio.Server", *servicer: Type) utility method to finish the job?

related #20 #56 #44 #19

class {{ service.py_name }}Base:
{% if service.comment %}
{{ service.comment }}

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this extra line meant to be here? I guess black will remove is anyway.

@roblabla
Copy link
Contributor Author

Having something like register_to_server is a good idea, though I wonder if an instance method is the best way to do it. How about instead defining a rpc_handlers field on the generated class, and then providing a betterproto.register_servicers(service, "grpc.aio.Server", *servicer: Type) utility method to finish the job?

The big downside of this is that it means the betterproto lib will have to depend on grpcio - and while depending on grpclib isn't a big problem since it's a pure python dependency, grpcio is a Cython dependency that links to C code, so it can be a very annoying dependency to have.

Is this a problem? And if it is, how do we work around it? Does Python have optional dependencies/modules?

@nat-n
Copy link
Collaborator

nat-n commented Jan 25, 2021

The big downside of this is that it means the betterproto lib will have to depend on grpcio - and while depending on grpclib isn't a big problem since it's a pure python dependency, grpcio is a Cython dependency that links to C code, so it can be a very annoying dependency to have.

Is this a problem? And if it is, how do we work around it? Does Python have optional dependencies/modules?

grpcio can be made an optional dependency of betterproto. This can be done using the same mechanism by which the plugin dependencies are only installed if protobuf[compiler] is specified.

Fortunately the register_servicers method I suggest can be implemented without a hard dependency on grpcio if it does a lazy import within the function. I'm thinking something like:

from typing import TYPE_CHECKING

if TYPE_CHECKING:
    import grpc
       
def register_servicers(server, "grpc.aio.Server", *servicers: Type):
    from grpc import method_handlers_generic_handler

    server.add_generic_rpc_handlers(
        tuple(
            method_handlers_generic_handler(
                servicer.__proto_path__, 
                servicer.__rpc_handlers__
            )
            for servicer in servicers
        )
    )

It's possible that the import could fail with a runtime error, but quite unlikely in practice, since the intended use case implies the user has already imported it elsewhere.

The type checker specific import of grpc is fine because it'll only run in dev, and protobuf is always available as a dev dependency.

This of course requires __proto_path__ to also be set on the generated class.

We now have a grpcio module in betterproto that defines the ServicerBase
Abstract Base Class and a register_servicers function.
Register_servicers takes multiple ServicerBase instances, and registers
them to the grpc AIO server.

The generated servicers now inherit from ServicerBase.
@nat-n nat-n mentioned this pull request Jan 25, 2021
@nat-n nat-n added the enhancement New feature or request label Apr 6, 2021
@isac322
Copy link

isac322 commented Apr 11, 2021

I am excited for grpcio supporting. Is there anything I can do to help this get merged (faster) in master ?

@nat-n
Copy link
Collaborator

nat-n commented Apr 11, 2021

I am excited for grpcio supporting. Is there anything I can do to help this get merged (faster) in master ?

@isac322 The two main things this PR is lacking are test coverage for the new functionality, and developer time to push it over the line. If you could contribute some test cases then that would certainly help. Having a look at the testing guide and related tests for the grpclib services would be a good place to start.

@roblabla
Copy link
Contributor Author

Yeah, sorry, I've been meaning to get back to this, but life's been keeping me busy. I'm not going to be able to get back to this for at least a week or two. Hopefully after that, I should have some time to push this PR further towards completion.

Thanks for the link to the testing guide, that will definitely be useful!

@jalaziz
Copy link

jalaziz commented Jan 30, 2022

We've recently come across a need for this as grpclib is missing certain important features (e.g. grpc. max_connection_age_ms) and we'd really like to avoid dealing with the awful developer experience of standard grpc libraries.

@roblabla any chance you're still working on this? I'd be happy to try and help get this over the line if you're unlikely to work on this anytime soon.

@roblabla
Copy link
Contributor Author

Unfortunately, I'm not working on this anymore, and I don't have the bandwidth to bring this to the finish line.

@jalaziz jalaziz mentioned this pull request Jan 30, 2022
@jalaziz
Copy link

jalaziz commented Jan 30, 2022

I've started a new rebased PR in #328.

@Gobot1234 Gobot1234 closed this Jan 31, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Generation without gRPC
5 participants