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

optional custom index settings in vectorstore #45

Closed
wants to merge 10 commits into from

Conversation

giacbrd
Copy link
Contributor

@giacbrd giacbrd commented Sep 17, 2024

Hi, I have simply added a parameter to the ElasticsearchStore class in order to set default settings at index creation, that is made by the VectorStore helper in the Elasticsearch library. It is just arguments traversing calls, there is not much to test. There is also another minor fix in the doctring.

@giacbrd
Copy link
Contributor Author

giacbrd commented Nov 19, 2024

The main benefit of this parameter is to pre-configure the index when using the Elasticsearch vector store, which implicitly creates an index when first documents are added. One can, for example, set a correct number of replicas for his cluster, so that he has not to change it manually after creation, with all the waste of computational effort this implies

* async support

* format

* fixed unit test failures

* sort imports

* more code reformat

* typing fixes

* typing fixes

* one more reformat

* typing fixes

* docs

* use sed to change get_messages() to messages
@miguelgrinberg
Copy link
Collaborator

miguelgrinberg commented Nov 27, 2024

@giacbrd I had the intention to merge this PR, but it seems I'm not allowed to make changes on your PR branch. Would you like to do the rebase yourself, or should I create a separate PR with the changes? The changes now need to be made to the langchain-elastic/libs/elasticsearch/langchain_elasticsearch/_async/vectorstores.py file. Then run make run_unasync to generate the sync version, and finally run make format to make sure the source code is correctly formatted.

@miguelgrinberg
Copy link
Collaborator

@giacbrd Thanks, but something does not look right. This PR should be tiny, somehow it is now including all the async changes I've merged the other day, which are already merged. Maybe it'll be easier to start a fresh PR? You can fix this one if you like, but it is tricky. One way would be with a rebase from main, which is what I suggested above.

@giacbrd
Copy link
Contributor Author

giacbrd commented Nov 28, 2024

hi @miguelgrinberg yes I am working on it, I have probably messed up something by combining the wrong git commands. The code is fine, but I am still not solving the git mess. I am going to open a new PR

@giacbrd giacbrd closed this Nov 28, 2024
@giacbrd giacbrd deleted the index-settings-param branch November 28, 2024 23:21
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.

4 participants