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

ElasticsearchStore async methods implementation #47

Open
wants to merge 12 commits into
base: main
Choose a base branch
from

Conversation

giacbrd
Copy link
Contributor

@giacbrd giacbrd commented Oct 3, 2024

The ElasticsearchStore class inherits the async methods implemented in VectorStore, that are actually not concurrent. The official Elasticsearch library now implements a helper for vector search, with both a sync and an async version, thanks to the developers of langchain-elastic!

The goal of this PR is to better integrate the helper in ElasticsearchStore, so that its async methods can be overridden with proper implementations.

I have considered different approaches, but there is critical difference in the design of the helper and the VectorStore class. The first has two completely independent classes for sync/async, the second exposes sync and async methods in the same abstraction. I am trying to follow the simplest approach possible, at the cost of:

  • Creating two Elasticsearch clients in ElasticsearchStore, even when only one of sync or async approach is used
  • Reducing the "availability" of the async methods, in order to make ElasticsearchStore robust on legacy code

Hope to receive feedback and improvement hints

@giacbrd giacbrd marked this pull request as ready for review October 8, 2024 11:05
@giacbrd
Copy link
Contributor Author

giacbrd commented Oct 8, 2024

The proposed solution makes use of two new parameters on the constructor of ElasticsearchStore. Their combination with other parameters it is not "linear", but the new parameters are unset by default. They activate the async implementations through the AsyncElasticsearch client and AsyncVectorStore. I think that async activation should be explicitly set when needed, because the use of AsyncElasticsearch can have some side effects on the process, that one must manage, e.g. closing the async connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant