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: Add AnswerJoiner new component #8122

Merged
merged 14 commits into from
Aug 1, 2024
Merged

feat: Add AnswerJoiner new component #8122

merged 14 commits into from
Aug 1, 2024

Conversation

vblagoje
Copy link
Member

@vblagoje vblagoje commented Jul 30, 2024

Why:

Adds an AnswerJoiner to merge multiple lists of answers using various join modes, addressing the need for simplified answer aggregation.

What:

  1. Added an AnswerJoiner class in haystack/components/joiners/answer_joiner.py for merging lists of answers.
  2. Modified haystack/components/joiners/__init__.py to include AnswerJoiner in the module exports.
  3. Added unit tests in test/components/joiners/test_answer_joiner.py to validate the functionality of AnswerJoiner.

How can it be used:

  • Similar to DocumentJoiner

How did you test it:

  • Conducted unit tests including initialization tests, to/from dictionary serialization, empty list handling, and checking for correct answer merging.
  • Verified custom join functions and error handling for unsupported join modes using pytest.

Notes for the reviewer:

  • Review the JoinMode enumeration in answer_joiner.py to ensure future proofing for additional join modes.
  • Check the logic within _concatenate method for performance implications with large lists.
  • Validate the custom join function handling to ensure robustness against edge cases.

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

coveralls commented Jul 30, 2024

Pull Request Test Coverage Report for Build 10196642167

Details

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

Totals Coverage Status
Change from base Build 10177216364: 0.04%
Covered Lines: 6864
Relevant Lines: 7619

💛 - Coveralls

@vblagoje
Copy link
Member Author

Looking for some early feedback before I wrap up @sjrl
Note the option to add custom_join_function as I found concatenate only to be limiting for all different types of Answers and potential use case scenarios. With custom_join_function we can grow this component and then standardize emerging join functions by adding them next to concatenate. This way we get the best of both worlds.
LMK

@sjrl
Copy link
Contributor

sjrl commented Jul 30, 2024

Hey @vblagoje as long as the custom join function is specifiable in a yaml file then I'm all aboard making it easy to extend in the future.

@sjrl
Copy link
Contributor

sjrl commented Jul 30, 2024

So @vblagoje if you could add a test for to_dict, from_dict and also loading the component from a yaml file then the custom function all sounds good to me :)

@vblagoje vblagoje changed the title draft: Add Answer joiner feat: Add Answer joiner Jul 30, 2024
@vblagoje vblagoje marked this pull request as ready for review July 30, 2024 14:03
@vblagoje vblagoje requested review from a team as code owners July 30, 2024 14:03
@vblagoje vblagoje requested review from dfokina and Amnah199 and removed request for a team July 30, 2024 14:03
@vblagoje vblagoje changed the title feat: Add Answer joiner feat: Add AnswerJoiner new component Jul 30, 2024
@vblagoje
Copy link
Member Author

vblagoje commented Jul 30, 2024

Note to @vblagoje @dfokina - we need to add this component to documentation, not sure if worthy of social but docs definitely

@vblagoje vblagoje added this to the 2.4.0 milestone Jul 30, 2024
@vblagoje
Copy link
Member Author

@sjrl while I'm on PTO please feel free to move this one forward, anyone else welcome as well 🙏

@sjrl
Copy link
Contributor

sjrl commented Jul 31, 2024

Hey @Amnah199 I'd really appreciate your review on this, since I've made some changes so it feels odd for me to approve my own work 😅

@sjrl sjrl linked an issue Aug 1, 2024 that may be closed by this pull request
@sjrl
Copy link
Contributor

sjrl commented Aug 1, 2024

Hey @dfokina could you look over the doc strings to check if this okay with the recent effort of rehauling doc strings for components?

@sjrl sjrl requested a review from Amnah199 August 1, 2024 07:25
Copy link
Contributor

@Amnah199 Amnah199 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!

@dfokina
Copy link
Contributor

dfokina commented Aug 1, 2024

Thank you for tagging me @sjrl , going over it now!

Copy link
Contributor

@dfokina dfokina left a comment

Choose a reason for hiding this comment

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

Docstrings updated

@sjrl sjrl merged commit 25d3520 into main Aug 1, 2024
18 checks passed
@sjrl sjrl deleted the answer_joiner branch August 1, 2024 10:51
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.

feat: Request for a AnswerJoiner component
5 participants