fix: make reranker propely optional - #1690
Conversation
|
@Benebo7 is attempting to deploy a commit to the Rohan Verma's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@AnishSarkar22 Can you review this? |
Summary
RERANKERS_ENABLEDis documented in both.env.examplefiles andconfigbuilds a liveRerankerwhen it's set, butsearch_knowledge_basecallsbuild_context(query, hits, registry)without thererankerargument. Sorerank_hitsalways receivesNoneand returns hits untouched. The switch exists and does nothing.This wires it through.
rank()is a blocking cross-encoder call, so the enabled path goes throughasyncio.to_threadto keep it off the event loop. The disabled path stays exactly as it is today.get_reranker_instance()returnsNonewheneverRERANKERS_ENABLEDis off, so nothing changes for anyone who doesn't opt in: same call, same code path, no thread. The default stays FALSE, so cloud is unaffected. What changes is that self-hosted operators can now actually turn it on, and cloud operators can actually test and measure metrics if wanted.Testing
Measured on the dev stack against a real indexed workspace.
build_contextcosts ~2 ms.A typical turn in my logs spends 46 to 72 seconds, almost all of it in LLM calls, so reranking lands around 5% of turn time.
Open Questions (Not in this PR)
Wiring the reranker up surfaced two things that affect how it performs. Both are judgment calls that depend on knowing the cloud setup, so I've left them out and am raising them here instead.
.env.examplesuggestsms-marco-MiniLM-L-12-v2via flashrank.ms-marco-MiniLM-L-6-v2scores the same on MS MARCO (74.30 vs 74.31 NDCG@10) and was consistently faster in my measurements: roughly 3x on small result sets, narrowing to 1.4x once documents get large enough that both truncate. The catch is that FlashRank doesn't ship L-6, so using it means switchingRERANKERS_MODEL_TYPEto cross-encoder, which means PyTorch instead of ONNX.Trade-off Summary
Happy to open follow-ups for either once you've had a look. You'd know better than I would whether the cloud hits the concurrency case at all.
High-level PR Summary
This PR fixes the reranker feature to actually work when enabled. Previously, the
RERANKERS_ENABLEDconfiguration flag existed but was never used because thererankerargument was not passed tobuild_context(), causing search results to never be reranked. The fix conditionally retrieves the reranker instance and, when enabled, wraps thebuild_context()call inasyncio.to_thread()to prevent blocking the event loop during the CPU-intensive cross-encoder operation. When the flag is disabled (the default), behavior remains identical to the current implementation.⏱️ Estimated Review Time: 5-15 minutes
💡 Review Order Suggestion
surfsense_backend/app/agents/chat/multi_agent_chat/subagents/builtins/knowledge_base/tools/search_knowledge_base.py