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

Ollama add OkhttpClient inject #911

Merged
merged 5 commits into from May 7, 2024

Conversation

Martin7-1
Copy link
Contributor

@Martin7-1 Martin7-1 commented Apr 9, 2024

Context

see #586

Change

Add OkhttpClient injection in Ollama models

Checklist

Before submitting this PR, please check the following points:

  • I have added unit and integration tests for my change
  • All unit and integration tests in the module I have added/changed are green
  • All unit and integration tests in the core and main modules are green
  • I have added/updated the documentation
  • I have added an example in the examples repo (only for "big" features)
  • I have added my new module in the BOM (only when a new module is added)

Checklist for adding new embedding store integration

  • I have added a {NameOfIntegration}EmbeddingStoreIT that extends from either EmbeddingStoreIT or EmbeddingStoreWithFilteringIT

@Martin7-1 Martin7-1 changed the title add OkhttpClient inject Ollama add OkhttpClient inject Apr 12, 2024
@langchain4j langchain4j added the P2 High priority label Apr 23, 2024
Copy link
Owner

@langchain4j langchain4j left a comment

Choose a reason for hiding this comment

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

Hi @Martin7-1 thanks a lot!

Please avoid adding OkHttpClient to Ollama*Model API as we plan to move away from okhttp pretty soon.

Instead, I would propose to allow the user to do the following (we already have this in OpenAiChatModel):

OllamaChatModel.builder()
                ...
                .defaultHeaders(Map.of("Authorization", "Bearer ..."))
                .build();

WDYT?

@Martin7-1
Copy link
Contributor Author

Hi @Martin7-1 thanks a lot!

Please avoid adding OkHttpClient to Ollama*Model API as we plan to move away from okhttp pretty soon.

Instead, I would propose to allow the user to do the following (we already have this in OpenAiChatModel):

OllamaChatModel.builder()
                ...
                .defaultHeaders(Map.of("Authorization", "Bearer ..."))
                .build();

WDYT?

@langchain4j I think it's a good idea! Will change soon.

@Martin7-1
Copy link
Contributor Author

@langchain4j Hi! I've change it without OkhttpClient in constructor. Please have a look. BTW, there seems no official docker image with Auth, but I found some community docker image which implement it with Caddy, such as ollama-auth and ollama_proxy_server. Maybe we need to choose community docker image to test custom headers. But it seems not a good way due to consistency...WDYT? Do you have any idea to test it with official docker image?

Copy link
Owner

@langchain4j langchain4j left a comment

Choose a reason for hiding this comment

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

@Martin7-1 thank you!

@langchain4j
Copy link
Owner

@Martin7-1 regarding testing, we can do manual testing here if there is no easy way to automate it.

@Martin7-1
Copy link
Contributor Author

@Martin7-1 regarding testing, we can do manual testing here if there is no easy way to automate it.

Ok. I will add some tests.

Copy link
Owner

@langchain4j langchain4j left a comment

Choose a reason for hiding this comment

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

@Martin7-1 thank you a lot!

@langchain4j langchain4j merged commit e36ef57 into langchain4j:main May 7, 2024
6 checks passed
@Martin7-1
Copy link
Contributor Author

@Martin7-1 Could you please add this new parameter to https://github.com/langchain4j/langchain4j-spring/blob/main/langchain4j-ollama-spring-boot-starter/src/main/java/dev/langchain4j/ollama/spring/AutoConfig.java?

@langchain4j ok. I've done it in langchain4j-spring#21.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P2 High priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants