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

delete_all_documents() replaced by delete_documents() #1377

Merged
merged 3 commits into from
Aug 30, 2021

Conversation

ramgarg102
Copy link
Contributor

@ramgarg102 ramgarg102 commented Aug 30, 2021

Proposed changes:

  • delete_all_documents() has been replaced by delete_documents() in all the files containing this method.
  • There are 6 files with warning logs regarding the same issue. I have added a commit message for those files (warning logs to be fixed).

Status (please check what you already did):

  • First draft (up for discussions & feedback)
  • Final code
  • Added tests
  • Updated documentation

@review-notebook-app
Copy link

Check out this pull request on  ReviewNB

See visual diffs & provide feedback on Jupyter Notebooks.


Powered by ReviewNB

@julian-risch julian-risch self-requested a review August 30, 2021 11:52
@julian-risch
Copy link
Member

Hi @ramgarg102 your PR looks very good to me. Two tests are failing for different reasons though. test_connector.py fails because there is an issue with the test case. You do not need to worry about that test case. We are working on this in a separate issue #1372
The second failing test case requires some small changes. test_weaviate.py fails because there is only an implementation of delete_all_documents() for the weaviate document store but no implementation of delete_documents().
Could you please rename the method delete_all_documents() in

def delete_all_documents(self, index: Optional[str] = None, filters: Optional[Dict[str, List[str]]] = None):
to delete_documents() and, in addition, add a method called delete_all_documents() like the following:

    def delete_all_documents(self, index: Optional[str] = None, filters: Optional[Dict[str, List[str]]] = None):
        """
        Delete documents in an index. All documents are deleted if no filters are passed.
        :param index: Index name to delete the document from.
        :param filters: Optional filters to narrow down the documents to be deleted.
        :return: None
        """
        logger.warning(
                """DEPRECATION WARNINGS: 
                1. delete_all_documents() method is deprecated, please use delete_documents method
                For more details, please refer to the issue: https://github.com/deepset-ai/haystack/issues/1045
                """
        )
        self.delete_documents(index, filters)

You can have a look at this example in the elasticsearchdocument store: https://github.com/deepset-ai/haystack/blob/master/haystack/document_store/elasticsearch.py#L973

After you have made these changes to document_store/weaviate.py we are ready to merge! 👍

@ramgarg102
Copy link
Contributor Author

Hey @julian-risch , I have renamed the delete_all_documents() method to delete_documents() and added the new delete_all_documents() method.

@julian-risch
Copy link
Member

Hey @julian-risch , I have renamed the delete_all_documents() method to delete_documents() and added the new delete_all_documents() method.

That's great! The test cases for the weaviate document store are passing now.
In the mean time, we also have a fix for the failing test case in test_connector.py and after we have merged this fix I will also merge your changes. Thank you very much for your contribution! 🙂

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.

Looks good to me! 👍

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.

2 participants