Tweaks to generated code for ocaml-grpc

[mbarbin]

Thank you for ocaml-protoc.3.0!

I'm currently working on modifications to ocaml-grpc to enhance the typing of
gRPC services and facilitate the selection of serialization backends.

I've added a proof of concept for ocaml-protoc.3.0 support in this PR. While it's functional, I believe the interface exposed by the generated code could
benefit from minor adjustments.

For instance, consider the services interface generated for the greeter-protoc example taken from the PR:

module Greeter : sig
  open Pbrt_services
  open Pbrt_services.Value_mode
  
  module Client : sig
    val sayHello : (hello_request, unary, hello_reply, unary) Client.rpc
  end
  
  module Server : sig
    val make : 
      sayHello:((hello_request, unary, hello_reply, unary) Server.rpc -> 'handler) ->
      unit -> 'handler Pbrt_services.Server.t
  end
end

To construct a Grpc.Rpc.t specification, we need:

1. Pbrt_service.Client.rpc (for request encoding and response decoding)
2. Pbrt_service.Server.rpc (for request decoding and response encoding)
3. Pbrt_service.Server.t (for accessing the packages, service, and rpc names)

As shown in the generated interface above, the first value (Client.sayHello)
is readily available. However, accessing the second and third values requires
applying a dummy closure to Server.make, which somewhat misuses the current
interface exposed by the generated code.

A possibly more suitable interface for the specification-oriented design I'm
developing would repackage the value as follows:

module SayHello : sig
  val client : (hello_request, unary, hello_reply, unary) Client.rpc
  val server : (hello_request, unary, hello_reply, unary) Server.rpc
end

This repackaged SayHello could then be used as a single first-class module
argument to a function Grpc_protoc.make_rpc that would build the specification
accordingly.

I'd appreciate your thoughts on this. To note that the work I'm doing on
ocaml-grpc is very exploratory, so this is really just an early discussion at
this stage.

Thanks!

[c-cube]

Hi Mathieu, thanks for starting the discussion!

On Tue, 12 Dec 2023, Mathieu Barbin wrote: I've added a proof of concept for `ocaml-protoc.3.0` support in [this PR](dialohq/ocaml-grpc#48). While it's functional, I believe the interface exposed by the generated code could benefit from minor adjustments. To construct a `Grpc.Rpc.t` specification, we need: 1. Pbrt_service.Client.rpc (for request encoding and response decoding) 2. Pbrt_service.Server.rpc (for request decoding and response encoding) 3. Pbrt_service.Server.t (for accessing the packages, service, and rpc names) As shown in the generated interface above, the first value (`Client.sayHello`) is readily available. However, accessing the second and third values requires applying a dummy closure to `Server.make`, which somewhat misuses the current interface exposed by the generated code. A possibly more suitable interface for the specification-oriented design I'm developing would repackage the value as follows: ```ocaml module SayHello : sig val client : (hello_request, unary, hello_reply, unary) Client.rpc val server : (hello_request, unary, hello_reply, unary) Server.rpc val service : unit Pbrt_services.Server.t end ``` This repackaged `SayHello` could then be used as a single first-class module argument to a function `Grpc_protoc.make_rpc` that would build the specification accordingly. I'd appreciate your thoughts on this. To note that the work I'm doing on `ocaml-grpc` is very exploratory, so this is really just an early discussion at this stage.

So the way ocaml-protoc services work, currently, is making the assumption that client side, and server side, are independent use cases. I'd never package them together into a `Grpc.t`; instead: - on the client side, the library should allow the user to call individual methods directly (each stub is independent). That's more or less the easy part. - on the server side, the `Server.make` is there to bundle together all the implementation side of a given service; then, a list of bundled services can be used together as part of a network server (e.g. adding all of them, in bulk, to a router). So I never use server stubs independently, they're always bundled as full services alongside the user provided implementation. The `'handler` parameter type is there to allow server implementations to use whatever concurrency feature they wish — could be `req -> res Lwt.t`, `req -> (res, error) result Eio.promise`, etc. In this context I'm not sure how it'd work to make the bundles you need. The individual server stubs are in the generated code. They're not really intended to be used directly, but I guess they could be? They look like this: ```ocaml let _rpc_add : (add_req,unary,i32,unary) Server.rpc = (Server.mk_rpc ~name:"add" ~req_mode:Server.Unary ~res_mode:Server.Unary ~encode_json_res:encode_json_i32 ~encode_pb_res:encode_pb_i32 ~decode_json_req:decode_json_add_req ~decode_pb_req:decode_pb_add_req () : _ Server.rpc) ``` Would this help?

[mbarbin]

Hi Simon,

Thank you for your insightful response! It seems I initially misunderstood the
intended usage pattern for the Server.make bundles. Your clarifications have
been invaluable, and I've been able to make further adjustments to the
ocaml-grpc PR to utilize the bundles more effectively. As a result, the latest
version of the PR does not necessitate any modifications to ocaml-protoc for this
integration.

I appreciate your time and assistance. I'll be closing this issue for now, as I
believe there's no further action required on your part. Should the need arise,
I'll reopen it with additional information.

Thanks again!

[c-cube]

Great! I tried to write a bit more about services, cause it's not obvious: https://github.com/mransan/ocaml-protoc#server-side Feedback welcome! :)

[tmcgilchrist]

I tried out ocaml-protoc on the routeguide example from ocaml-grpc over the weekend.

A few points of feedback:

• I missed the user defined deriving for protobuf types. There is built in pretty printing but I think this should be user extensible. eg [@@deriving show, eq, yojson] if I want them.
• The Int31/32 split is a bit annoying but understandable.
• Personally I would prefer types of Route_guide.Person.t over Route_guide.person
• Using exceptions over result types, again personal preference for result types.
• Swapping around the signature order for Encoder, chaining with the encoder is awkward. Lots of code doing this rather than using |> operator.

let encoder = Pbrt.Encoder.create () in
Route_guide.encode_pb_route_summary summary encoder;
Pbrt.Encoder.to_string encoder

I didn't get to running benchmarking or seeing how the allocation differs from ocaml-protoc-plugin
Changes here https://github.com/tmcgilchrist/ocaml-grpc/tree/ocaml-protoc-route-guide note there is no exception handling for decode errors.

[c-cube]

Thank you for the feedback! :)

There is built in pretty printing but I think this should be user extensible. eg [@https://github.com/deriving show, eq, yojson] if I want them.

Seems like the doc wasn't updated, but if you look at, say, src/tests/integration-tests/test20.proto you can see this option: option (ocaml_type_ppx) = "deriving show". There's also, in src/tests/integration-tests/test21.proto, an option for all types: option (ocaml_all_types_ppx) = "deriving show";. And finally you can pass it on the command line: ocaml-protoc --ocaml_all_types_ppx "deriving show".

Route_guide.Person.t over Route_guide.person

Yeah I hear you. On the other hand this is really annoying in the presence of mutually recursive types (I personally don't touch recursive modules, ever, and it seems that mransan didn't either).

Using exceptions over result types, again personal preference for result types.

Result types can be cleaner. On the other hand, exceptions are the faster option in this case, especially in OCaml! It saves a lot of intermediate allocations, and possibly closures if you use a monad to handle results.

Swapping around the signature order for Encoder, chaining with the encoder is awkward. Lots of code doing this rather than using |> operator.

I agree. I think it's a papercut, and with hindsight, since I just broke the code generation anyway I could have fixed that. It was previously useful because things like nested messages would rely on partial application, but now it's not the case anymore because of the work I did to reduce closure allocations.

If you get to benchmark against ocaml-protoc-plugin's runtime, you could try to use my branch of ocaml-benchmark which measures allocations. I think core-bench is also a very robust benchmark tool that does the same.

On a past version (2.4 ish?) I seem to recall I benchmarked pbrt against ocaml-protoc-plugin's runtime and it was already significantly faster. Now this new version is almost twice as fast in some cases, so my expectation is that it'll show up in your benchmarks if you have time to run them :).

if you benchmark your gRPC branch, see if you find a way to reuse the Pbrt.Encoder.t values (after calling Pbrt.Encoder.reset on them). This reuses the internal buffer.

[tmcgilchrist]

Seems like the doc wasn't updated, but if you look at, say, src/tests/integration-tests/test20.proto you can see this option: option (ocaml_type_ppx) = "deriving show". There's also, in src/tests/integration-tests/test21.proto, an option for all types: option (ocaml_all_types_ppx) = "deriving show";. And finally you can pass it on the command line: ocaml-protoc --ocaml_all_types_ppx "deriving show".

Super, that cli option is exactly what I needed. I'll add an example to the grpc repo for how to use ocaml-protoc.

I agree. I think it's a papercut, and with hindsight, since I just broke the code generation anyway I could have fixed that. It was previously useful because things like nested messages would rely on partial application, but now it's not the case anymore because of the work I did to reduce closure allocations.

It would be nice to include if there is another breaking API change.

On a past version (2.4 ish?) I seem to recall I benchmarked pbrt against ocaml-protoc-plugin's runtime and it was already significantly faster. Now this new version is almost twice as fast in some cases, so my expectation is that it'll show up in your benchmarks if you have time to run them :).

if you benchmark your gRPC branch, see if you find a way to reuse the Pbrt.Encoder.t values (after calling Pbrt.Encoder.reset on them). This reuses the internal buffer.

Good to know that, thanks.

[mbarbin]

Hello Simon,

I hope this message finds you well. I'm reaching out to you again, this time in
a slightly different context related to our previous discussion here.

In addition to the work I mentioned on ocaml-gprc, I'm also working on a
personal project where I'm exploring an extended API to ocaml-grpc for a
specific use.

As per our previous discussions, I understand that the individual server stubs
are part of the generated code and they're not typically used directly. However,
I'm considering the possibility of their direct use.

Before making any concrete suggestions, I'm planning to conduct tests to verify
this approach and evaluate its potential benefits. I've begun preliminary work
to expose the stubs here.

The individual server stubs are in the generated code. They're not really
intended to be used directly, but I guess they could be?

It seemed from our previous conversation that you might be open to considering
such a change. This has encouraged me to explore this path. I wanted to keep you
informed, so it won't be a surprise if I propose a pull request based on this
work. I'll keep you updated on my progress.

Thank you for your time and consideration!

[c-cube]

Sounds good, I'm curious to see what use you make of it! :-)

I've hidden them in the first place because I think the current API is better in the use cases I had in mind (adding a whole service to a server), but it's not much of a change to also expose the individual RPC endpoints. I'm open to it.