grpc-requests / grpc_requests

python grpc reflection client
https://pypi.org/project/grpc-requests/
Apache License 2.0
32 stars 5 forks source link

GRPC Method Descriptor Exposure #55

Closed artificial-aidan closed 8 months ago

artificial-aidan commented 9 months ago

I'm working on some features in which we are attaching custom options to grpc methods. This can be exposed with something like this:

for method_proto in svc_desc_proto.method:
            method_name = method_proto.name
            method_desc: MethodDescriptor = service_descriptor.methods_by_name[method_name]

            options = method_desc.GetOptions()

I had started putting together a PR that exposed the MethodOptions output of the GetOptions call, but then I realized that maybe it just makes sense to put the MethodDescriptor in the MethodMetadata instead of pulling out individual fields. This would allow for advanced usage if needed, and not require upkeep.

And I guess while at it we could put the ServiceDescriptor somewhere as well. And probably the FieldDescriptors.

But the MethodDescriptor had the most obvious place to store it.

class MethodMetaData(NamedTuple):
    input_type: Any
    output_type: Any
    method_type: MethodType
    handler: Any
    descriptor: MethodDescriptor

metadata[method_name] = MethodMetaData(
    method_type=method_type,
    input_type=input_type,
    output_type=output_type,
    handler=handler,
    descriptor=method_desc
)
ViridianForge commented 9 months ago

That does sound like it makes a good bit of sense - would perhaps allow making some of my own hamfisted attempts to open up access to fields information a bit more elegant.

If you wanted to put up a PR, that'd be much appreciated - for my own part, I'm going to be pretty swamped until after the holidays with my day to day.

ViridianForge commented 8 months ago

You're very correct @artificial-aidan - the method descriptor exposure is pretty straight forward.

I'm going to work on a bit of a middle ground on the service and file descriptors, and add them to the base client in an accessible manner, at least for this pass.

artificial-aidan commented 8 months ago

Sounds good to me. Do you still want me to put something together for method descriptors?

I think a nice middle ground of providing the basic functionality, while exposing the descriptor protos is a good place to be. If you are leaving the simplicity that this library has created (which is awesome) then maybe you just use the raw protos.

ViridianForge commented 8 months ago

I have got the method descriptors started, just need to write tests and get it up - should have some time to finish it up in the next day or two.

ViridianForge commented 8 months ago

https://github.com/wesky93/grpc_requests/pull/56 Sorry that took a bit - this ought to expose the MethodDescriptors a bit more cleanly.

When it comes to the Service and File Descriptors - I think that could be accomplished to some degree without major changes by making the currently private _get_file_descriptor_by_symbol method public and adding a convenience method around self._desc_pool.FindServiceByName(name) (and adding usage examples).

Though, rather than tacking that on to this PR - I figured I'd bounce it off of you to see if that idea would match your use case.

artificial-aidan commented 8 months ago

That looks great. And exposing the get_ methods for service and file descriptors would work fine. Allows for advanced usage if needed, and simple usage too.

ViridianForge commented 8 months ago

Thanks! Those enhancements will be tracked in Issue 57. Once those are merged to develop, we'll get version 0.1.14 up.