-
Notifications
You must be signed in to change notification settings - Fork 1
feat: initial api & client implementation #27
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
Conversation
|
@ThomasVitale @lordofthejars take a look and let me know what you think of this approach. I didn't get into ServiceLoaders or anything like that. I think that overcomplicates things. |
145e42f to
14aae2d
Compare
lordofthejars
left a comment
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.
Wow almost everything is done hahaha we are almost ready to go. Looks good to me just one small comment.
Well, not really :) This is just a design idea that I've only implemented for one of the model objects to show the idea/pattern. If we want to adopt this pattern then we need to implement it across the board. Then we need documentation, release automation, etc :) |
Perfect, looks good to me, then we erge this one and we adapt the rest of the model, or we push ito this PR? |
If we like this model then I'll fill in this PR with the rest of the API (its currently marked as draft). I'd like Thomas to weigh in first though, since he was the one who built the initial version. |
|
Thanks for this PR! In general, I like the approach, but I have two concerns:
|
Agreed. And I realized that doing this required "leaking" That being said, could that be an implementation detail? Since they are annotations, could we support both jackson databind 2.x & 3.x? But then that would violate #23 (comment). We definitely need to support both 2.x & 3.x, AND I think backwards-compatibility & immutability is super important.
I agree with what you've said. In a perfect world I'd like to have everything, so maybe we need to prioritize our list of requirements? We all know that software engineering is mostly about understanding that there isn't a "perfect" solution to anything - its about knowing & evaluating trade-offs. I think we need to prioritize our requirements by importance. I've taken a stab at what I think. Maybe the best solution here is to go back to simple Java Pojos for the model classes and stop trying to be "cute" about it? The code isn't as clean and it allows for mutability, but it also allows for the most customization/generalization? |
|
Maybe we can start with POJOs and if we see taht the project got a high level of adoption, that lot of different Java frameworks are adopting and so on, then we can always refactor. |
24a7767 to
6029285
Compare
Signed-off-by: Eric Deandrea <[email protected]>
Signed-off-by: Eric Deandrea <[email protected]>
Signed-off-by: Eric Deandrea <[email protected]>
Support both Jackson 2 & Jackson 3 Signed-off-by: Eric Deandrea <[email protected]>
Support both Jackson 2 & Jackson 3 Signed-off-by: Eric Deandrea <[email protected]>
|
@ThomasVitale @lordofthejars I've found a way to support both Jackson 2 & 3 in the API & client Since they are annotations, I can have both Jackson 2 & 3 at the API level. Then at the client level I can support both 2 & 3 through a Factory interface, which can detect which client to give based on whats on the classpath. |
Support both Jackson 2 & Jackson 3 Signed-off-by: Eric Deandrea <[email protected]>
Support both Jackson 2 & Jackson 3 Signed-off-by: Eric Deandrea <[email protected]>
api/src/main/java/ai/docling/api/convert/response/DocumentResponse.java
Outdated
Show resolved
Hide resolved
Support both Jackson 2 & Jackson 3 Signed-off-by: Eric Deandrea <[email protected]>
Convert all to POJOs Signed-off-by: Eric Deandrea <[email protected]>
|
@ThomasVitale @lordofthejars @oscerd I've gone ahead and converted everything to simple POJOs, removing all the builders completely. It isn't as clean, but like @ThomasVitale mentioned at some point we may want to try and auto-generate these somehow, so I wanted to keep them as simple as possible. |
|
Yes go ahead. For me it is fine, especially to have a first release and start the integration with other projects. Then if sometimes we are able to automate, perfect, and if not, well it is a valid approach after all |
Rename modules Signed-off-by: Eric Deandrea <[email protected]>
|
I also thought that as part of this we should clean up folder/project names a bit
The reason for |
Fix build Signed-off-by: Eric Deandrea <[email protected]>
|
I like and agree with the plan of starting with simple POJOs. It's a good foundation and should also make it less complicated to automate the generation in the future. I also like the new naming strategy for the modules. Besides supporting more use cases in the future, it's cleaner to have the folder and the Maven module named the same. Tomorrow, I'll have a better look at the PR and complete the review. |
Here's a crazy thought I just had. I'm really not in love with all the "noise" or the POJO approach, even if it is the best approach. As much as I hate admitting this to myself, what about if we used something like lombok? We kept it self-contained an unexposed, but it could help make the code we're maintaining very clean and get the benefits of immutability with builders while still having very clean code. I'm not a lombok fan in general, but this might not be a bad use case for it? We could even keep our own code in |
|
I am not a big fan of Lombok, but given the nature of the project I'd say taht everything that can help us on less maintainance is well welcomed. |
Me neither, but as an experiment I took the package ai.docling.api.health;
import org.jspecify.annotations.Nullable;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
@JsonInclude(JsonInclude.Include.NON_EMPTY)
public class HealthCheckResponse {
@JsonProperty("status")
private String status;
@Nullable
public String getStatus() {
return status;
}
public void setStatus(@Nullable String status) {
this.status = status;
}
public HealthCheckResponse withStatus(@Nullable String status) {
setStatus(status);
return this;
}
@Override
public String toString() {
return "HealthCheckResponse{" +
"status=" + (status == null ? "null" : "'" + status + "'") +
'}';
}
}Using lombok (& not exposing it outside of the package ai.docling.api.health;
import org.jspecify.annotations.Nullable;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import ai.docling.api.health.HealthCheckResponse.Builder;
@JsonInclude(JsonInclude.Include.NON_EMPTY)
@tools.jackson.databind.annotation.JsonDeserialize(builder = Builder.class)
@lombok.extern.jackson.Jacksonized
@lombok.Builder(toBuilder = true)
@lombok.Getter
@lombok.ToString
public class HealthCheckResponse {
@JsonProperty("status")
@Nullable
private String status;
@tools.jackson.databind.annotation.JsonPOJOBuilder(withPrefix = "")
public static class Builder { }
}And this is what would end up in the jar file: package ai.docling.api.health;
import com.fasterxml.jackson.annotation.JsonInclude;
import com.fasterxml.jackson.annotation.JsonProperty;
import com.fasterxml.jackson.annotation.JsonInclude.Include;
import com.fasterxml.jackson.databind.annotation.JsonDeserialize;
import com.fasterxml.jackson.databind.annotation.JsonPOJOBuilder;
import org.jspecify.annotations.Nullable;
@JsonInclude(Include.NON_EMPTY)
@JsonDeserialize(
builder = Builder.class
)
@tools.jackson.databind.annotation.JsonDeserialize(
builder = Builder.class
)
public class HealthCheckResponse {
@JsonProperty("status")
private @Nullable String status;
HealthCheckResponse(@Nullable String status) {
this.status = status;
}
public static Builder builder() {
return new Builder();
}
public Builder toBuilder() {
return (new Builder()).status(this.status);
}
public @Nullable String getStatus() {
return this.status;
}
public String toString() {
return "HealthCheckResponse(status=" + this.getStatus() + ")";
}
@JsonPOJOBuilder(
withPrefix = ""
)
@tools.jackson.databind.annotation.JsonPOJOBuilder(
withPrefix = ""
)
public static class Builder {
private String status;
Builder() {
}
@JsonProperty("status")
public Builder status(@Nullable String status) {
this.status = status;
return this;
}
public HealthCheckResponse build() {
return new HealthCheckResponse(this.status);
}
public String toString() {
return "HealthCheckResponse.Builder(status=" + this.status + ")";
}
}
}If new attributes were needed then we'd just have to add the attributes to the class. Lombok would take care of everything else. |
|
I'd say go ahead, let's keep things simple for now, moreover, I thikn we should monitor the number of downloads and if we see there is a massive adoption, then, let's figure out to make things more beauty |
Use lombok Signed-off-by: Eric Deandrea <[email protected]>
|
I went ahead and did the lombok thing across the API. It fully supports jackson 2 & 3 nor does it leak any Lombok things outside of the API. It has a nice builder approach and it uses immutable objects |
|
I'm not a fan of Lombok either, but I understand the context and see the value in this case :) |
|
@ThomasVitale are you good with this approach and everything I've done here? |
.github/workflows/build.yml
Outdated
| - client | ||
| - testing | ||
| - testcontainers | ||
| - docling-api |
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.
Since the idea is to possibly have a docling-client module in the future using Docling directly (no Docling Serve like in docling-serve-client), I wonder if we should similarly have a docling-serve-api module (for all the code currently in docling-api) and a docling-api module that would only include the Docling core APIs (e.g. Document). In alternative, single module, but two sub packages (core, serve)?
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.
Sorry I've totally changed this comment after thinking about it a bit.
That might not be a bad idea. We can just rename docling-api to docling-serve-api for now. We can build a placeholder for docling-api (or introduce it later on).
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.
@ThomasVitale i've gone ahead and restructured this. I created a new top-level folder docling-serve and moved both docling-serve-api (formerly docling-api) and docling-serve-client into this folder.
I also changed the package structure for docling-serve-api (formerly docling-api) to ai.docling.api.serve (formerly ai.docling.api).
See dfa6755
|
@edeandrea I reviewed API and Testcontainers modules, and it all looks good to me. Only had the one comment about the naming of the API module/namespace I'm still going through the Client module changes. |
|
@ThomasVitale are you ok if I go ahead and merge this? Since it involves directory/name changes I'd like to get this in as I'm working on other things (version tester, release automation, etc) that will be impacted by this. We can always improve it as we move along. |
|
Yes, let's merge this! |
An idea for api design. This still allows using Java records to implement the model objects, but also makes these objects backwards-compatible by introducing interfaces & builders.
NOTE I've only applied the pattern to the
DocumentResponseso that we can decide if this is a good approach or not. If so, we can apply this pattern to the others.FIxes #23
Fixes #24