-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
VectorStore for GenAI integrations #2528
Conversation
A documentation preview will be available soon. Request a new doc build by commenting
If your PR continues to fail for an unknown reason, the doc build pipeline may be broken. Elastic employees can check the pipeline status here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this draft! I've reviewed with an eye towards the big things that could be improved to make this feel native in the Python client. I've not looked at the detail of the code, since I don't expect to bring value here.
The big question is going to be the name of this module and its location in the client.
- I don't think
store
works here: we have to communicate that this is an LLM/GenAI thing. I would thus call itvectorstore
orvector_store
. (Indeed, according to PEP 8, underscores can be used in the module name if it improves readability.) - Currently, all the manually written code is in
elasticsearch.helpers
which felt like the natural location for this new code initially. But I'm less sure now, given existing helpers are simply functions and this is a lot more code with a lot more state. So I'm tempted to think thatelasticsearch.vector_store
is a better location thanelasticsearch.helpers.vector_store
.
Co-authored-by: Quentin Pradet <[email protected]>
It was called vectorstore for the longest time :) I renamed it because of the BM25 capabilities. But you're right, it's more appealing/familiar to people with vector in the name. |
I share this sentiment (but don't have a strong opinion). |
I think
Since this code consumes the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewed two small things, but please let me know when you need a more complete review.
test_elasticsearch/test_server/test_vectorstore/test_vectorestore copy.py1
Outdated
Show resolved
Hide resolved
d4a84c1
to
543b49f
Compare
543b49f
to
d397982
Compare
@pquentin Can I get your eyes on this again? 👀 Some questions from my side:
|
wait for `semantic_text` to land
👍 reformatting...
I will do this in a follow-up PR because to do it right, I would like create 2 directories _async and _sync like in the application code. That would involve moving existing tests and I would like to keep that out of this PR. |
- Strategy suffix - Sphinx docstrings - add user agent to EmbeddingService - raise ConflictError - various cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! We looked at this together and found a few really minor formatting things to change. I'll test that quickly and then we can merge! 🎉
test_elasticsearch/test_server/test_helpers_vectorstore/_test_utils.py
Outdated
Show resolved
Hide resolved
raise ValueError("specify a query_vector") | ||
|
||
if self.distance is DistanceMetric.COSINE: | ||
similarityAlgo = ( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: We could consider unit testing all those combinations with ReferenceJson and help from GitHub Copilot to generate the tests. But then up to you as this is also quite simple and the tests are going to look a lot like the original code anyway.
Maybe focus on the raised exceptions, using code like with pytest.raises(match="specify a query_vector"): ...
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Testing the error states now. Assertions for the ES queries are part of the integration tests.
test_elasticsearch/test_server/test_helpers_vectorstore/test_vectorstore.py
Outdated
Show resolved
Hide resolved
4f28761
to
f32ceb2
Compare
e00d182
to
d27f9f8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! LGTM.
* ElasticsearchStore * Update elasticsearch/store/_utilities.py Co-authored-by: Quentin Pradet <[email protected]> * rename; depend on client; async only * generate _sync files * add cleanup step for _sync generation * fix formatting * more linting fixes * batch embedding call; infer num_dimensions * revert accidental changes * keep field names only in store; apply metadata mappings in store * fix typos in file names * use `elasticsearch_url` fixture; create conftest.py * export relevant classes * remove Semantic strategy wait for `semantic_text` to land * es_query is sync * async strategies * cleanup old file * add docker-compose service with model deployment * optional dependencies for MMR * only test sync parts * cleanup unasync script * nox: install optional deps * fix tests with requests remembering Transport * fix numpy typing * add user agent default argument * move to `elasticsearch.helpers.vectorstore` * use Protocol over ABC * revert Protocol change because Python 3.7 * address PR feedback: - Strategy suffix - Sphinx docstrings - add user agent to EmbeddingService - raise ConflictError - various cleanup * improve docstring * fix metadata mappings issue * address PR feedback * add error tests for strategies * canonical names, keyword args only * fix sparse vector strategy bug (duplicate `size`) * all wildcard deletes in compose ES --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit c2b0ca3)
* ElasticsearchStore * Update elasticsearch/store/_utilities.py Co-authored-by: Quentin Pradet <[email protected]> * rename; depend on client; async only * generate _sync files * add cleanup step for _sync generation * fix formatting * more linting fixes * batch embedding call; infer num_dimensions * revert accidental changes * keep field names only in store; apply metadata mappings in store * fix typos in file names * use `elasticsearch_url` fixture; create conftest.py * export relevant classes * remove Semantic strategy wait for `semantic_text` to land * es_query is sync * async strategies * cleanup old file * add docker-compose service with model deployment * optional dependencies for MMR * only test sync parts * cleanup unasync script * nox: install optional deps * fix tests with requests remembering Transport * fix numpy typing * add user agent default argument * move to `elasticsearch.helpers.vectorstore` * use Protocol over ABC * revert Protocol change because Python 3.7 * address PR feedback: - Strategy suffix - Sphinx docstrings - add user agent to EmbeddingService - raise ConflictError - various cleanup * improve docstring * fix metadata mappings issue * address PR feedback * add error tests for strategies * canonical names, keyword args only * fix sparse vector strategy bug (duplicate `size`) * all wildcard deletes in compose ES --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit c2b0ca3)
* ElasticsearchStore * Update elasticsearch/store/_utilities.py Co-authored-by: Quentin Pradet <[email protected]> * rename; depend on client; async only * generate _sync files * add cleanup step for _sync generation * fix formatting * more linting fixes * batch embedding call; infer num_dimensions * revert accidental changes * keep field names only in store; apply metadata mappings in store * fix typos in file names * use `elasticsearch_url` fixture; create conftest.py * export relevant classes * remove Semantic strategy wait for `semantic_text` to land * es_query is sync * async strategies * cleanup old file * add docker-compose service with model deployment * optional dependencies for MMR * only test sync parts * cleanup unasync script * nox: install optional deps * fix tests with requests remembering Transport * fix numpy typing * add user agent default argument * move to `elasticsearch.helpers.vectorstore` * use Protocol over ABC * revert Protocol change because Python 3.7 * address PR feedback: - Strategy suffix - Sphinx docstrings - add user agent to EmbeddingService - raise ConflictError - various cleanup * improve docstring * fix metadata mappings issue * address PR feedback * add error tests for strategies * canonical names, keyword args only * fix sparse vector strategy bug (duplicate `size`) * all wildcard deletes in compose ES --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit c2b0ca3) Co-authored-by: Max Jakob <[email protected]>
* ElasticsearchStore * Update elasticsearch/store/_utilities.py Co-authored-by: Quentin Pradet <[email protected]> * rename; depend on client; async only * generate _sync files * add cleanup step for _sync generation * fix formatting * more linting fixes * batch embedding call; infer num_dimensions * revert accidental changes * keep field names only in store; apply metadata mappings in store * fix typos in file names * use `elasticsearch_url` fixture; create conftest.py * export relevant classes * remove Semantic strategy wait for `semantic_text` to land * es_query is sync * async strategies * cleanup old file * add docker-compose service with model deployment * optional dependencies for MMR * only test sync parts * cleanup unasync script * nox: install optional deps * fix tests with requests remembering Transport * fix numpy typing * add user agent default argument * move to `elasticsearch.helpers.vectorstore` * use Protocol over ABC * revert Protocol change because Python 3.7 * address PR feedback: - Strategy suffix - Sphinx docstrings - add user agent to EmbeddingService - raise ConflictError - various cleanup * improve docstring * fix metadata mappings issue * address PR feedback * add error tests for strategies * canonical names, keyword args only * fix sparse vector strategy bug (duplicate `size`) * all wildcard deletes in compose ES --------- Co-authored-by: Quentin Pradet <[email protected]> (cherry picked from commit c2b0ca3) Co-authored-by: Max Jakob <[email protected]>
We want to add a higher-level abstraction for using Elasticsearch in Python. The main motivation is to make integrations into 3rd party GenAI libraries easier and give users a handful of options to choose from without diving into the Query DSL. Ideally the abstraction also works for direct users of the elasticsearch-py client.
Integrations
TODO
helpers.vectorstore
test it once there is a Docker image with it orremove itConsider limiting values for--> CPU-intensive MMR function is now sync onlyk
andnum_candidates
for (async) MMR-serverless
package --> nothing here, @pquentin will take care of this.