Skip to content
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

Getting started documentation for ocaml-grpc #25

Merged
merged 12 commits into from
May 23, 2023

Conversation

tmcgilchrist
Copy link
Collaborator

@tmcgilchrist tmcgilchrist commented Mar 17, 2023

Examples

Fixing the GRPC documentation one tutorial at a time.

TODO

  • Package examples so they compile in CI
  • Provide a quick start guide for GRPC equivalent to gRPC in Go
  • Provide routeguide implementation in OCaml based on tonic in Rust
  • Integrate with mdx to validate examples in Markdown format.

Based off the Diátaxis framework for technical documentation writing.

@quernd
Copy link
Collaborator

quernd commented Mar 17, 2023

Thank you for this initiative! This kind of tutorial was sorely missing. Given that we have an Eio port in the making that has feature parity with the Lwt implementation (we haven't battle-tested it yet), we can easily port this tutorial too so we can offer it in two flavors (or even three including Async).

Comment on lines 162 to 167
let greeter_service =
Server.Service.(
v () |> add_rpc ~name:"SayHello" ~rpc:(Unary say_hello) |> handle_request)

let server =
Server.(
v () |> add_service ~name:"helloworld.Greeter" ~service:greeter_service)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Relating to my above comment, with ocaml-protoc-plugin one can actually get the RPC and service names from the .proto file which is quite neat. (Mentioning this as a sidenote, not necessarily for inclusion in the tutorial -- we were considering writing a somewhat opinionated wrapper that would remove a lot of boilerplate)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

we were considering writing a somewhat opinionated wrapper that would remove a lot of boilerplate.

That would be a good improvement for the API in my opinion, however I've only recently started using this library and don't have a considered opinion on how that should look.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The service name and function name are squashed into the val name field on the generated code. eg

    module GetFeature = struct
      let name = "/routeguide.RouteGuide/GetFeature"
      module Request = Point
      module Response = Feature
    end      

What about making a change to the Message module type to include returning the components?

module type Message = sig
  type t
  val name : () -> string  (* returning GetFeature*)
  val service_name: () -> string (* returning routeguide.RouteGuide *)
  val from_proto: Reader.t -> t Result.t
  val to_proto: t -> Writer.t
end

Copy link
Collaborator

Choose a reason for hiding this comment

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

What about making a change to the Message module type to include returning the components?

This is a good idea, and it's not hard to do. In fact, we already did something like that in our fork. I could have sworn it was part of the changes that have been merged recently, but alas it wasn't. Here's the commit for reference:
dialohq/ocaml-protoc-plugin@beeffb2

There have been a few changes upstream so it won't merge cleanly anymore. I'll try to rebase it and open a new PR.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That would be great, let me know if I can help with it.

Copy link
Collaborator

@quernd quernd May 2, 2023

Choose a reason for hiding this comment

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

That would be great, let me know if I can help with it.

Feel free to give it a shot if you have some time to spare. Otherwise I'll get back to it next week as I'm still on vacation this week.

issuu/ocaml-protoc-plugin#28 is the PR we branched off of. It later got merged with some more changes, and some more cleanup was done in issuu/ocaml-protoc-plugin#43.

It's probably quickest to make these changes starting from the current trunk though.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@tmcgilchrist
Copy link
Collaborator Author

Testing the EIO version and writing up the code for that is on my list.
Doing an Aysnc version is less interesting, I don't use it and have less experience with it. However it would not be that hard for someone else to do it if they want to.

@wokalski
Copy link
Contributor

hey, amazing work. thanks for this!

@tmcgilchrist tmcgilchrist marked this pull request as ready for review April 28, 2023 00:03
@tmcgilchrist
Copy link
Collaborator Author

This is ready for review now @quernd and @wokalski

@tmcgilchrist tmcgilchrist force-pushed the documentation branch 3 times, most recently from 1e65c74 to c6a2e74 Compare April 28, 2023 01:02
Copy link

@novemberkilo novemberkilo left a comment

Choose a reason for hiding this comment

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

A few fixes as I worked through the tutorial.

As discussed, dune-project requires an update -- currently dune build fails.

examples/helloworld-tutorial.md Show resolved Hide resolved
examples/helloworld-tutorial.md Show resolved Hide resolved
examples/helloworld-tutorial.md Show resolved Hide resolved
examples/helloworld-tutorial.md Show resolved Hide resolved
Copy link
Collaborator

@quernd quernd left a comment

Choose a reason for hiding this comment

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

Just a couple of nitpicks. I think we should just merge this PR after these small fixes. It will be good for visibility!
Please don't forget to add your name to the list of authors :)

One more thing: You converted the greeter examples to use ocaml-protoc-plugin instead of ocaml-protoc. I'm okay with that but it means we the README in the root directory is not accurate anymore.

examples/helloworld-tutorial.md Outdated Show resolved Hide resolved
<!-- $MDX skip -->
```shell
{
"message": "Hello Tonic!"
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
"message": "Hello Tonic!"
"message": "Hello OCaml!"

grpc-async.opam Outdated Show resolved Hide resolved
examples/routeguide-tutorial.md Outdated Show resolved Hide resolved
@tmcgilchrist
Copy link
Collaborator Author

Just a couple of nitpicks. I think we should just merge this PR after these small fixes. It will be good for visibility! Please don't forget to add your name to the list of authors :)

I've done the smaller fixes, so it is ok to merge.

The tutorial pages don't flow the way I would like but I can fix that up in another PR this week.

@quernd quernd merged commit 36d2a1b into dialohq:main May 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants