Skip to content

chore: Enable keyword search for Milvus inline #3073

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

varshaprasad96
Copy link
Contributor

What does this PR do?

With milvus-io/milvus-lite#294 - Milvus Lite supports keyword search using BM25. While introducing keyword search we had explicitly disabled it for inline milvus. This PR removes the need for the check, and enables inline::milvus for tests.

Test Plan

Run llama stack with inline::milvus enabled:

pytest tests/integration/vector_io/test_openai_vector_stores.py::test_openai_vector_store_search_modes --stack-config=http://localhost:8321 --embedding-model=all-MiniLM-L6-v2 -v
INFO     2025-08-07 17:06:20,932 tests.integration.conftest:64 tests: Setting DISABLE_CODE_SANDBOX=1 for macOS                                        
=========================================================================================== test session starts ============================================================================================
platform darwin -- Python 3.12.11, pytest-7.4.4, pluggy-1.5.0 -- /Users/vnarsing/miniconda3/envs/stack-client/bin/python
cachedir: .pytest_cache
metadata: {'Python': '3.12.11', 'Platform': 'macOS-14.7.6-arm64-arm-64bit', 'Packages': {'pytest': '7.4.4', 'pluggy': '1.5.0'}, 'Plugins': {'asyncio': '0.23.8', 'cov': '6.0.0', 'timeout': '2.2.0', 'socket': '0.7.0', 'html': '3.1.1', 'langsmith': '0.3.39', 'anyio': '4.8.0', 'metadata': '3.0.0'}}
rootdir: /Users/vnarsing/go/src/github/meta-llama/llama-stack
configfile: pyproject.toml
plugins: asyncio-0.23.8, cov-6.0.0, timeout-2.2.0, socket-0.7.0, html-3.1.1, langsmith-0.3.39, anyio-4.8.0, metadata-3.0.0
asyncio: mode=Mode.AUTO
collected 3 items                                                                                                                                                                                          

tests/integration/vector_io/test_openai_vector_stores.py::test_openai_vector_store_search_modes[None-None-all-MiniLM-L6-v2-None-384-vector] PASSED                                                   [ 33%]
tests/integration/vector_io/test_openai_vector_stores.py::test_openai_vector_store_search_modes[None-None-all-MiniLM-L6-v2-None-384-keyword] PASSED                                                  [ 66%]
tests/integration/vector_io/test_openai_vector_stores.py::test_openai_vector_store_search_modes[None-None-all-MiniLM-L6-v2-None-384-hybrid] PASSED                                                   [100%]

============================================================================================ 3 passed in 4.75s =============================================================================================

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 8, 2025
@varshaprasad96
Copy link
Contributor Author

@franciscojavierarceo a quick one. With this we will have feature parity between inline and remote milvus for search modes.

@franciscojavierarceo
Copy link
Collaborator

Nice! I approved the workflow, let's wait for the tests to pass.

@franciscojavierarceo
Copy link
Collaborator

Shouldn't you update pyproject.toml for the latest release? Or do we already have the latest version that supports bm25?

Copy link
Collaborator

@franciscojavierarceo franciscojavierarceo left a comment

Choose a reason for hiding this comment

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

Looks like we must have the latest version. Thanks for the quick turnaround on this!!!

leseb
leseb previously requested changes Aug 8, 2025
Copy link
Collaborator

@leseb leseb left a comment

Choose a reason for hiding this comment

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

We should really have a constraint on the minimal required version for milvus-lite. +1 on #3073 (comment)

@varshaprasad96
Copy link
Contributor Author

@leseb @franciscojavierarceo The reason Milvus-lite version was updated, was because we are locking pymilvus>=2.5 (https://github.com/meta-llama/llama-stack/blob/1677d6bffdf0a002abe3b827c460df4930ee83c8/uv.lock#L1746) and Milvus-lite is a transitive dependency. I've also updated to pin the Milvus-lite version to be sure that we are importing the one which has this change implemented.

Copy link
Contributor

@ChristianZaccaria ChristianZaccaria left a comment

Choose a reason for hiding this comment

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

/lgtm nice!!

@varshaprasad96
Copy link
Contributor Author

@leseb could we get this merged this, please? We would need it for upcoming release.

Copy link
Collaborator

@bbrowning bbrowning left a comment

Choose a reason for hiding this comment

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

The changes look reasonable, self-contained, tested, and feedback looks to be addressed. Thanks!

@franciscojavierarceo franciscojavierarceo dismissed leseb’s stale review August 14, 2025 13:45

Dismissing since Seb's request was addressed. 👍

@franciscojavierarceo franciscojavierarceo added the re-record-tests Spin up ollama, inference and record responses for later use label Aug 14, 2025
@franciscojavierarceo franciscojavierarceo added re-record-tests Spin up ollama, inference and record responses for later use and removed re-record-tests Spin up ollama, inference and record responses for later use labels Aug 14, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Meta Open Source bot. re-record-tests Spin up ollama, inference and record responses for later use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants