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

feat!: SentenceWindowRetriever returns List[Document] with docs ordered by split_idx_start #8590

Draft
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

davidsbatista
Copy link
Contributor

@davidsbatista davidsbatista commented Nov 27, 2024

Related Issues

Proposed Changes:

SentenceWindowRetriever now returns in context_documents a List[Document] instead of List[List[Document] with the documents ordered by split_idx_start

How did you test it?

  • integration tests and manual verification

Checklist

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels Nov 27, 2024
@davidsbatista davidsbatista changed the title refactor: SentenceWindowRetriever returns a List[Document] instead of List[List[Document] with documents ordered by split_idx_start refactor: SentenceWindowRetriever returns List[Document] with docs ordered by split_idx_start Nov 27, 2024
@coveralls
Copy link
Collaborator

coveralls commented Nov 27, 2024

Pull Request Test Coverage Report for Build 12143707645

Details

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.001%) to 90.309%

Totals Coverage Status
Change from base Build 12136036604: 0.001%
Covered Lines: 8042
Relevant Lines: 8905

💛 - Coveralls

@davidsbatista davidsbatista marked this pull request as ready for review December 3, 2024 10:15
@davidsbatista davidsbatista requested review from a team as code owners December 3, 2024 10:15
@davidsbatista davidsbatista requested review from dfokina, Amnah199 and julian-risch and removed request for a team December 3, 2024 10:15
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.

We need to change the output_type for the run method in https://github.com/deepset-ai/haystack/pull/8590/files#diff-750f6a7bf921c73e8d49b501612d4302738c3ddbe8b2a78d866493db722388e5R146
The docstring needs to be updated accordingly.
The PR should be feat!: instead of refactor: as we the changes are user-facing and change how the component can be used.

@davidsbatista davidsbatista changed the title refactor: SentenceWindowRetriever returns List[Document] with docs ordered by split_idx_start feat!: SentenceWindowRetriever returns List[Document] with docs ordered by split_idx_start Dec 3, 2024
@davidsbatista davidsbatista changed the title feat!: SentenceWindowRetriever returns List[Document] with docs ordered by split_idx_start feat!: SentenceWindowRetriever returns List[Document] with docs ordered by split_idx_start Dec 3, 2024
@julian-risch julian-risch self-requested a review December 3, 2024 16:22
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. As this is a breaking change, we'll need to wait until the deprecation notice was released before merging this PR. Let's put it back into draft PR mode.

Once this is merged, we can remove the deprecation note also from the documentation page.

@julian-risch julian-risch added this to the 2.9.0 milestone Dec 3, 2024
@davidsbatista davidsbatista marked this pull request as draft December 3, 2024 16:28
@julian-risch julian-risch removed the request for review from Amnah199 December 3, 2024 16:28
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.

Updates to the SentenceWindowRetriever
3 participants