Repository navigation
LLMRanker returns documents unranked on None query instead of crashing - #12799
Conversation
|
@ege-arhan is attempting to deploy a commit to the deepset Team on Vercel. A member of the Team first needs to authorize it. |
feiiiiii5
left a comment
There was a problem hiding this comment.
Verified this in two throwaway checkouts of the repo (one at the PR base 52df9672a7, one at head dcad1fd048), with the package installed editable from each tree, Python 3.11, macOS. Only test/components/rankers/test_llm_ranker.py was run — not the full suite, and no mypy/hatch run.
The bug is real and reachable. At the base commit, calling LLMRanker.run with query=None fails exactly as the release note says:
E AttributeError: 'NoneType' object has no attribute 'strip'
haystack/components/rankers/llm_ranker.py:256
and with an int query, AttributeError: 'int' object has no attribute 'strip' at the same line.
The new tests do exercise the change rather than restate it. Copying this PR's test file into the base tree gives:
base 52df9672a7 -> 3 failed, 46 passed, 3 skipped
test_run_non_string_query_returns_fallback[None]
test_run_non_string_query_returns_fallback[123]
TestLLMRankerAsync::test_run_async_none_query_returns_fallback
this head dcad1fd048 -> 49 passed, 3 skipped
which matches the "49 passed, 3 live deselected" you reported, and the three failures line up one-to-one with the two guarded call sites.
Two notes, neither blocking:
- For a non-string query the log line still reads
Empty query provided to LLMRanker. Returning documents without reranking.(llm_ranker.py:257and:324). Someone chasing a miswired upstream component will see "empty query" and look at the wrong thing — including the received type, e.g.f"Ignoring non-string query of type {type(query).__name__}..."for that branch, makes the warning actionable. Feel free to skip it. - I checked whether the fallback (rather than raising) is the right shape, because
queryis typedstrand aNonehere means something upstream is broken. Reusing the existing empty-query path is the consistent choice: that path already logs and returns documents unranked, and both call sites now behave the same. Raising would be a behaviour change for the empty-string case too, so it is a bigger question than this PR — flagging it only so a reviewer does not ask you to redesign mid-review.
Disclosure, per CONTRIBUTING's AI-assistance note: this comment was written by an AI agent working autonomously for @feiiiiii5, and no human read it before posting. Every number above is copied from the runs described, not summarised from memory; the checkouts, the single test module and its limits are stated so anyone can repeat or contradict it.
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Head branch was pushed to by a user without write access
|
Hi @ege-arhan, thanks a lot for your contribution! 🙏 We noticed that the Contributor License Agreement (CLA) check ( To get your PR reviewed, please sign the CLA via the link in the |
db06a0c to
d85f05b
Compare
|
Thanks for signing the CLA, @ege-arhan! 🎉 This PR is now ready for review again and the reviewer has been re-assigned. |
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
||||||||||||||||||||||||
Pipeline wiring can hand LLMRanker a None query when an upstream component emits nothing. Both run and run_async called query.strip() unconditionally, so that turned into AttributeError mid-pipeline.
The guard now treats anything that isn't a string the same as the existing empty-query path: log the warning and hand the deduplicated documents back untouched. No LLM call happens. The sync and async paths share the behavior, and I added regression coverage for None (both paths) plus a non-string int case on run.
Ran the ranker suite (49 passed, 3 live deselected) and ruff check/format clean on the touched files.