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

Draft: Decouple modules from (ok)http client #1103

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

langchain4j
Copy link
Owner

Issue

#1044

Change

This is the first step towards decoupling modules from (ok)http client.
This PR introduces an abstraction layer (in langchain4j-core module) over various http clients.
Main components:

  • HttpClient
  • HttpClientBuilder
  • HttpRequest
  • HttpResponse

This PR also provides 2 implementations of HttpClient:

  • OkHttpHttpClient (uses okhttp that is currently used in most modules, but does not use retrofit)
  • SpringRestClientHttpClient (uses RestClient from Spring)

More implementations (e.g. JDK 11 HttpClient) can be added later.

The first module to migrate is langchain4j-anthropic, this PR decouples it from okhttp.
Users are able to provide a HttpClientBuilder when creating Anthropic*Model. If not provided, default (okhttp) will be loaded via SPI.

With this change, request/response logging is also unified and moved to HttpRequestLogger and HttpResponseLogger. So all modules can now get logging for free.

General checklist

  • There are no breaking changes
  • I have added unit and integration tests for my change
  • I have manually run all the unit and integration tests in the module I have added/changed, and they are all green
  • I have manually run all the unit and integration tests in the core and main modules, and they are all green

Checklist for adding new model integration

  • I have added my new module in the BOM

@langchain4j
Copy link
Owner Author

Hi @geoand and @ThomasVitale
Could you please take a quick look at this draft? 🙏

@langchain4j langchain4j changed the title Draft: HTTP Clients Draft: Decouple modules from (ok)http client May 14, 2024
@geoand
Copy link
Contributor

geoand commented May 14, 2024

Nice!

Right off the bat I would say that I don't believe there should be a Spring specific implementation in the core repo. Spring REST Template is not really something many (any?) non Spring application use.
If anything, there could be a JDK HttpClient module (which would require Java 11+ though).

@geoand
Copy link
Contributor

geoand commented May 14, 2024

Also for the Quarkus integration, I don't really think this will make much of a difference as we really want to continue using the declarative clients

@langchain4j
Copy link
Owner Author

langchain4j commented May 14, 2024

@geoand thanks for the feedback! Spring specific things will be moved to langchain4j-spring repo (once this PR is about to be finalized), there are a few TODOs in the PR.
Regarding JDK 11 HttpClient, I guess it makes total sense, but probably can be delayed a bit, since there are so many other higher-prio issues.

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.

None yet

2 participants