-
-
Notifications
You must be signed in to change notification settings - Fork 9
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
refactor: ChromaDB migration #20
Conversation
Wow excellent work. I'll review this today and look at getting this deployed on our azure instance. I have some terraform that should make that a quick process. |
Hey @jpmcb. The We are currently using the CPU provider. repo-query/src/embeddings/onnx.rs Lines 22 to 24 in afc4d19
Moving forward, if needed, we can try some of the others if the hardware support is available. |
GPU support would be cool, but right now, for the way we have it deployed through Container Apps, there aren't GPU instances available. This is essentially their serverless container runtime and doesn't involve alot of infrastructure. We'd need to look at a more intensive deployment strategy, maybe through Container Instances or Azure's Kubernetes service, that would let us deploy GPU instances. For now, until we need to start scaling up in that way, I think the Container Apps deployment with CPU |
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.
Looks like a pretty clean swap. Really appreciate you kicking off the chroma client work as well.
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.
I was having some snags running the image locally on arm64 so i opened: chroma-core/chroma#972
But I was able to rebuild the image and this works well - nice work!
Performance seems slightly worse than qdrant locally for me. But will have to try that on real hardware.
fn insert_repo_embeddings(&self, repo: RepositoryEmbeddings) -> Result<()>; | ||
|
||
async fn get_relevant_files( | ||
fn get_relevant_file_paths( | ||
&self, | ||
repository: &Repository, | ||
query_embeddings: Embeddings, | ||
limit: usize, | ||
) -> Result<RepositoryFilePaths>; | ||
|
||
async fn get_file_paths(&self, repository: &Repository) -> Result<RepositoryFilePaths>; | ||
fn get_file_paths(&self, repository: &Repository) -> Result<RepositoryFilePaths>; | ||
|
||
async fn is_indexed(&self, repository: &Repository) -> Result<bool>; | ||
fn is_indexed(&self, repository: &Repository) -> Result<bool>; |
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.
Makes sense we'd remove async
since before we were just await
on each call anyways 👍🏼
OPENAI_API_KEY= #REQUIRED | ||
WEBSERVER_PORT= #REQUIRED | ||
CHROMA_URL= #Defaults to http://localhost:8000 |
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.
Where does this get used to set the Chroma client? I'm not seeing any other references to this in the code. Is it being consumed by the Chroma rust client?
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.
Yes. The Chromadb-rs lib uses the CHROMA_URL
env by default.
Line 21 in 37bd468
let client = ChromaClient::new(Default::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.
There are a few non-starters to getting this merged:
- [Feature Request]: Remove re-build during docker container entry chroma-core/chroma#1002
- Rebuilding during container entry causes quite a lengthy gap for the repo-query service to be ready. Ideally, since we're deploying all this in a conainer app environment, we'd not have to wait for container bits to be rebuilt
- [Feature Request]: Provide a precompiled version of chroma-hnswlib chroma-core/chroma#993
- If we had a precompiled form of the chroma database that we could drop into our own container, that would alleviate this pain, so we may want to watch that issue as well
- [Bug]:
ghcr.io/chroma-core/chroma
does not support arm64 architecture chroma-core/chroma#972- The team is working on multi-arch builds and that will help alot with local development for our contributor community
Until then, we should hold this migration and stick with Qdrant since it's working out of the box in our container environments.
Description
This PR intends to migrate the project to use ChromaDB by implementing the
RepositoryEmbeddingsDB
trait for the ChromaDB client.repo-query/src/db/mod.rs
Lines 9 to 22 in 117a933
The README has been updated with the new local development steps
The ChromaDB service has been added to the
docker-compose.yaml
file.What type of PR is this? (check all applicable)
Related Tickets & Documents
Resolves #19
Mobile & Desktop Screenshots/Recordings
Added tests?
Added to documentation?