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

Re-write the Ollama dev service #1148

Merged
merged 1 commit into from
Dec 11, 2024

Conversation

edeandrea
Copy link
Collaborator

@edeandrea edeandrea commented Dec 10, 2024

The current implementation of the Ollama dev service is a hot mess. It doesn't work properly. Therefore I have done the following things:

  1. Merged the quarkus-langchain4j-ollama-devservices artifact into the quarkus-langchain4j-ollama-deployment artifact.
  2. The dev service is aware of whether or not there is a local Ollama service running on port 11434. If so, it doesn't do anything.
  3. If there isn't a local Ollama service running on port 11434 then it will start an ollama/ollama:latest container on a random port. It will also expose a few configuration properties with values of the host & port the container is running on, which can be used by applications to manually configure things if need be.
  4. The container shares the filesystem location with the Ollama client (user.home/.ollama), so any models downloaded by the client or the container are shared with each other, and therefore only need to be downloaded once by one or the other.
  5. The pulling of the required models hasn't changed. The main dev service still uses the Ollama rest api to pull the required models. It is simply passed a URL, which could be the local Ollama client, or it could be a url to a container. It doesn't matter at that point.

The documentation has also been updated to reflect everything.

@edeandrea edeandrea requested a review from a team as a code owner December 10, 2024 15:54

This comment has been minimized.

@edeandrea edeandrea force-pushed the update-ollama-dev-service branch 2 times, most recently from e98aa36 to a0b9be3 Compare December 10, 2024 16:04

This comment has been minimized.

@geoand
Copy link
Collaborator

geoand commented Dec 10, 2024

Thanks a lot for this work @edeandrea.

Just to have a public record what I also said in public to you: I don't plan to spend any time on this whatsoever, so if you believe this solves a use case you have and you are willing to assume the full maintainance of all this Ollama in a container stuff, I'm willing to accept it.

@edeandrea
Copy link
Collaborator Author

Thanks a lot for this work @edeandrea.

Just to have a public record what I also said in public to you: I don't plan to spend any time on this whatsoever, so if you believe this solves a use case you have and you are willing to assume the full maintainance of all this Ollama in a container stuff, I'm willing to accept it.

Sounds good!

@geoand
Copy link
Collaborator

geoand commented Dec 10, 2024

👌

@edeandrea
Copy link
Collaborator Author

@geoand one thought/question I had while working on this - the devservice stuff that you wrote in core/deployment/src/main/java/io/quarkiverse/langchain4j/deployment/devservice (DevServicesOllamaProcessor, OllamaClient, JdkOllamaClient, etc)...

Should those really belong in the core deployment module? Or should those belong in the ollama deployment module?

Whats weird is

- it seems there is devservice stuff thats specific to ollama located in the quarkus.langchain4j.devservices config item? Doesn't this belong nested under .ollama. as well?

I can do this additional cleanup if you think it makes sense. Otherwise I'll leave it as is.

@geoand
Copy link
Collaborator

geoand commented Dec 10, 2024

Should those really belong in the core deployment module?

Yes, because the hope is that at some point Ollama will be able to provide all OpenAI APIs as well

@edeandrea
Copy link
Collaborator Author

Yes, because the hope is that at some point Ollama will be able to provide all OpenAI APIs as well

Got it. I'll leave things as is then.

@geoand
Copy link
Collaborator

geoand commented Dec 10, 2024

👌

@edeandrea edeandrea force-pushed the update-ollama-dev-service branch from 1798403 to 0258cdb Compare December 10, 2024 18:26

This comment has been minimized.

The current implementation of the Ollama dev service is a hot mess. It doesn't work properly. Therefore I have done the following things:

1) Merged the `quarkus-langchain4j-ollama-dev-service` artifact into the `quarkus-langchain4j-ollama-deployment` artifact.
2) The dev service is aware of whether or not there is a local Ollama service running on port `11434`. If so, it doesn't do anything.
3) If there isn't a local Ollama service running on port `11434` then it will start an `ollama/ollama:latest` container on a random port. It will also expose a few configuration properties with values of the host & port the container is running on.
4) The container shares the filesystem location with the Ollama client (`user.home/.ollama`), so any models downloaded by the client or the container are shared with each other, and therefore only need to be downloaded once.
@edeandrea edeandrea force-pushed the update-ollama-dev-service branch from b9e197e to d94639b Compare December 10, 2024 18:29
Copy link

quarkus-bot bot commented Dec 10, 2024

Status for workflow Build (on pull request)

This is the status report for running Build (on pull request) on commit d94639b.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Copy link
Collaborator

@geoand geoand left a comment

Choose a reason for hiding this comment

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

Very nice work!

@geoand geoand merged commit dc1d4ae into quarkiverse:main Dec 11, 2024
64 checks passed
@edeandrea edeandrea deleted the update-ollama-dev-service branch December 11, 2024 12:38
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.

2 participants