Skip to content

Conversation

bwook00
Copy link

@bwook00 bwook00 commented Aug 7, 2025

What does this PR do?

Keyword Search

keyword search is based on chroma cookbook

Delete Chunk

I added delete chunk function

Hybrid Search

Hybrid Search is not yet officially supported by Chroma chroma issue

This issue is related #3008 but I don't know this is end of issue
So, If I have to do something more, feel free to comment!

close #3008

Test Plan

I add test code and run by below command

pytest tests/unit/providers/vector_io/remote/test_chroma.py -v -s --tb=long --disable-warnings --asyncio-mode=auto 

@meta-cla meta-cla bot added the CLA Signed This label is managed by the Meta Open Source bot. label Aug 7, 2025
@bwook00 bwook00 changed the title Implement keyword search and delete_chunk at ChromaDB feat: Implement keyword search and delete_chunk at ChromaDB Aug 7, 2025
@bwook00
Copy link
Author

bwook00 commented Aug 12, 2025

@franciscojavierarceo If you have time, Can you review my PR? 😀

@r3v5
Copy link
Contributor

r3v5 commented Aug 15, 2025

Thanks for your PR! Could you please implement hybrid search for Chroma DB too?

I have introduced Reranker class for that in #3064. This PR will be merged soon hence you will be able to use generic reranker infrastructure.

@bwook00
Copy link
Author

bwook00 commented Aug 18, 2025

@r3v5 Hi, Thanks for your review!

I want to check something.

We have both vector and keyword search method for ChromaDB.
Although not officially supported by Chroma, is it correct that implement hybrid search using the Reranker class?
(like remote/vector_io/pgvector/pgvector.py file, as described in #3064?)

@r3v5
Copy link
Contributor

r3v5 commented Aug 18, 2025

@r3v5 Hi, Thanks for your review!

I want to check something.

We have both vector and keyword search method for ChromaDB. Although not officially supported by Chroma, is it correct that implement hybrid search using the Reranker class? (like remote/vector_io/pgvector/pgvector.py file, as described in #3064?)

Yes

@bwook00
Copy link
Author

bwook00 commented Aug 18, 2025

@r3v5 Thank you for your quick reply.

I implement hybrid search.
If I add something more, feel free to comment me!

@r3v5
Copy link
Contributor

r3v5 commented Aug 19, 2025

@r3v5 Thank you for your quick reply.

I implement hybrid search. If I add something more, feel free to comment me!

Hi @bwook00! Nice work. One suggestion, don't add explicitly Reranker code to your commits since it was already introduced in another PR. Just wait for #3064 merged (already got approved and received lgtm) and then pull from origin main and then do rebase from main hence you will be able to use Reranker out of the box in Llama Stack.

@bwook00
Copy link
Author

bwook00 commented Aug 20, 2025

@r3v5 Okay! I'm going to wait for #3064 merged.
(If it gets merged, I'd really appreciate it if you could mention me once😄)

Thank you for your quick and kind review!

@r3v5
Copy link
Contributor

r3v5 commented Aug 24, 2025

@r3v5 Thank you for your quick reply.
I implement hybrid search. If I add something more, feel free to comment me!

Hi @bwook00! Nice work. One suggestion, don't add explicitly Reranker code to your commits since it was already introduced in another PR. Just wait for #3064 merged (already got approved and received lgtm) and then pull from origin main and then do rebase from main hence you will be able to use Reranker out of the box in Llama Stack.

The name of the class Reranker switched to WeightedInMemoryAggregator, the code directory is the same

@bwook00
Copy link
Author

bwook00 commented Aug 26, 2025

@r3v5 Thank you for your quick reply.
I implement hybrid search. If I add something more, feel free to comment me!

Hi @bwook00! Nice work. One suggestion, don't add explicitly Reranker code to your commits since it was already introduced in another PR. Just wait for #3064 merged (already got approved and received lgtm) and then pull from origin main and then do rebase from main hence you will be able to use Reranker out of the box in Llama Stack.

The name of the class Reranker switched to WeightedInMemoryAggregator, the code directory is the same

Okay! I'm going to wait for #3064 merged.

Copy link
Contributor

Choose a reason for hiding this comment

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

Please rebase your PR to exclude llama_stack/providers/utils/vector_io/vector_utils.py from your PR. Rebase from main since #3064 was merged

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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement v1/vector_stores/{vector_store_id}/search for Chroma
2 participants