Explore API with decode/encode in the library (draft)

[mbarbin]

Hello,

I'm exploring some RPCs libraries in Ocaml that are transitioning to Eio and ocaml-grpc caught my attention. It is nice to see such project under development 😃 Thank you!

I've noticed in the current interface the serialization of messages is left for the user of the library to handle. I wonder why you made that design choice. I was definitely surprised about that when I first looked at the Grpc_eio's examples.

Is it because you prefer not to depend on a particular protoc plugin in the implementation of Grpc_eio, or is there some specific use case where it is necessary to have a fine-grain control over de-serialization error e.g. while iterating through a stream? Do you expect that different users of Grpc_eio may be using different OCaml protoc plugins provider (and/or do you want to specifically support this)?

If you allow for Grpc_eio to depend on a chosen protoc plugin, it is likely some tighter integration becomes possible. In fact, it's perhaps even possible for users not to be exposed to any interface, function or type from the plugin.

In this PR I made an exploration along this idea, and drafted an Grpc_eio interface that handles the serialization and service names registering with a stronger reliance on the interfaces generated by ocaml-protoc-plugin. I'd be curious to hear your thoughts about this.

If you're interested in this sort of direction, I suspect there's more to be done. I'm thinking for example having the plugin generate a marker for the kind of interface each rpc have (server_streaming, client_streaming, unary, bidirectional) - it should be possible to prevent e.g. implementing an interface with the wrong kind (currently the kind annotation from the *.proto file doesn't make it to the OCaml generated code, so consistency is to be carefully achieved). There's probably more, I haven't thought much about it for now, I'd rather get some feedback from you first.

Thank you!

[quernd]

