-
Notifications
You must be signed in to change notification settings - Fork 27
Add asynchronous support to WeaviateVectorStore through optional WeaviateAsyncClient #233
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
base: main
Are you sure you want to change the base?
Conversation
|
@hsm207 Could you take a look at this? |
|
@BobMerkus There are conflicts that need to be resolved. |
|
Hey @BobMerkus, Appreciate the contribution! Just FYI, I’m not with Weaviate anymore so I have zero say on this integration these days. Honestly, I don’t even know who’s running the show or what the maintenance policy is for this integration now. You’ll probably get a quicker answer if you ping the Weaviate community manager in the Weaviate Community Slack channel. Good luck! 🚀 |
adf54bd to
1c238cf
Compare
Have rebased the branch, should be fixed now |
Hi @hsm207, Thanks for your response! No worries, this is not really a priority to me but it seems like a nice feature to support. Thanks for the suggestion, I'll check the Weaviate Community Slack channel. Good luck to you as well 👾 |
|
@BobMerkus Thanks for the contribution! Could you please update your branch with the latest changes from main so that the checks can run and we can proceed with the review? |
1c238cf to
1937973
Compare
@DhruvGorasiya I have rebased the branch, should be ready for CI checks now. Please let me know if any changes are required. |
1937973 to
2f477a6
Compare
Asynchronous support for WeaviateVectorStore
This PR adds adds an optional WeaviateAsyncClient to
WeaviateVectorStore.client_asyncin order to fully support the asynchronous methods implemented by the base VectorStore.Closes: #207
Description
There is no proper support for the asynchronous methods on the
WeaviateVectorStore, as mentioned in Async client support #207 . TheVectorStorecurrently uses therun_in_executorlangchain utility on all the synchronous methods of theWeaviateVectorStore, leading to a lot of resource warnings when used inside async context. This implementation should solve these issues and lead to overall efficiency improvements when used inside async context. The async tests run about 2x as fast compared to the original tests (48s vs.23s for me), even though there are more of them.I've attempted to implement this with backwards compatibility, so when no
client_asyncis passed along the defaultclient, the methods still call the synchronous methods in the same way the base vector store currently does. Overall the test coverage has been increased to 98.66% through mocking and error asserts.Please let me know if any changes are required, there still seems to be some room for improvements regarding shared logic between the sync/async implementation. Another approach would be to support
client: WeaviateClient | WeaviateAsyncClientand decide which implementation to use based on the type of client supplied. I took a brief look at this, but it requires a lot of refactoring of the current implementation. This approach will arguably result in cleaner and more efficient code as it avoids the need to pass a sync client along with the async client.