fix: Remove unnecessary connection test and add close/close_async methods to WeaviateDocumentStore#2891
fix: Remove unnecessary connection test and add close/close_async methods to WeaviateDocumentStore#2891maxdswain wants to merge 5 commits intodeepset-ai:mainfrom
close/close_async methods to WeaviateDocumentStore#2891Conversation
maxdswain
changed the title
WeaviateDocumentStoreclose/close_async methods to WeaviateDocumentStore
github-actions
bot
added
the
type:documentation
anakin87
left a comment
anakin87
left a comment
There was a problem hiding this comment.
Thank you for the contribution!
I left some comments.
| def close(self) -> None: | ||
| """ | ||
| Close the synchronous Weaviate client connection. | ||
| """ |
There was a problem hiding this comment.
First of all, in this method, I'd set self._collection = None
| self._client = None | ||
|
|
||
| async def close_async(self) -> None: | ||
| """ |
There was a problem hiding this comment.
let's set self. _async_collection = None here
| yield store | ||
| store.client.collections.delete(collection_settings["class"]) | ||
| store.close() | ||
|
|
There was a problem hiding this comment.
Let's add a test_close test, similar in spirit to this Pinecone test
| ) | ||
| yield store | ||
| store.client.collections.delete(collection_settings["class"]) | ||
| store.close() |
There was a problem hiding this comment.
A lot of the async tests call write_documents which uses the sync client so I think it is yes.
| store.client.collections.delete(collection_settings["class"]) | ||
| store.close() | ||
| await store.close_async() | ||
|
|
There was a problem hiding this comment.
Let's add a test_close_async test
|
Thanks for the feedback and review @anakin87, it's much appreciated I'll make sure to be more thorough in the tests I add in future PRs. I believe I have addressed all of your comments now :) |
2 participants
Related Issues
Proposed Changes:
self._client.connect()/self._async_client.connect()is called allowing bothself._client.collections.list_all(simple=True)andself._async_client.collections.list_all(simple=True)to be removed.closeandasync_closemethods toWeaviateDocumentStoremirroring what is done inValkeyDocumentStoreandPineconeDocumentStore.How did you test it?
Modified existing integration tests.
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:.