perf(memory): deterministic fast path for retrieve_memory — skip the model walk when data is present - #4768
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds an environment-gated deterministic retrieval fast path for the ChangesMemory fast path
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant run_subagent
participant try_deterministic_memory_retrieval
participant fast_retrieve
run_subagent->>try_deterministic_memory_retrieval: agent_memory delegation
alt enabled, grounded, and non-empty query
try_deterministic_memory_retrieval->>fast_retrieve: query with bounded options
fast_retrieve-->>try_deterministic_memory_retrieval: hits or empty/error
alt hits found
try_deterministic_memory_retrieval-->>run_subagent: completed formatted outcome
else no usable result
try_deterministic_memory_retrieval-->>run_subagent: None
end
else fast path unavailable
try_deterministic_memory_retrieval-->>run_subagent: None
end
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b1a6469e61
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/openhuman/agent/harness/subagent_runner/ops/runner.rs`:
- Around line 186-204: The config load in the subagent runner currently swallows
failures via Config::load_or_init().await.ok()? with no observability. Update
the runner logic in the same path as fast_retrieve to handle load_or_init errors
explicitly, log them with tracing::warn! (including task_id and the error
details, similar to the fast_retrieve branch), and then fall back to None
instead of silently discarding the failure.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 14961c47-f3ec-421f-92a9-73c1c89328a0
📒 Files selected for processing (1)
src/openhuman/agent/harness/subagent_runner/ops/runner.rs
|
Verdict: approve. The PR satisfies #4677 — it short-circuits the Verified:
Non-blocking nit:
Both review threads are genuinely resolved: Codex's relevance-floor P2 is addressed by the entity-grounding guard ( |
YellowSnnowmann
left a comment
There was a problem hiding this comment.
Approving — feature satisfied, no blocker/major issues, review comments addressed in code, CI green. Detailed review in the PR comment above.
|
@YellowSnnowmann good catch on the non-blocking nit — addressed in 28d2c72. The fast-path early return now routes through a shared |
…odel walk when data is present) retrieve_memory (chat delegate) and call_memory_agent both spawn the `agent_memory` sub-agent via run_subagent, which runs a model-driven walk (up to max_iterations LLM round-trips: walk -> drill_down -> fetch_leaves -> synthesize). On the eval tiers that is ~30-40s PER call even when data is present, dominating agent-turn latency (contrast goal_get 0s, thread_message_list 7ms). The cost is the per-iteration model round-trips, not the retrieval: the deterministic `fast_retrieve` (E2GraphRAG — query-entity + dense/semantic recall + cosine rerank, no LLM) returns the same tree hits in a single ~1-3s pass, but was only wired into the benchmark harness, never the production retrieve path. Add a deterministic fast path in run_subagent, gated on the agent being `agent_memory` (so it covers both the retrieve_memory delegate and call_memory_agent). Run fast_retrieve on the task prompt and, when it finds data, return the hits directly (iterations=0, Completed), skipping the model walk. Fall through to the full sub-agent when the fast path is disabled, when config/retrieval errors, or when nothing is found (the empty/degraded case is respects the ambient source-scope, so memory scoping is preserved. Env kill-switch OPENHUMAN_MEMORY_FAST_PATH=0 (or false/no/off) forces the full walk for A/B without a rebuild. Unit tests cover the pure pieces: env opt-out parsing, the empty->None fallback, hit formatting (scope/content/score, numbering, singular/plural), and oversized-body truncation. Closes tinyhumansai#4677
…st path Address PR review on the deterministic memory fast path (tinyhumansai#4677): - Codex (P2): require entity/topic grounding before short-circuiting. Without it, an ungrounded query against a populated profile takes fast_retrieve's pure global-dense branch, which reranks/truncates whatever summaries exist and would surface unrelated top-k memories as a completed retrieval instead of letting the model agent judge relevance. Extract query entities first; defer to the full sub-agent when the query yields none. hit.score is not a usable relevance floor here (local branch = entity-coverage count, global branch = summary importance, not query relevance), so grounding is the correct gate. - CodeRabbit: config load failure was swallowed via .ok()?, silently and permanently degrading the fast path with zero observability — the exact latency regression this PR fixes. Log a warning before falling back, matching the fast_retrieve error path.
…chars cap The deterministic memory fast path early-returned before run_subagent's max_result_chars truncation, so a cap added to agent_memory later would be silently ignored on the fast path. Extract the truncation into a shared apply_max_result_chars helper and apply it in both paths. No behavior change today (agent_memory is uncapped and the fast-path block is self-bounded); future-proofs against the paths diverging. Addresses @YellowSnnowmann review nit on runner.rs.
|
Rebased onto main, resolved conflicts in |
28d2c72 to
5e99319
Compare
oxoxDev
left a comment
There was a problem hiding this comment.
Review ✅ — approve
Sound, low-risk. The fast path is gated strictly on definition.id == "agent_memory", placed after the spawn-depth + tier gates, so no other sub-agent routing through the generic run_subagent is affected. try_deterministic_memory_retrieval returns None (→ unchanged model walk) on any of {disabled, empty query, config err, ungrounded, retrieval err, zero hits}, so every degraded path falls back.
Verified
- Parity:
agent_memoryissandbox_mode="read_only"; its tools record no access/recency/touch writes, so the walk has no side effects the fast path skips. The grounded branch hydrates leaf chunks (not summary-only), so a hit isn't categorically shallow. Zero-hit → fallback covers the empty-store false-negative. - Determinism real: sort by coverage→latest_ts→id, no LLM sampling; spaCy-down degrades to regex grounding → more queries defer to the walk (safe).
Non-blocking
- [low]
format_deterministic_memory_hitsprintshit.scoreas(relevance X.XX), but in the grounded (resolve_local) branch that score is an entity-pair coverage count (2.0, 3.0…), not a 0–1 relevance — your own commit-2 note says it "is not a usable relevance floor." The parent model may misweight "(relevance 3.00)". Relabel per-branch (e.g. "entity matches N") or normalize. - [low] Recall-depth reduction is silent:
fast_retrieveis a bounded single pass (max_hops=2, no model-drivendrill_down/re-query). A grounded but multi-hop query where the walk would drill deeper returns a shallower non-empty result that ships asCompleted(fallback only fires on empty). Intended trade with the env kill-switch, but nothing measures parity — worth calling out; ideally a populated-store parity smoke. - [low]
extract_query_entitiesruns twice (guard + insidefast_retrieve) → 2× spaCy round-trips; thread the ids through. Test gap: nothing asserts fires-only-for-agent_memory / ungrounded-defers / empty-falls-through (unit-testable with a fake config).
CodeRabbit config-swallow, Codex no-match gating, and the max_result_chars divergence are all fixed in commits 2–3.
Problem (issue #4677)
retrieve_memorytakes ~30–40s per call even when it finds data, dominating agent-turn latency (contrastgoal_get0.0s,read_workspace_state0.07s,thread_message_list0.007s). One turn spentretrieve_memory ×4 = 32.5 + 39.1 + 30.8 + 38.7 ≈ 141s.Where the latency goes (verified in code)
retrieve_memory(the chat delegate) andcall_memory_agentboth spawn theagent_memorysub-agent viarun_subagent(subagent_runner/ops/runner.rs). That agent runs a model-driven walk — up tomax_iterationsLLM round-trips (walk → drill_down → fetch_leaves → synthesize). On the eval tiers, those per-iteration round-trips are the ~30–40s; the retrieval work itself is not the bottleneck.The deterministic retriever
fast_retrieve(memory_tree/retrieval/fast.rs— E2GraphRAG: query-entity + dense/semantic recall + cosine rerank, no LLM in the loop) already returns the same tree hits in a single ~1–3s pass (one entity extraction + one query embed + DB reads). But it was only wired into the benchmark harness (agent_memory/ops.rs), never the production retrieve path — so every real retrieval paid the full model loop.Fix (minimal, targeted, covers both entry paths)
In
run_subagent(the shared seam for theretrieve_memorydelegate andcall_memory_agent), before the model walk, add a deterministic fast path gated on the agent beingagent_memory:fast_retrieveon the task prompt;iterations = 0,Completed), skipping the model walk;fast_retrievereads the ambient source-scope, so memory scoping is preserved; these are the same recall calls the model's first tool step would have made. Env kill-switchOPENHUMAN_MEMORY_FAST_PATH=0(false/no/off) forces the full walk for A/B without a rebuild.Result: the data-present happy path drops from ~35s to ~1–3s (no LLM round-trips), while hard/empty queries still get the full walk.
This is distinct from #4655 (which only fixes the empty/degraded early-exit); this targets the happy path.
Repo note
The fix is in openhuman, not
vendor/tinyagents: the harness sub-agent loop is generic and correct — the slowness came from openhuman routing memory retrieval through a model-driven agent when a deterministic retriever already existed. No tinyagents change needed.Tests
Pure, deterministic unit tests in
runner.rs(fast_path_tests): env opt-out parsing (default on;0/false/no/off→ off), the empty→Nonefallback, hit formatting (scope/content/score, numbering, singular/plural wording), and oversized-body truncation.Checks
Per the active no-local-builds directive (disk pressure):
cargo fmtonly — no build/test locally; CI verifies.Closes #4677
Summary by CodeRabbit