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

Add update_document_meta to InMemoryDocumentStore #2689

Merged
merged 11 commits into from Jul 7, 2022

Conversation

bogdankostic
Copy link
Contributor

@bogdankostic bogdankostic commented Jun 20, 2022

Proposed changes:
InMemoryDocumentStore was the only DocumentStore that did not support update_document_meta. This PR adds the update_document_meta method to the InMemoryDocumentStore.

Status (please check what you already did):

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

Breaking Change

As we added update_document_meta to BaseDocumentStore, we needed to change the signature of update_document_meta in ElasticsearchDocumentStore.

# Old signature:
update_document_meta(self, id: str, meta: Dict[str, str], headers: Optional[Dict[str, str]] = None, index: str = None)

# New signature:
update_document_meta(self, id: str, meta: Dict[str, str],  index: str = None, headers: Optional[Dict[str, str]] = None)

@bogdankostic
Copy link
Contributor Author

The failing integration-tests-windows (pipelines) test also fails on master and is unrelated to the changes made in this PR.

@julian-risch julian-risch requested review from tstadel and removed request for masci July 6, 2022 13:11
Copy link
Member

@tstadel tstadel left a comment

Choose a reason for hiding this comment

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

LGTM! However we should enforce this method for all document stores by making update_document_meta an abstract method of BaseDocumentStore if there is not a good reason against it.

@@ -480,6 +480,19 @@ def get_document_count(
)
return len(documents)

def update_document_meta(self, id: str, meta: Dict[str, Any], index: str = None):
Copy link
Member

Choose a reason for hiding this comment

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

Can we make that now an abstractmethod of BaseDocumentStore? I think the only document store that does not support it would be DeepsetCloudDocumentStore and given its read-only nature that is not a show-stopper here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added it to BaseDocumentStore

@tstadel
Copy link
Member

tstadel commented Jul 7, 2022

@bogdankostic The breaking change tag is because of the changed order of args within ElasticsearchDocumentStore's implementation, right?

Copy link
Member

@tstadel tstadel left a comment

Choose a reason for hiding this comment

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

LGTM!

@bogdankostic
Copy link
Contributor Author

@bogdankostic The breaking change tag is because of the changed order of args within ElasticsearchDocumentStore's implementation, right?

Yes, exactly.

@bogdankostic bogdankostic merged commit 195aed9 into master Jul 7, 2022
@bogdankostic bogdankostic deleted the update_doc_meta_for_inmemdocstore branch July 7, 2022 13:44
Krak91 pushed a commit to Krak91/haystack that referenced this pull request Jul 26, 2022
* Add update_document_meta to InMemoryDocumentStore

* Fix typo

* Update Documentation & Code Style

* Add update_document_meta to BaseDocumentStore

* Update Documentation & Code Style

* Fix mypy

* Update Documentation & Code Style

* Add update_document_meta to MockDocumentStore

Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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.

None yet

2 participants