Repository navigation
Conversation
…/B path The LLM write-path reasoner (LlmReasoner::http_chat_call) built its reqwest client with `Client::new()` (no timeout) and sent requests with no `max_tokens`. A slow, hung, or runaway endpoint could therefore block the write path indefinitely, or a local model could generate for tens of seconds per call, making bulk ingest impractical. Add: - a per-request timeout (SYNAPTIC_LLM_TIMEOUT_SECS, default 90); on timeout the call errors and the reasoner fails open to its heuristic fallback, so ingest degrades rather than hangs. - a max_tokens cap (SYNAPTIC_LLM_MAX_TOKENS, default 512); extraction/resolution replies are small JSON, so this bounds per-call latency without truncating. Eval harness (run_eval): - `--qtype NAME` now drops conversations with no matching questions (previously only questions were filtered, so all conversations were still ingested — on the retrieval path that spawned one task per conversation this flooded the reasoner and wedged its HTTP client). - retrieval path: bounded ingest concurrency (RETRIEVAL_CONCURRENCY, default 2) instead of one spawned task per conversation, so a real LLM reasoner at ingest is not flooded with concurrent requests. - retrieval path honors `--exclude-superseded` (bi-temporal A/B): ingest is unchanged (KG on marks supersessions), only retrieval drops superseded memories, so recall@k of the two arms is directly comparable. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
|
Superseded by a fresh branch that includes the rustfmt fix (repo ruleset requires a new branch per iteration). |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a8c3620e68
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| let cfg = synaptic::MemoryConfig { | ||
| enable_knowledge_graph: true, | ||
| retrieval_excludes_superseded: exclude_superseded, | ||
| ..Default::default() |
There was a problem hiding this comment.
When --exclude-superseded is used and a superseded memory ranks within the initial top 10, runner::run_question still calls search(..., K), while AgentMemory::search applies the supersession filter only after the retrieval pipeline has already limited its results. The excluded arm can therefore return fewer than 10 entries rather than the next eligible entries, artificially lowering its recall/precision and corrupting the intended retrieval A/B comparison whenever a valid evidence turn is just below a filtered stale result. Fetch enough candidates (or filter before applying the retrieval limit) before scoring.
Useful? React with 👍 / 👎.
Summary
The LLM write-path reasoner (
LlmReasoner::http_chat_call) built its reqwest client withClient::new()(no timeout) and sent requests with nomax_tokens. Consequences found while running write-side eval experiments:Reasoner fixes (
src/memory/reasoning/llm.rs)Client::builder().timeout(...)—SYNAPTIC_LLM_TIMEOUT_SECS(default 90). On timeout the call errors and the reasoner fails open to its heuristic fallback, so ingest degrades rather than hangs.max_tokenscap —SYNAPTIC_LLM_MAX_TOKENS(default 512). Extraction/resolution replies are small JSON, so this bounds per-call latency without truncating real output.Eval harness fixes (
tools/eval/src/bin/run_eval.rs)--qtype NAMEnow drops conversations with no matching questions (previously only questions were filtered, so every conversation was still ingested — on the retrieval path, which spawns one task per conversation, this flooded the reasoner and wedged its HTTP client under concurrency).RETRIEVAL_CONCURRENCY, default 2) instead of one spawned task per conversation.--exclude-superseded(bi-temporal A/B): ingest is unchanged (KG on marks supersessions), only retrieval drops superseded memories, so recall@k of the two arms is directly comparable.Testing
synaptic/static-embeddings synaptic/llm-reasoning synaptic-eval/llm-reasoning).🤖 Generated with Claude Code