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

Fix and use delete_index instead of delete_documents in tests #2453

Merged
merged 18 commits into from
Apr 26, 2022

Conversation

tstadel
Copy link
Member

@tstadel tstadel commented Apr 25, 2022

To reset the document store for tests delete_index is cleaner than delete_documents as the latter does not remove properties that have been selected at index creation time (e.g. field mapping, embedding dimensions, similarity spaces). Additionally fixture document_store implements index deletion on its own for Milvus and Pinecone.

This PR facilitates and standardizes index cleaning in tests.

Proposed changes:

  • use delete_index instead of delete_documents in tests
  • properly implement delete_index for all document stores
  • add recreate_index param to Pincone, Milvus and Weaviate

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code

@tstadel tstadel added topic:document_store ignore-for-release-notes PRs with this flag won't be included in the release notes. topic:pinecone labels Apr 25, 2022
@tstadel tstadel requested a review from julian-risch April 25, 2022 08:54
@julian-risch
Copy link
Member

Hey @tstadel could you please briefly explain what's the problem with deleting just the documents and not the index in the tests? Is there any specific problem you came across?

@tstadel
Copy link
Member Author

tstadel commented Apr 25, 2022

Hey @tstadel could you please briefly explain what's the problem with deleting just the documents and not the index in the tests? Is there any specific problem you came across?

I updated the description above to answer it...

Copy link
Member

@julian-risch julian-risch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The changes look good to me. 👍 The many failing tests are worrying though. Please check that before merging, thanks. Seems all related to pymilvus.

@tstadel tstadel added topic:milvus type:bug Something isn't working topic:elasticsearch topic:tests and removed ignore-for-release-notes PRs with this flag won't be included in the release notes. labels Apr 26, 2022
@tstadel tstadel changed the title Use delete_index instead of delete_documents in tests Fix and use delete_index instead of delete_documents in tests Apr 26, 2022
@tstadel tstadel added the type:feature New feature or request label Apr 26, 2022
@tstadel tstadel merged commit 7498c7c into master Apr 26, 2022
@tstadel tstadel deleted the delete_index_in_tests branch April 26, 2022 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants