-
Notifications
You must be signed in to change notification settings - Fork 105
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
Add Milvus database compatibility with the RAG recipe #334
Conversation
@Shreyanand you need to sign your commits. git commit -a --amend -s |
@MichaelClifford PTAL |
@Shreyanand You will also need to update |
be21e2d
to
595054c
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did not have time to actually run the rag and make sure it works, I just focused on the DB parts and setting up the Makefile
. I defer to Michael for the actual Rag and DB population bits.
Needs a rebase. |
Also needs DCO |
Signed-off-by: Shreyanand <[email protected]> Co-authored-by: Michael Clifford <[email protected]> Co-authored-by: greg pereira <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm, but 2 tiny nits
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Commit suggestions dont sign with DCO, major bummer. You will have to rebase those commit with signing. Personally I use:
Alternatively we can just set DCO to pass, but I would prefer stuff stay signed if possible. |
Co-authored-by: Gregory Pereira <[email protected]> Update recipes/natural_language_processing/rag/app/rag_app.py Co-authored-by: Gregory Pereira <[email protected]> Update Readme and add review comments Signed-off-by: Shreyanand <[email protected]>
@@ -4,7 +4,7 @@ This demo provides a simple recipe to help developers start to build out their o | |||
|
|||
There are a few options today for local Model Serving, but this recipe will use [`llama-cpp-python`](https://github.com/abetlen/llama-cpp-python) and their OpenAI compatible Model Service. There is a Containerfile provided that can be used to build this Model Service within the repo, [`model_servers/llamacpp_python/base/Containerfile`](/model_servers/llamacpp_python/base/Containerfile). | |||
|
|||
In order for the LLM to interact with our documents, we need them stored and available in such a manner that we can retrieve a small subset of them that are relevant to our query. To do this we employ a Vector Database alongside an embedding model. The embedding model converts our documents into numerical representations, vectors, such that similarity searches can be easily performed. The Vector Database stores these vectors for us and makes them available to the LLM. In this recipe we will use [chromaDB](https://docs.trychroma.com/) as our Vector Database. | |||
In order for the LLM to interact with our documents, we need them stored and available in such a manner that we can retrieve a small subset of them that are relevant to our query. To do this we employ a Vector Database alongside an embedding model. The embedding model converts our documents into numerical representations, vectors, such that similarity searches can be easily performed. The Vector Database stores these vectors for us and makes them available to the LLM. In this recipe we can use [chromaDB](https://docs.trychroma.com/) or [Milvus](https://milvus.io/) as our Vector Database. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@Shreyanand Since the recipe is defined in the ai-lab.yaml file and should only have One vectorDB. I think we should only reference Milvus here and move any mentions of chroma to a chroma readme. Also you should update the ai-lab.yaml file to reference this DB instead.
import os | ||
|
||
model_service = os.getenv("MODEL_ENDPOINT","http://0.0.0.0:8001") | ||
model_service = f"{model_service}/v1" | ||
chunk_size = os.getenv("CHUNK_SIZE", 150) | ||
embedding_model = os.getenv("EMBEDDING_MODEL","BAAI/bge-base-en-v1.5") | ||
vdb_vendor = os.getenv("VECTORDB_VENDOR", "chromadb") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lets make milvus the new default.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should follow the same format as the model_servers/
remove this doc, and add a README under each vectorDB directory with all the info to run it.
@@ -0,0 +1,2 @@ | |||
FROM docker.io/milvusdb/milvus:master-20240426-bed6363f |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FROM docker.io/milvusdb/milvus:master-20240426-bed6363f | |
FROM docker.io/milvusdb/milvus:master-20240516-5b27a0cd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You should also add to the readme the step needed to run the application with the new vectorDB. I think the Env Variables are a little different now.
This PR adds the following changes:
@MichaelClifford I need to add documentation around this but I have tested it on my system and it works. What should be the next things to check to make sure this is polished?