Thank you for your interest in ocaml-grpc and your contribution!
Yes, the reasoning is not to depend on a particular plugin for encoding and decoding. This is not limited to protobuf by the way. I don't know how popular it is, but JSON is an alternative explicitly mentioned in the gRPC specification.
I know it's a bit cumbersome and lacking in type-safety, and we've had plans for a while to make it more ergonomic (see e.g. the discussion in this PR

One thing that comes to mind is to define an interface for encoding/decoding and make use of functors or first-class modules to use a specific backend. It can be almost as ergonomic for the user while still keeping the library flexible. I believe your code can be used as a starting point for such an effort, in particular the interfaces you define in protoc_rpc.mli, server.mli, client.mli could be implemented for other en/decoders as well. In order not to depend on ocaml-protoc-plugin, we could move the implementations to separate packages. (They are also orthogonal to the Lwt/Async/Eio axis.)

[wokalski]

We could also embrace one of the plugins/transports as the de-facto workflow for ocaml grpc and release work such as presented here as ocaml-grpc-protobuf (for example) with a more opinionated interface.

[mbarbin]

Hello,

Thank you for your insightful feedback.

In response to your comments, I've made additional modifications to the PR. I've
introduced a separate library that handles the correct protobuf serialization
invocation, while preserving a similar ergonomic interface. This approach aims
to balance type safety with the flexibility to accommodate different codecs.

Your feedback reminded me of this article on typing RPCs: https://blog.janestreet.com/typing-rpcs/

I appreciate the ongoing dialogue and am open to further discussions. If you
feel these changes would be more appropriate in the future, please feel free to
table this PR. If this direction aligns with your vision, I believe it would be
beneficial to refactor Grpc_eio to implement the non-typed RPC in terms of the
typed one (and consider scheduling the non-typed version for future
deprecation). This could be followed by further refinements, renames, and so on.

Thank you once again for your time and consideration!

[quernd]

Thank you, these changes look very good. Just one thing, I'd put as much of this as possible into the core grpc library because it isn't specific to Eio. Then grpc-lwt and grpc-async could easily be adapted to also make use of this work.

[mbarbin]

Hello,

Per your suggestion, I've moved the RPC specification into Grpc to make it
accessible to all three backends. Consequently, the grpc-protobuf library is
no longer eio-specific and should be usable by lwt and async
clients/servers. By adhering to this interface, the deserialization of messages
will be blocking in lwt and async, aligning with the current implementation
in the lwt and async examples where decoding is separate from actual IO.

However, I must admit that exploring a similar typed RPC interface for lwt and
async exceeds the time I initially allocated for this draft. I also lack
experience with this kind of multi-backend libraries. I would appreciate if you
could contribute to this draft by exploring similar changes for the lwt or
async backend.

I'm also considering switching the dependency order between typed and untyped
RPCs during this early exploration phase to ensure feasibility. While this might
not need to be merged as part of the same PR, drafting such a change could help
identify potential issues. Differences in error handling between lwt, async,
and eio might necessitate further changes to the RPC specification interface.
I'm willing to attempt this in grpc_eio if you agree.

Thank you for your time and consideration.

[quernd]

Thank you @mbarbin ! This looks very good.
I was certainly not implying that you should implement similar interfaces for Lwt or Async, but I appreciate the effort you put into making it possible to use both different concurrency backends and different serialization libraries. Your contribution is very timely given this announcement: https://discuss.ocaml.org/t/ann-ocaml-protoc-3-0/13611

The major new feature of ocaml-protoc 3.0 is the support for service declarations. These are essentially a way to describe RPC endpoints, next to the types used to interact with the endpoint (example 3; full generated code). This is typically what it used in systems such as gRPC 2. Now ocaml-protoc generates server and client stubs for each endpoint, that pack together the type definitions and the relevant (de)serializers; that code doesn’t presume anything about a concrete RPC system.

If your PR works well with the latest ocaml-protoc it would be a major win.

[mbarbin]

Thank you for sharing the announcement about ocaml-protoc 3.0 and for the
mention on OCaml Discuss. I appreciate it.

In line with your enthusiasm about work being released in this space, I've added
a new library, grpc-protoc, to the PR. This library enables the use of
ocaml-protoc.3.0 for serialization in ocaml-grpc. I've also included a copy
of the greeter example for illustration.

While this is a functional proof of concept, the interface isn't as compact as
ocaml-protoc-plugin. In ocaml-protoc, the necessary information is divided
across three values:

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 demonstrated in the example, the first value 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.

I believe that this can be improved by proposing minor changes to the
ocaml-protoc maintainers, to allow the code generated by ocaml-protoc to
more directly expose the information needed to build a complete client/server
RPC specification.

[mbarbin]

The unrelated diffs you see on this file are due to my editor config which uses format-dune-file on save. After carefully editing the file by other means in the first few commits on the PR, I eventually gave up on trying to keep the file non formatted. Before enabling code review, I think either of the 2 following follow up will be satisfactory:

1. I clean up the file and remove formatting changes from the PR
2. We add a commit upstream that formats the file, and rebase the PR on top, thus voiding all the formatting changes.

If you have no objections, I favor 2, but I'm open to 1 as well. Thanks!

[mbarbin]

With the help of clarifications from mransan/ocaml-protoc#224 I modified grpc-protoc to use ocaml-protoc's Server.make bundles more properly.

Consequently, I modified the interface in Grpc.Rpc to better support both ocaml-protoc-plugin and ocaml-protoc. I find this modified version somewhat complicated. I'll need some time to think about it and come back to it.

Meanwhile, I duplicated the routeguide example to use ocaml-protoc in order to illustrate the differences in generated interfaces from proto spec, and show how each integrates with ocaml-grpc.

[mbarbin]

I've taken some time to reflect on our discussion.

There seems to be a tension between crafting a user-friendly API tailored to
specific library combinations and managing the complexity and maintenance
overhead of ocaml-grpc.

In my view, the best approach to this tension might be a matter of personal
preference. Here, I'll outline my preferred approach for ocaml-gprc.

Let's use the routeguide example, built with eio and ocaml-protoc-plugin, as a
case study.

Consider the following dependency graph ¹:

This graph shows that the routeguide components depend on grpc-eio and
grpc-protoc-plugin.

As it stands, the user is the first component where both grpc-eio and
grpc-protoc-plugin are available. Therefore, any improvements to the cooperation
between these libraries would have to be implemented by the user, increasing
friction and potentially leading to duplicated efforts.

This brings me back to my initial message in this conversation. If ocaml-grpc
provided non-parametrized libraries that cater to specific combinations of
dependencies, I believe we could significantly improve the API.

Consider this alternative dependency graph:

This modified graph introduces a new component, grpc-protoc-plugin-eio, which
depends on both grpc-eio and grpc-protoc-plugin. The user, in turn, depends on
this new component.

Now, grpc-protoc-plugin-eio is the first component where both libraries are in
scope. As it falls under the responsibility of the ocaml-grpc maintainers, it
can serve as a canonical place to include all enhancements related to this
particular combination.

This approach shouldn't lead to code duplication. On the implementation side,
you can still organize your code using well-crafted internal libraries.

An additional benefit of having expanded libraries is that it reduces the
pressure to design common interfaces that are both elegant and stable.
Supporting both ocaml-protoc and ocaml-protoc-plugin presents the challenge of
creating a common interface that will inevitably inherit complexity from the
differences between the two libraries. A user who has chosen one particular
library shouldn't have to deal with this complexity. However, without a
dedicated library for their use, they will have to navigate the common interface
and potentially face this added complexity.

I've pushed additional changes to the PRs to add both libraries, grpc-protoc-eio
and grpc-protoc-plugin-eio. I believe these changes improve the user-facing
experience and start to provide a more stable interface.

Footnotes

1. Dependency graphs courtesy of dune-deps. ↩

[quernd]

Thank you @mbarbin for the additional work you put into this PR. I did like your commit supporting both ocaml-protoc and ocaml-protoc-plugin 45d4878 a lot. I don't even find it particularly complicated to use. It seems to come down to one extra application of a functor in order to bridge the differences between the serialization interfaces.

That being said, I'm not convinced that non-parameterized libraries are the way to go. I believe it is best not to introduce a coupling between concurrency/networking (Eio, Lwt, Async) and serialization. I'd also rather not maintain n x m libraries for all possible combinations.
You mention "improvements to the cooperation between these libraries" but I'm not sure what these would be. Do you have anything specific in mind?

I will have some more time next week to look further into this. @tmcgilchrist do you have any opinion on this matter?

[mbarbin]

Alright, I've rolled back the previous set of changes for now.

We can revisit the topic of non-parametrized libraries when you have the
opportunity to delve deeper into it. In the meantime, this rollback should bring
the PR closer to your preferences.

During the now-reverted changes, I had enhanced the type safety between unary
and stream modes. I've introduced a new commit to restore that functionality,
drawing inspiration from ocaml-protoc.

It seems to come down to one extra application of a functor in order to bridge
the differences between the serialization interfaces.

I'm intrigued to understand more about this. Thank you!

[tmcgilchrist]

Having played a bit with the latest typed rpc version, I like where it is now with grpc-protoc-plugin and grpc-protoc providing the glue between serialisation library and grpc itself, without binding it to a particular concurrency/networking choice. Seems like a nice point between full freedom of serialisation and providing good defaults. Nice work @mbarbin

When the design settles down, it would be good to review the generated documentation for the various libraries and provide a clear structure for new users on where to start. Pointing them towards Typed_rpc with a default protobuf serialisation setup.

It would be interesting (but not required here) to see if the handler construction can be made to error on duplicate handlers. Right now it matches the first one in the list and ignores any other potential matches.

ocaml-grpc/examples/routeguide/src/server.ml

Lines 183 to 186 in c0c4df5

	let server t clock =
	Server.Typed_rpc.server
	(Grpc_protoc_plugin.handlers
	[ get_feature t; list_features t; record_route t clock; route_chat t ])

Not sure whether the error handling for decode/encode errors lines up with the grpc spec. In general there are some interop issues with go/rust versions of gRPC that I've found, so it's probably no worse than the current released version. Ideally the Typed_server should do the right thing with respect to gRPC and http2 errors.

Finally I wonder what the extra Functor looks like for the generated code and what impact that will have on performance/allocation. I don't have a good benchmarking setup yet to answer this question.

[quernd]

Thanks @tmcgilchrist for your opinion. I also like where it is now, the balance feels right. Really nice work @mbarbin!
Since this is basically opt-in, and we're at 0.x releases still, I'd be in favour of merging. We can then review documentation and examples.
I will prepare a formatting fix like you suggested so we can rebase this to reduce clutter. I'm also willing to write the missing bits for Lwt, less so for Async because of lack of experience.

[mbarbin]

Hello, and Happy New Year! Thank you for your positive feedback.

I've reviewed the PR and made a few changes directly within it, as well as created separate PRs to help us move forward with the review and merge process.

Pre-requisite: #49 #50
Follow-up: #51

[tmcgilchrist]

I'm also willing to write the missing bits for Lwt, less so for Async because of lack of experience.

I'm ok to write the Async piece.

[mbarbin]

Hello @quernd,

I trust you're doing well. I wanted to provide additional context about the
current PRs I've initiated in relation to this draft.

Since this is basically opt-in, and we're at 0.x releases still, I'd be in
favour of merging. We can then review documentation and examples.

I appreciate your willingness to merge this work. As for the specifics of the
process, I would prefer not to merge the draft directly into the main branch.
Consequently, I've created several PRs to segment the work into manageable
units, which should facilitate the review and merge process.

My hope is that merging #54 will enable us to open independent PRs for the
remaining work on eio, lwt, and async.

#50 Add dependencies to fix opam-dune-lint
#52 Upgrade ocamlformat
#53 Introduce Typed RPC Specification
#54 Add typed rpc helpers

I plan for PRs #48 and #51 to remain in the Draft stage and be closed once
they've served their purpose:

#48 Explore Api (draft)
#51 Reversing Dependency (draft)

The structure I've proposed from #50 to #54 is flexible, so if you have a
different approach in mind, I'm open to reorganizing things to suit your
preference.

It's wonderful to see that both you, quernd, and tmcgilchrist have offered to
participate in implementing components. This is shaping up to be a truly
collaborative effort, which I'm thrilled about!

Thank you!

[quernd]

Thank you @mbarbin for structuring this to be more manageable to review! I am taking a look at the prerequisite PRs now.

[mbarbin]

Hello,

I wanted to provide an update on my progress and thoughts.

Non-parametrized library

After reflecting on our previous discussion, I've come to realize that my goal
isn't necessarily to cater to all combinations of concurrency and serialization.
Instead, I'm interested in establishing a "de-facto workflow", as @wokalski
worded early in the discussion. Given that Eio is poised to become the new
concurrency library for OCaml, it seemed logical to align my efforts with this
development. I've also grown to appreciate the flexibility that ocaml-grpc
provides with its generic building blocks. Consequently, I've decided to
continue my exploration of a de-facto RPC library for Eio in a separate
project¹.

Tests

Serialization (ocaml-protoc)

Work is currently underway² in ocaml-protoc to facilitate automatic roundtrip
testing of protoc specifications through serialization. I'd be interested to
hear your thoughts if you decide to test this with some of your protoc
interfaces.

RPC

Eio-rpc¹ is based on a preview package of ocaml-grpc that includes the
typed-rpc-eio implementation from #55. I'm in the process of writing
expect-tests that involve client/server pairs running in the background and
exchanging RPCs. I'm making good progress towards achieving comprehensive
coverage³, which should provide some assurance that the typed-rpc has been
thoroughly tested.

Progress and synchronization of work

I've noticed a tension between the desire to break down the work into reviewable
units and the need to provide a complete view of the code for easier interaction
and testing. The new code introduced in #53, #54, #55 isn't exercised in the
tree, and if you plan to start drafting the Lwt and Async parts, you'll
likely have to duplicate the commit chain in another PR.

One potential solution could be to create a long-lived branch (e.g.,
"typed-rpc") and direct typed RPC related PRs to that branch. This approach
would allow for careful review of segmented parts while also providing a staging
area (the branch) before deciding whether to merge the full picture into the
main branch. However, this approach may also have its drawbacks, especially if
there are frequent concurrent PRs on the main branch. I thought it was worth
mentioning as a possible strategy.

Footnotes

1. https://github.com/mbarbin/eio-rpc ↩ ↩²

2. https://github.com/mransan/ocaml-protoc/pull/233 ↩

3. https://coveralls.io/github/mbarbin/eio-rpc?branch=main ↩

[wokalski]

Thanks for the writeup. I will be thinking about this now as I'm moving some of our internal endpoints to grpc. I'll be looking at hyper opinionated codegen where you have protoc in -> ready to use RPC methods out, from what I saw your current solution depends on some boilerplate to connect things.

I'm also thinking about integrating telemetry in the generated code as well.

[mbarbin]

Hi @wokalski,

I wasn't working on grpc lately. Great to hear from you again!

I tend to view 'opinionated' as 'pragmatic' and 'unopinionated' as 'uninstantiated'. I mention this to help us communicate more effectively. When it comes to boilerplate, I'm open to discussing it further. The perception of what's burdensome can vary, so feel free to get specific.

Remember, all my work in the grpc scope has been exploratory and in draft form. I'm keen to continue this exploration and your input helps me do that.

Looking forward to our continued discussions.

[mbarbin]

Hi @wokalski and team,

I hope this message finds you well. I've noticed the recent activity in #56 and the refactor happening in the repository. I'm thrilled to see the project moving forward.

I'm looking forward to the new library and I'm interested in contributing to it once it's more stable. In the meantime, given the extent of the refactor, I understand that my open PRs may no longer apply and could potentially cause merge conflicts.

With this in mind, I'm proposing to close them (unless you have strong objections ?). This way, you can focus on the refactor without having to worry about potential merge conflicts. Please feel free to use any part of my contributions that you find useful, or disregard anything that doesn't fit into the new refactor. All my contributions are intended to be compatible with the existing license of the repository.

Thank you for your hard work on this project and good luck with the refactor. I'm excited to see where the project goes next.

Best,
Mathieu

[wokalski]

Thank you for the message and following the project. Yes - I will close the PRs but I won't shy away from stealing some ideas 😄. Also - if you'd like I'd appreciate help with the PR most notably around refactoring Lwt/async implementations to follow eio (I also wrote it all so that there's as much code reuse as possible). A contribution of moving away from the string based APIs for (de)coding would be welcome as well.

[mbarbin]

Understood. There's nothing remaining in this draft (#48) that hasn't already been included in #53, #54, #55, so I'll proceed with closing it now. I'll leave the others open for you to close at your convenience. Thank you!

Regarding the lwt/async refactoring, I'll defer to @quernd and @tmcgilchrist, who have previously expressed interest in contributing to these parts in the course of this PR conversation.

As for the transition away from string-based APIs, I agree that it's a sensible direction for the project but I currently don't have any performance-sensitive use cases, so I may not be the best person to tackle this.

I will be awaiting the completion of the refactoring and am optimistic about adapting my usage to align with it. My starting point will likely be eio-rpc. Thanks!

[tmcgilchrist]

Happy to implement an lwt and/or async version when the refactoring has settled down, as keeping the three implementations is useful for me.

[wokalski]

Thanks. I will let you know once I'm done. I'm working on it actively shifting between the implementation of the lib and using it in the app to polish the api.