Skip to content
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

Weaviate: make client an external param to WeaviateDocumentStore #471

Open
hsm207 opened this issue Feb 22, 2024 · 3 comments
Open

Weaviate: make client an external param to WeaviateDocumentStore #471

hsm207 opened this issue Feb 22, 2024 · 3 comments
Labels
feature request Ideas to improve an integration integration:weaviate

Comments

@hsm207
Copy link
Contributor

hsm207 commented Feb 22, 2024

Is your feature request related to a problem? Please describe.
It is the responsibility of the user to make sure the client connections are closed. However, the client is created inside WeaviateDocumentStore so user may forget this step.

Describe the solution you'd like
Remove all the client related config e.g. url and replace it with a param called client: weaviate.WeaviateClient.
The user will initialize the client outside WeaviateDocumentStore according to their preference and then pass it to WeaviateDocumentStore.

Describe alternatives you've considered
Expect user will remember to run WeaviateDocumentStore(...)._client.close()

Additional context
See official docs about closing connections

@hsm207 hsm207 added the feature request Ideas to improve an integration label Feb 22, 2024
@silvanocerza
Copy link
Contributor

This is not feasible.

All Document Stores and Components must be serialisable to JSON, in turn all their inputs must be serialisable too.
If we pass the Weaviate Client in the constructor the WeaviateDocumentStore won't be able to support serialization in any case.

I would suggest implementing __del__ if we need to explicitly close the connection.

@hsm207
Copy link
Contributor Author

hsm207 commented Feb 22, 2024

__del__ isn't a destructor.

If it is not feasible, then I'd rather we document to tell users to run WeaviateDocumentStore(...)._client.close() when they are done. Otherwise, they might encounter weird memory leak bugs.

@silvanocerza
Copy link
Contributor

I know it's not a destructor but it's called when the garbage collection runs for that object. If written properly it can safely close an existing connection. So in my book it's good enough as a failsafe in case the user forgets to explictly close the client or just doesn't want to.

Also I wouldn't tell the user to access the _client field as it's meant to be part of the private interface of the Document Store, we could add an explicit WeaviateDocumentStore.close_connection() though.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request Ideas to improve an integration integration:weaviate
Projects
None yet
Development

No branches or pull requests

3 participants