Conversation
masci
force-pushed
the
massi/test-weaviate
branch
from
014057e to
823305c
Compare
masci
force-pushed
the
massi/test-weaviate
branch
from
823305c to
507b8e4
Compare
masci
requested review from
ZanSara and
julian-risch
and removed request for
a team
julian-risch
left a comment
julian-risch
left a comment
There was a problem hiding this comment.
Looks much cleaner to me than before, great! 👍 Only two small changes that I would like to see where I commented. Further, I was wondering about the weaviate_fixture in conftest.py where there is still version weaviate:1.14.1. Other than that the PR looks ready to merge to me.
| docs = ds.query(query_text) | ||
| assert len(docs) == 3 | ||
|
|
||
| # BM25 retrieval WITH filters is not yet supported as of Weaviate v1.14.1 |
There was a problem hiding this comment.
Version number in this comment should be upgraded.
There was a problem hiding this comment.
Let's do this in a different PR, opened an issue for that #3553
ZanSara
left a comment
ZanSara
left a comment
There was a problem hiding this comment.
With this suite is getting really evident that we have many flavors of docstores: keyword docstores, docstores supporting labels, docstores supporting query_by_embedding... Really nice to see this trait coming out so clearly. Something to keep in mind if we ever decide to review the API.
|
|
||
| @pytest.mark.integration | ||
| def test_delete_index(self, ds, documents): | ||
| """Contrary to other Document Stores, this doesn't raise if the index is empty""" |
There was a problem hiding this comment.
Probably not for this PR, but let's raise an issue on this inconsistency. Also faiss has the same issue.
There was a problem hiding this comment.
Fully agree, I planned to do this once all the document store tests are moved to their own classes (read: when I can kill test_document_store.py). This way I will have a precise picture of all the inconsistencies.
| assert ds.get_document_count(index="custom_index") == 0 | ||
|
|
||
| @pytest.mark.integration | ||
| def test_query_by_embedding(self, ds, documents): |
There was a problem hiding this comment.
I think this test should go in the base suite. All docstores should support query_by_embedding
There was a problem hiding this comment.
Agree, I would do this in a dedicated PR because doing it here might bring in unrelated changes to other document stores
| assert len(docs) == 3 | ||
|
|
||
| @pytest.mark.integration | ||
| def test_query(self, ds, documents): |
There was a problem hiding this comment.
Theoretically this is a test that all KeywordDocumentStore subclasses should pass, which means, all docstores supporting BM25. Worth making another test base suite and moving this one in there?
There was a problem hiding this comment.
If the majority of the document stores support BM25 we should move this method to the base class and let the ones that doesn't support it override and skip the test.
julian-risch
left a comment
julian-risch
left a comment
There was a problem hiding this comment.
LGTM now! Thank you for addressing the change requests so quickly. 👍
3 participants
Related Issues
Proposed Changes:
Move Weaviate tests into their own class deriving from
DocumentStoreBaseTestAbstractHow did you test it?
pytest -m"document_store and integration" test/document_stores/test_weaviate.pyNotes for the reviewer
data_objects were patched to avoid incurring in the warning"The non-class namespaced APIs (None value for
class_name) are going to be removed in the future versions of the Weaviate Server and Weaviate Python Client."Checklist