fix: correct knowledge base ranking behavior - #9454
Open
lxfight wants to merge 2 commits into
Open
Conversation
📄 Knowledge reviewDosu skipped reviewing this PR because your organization has used its |
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- The rerank error handling now raises generic
ValueErrorwith fixed messages; consider including key details from the raw response (e.g., status code or a truncated body) so callers and logs can more easily diagnose integration issues. - In
rank_fusion.py, the final sort key usescidas a tiebreaker; ifchunk_idcan be non-string types or mixed formats, you may want to normalize it explicitly (e.g., cast to string) or document the expected type to avoid subtle ordering changes.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The rerank error handling now raises generic `ValueError` with fixed messages; consider including key details from the raw response (e.g., status code or a truncated body) so callers and logs can more easily diagnose integration issues.
- In `rank_fusion.py`, the final sort key uses `cid` as a tiebreaker; if `chunk_id` can be non-string types or mixed formats, you may want to normalize it explicitly (e.g., cast to string) or document the expected type to avoid subtle ordering changes.
## Individual Comments
### Comment 1
<location path="tests/unit/test_rank_fusion.py" line_range="63" />
<code_context>
+
+
+@pytest.mark.asyncio
+async def test_rank_fusion_prefers_dense_rank_when_scores_are_equal():
+ dense_results = [
+ make_dense_result("dense-first", 0.99),
</code_context>
<issue_to_address>
**suggestion (testing):** Add coverage for cases where dense ranks are tied and sparse ranks act as the secondary tiebreaker.
The new tests cover dense-rank preference and `chunk_id` as the final tiebreaker. To fully exercise `(-score, dense_rank, sparse_rank, cid)`, please add a test where multiple candidates share the same fused score and dense rank (or have no dense rank), but differ in sparse rank. Assert that the ordering follows `sparse_rank` and remains stable regardless of input order, so we explicitly verify `sparse_rank` is used as the secondary tiebreaker when dense ranks don’t distinguish candidates.
Suggested implementation:
```python
results = await RankFusion(kb_db=None).fuse(
dense_results=dense_results,
sparse_results=sparse_results,
@pytest.mark.asyncio
async def test_rank_fusion_uses_sparse_rank_as_secondary_tiebreaker_when_dense_ranks_tied():
# Two candidates with identical scores and (effectively) identical dense rank,
# but different sparse ranks; ordering should follow sparse_rank and be stable
# regardless of input order.
sparse_a = make_sparse_result("sparse-a", "kb", 10.0, 1)
sparse_b = make_sparse_result("sparse-b", "kb", 10.0, 2)
# Explicitly set sparse_rank to ensure we can control it even if the helper
# doesn't derive it from position.
# Adjust attribute name here if your sparse result objects use a different field.
sparse_a.sparse_rank = 0
sparse_b.sparse_rank = 1
# No dense results: fused ordering must use sparse_rank as the secondary
# tiebreaker for candidates with equal fused score.
fusion = RankFusion(kb_db=None)
# First, fuse in one order
results_order_1 = await fusion.fuse(
dense_results=[],
sparse_results=[sparse_b, sparse_a],
)
# Then, fuse in the opposite order
results_order_2 = await fusion.fuse(
dense_results=[],
sparse_results=[sparse_a, sparse_b],
)
# In both cases, sparse_a (with better sparse_rank) should come first, and
# the ordering should be stable regardless of input order.
assert [r.chunk_id for r in results_order_1] == [sparse_a.chunk_id, sparse_b.chunk_id]
assert [r.chunk_id for r in results_order_2] == [sparse_a.chunk_id, sparse_b.chunk_id]
```
1. Ensure that `sparse_rank` is indeed the attribute name used by your sparse result objects; if it's named differently (e.g. `rank` or `sparse_position`), update the two assignments accordingly.
2. Place the new test so that it is *outside* the body of `test_rank_fusion_prefers_dense_rank_when_scores_are_equal` (i.e. after that function's closing parenthesis and any assertions). The SEARCH/REPLACE block above may need slight adjustment to match the exact code around the end of that test.
3. If `make_sparse_result` already sets `sparse_rank` based on an argument or its position, you can omit the explicit `sparse_a.sparse_rank` / `sparse_b.sparse_rank` assignments and instead construct the objects so that they have distinct `sparse_rank` values.
4. If your fused result objects don't expose `chunk_id` as `r.chunk_id`, update the two assertions to use whatever identifier you normally use to distinguish candidates (e.g. `r.doc_id`), while keeping the ordering checks the same.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
5 tasks
Member
Author
|
Reviewed the remaining high-level suggestions:
No production change is needed for these two points. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Knowledge base rank fusion could produce unstable ordering for tied candidates, while the vLLM rerank provider could silently treat malformed or failed HTTP responses as empty results. This PR makes both failure modes explicit and deterministic.
Modifications / 改动点
Screenshots or Test Results / 运行截图或测试结果
Verification steps:
Results:
Checklist / 检查清单
/ 如果 PR 中有新加入的功能,已经通过 Issue / 邮件等方式和作者讨论过。
/ 我的更改经过了良好的测试,并已在上方提供了“验证步骤”和“运行截图”。
requirements.txtandpyproject.toml./ 我确保没有引入新依赖库,或者引入了新依赖库的同时将其添加到
requirements.txt和pyproject.toml文件相应位置。/ 我的更改没有引入恶意代码。
Summary by Sourcery
Clarify and harden knowledge base ranking by making rank fusion tie-breaking deterministic and enforcing strict validation of vLLM rerank API responses.
Bug Fixes:
Tests: