Skip to content

Conversation

@marcelklehr
Copy link
Member

it doesn't seem to work

it doesn't seem to work

Signed-off-by: Marcel Klehr <mklehr@gmx.net>
@marcelklehr marcelklehr requested a review from kyteinsky July 4, 2025 12:08
Copy link
Contributor

@kyteinsky kyteinsky left a comment

Choose a reason for hiding this comment

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

the test fail seems to be coming from the llama_cpp_python library, should not matter here.

@marcelklehr
Copy link
Member Author

the test fail seems to be coming from the llama_cpp_python library,

Is that a bug in the test setup then or in the backend?

@kyteinsky
Copy link
Contributor

Is that a bug in the test setup then or in the backend?

Both in a way. The error comes from the backend and in the test setup, we install the python deps from the requirements.txt (https://github.com/nextcloud/context_chat/actions/runs/16073429868/workflow?pr=137#L178) file which is non-versioned. The versioning load is taken by the docker container where a particular version of llama_cpp_python pre-built package is installed and for the other packages, it the latest version.
This is tested and locked in docker until we change any packages or deliberately invalidate the docker cache to update the packages. So it should not be an issue since the current prod version works.

But yeah here we can't test it right now with this bug, might be worth to pin the package in ccb for a while.

@marcelklehr
Copy link
Member Author

mmh, can we take a leaf out of the book of llm2? Cause it seems to work there?

@kyteinsky
Copy link
Contributor

removing a dependency from the poetry is not straightforward though, it modifies the lock file. We need to do this to ship one lock file for the manual install in CI and remove it for the docker build. Even after we switch to building llama-cpp-python, it should be in a different build context in the docker container so we ship the lighter runtime image of cuda only, landing us in the same position.
it seems like the issue was fixed upstream and a new release is available. Let's see if the tests are fixed.

@marcelklehr marcelklehr merged commit 65b46db into main Jul 14, 2025
16 of 19 checks passed
@marcelklehr marcelklehr deleted the fix/dont-remove-prefix-from-paths branch July 14, 2025 13:10
@kyteinsky kyteinsky mentioned this pull request Jul 21, 2025
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.

3 participants