Skip to content

fix: make reranker propely optional - #1690

Open
Benebo7 wants to merge 1 commit into
MODSetter:devfrom
Benebo7:fix/reimpementing-reranker
Open

fix: make reranker propely optional#1690
Benebo7 wants to merge 1 commit into
MODSetter:devfrom
Benebo7:fix/reimpementing-reranker

Conversation

@Benebo7

@Benebo7 Benebo7 commented Aug 17, 2026

Copy link
Copy Markdown
Contributor

Summary

RERANKERS_ENABLED is documented in both .env.example files and config builds a live Reranker when it's set, but search_knowledge_base calls build_context(query, hits, registry) without the reranker argument. So rerank_hits always receives None and 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 through asyncio.to_thread to keep it off the event loop. The disabled path stays exactly as it is today.

reranker = RerankerService.get_reranker_instance()
if reranker is not None:
    rendered = await asyncio.to_thread(
        build_context, cleaned_query, hits, registry, reranker=reranker
    )
else:
    rendered = build_context(cleaned_query, hits, registry)

get_reranker_instance() returns None whenever RERANKERS_ENABLED is 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.

  • Flag OFF: build_context costs ~2 ms.
  • Flag ON: Reranking adds 0.8 to 2.1 s per search depending on document count.

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.

  1. The default model: .env.example suggests ms-marco-MiniLM-L-12-v2 via flashrank. ms-marco-MiniLM-L-6-v2 scores 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 switching RERANKERS_MODEL_TYPE to cross-encoder, which means PyTorch instead of ONNX.
  2. Concurrency: The agent fires searches in parallel. Since the cross-encoder releases the GIL and runs in native code, those genuinely compete for cores. Without a limit they all finish at the slowest one's pace. A semaphore around the reranking call gave each search predictable latency, and at higher concurrency it improved wall-clock time too. Whether that helps at all depends on whether reranking threads actually exceed available cores on your infrastructure. Sizing it also needs to account for worker count, since this optimization is targeting cloud, and I can't tell how the cloud deployment handles that.

Trade-off Summary

Decision Upside Cost
Switch default to L-6 1.4 to 3x faster, same benchmark quality Needs cross-encoder backend (PyTorch, not ONNX); +24 MB RSS; 1.4 s model load vs 0.2 s
Keep L-12 / flashrank ONNX runtime purpose-built for inference; faster startup Roughly half the throughput for no measurable quality gain
Add a concurrency limit Predictable per-search latency; better wall-clock under load Adds queueing when cores are free; sizing depends on cores and worker count
No limit (today) No queueing when capacity is free Parallel searches contend; all finish at the slowest one's pace

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_ENABLED configuration flag existed but was never used because the reranker argument was not passed to build_context(), causing search results to never be reranked. The fix conditionally retrieves the reranker instance and, when enabled, wraps the build_context() call in asyncio.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
Order File Path
1 surfsense_backend/app/agents/chat/multi_agent_chat/subagents/builtins/knowledge_base/tools/search_knowledge_base.py

Need help? Join our Discord

@vercel

vercel Bot commented Aug 17, 2026

Copy link
Copy Markdown

@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.

@coderabbitai

coderabbitai Bot commented Aug 17, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: 6e1e10ed-e926-4743-836b-d6c8be71ba73

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@MODSetter

Copy link
Copy Markdown
Owner

@AnishSarkar22 Can you review this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants