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

proposal: Add RecentnessRanker component #5289

Merged
merged 16 commits into from
Jul 17, 2023
Merged

proposal: Add RecentnessRanker component #5289

merged 16 commits into from
Jul 17, 2023

Conversation

elundaeva
Copy link
Contributor

@elundaeva elundaeva commented Jul 6, 2023

This proposal is for adding RecentnessRanker to Haystack. The ranker will allow to have retrieved documents sorted not only by relevance (default) but also with recency factored in. Here's the link to the code for this reranker: https://github.com/deepset-ai/haystack/pull/5301/files. The proposal contains an example pipeline setup with RecentnessRanker as well as some example use cases where sorting by recency can be important (e.g. when setting up QA on news content).

@github-actions github-actions bot added the type:documentation Improvements on the docs label Jul 6, 2023
@elundaeva elundaeva added proposal and removed type:documentation Improvements on the docs labels Jul 6, 2023
@elundaeva elundaeva changed the title Proposal to add Recency ReRanker to Haystack [DRAFT] [DRAFT] Proposal to add Recency ReRanker to Haystack Jul 6, 2023
@elundaeva elundaeva changed the title [DRAFT] Proposal to add Recency ReRanker to Haystack [DRAFT] Proposal to add Timo's Recency ReRanker to Haystack Jul 6, 2023
@ZanSara
Copy link
Contributor

ZanSara commented Jul 6, 2023

Hey @elundaeva! I'll remove the proposal tag, as it has a different meaning and doesn't apply here 🙂

@sjrl
Copy link
Contributor

sjrl commented Jul 10, 2023

Hey @ZanSara just to let you know, @elundaeva will be splitting this up into two separate PRs, one for the proposal and one for a draft PR of the code changes.

@elundaeva elundaeva changed the title [DRAFT] Proposal to add Timo's Recency ReRanker to Haystack Proposal to add Timo's Recency ReRanker to Haystack Jul 10, 2023
@elundaeva elundaeva marked this pull request as ready for review July 10, 2023 08:38
@elundaeva elundaeva requested review from a team as code owners July 10, 2023 08:38
@elundaeva elundaeva requested review from bogdankostic and removed request for a team July 10, 2023 08:38
@FHardow FHardow removed their request for review July 10, 2023 08:39
Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

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

Thanks @elundaeva, I think this would be a great addition to Haystack! Before approving this proposal, I would like the Detailed design section to be more comprehensive.

proposals/text/5289-recentness-ranker.md Outdated Show resolved Hide resolved
@bogdankostic bogdankostic added the proposal:review Proposal is in "Review" state label Jul 11, 2023
Copy link
Contributor

@bogdankostic bogdankostic left a comment

Choose a reason for hiding this comment

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

Thanks for making the requested changes, @elundaeva! This looks good to me now!

According to our proposal process, the proposal needs the approval from three core contributors to move to the final comment phase. I'll share it with the rest of the core team :)

@anakin87 anakin87 self-requested a review July 11, 2023 15:47
Copy link
Member

@anakin87 anakin87 left a comment

Choose a reason for hiding this comment

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

LGTM!
Thx @elundaeva...

@bogdankostic bogdankostic changed the title Proposal to add Timo's Recentness Ranker to Haystack proposal: Add RecentnessRanker component Jul 11, 2023
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.

Looks nice to me but I have some critiques.

I think we should remove the links that point to private repos, the community won't be able to access them and that's a bum. We already have #5301 to show the code and that's fine.

I saw the relative PR mentions a paper in regards of the k value used for the reciprocal rank fusion method, can we maybe link it here in the proposal?

Also would love if the PR description was formatted a bit more, deleting the template and briefly explaining the proposal would have been fine. 💙

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.

Awesome! 🚀

@elundaeva elundaeva merged commit e0cf142 into main Jul 17, 2023
4 checks passed
@elundaeva elundaeva deleted the RecencyReranker branch July 17, 2023 14:33
@elundaeva
Copy link
Contributor Author

Thank you everyone for the feedback, the proposal is now merged 🙂 💯

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
proposal:review Proposal is in "Review" state proposal type:documentation Improvements on the docs type:enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants