feat: make rerank timeout configurable via rerankTimeoutMs#356
feat: make rerank timeout configurable via rerankTimeoutMs#356rwmjhb merged 2 commits intoCortexReach:masterfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
|
Hi @rwmjhb, thanks for the detailed review! Both issues have been addressed: 1. Schema completeness ✅
2. Timeout cleanup in finally ✅ Updated push — the branch \eat/rerank-timeout-configurable\ now contains all fixes. Thanks! |
|
Hi, I noticed the current default for |
AliceLJY
left a comment
There was a problem hiding this comment.
Thanks for the rerankTimeoutMs addition — the core feature is clean and correct! However the diff contains much more than the timeout change: large deletions of TraceCollector, RetrievalStatsCollector, retrieveWithTrace(), and several config interface fields (omitDimensions, memoryCompaction, sessionCompression etc.) from index.ts that aren't mentioned in the PR description.
Could you clarify whether these deletions are intentional? If yes, they should be in a dedicated refactor PR so they can be reviewed independently and won't conflict with #365/#367 which appear to have the same deletions. If not, please rebase to remove the unrelated changes.
Happy to approve the timeout feature once the scope is clean!
b1f1ad9 to
a01e47e
Compare
Review feedback addressed ✅All three items from the review have been implemented:
The PR now contains two clean commits on top of latest master:
|
AliceLJY
left a comment
There was a problem hiding this comment.
Nice work! The try/finally cleanup for clearTimeout is the right pattern — especially important now that timeouts can be 120s+, you don't want orphaned timers on fast-failure paths.
Re @ggzeng's suggestion about bumping the default to 10s: keeping 5s is fine for backward compatibility, and anyone who needs more can configure it.
Clean feature, good review response. LGTM!
|
Hi, I noticed this PR uses a 5s default for For self-hosted reranker setups where network latency is typically higher, a 10s default would be more practical and avoid unnecessary timeouts. The 5s default might work well for cloud-hosted endpoints but could be tight for on-prem deployments. Would you consider raising the default to 10s (or making it more easily configurable)? |
…in finally Reviewer: rwmjhb (CortexReach) 1. Schema completeness: add rerankTimeoutMs to openclaw.plugin.json retrieval schema and index.ts PluginConfig.retrieval 2. Timeout cleanup: move clearTimeout into finally block so timer is always cleared even on fast failure paths 3. Also update warning message to show actual configured timeout value Closes CortexReach#346
cc669b1 to
757cc9d
Compare
|
Thanks for the feedback @ggzeng! The 5s default was chosen to maintain backward compatibility — it's the same as the previous hardcoded value. Cloud API users (Jina, SiliconFlow, etc.) typically respond within 1-3s, so 5s is reasonable for them. For self-hosted/local rerankers, the PR already makes it configurable via: { "retrieval": { "rerankTimeoutMs": 10000 } }10s is a reasonable default for self-hosted setups. However, changing the default from 5s to 10s would slightly increase wait time for cloud API users who hit the timeout. We could alternatively:
What's your preference? Happy to adjust the default if there's consensus. Also, if you're interested in maintaining this feature, feel free to take over the PR since you have a similar PR #371 with the 10s default approach. |
feat: make rerank timeout configurable via rerankTimeoutMs
Summary
Add a configurable
rerankTimeoutMsparameter toRetrievalConfigso that users running local/CPU-based rerank servers can adjust the timeout without patching source code. Closes #346.Changes (4 lines across 1 file)
1.
src/retriever.ts—RetrievalConfiginterface2.
src/retriever.ts—DEFAULT_RETRIEVAL_CONFIG3.
src/retriever.ts—rerankResults()method4.
src/retriever.ts— Warning messageUsage Example
{ "plugins": { "entries": { "memory-lancedb-pro": { "config": { "retrieval": { "rerankTimeoutMs": 120000 } } } } } }Local Model Example (BAAI/bge-reranker-base via reranker_server.py)
{ "plugins": { "entries": { "memory-lancedb-pro": { "config": { "retrieval": { "rerank": "cross-encoder", "rerankProvider": "siliconflow", "rerankEndpoint": "http://127.0.0.1:18799/v1/rerank", "rerankTimeoutMs": 120000 } } } } } }Backward Compatibility
5000msis identical to the current hardcoded behaviorrerankTimeoutMssee no changeIssue
Closes #346