fix: DocumentJoiner top_k=0 is treated as unset instead of returning … - #12217
fix: DocumentJoiner top_k=0 is treated as unset instead of returning …#12217manjunathbhaskar wants to merge 2 commits into
Conversation
…zero documents Both the run() and __init__() top_k parameters were checked with a truthy check (if top_k / elif self.top_k), so top_k=0, a legitimate request to return no documents, was silently treated as unset and fell back to the other value instead. Introduced in deepset-ai#7709 when the run() top_k parameter was added. Adds a regression test covering the run-time top_k=0 case.
|
@manjunathbhaskar is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
|
|
Coverage report
Click to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||||||||||||||
ebarkhordar
left a comment
ebarkhordar
left a comment
There was a problem hiding this comment.
Verified this on f4058ac against main f081d94, in a clean python:3.13-slim container with the joiner driven directly (two 3-document lists):
| case | main | this PR |
|---|---|---|
DocumentJoiner(top_k=5).run(docs, top_k=0) |
5 | 0 |
DocumentJoiner(top_k=0).run(docs) |
6 | 0 |
The first row is the case the description and the new test cover. The second row is the elif self.top_k is not None: half, and it is a behaviour change of its own: an init-time top_k=0 used to return every document and now returns none. It reads intentional to me, but it is currently untested and unmentioned, so a second assertion would pin it.
Two things that may affect how you want to land this:
AnswerJoiner carries the same pattern at haystack/components/joiners/answer_joiner.py:138-139 (top_k = top_k or self.top_k, then if top_k:). Measured on this branch: AnswerJoiner(top_k=2).run(answers, top_k=0) returns 2, and AnswerJoiner(top_k=0).run(answers) returns all 3.
Elsewhere in the codebase top_k=0 is rejected rather than honoured. LostInTheMiddleRanker(top_k=0) and MetaFieldRanker(meta_field="m", top_k=0) both raise ValueError: top_k must be > 0, and the in-memory retrievers raise on it too. This PR makes the joiners the only components where 0 is a meaningful value. Is that the split you want, or should the joiners validate instead? Asking because DocumentJoiner.__init__ validates neither bound today, and DocumentJoiner(top_k=-1) quietly drops the last document (5 of 6, unchanged by this PR).
|
Thanks for testing this so thoroughly, the init-time case is a good catch, and so is AnswerJoiner having the same pattern. On the direction question. My reasoning for honoring 0 in joiners rather than raising was that a joiner operates on documents that already went through retrieval, so a caller passing top_k=0 at runtime is intentionally suppressing a branch's output rather than making a retrieval-count mistake, which is the case ValueError guards against in the rankers and retrievers. That said, I don't think this is my call to make for the sake of consistency across the codebase, so I'd rather ask than assume. Do you want joiners to keep honoring 0, or should they raise ValueError like LostInTheMiddleRanker and MetaFieldRanker do? Once you've picked a direction I'll update this PR accordingly, add AnswerJoiner so both joiners behave the same way, and add the missing init test. On negative top_k, since it currently falls through to Python slice semantics unvalidated in several components already, I'd treat that as a separate, existing issue rather than something to fold into this PR. One thing worth flagging while we're on it. The same root pattern, truthy checking an optional integer where 0 is a legitimate value instead of checking is not None, showed up independently in an unrelated project I looked at recently, llama_index's get_top_k_embeddings has the identical bug (already tracked there as issue 22508, similarity_top_k=0 silently returns everything). Not proposing anything for this PR, just flagging it as a pattern worth a second look wherever else top_k style parameters get resolved, since it seems to recur whenever an optional count parameter defaults through a truthy check rather than an explicit None check. |
4 participants
…zero documents
Both the run() and init() top_k parameters were checked with a truthy check (if top_k / elif self.top_k), so top_k=0, a legitimate request to return no documents, was silently treated as unset and fell back to the other value instead.
Introduced in #7709 when the run() top_k parameter was added.
Adds a regression test covering the run-time top_k=0 case.
Related Issues
Proposed Changes:
How did you test it?
Notes for the reviewer
Checklist
fix:,feat:,build:,chore:,ci:,docs:,style:,refactor:,perf:,test:and added!in case the PR includes breaking changes.