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.

Sign up for GitHub

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

Jump to bottom

fix: InMemoryDocumentStore not sharing some document stats with other instances #7792

Merged
merged 1 commit into from
Jun 4, 2024
Merged

fix: InMemoryDocumentStore not sharing some document stats with other instances #7792

merged 1 commit into from
Jun 4, 2024

Conversation

silvanocerza
Copy link
Contributor

Related Issues

Proposed Changes:

This PR fixed the InMemoryDocumentStore so that all the information that must be shared with other instances that use the same index is actually shared.

How did you test it?

I fixed and ran the end to end test that was failing.

Notes for the reviewer

I'm ignoring the release notes as this features has not be released yet.

Checklist

@silvanocerza silvanocerza added the ignore-for-release-notes PRs with this flag won't be included in the release notes. label Jun 3, 2024
@silvanocerza silvanocerza self-assigned this Jun 3, 2024
@silvanocerza silvanocerza requested a review from a team as a code owner June 3, 2024 15:49
@silvanocerza silvanocerza requested review from davidsbatista and removed request for a team and davidsbatista June 3, 2024 15:49
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 9353300023

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 5 unchanged lines in 1 file lost coverage.
  • Overall coverage increased (+0.02%) to 89.768%
Files with Coverage Reduction New Missed Lines %
document_stores/in_memory/document_store.py 5 98.23%
Totals Coverage Status
Change from base Build 9351549258: 0.02%
Covered Lines: 6703
Relevant Lines: 7467
💛 - Coveralls

julian-risch
julian-risch approved these changes Jun 4, 2024
View reviewed changes
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.

LGTM! 👍 I ran the e2e test also with include_outputs_from={"bm25_retriever", "embedding_retriever"} and the output of the bm25_retriever is as expected.

@julian-risch julian-risch changed the title Fix InMemoryDocumentStore not sharing some document stats with other instances fix: InMemoryDocumentStore not sharing some document stats with other instances Jun 4, 2024
@silvanocerza silvanocerza merged commit 26b263e into main Jun 4, 2024
24 checks passed
@silvanocerza silvanocerza deleted the fix-memory-docstore-sharing branch June 4, 2024 08:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julian-risch julian-risch julian-risch approved these changes

Assignees

@silvanocerza silvanocerza

Labels
ignore-for-release-notes PRs with this flag won't be included in the release notes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Hybrid doc search e2e fails with DuplicateDocumentError
3 participants
@silvanocerza @coveralls @julian-risch