-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
feat: Check version of Elasticsearch server and add support for Elasticsearch <= 7.5 #5320
Conversation
Pull Request Test Coverage Report for Build 5543058076
💛 - Coveralls |
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.
Looks good. Minor improvements requested
@@ -106,6 +106,8 @@ def __init__( | |||
raise DocumentStoreError( | |||
f"Invalid value {similarity} for similarity, choose between 'cosine', 'l2' and 'dot_product'" | |||
) | |||
client_info = self.client.info() | |||
self.server_version = tuple(int(num) for num in client_info["version"]["number"].split(".")) |
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.
Nice touch, tuple to make it immutable, eh?
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.
Yes, also to make it consistent with the type of VERSION
from the elasticsearch client.
@@ -376,6 +382,22 @@ def test_write_documents_req_for_each_batch(self, mocked_document_store, documen | |||
mocked_document_store.write_documents(documents) | |||
assert mocked_bulk.call_count == 5 | |||
|
|||
@pytest.mark.unit | |||
def test_get_vector_similarity_query(self, mocked_document_store): | |||
vec_sim_query = mocked_document_store._get_vector_similarity_query(np.random.rand(3).astype(np.float32), 10) |
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.
Is this some smoke test? What's the motivation behind it? Perhaps a comment or two would help a lot :-)
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.
I added some doc strings to the new tests.
|
||
@pytest.mark.unit | ||
def test_get_vector_similarity_query_es_7_5_and_below(self, mocked_document_store): | ||
# Patch server version to be 7.5.0 |
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.
Yeah here too, just a bit more motivation for why are we testing this, short comment?
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.
Awesome 🚀
Related Issues
Proposed Changes:
This PR adds a check to
elasticsearch/es7.py
andelasticsearch/es8.py
that checks the version of the connected Elasticsearch server and logs a warning if the server version is not consistent with the version of the usedElasticsearchDocumentStore
.Also, this PR adds support for Elasticsearch versions <= 7.5 by using a different vector similarity query for Elasticsearch servers with versions <= 7.5.
How did you test it?
I added two unit tests to test that the
"source"
field of the ES query is formatted according to the used version.Notes for the reviewer
Checklist
fix:
,feat:
,build:
,chore:
,ci:
,docs:
,style:
,refactor:
,perf:
,test:
.