langchaingo icon indicating copy to clipboard operation
langchaingo copied to clipboard

feat(tests): use testcontainers-go's Ollama module for running tests with Ollama

Open mdelapenya opened this issue 1 year ago • 4 comments

  • fix(llms:ollama): be more explicit in the test propmt
  • chore(llms:ollama): support running ollama tests with testcontainers-go's Ollama module
  • chore(stores:milvus): support running milvus tests with testcontainers-go's Ollama module
  • chore(tests): use require as much as possible
  • fix(stores:redis): update docs count in tests
  • chore(stores:redis): support running Redis tests with testcontainers-go's Ollama module
  • fix(stores:weaviate): use proper query attributes when creating test store
  • chore(stores:weaviate): refine assertion in test
  • chore(stores:weaviate): support running Weaviate tests with testcontainers-go's Ollama module

PR Checklist

  • [x] Read the Contributing documentation.
  • [x] Read the Code of conduct documentation.
  • [x] Name your Pull Request title clearly, concisely, and prefixed with the name of the primarily affected package you changed according to Good commit messages (such as memory: add interfaces for X, Y or util: add whizzbang helpers).
  • [x] Check that there isn't already a PR that solves the problem the same way to avoid creating a duplicate.
  • [x] Provide a description in this PR that addresses what the PR is solving, or reference the issue that it solves (e.g. Fixes #123).
  • [x] Describes the source of new concepts.
  • [ ] References existing implementations as appropriate.
  • [ ] Contains test coverage for new functions.
  • [x] Passes all golangci-lint checks.

What does this PR do?

It uses Testcontainers for Go's Ollama module enabling the execution of the integration tests that interact with a local LLM. Before this PR, tests are skipped in the case 1) there is no OPENAI key in the env, 2) there is not OLLAMA_URL/OLLAMA_MODEL in the env.

This PR bypasses that limitation using the Ollama module in a container, using a small LLM like llama3.2:1b, which fits easily in CPUs instead of GPUs.

To run the docker images for Ollama, we are using mdelapenya's Docker Hub, where I push already backed Ollama images with the models already loaded, so the build times gets reduced only to the pull time. Please check https://hub.docker.com/r/mdelapenya/llama3.2, and the repository that generates the Docker images: https://github.com/mdelapenya/dockerize-ollama-models/

This PR tries to honour the current approach: if the env vars exist, they will be used, so users can test with their own local environment if needed.

Why is it important?

Adds the ability to test locally, avoiding the skipping of lots of tests at CI time.

Other concers

Weaviate tests are failing for me with the current model I chose, not sure if with OpenAI it will work as expected. Also they fail in the chains call, as the relevant documents seem to be not used in the chain call, therefore, the model always responds with "I don't know", making the tests to fail. Can anybody check with OpenAI?

mdelapenya avatar Nov 29 '24 17:11 mdelapenya

@codefromthecrypt @devalexandre I think this would close https://github.com/tmc/langchaingo/issues/994. Could you please take a look?

mdelapenya avatar Nov 29 '24 17:11 mdelapenya

on the benefits of this, not just do we skip less tests, but we also understand failure cases common for local model serving runtimes. This includes aspects about the service (e.g. ollama only recently supports streaming with tool calls) and also open models.

I use all-minilm:33m and qwen2.5:0.5B in simple tests, or the normal size qwen2.5 if the test is a little hard to control.

Also, there's an alternative to testcontainers-go which is running ollama directly. While that's not the aim of this PR it has some pros and cons to it, and possibly something to note as an alternative considered.

codefromthecrypt avatar Dec 02 '24 06:12 codefromthecrypt

@codefromthecrypt you are probably interested in https://github.com/testcontainers/testcontainers-go/issues/2906 😉

mdelapenya avatar Dec 02 '24 09:12 mdelapenya

@mdelapenya thanks I subscribed. Not sure if they will accept it or prior art around native process management. However, it does feel like it can workaround the lifecycle issue if accepted.

codefromthecrypt avatar Dec 06 '24 04:12 codefromthecrypt