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: allow DocumentJoiner to accept top_k parameter in run method #7709

Merged
merged 2 commits into from
May 23, 2024

Conversation

Varun-Krishnan1
Copy link
Contributor

Related Issues

Proposed Changes:

The issue was caused by the DocumentJoiner component's run method not accepting the top_k parameter. This resulted in a ValueError when trying to pass top_k at query time using pipe.run("DocumentJoiner": {"top_k": top_k}).

To solve this issue, I updated the run method to accept the top_k parameter and use it to limit the number of returned documents. The relevant lines in the run method were modified to check for the top_k parameter and apply it accordingly.

How did you test it?

I added a unit test to the test_document_joiner.py file to ensure the top_k parameter works correctly when provided in the run method. The test verifies that the run method limits the number of returned documents to the specified top_k value.

Notes for the reviewer

None

Checklist

  • I have read the contributors guidelines and the code of conduct
  • I have updated the related issue with new insights and changes
  • I added unit tests and updated the docstrings
  • I've used one of the conventional commit types for my PR title: fix:, feat:, build:, chore:, ci:, docs:, style:, refactor:, perf:, test:.
  • I documented my code
  • I ran pre-commit hooks and fixed any issue
  • I have added a release note file using reno

@Varun-Krishnan1 Varun-Krishnan1 requested review from a team as code owners May 16, 2024 23:03
@Varun-Krishnan1 Varun-Krishnan1 requested review from dfokina and silvanocerza and removed request for a team May 16, 2024 23:03
@CLAassistant
Copy link

CLAassistant commented May 16, 2024

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added topic:tests type:documentation Improvements on the docs labels May 16, 2024
@coveralls
Copy link
Collaborator

Pull Request Test Coverage Report for Build 9120287116

Details

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

Totals Coverage Status
Change from base Build 9116506765: 0.002%
Covered Lines: 6594
Relevant Lines: 7279

💛 - Coveralls

@silvanocerza silvanocerza self-assigned this May 23, 2024
Copy link
Contributor

@silvanocerza silvanocerza left a comment

Choose a reason for hiding this comment

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

Nice, thanks!

@silvanocerza silvanocerza merged commit badb05b into deepset-ai:main May 23, 2024
26 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic:tests type:documentation Improvements on the docs
Projects
None yet
Development

Successfully merging this pull request may close these issues.

DocumentJoiner component's run method doesn't accept a top_k value
4 participants