-
Notifications
You must be signed in to change notification settings - Fork 5
feat(client): implement docling-serve-grpc service #329
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
base: main
Are you sure you want to change the base?
Conversation
|
Thank you @krickert for this! You've definitely put a lot of thought & time into this. If you don't mind, let me take a few days to go through all of this. I've just returned to the office this morning after being out all week, and I want to give this the thought and review it deserves. |
:java_duke: JaCoCo coverage report
|
|
||||||||||||||
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
|
@all-contributors add @krickert for code,idea |
|
I've put up a pull request to add @krickert! 🎉 |
|
@all-contributors add @krickert for documentation, tests |
|
I've put up a pull request to add @krickert! 🎉 |
|
Hi @krickert - when you created the pull request it does not look like you checked the box "Allow edits from maintainers". Is that something you can turn on? I'd like to add a few things so that the CI will pick up these new modules and run the tests/code coverage on the PR. |
|
Oh no problem!! Done. |
Signed-off-by: Kristian Rickert <krickert@gmail.com>
Signed-off-by: Kristian Rickert <krickert@gmail.com>
|
OK I was able to do it I think? |
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you very much @krickert for this! You've definitely put a lot of thought & work into this, so its much appreciated.
Sorry for the delay in my review. I wanted to look over it, give it some time to settle in my brain and think about it a bit.
I'd also like @ThomasVitale to look through this as well.
There are a few small general comments I have that I saw in lots of places (note to self - I need to update the contributing guidelines with these so that people are aware):
- No wildcard imports please
- While we do need to put some linting in place, there is a
.editorconfigfile in the repository. It would be great if you could configure your IDE to adhere to whats in there.
The biggest part of this that I want to make sure I understand is that DoclingServeGrpcService is a gRPC server that wraps DoclingServeApi. Then the gRPC definitions from the protobuf files can interact with the server?
The thought being that at some point if docling itself supported a gRPC server out of the box, then DoclingServeGrpcService (& the dependency on docling-serve-api) wouldn't even be needed?
If thats the case, should we structure this similar to how we've structured the REST side? One module just for the API (which in the case of gRPC would mostly just be the protobuf definitions)? And then a client module, which would be the wrapper thing thats here?
That way, when/if docling has a grpc server out of the box, the client module would just get deprecated and cease to exist?
@ThomasVitale I'm curious as to your overall thoughts as well.
docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/DoclingServeConvertApi.java
Outdated
Show resolved
Hide resolved
docling-serve/docling-serve-api/src/main/java/ai/docling/serve/api/DoclingServeChunkApi.java
Outdated
Show resolved
Hide resolved
| * @return a {@link TaskStatusPollResponse} containing the task ID and initial status | ||
| * @throws ai.docling.serve.api.validation.ValidationException If request validation fails for any reason. | ||
| */ | ||
| TaskStatusPollResponse submitChunkHybridSource(HybridChunkDocumentRequest request); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as above and on DoclingServeConvertApi
I'm not sure of the purpose for this method? The async variant returns CompletionStage so that the end user doesn't have to do any of the polling/waiting. It's handled directly by the API.
Shouldn't it be the same here? I'm not sure why we want to force the user to have to poll on their own?
...ing-serve/docling-serve-client/src/main/java/ai/docling/serve/client/DoclingServeClient.java
Outdated
Show resolved
Hide resolved
| @Override | ||
| public TaskStatusPollResponse submitChunkHierarchicalSource(HierarchicalChunkDocumentRequest request) { | ||
| return this.chunkOps.submitChunkHierarchicalSource(request); | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Similar comment as on DoclingServeConvertApi & DoclingServeChunkApi.
I'm not sure of the purpose for this method? The async variant returns CompletionStage so that the end user doesn't have to do any of the polling/waiting. It's handled directly by the API.
Shouldn't it be the same here? I'm not sure why we want to force the user to have to poll on their own?
...-serve/docling-serve-grpc/src/main/java/ai/docling/serve/grpc/v1/mapping/ServeApiMapper.java
Show resolved
Hide resolved
| * Proto → Java: for incoming gRPC requests that need to call the REST client. | ||
| * Java → Proto: for REST client responses that need to be returned as gRPC responses. | ||
| */ | ||
| public class ServeApiMapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Can probably be declared
final - I totally understand the purpose of this class, but is there a way we can think of an easy way to maintain it rather than hard-coding everything? Maintainability is something we spent a lot of time thinking about up front. What about using something like Mapstruct? Mapstruct itself wouldn't need to be exposed outside of the module - it generates static code at build time via an annotation processor.
@ThomasVitale thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DoclingServeGrpcService. Yup - a bridge/wrapper. I actually have an open ticket with Docling to support gRPC natively (they haven't bit yet), so the goal of this implementation is to serve as a proof-of-concept to motivate them to move in that direction.
To your question about structure: While mirroring the REST setup feels intuitively consistent, I strictly avoid a 1:1 mapping between REST and Protobuf. They play by different rules, and treating them the same introduces long-term risks.
Here is why I want to stick to the standard "v1" Protobuf definition approach:
Best to think of protobufs as Data, not just APIs. Unlike REST, which is ephemeral, Protobuf messages are often used as immutable records for storage (Kafka, logs, cold archival). I plan to use these definitions for long-term storage of Docling outputs. Makes sense too right? It's a document service type output. They even want to standardize this model more, which suggests it'll settle down and not change too much anyway.
If we couple the .proto package structure to our current temporary wrapper structure (to match REST), we bake implementation details into the data.
Therefore --- If Docling later releases a native server and we switch to it, or if we refactor our wrapper, we break backward compatibility with every record we’ve ever stored (and would've had to make definition changes anyway). I want to avoid needing a complex migration/mapping class just to read data we saved a year ago.
I also avoid "REST-in-Protobuf" syntax. When we try to mirror REST 1:1, we usually end up with "bad" Protobuf definitions. We miss out on streaming capabilities, we end up with unnecessary empty request/response objects, or we create definitions that mimic a specific language's (Java/Python) object structure rather than a language-neutral contract.
The API module should contain pure, immutable v1 definitions that look like they came from a native Docling server, agnostic of our specific wrapper.
If/when Docling releases a server, our wrapper module (DoclingServeGrpcService) effectively becomes obsolete and can be deleted. However, because we kept the definitions standard and separate:
- We won't have to change the data format.
- The generated clients (stubs) will continue to work with zero coding changes.
- We avoid the old SOAP-style integration nightmares; the auto-generated stubs just handle the switch.
So if you're ok with this - let’s keep the definitions (the Schema) distinct from the implementation (the Wrapper). The Schema should be designed as if the "perfect" Docling gRPC server already exists and not let the JSON/REST bleed into it. This allows us to use these objects in Kafka, mobile apps, or storage without worrying about the internal structure of our temporary bridge service.
I know it's not as intuitive, but you really don't even need to make separate tags and releases of protobufs if you design it well since the definitions should always line up and a schema registry can automatically track the changes.
| import ai.docling.serve.api.task.response.TaskStatus; | ||
| import ai.docling.serve.api.task.response.TaskStatusMetadata; | ||
| import ai.docling.serve.api.task.response.TaskStatusPollResponse; | ||
| import ai.docling.serve.v1.*; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please no wildcard imports. Please try to format/adhere to the .editorconfig.
If you're using IntelliJ you can configure it to automatically respect it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll setup intellj to lint better..
| * Each field is explicitly mapped to maintain strong typing throughout the gRPC stack. | ||
| */ | ||
| @SuppressWarnings("DataFlowIssue") | ||
| public class DoclingDocumentMapper { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pretty much all the same comments I left in ServeApiMapper
| /** | ||
| * Shared utilities for proto ↔ Java mapping. | ||
| */ | ||
| final class ProtoMapping { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's already a Utils class in docling-serve-api that you could add this to.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Will do...
…rve-grpc service Signed-off-by: Kristian Rickert <krickert@gmail.com>
…in `docling-serve-grpc` service Signed-off-by: Kristian Rickert <krickert@gmail.com>
|
Still workin' on the changes - I can get to the rest tonight. Worked out the obvious ones already, looked into MapStruct protobuf plugin, it would be adding another layer though and it's just one API we're mirroring. If the plan is to mirror many, I'd recommend this but the scope of the mapping was 3 classes. I'm going to run this through potentially 100s of millions of docs, so any bugs are going to be found. Finally, it would be fastest this way and any "gotchas" that come out of the REST api can be mapped here. But maintaining the API isn't that bad - it took me less than a day to write up the API. In return, we get a 1:1 logical mapping with all the protobuf advantages of backward compatibility. Since there's a PR open on the main docling line, let's see where that goes? If the server moves there, the proto client would be available OOTB and you can write a quarkus plugin without this java layer, too. |
I appreciate you looking into things! Re: the mapping stuff - there is a LOT of logic in the mapping classes, which looks like it is mapping between the protobuf objects and the @ThomasVitale and I are only 2 people working part time on this project, so at the beginning we said that we really needed to favor maintainability as one of the top concerns. I wasn't aware there was a mapstruct protobuf thing, I was merely talking about Mapstruct the java mapper. It provides a nice way to map from one object structure to another in a declarative fashion. Now I'm not saying i want to force it, but just something to look into. |
I think it's worth looking into. I'm looking for a better way too - I use CEL on protobufs on my mainline project to handle my mappings between protobufs... it's faster than MapStruct I'm sure - but would be worth looking into as well. CEL works on both protobufs and pojos, so I think that would be a bit less brittle because it's an industry standard and not just a java library - so I think it can map between the REST Jackson objects to the Protobufs well and even put expressions (like in my case, the default values). Another suggestion - if you consider it - having the grpc layer be the client instead of a java layer. I think this layer is still needed though, until we get a true grpc client from the server then I'd go that route instead. But even with this, you can just use the grpc client then you don't have to maintain any code just the mapping layer and offer all the functionality of the server on any client... it'll also make it attractive to those who have a grpc environment and would prefer this layer over rest. just a thought :) |
|
I'll have a look and share my comments later today. Thanks for your patience! |
|
HTML test reports are available as workflow artifacts (zipped HTML). • Download: Artifacts for this run |
|
Thanks so much @krickert for suggesting this feature and for the work you've been doing. It's much appreciated! And I particularly appreciated all the insightful information and guidelines for gRPC API design that you shared together with the code. As I'm no GRPC expert, that was helpful to me. I'd like to share some thoughts from different perspectives. Considering an architectural perspective, I haven't fully grasped the benefits of introducing gRPC if the network calls to the Docling Serve API is done with JSON over HTTP (so the potential bottleneck of JSON parsing and polling remains). If I understand correctly, considering Docling Serve doesn't have a gRPC API, the goal would be more on providing streaming-friendly APIs to the developers using Docling Serve rather than performance. Is that right? If so, wouldn't Quarkus with its convenient reactive core model already offer a nice developer experience via the Mutiny APIs (and its operators to build reactive streams) in the current Quarkus Docling project, using the async Docling Serve APIs? Considering the maintenance perspective for the Docling Java project, I'm not sure about adding this support in this project at this stage. Here's some of my considerations.
To sum up, based on all above, I personally don't feel confident introducing this gRPC module into Docling Java. And one of the key points I'd like to mention again is that this project is not GA yet. It's actively evolving and will hopefully reach the first GA version this year. Adding gRPC support now without having official support in the core Docling Serve project doesn't seam a good fit as it will introduce quite some work that would widen the scope of the project and make it harder to get to the first GA release. Things would have been different if Docling Serve had already a gRPC API. But the fact that it doesn't and we need to implement a bridge and design the API/data model for it worries me. Once again, thank you @krickert for all the work you did! I hope what I shared made sense. If I misunderstood something, please let me know, thank you! @edeandrea I would also like to hear your thoughts about what I wrote. |


This pull request introduces a professional gRPC client and service bridge for Docling Serve, designed for enterprise-grade pipeline integrations and long-term archival storage.
Key Enhancements
ai.docling.serve.v1contract with strict versioning throughout both Protobuf and Java packages.Closes #325