fix(memory/holographic): track retrieval counts across retriever paths - #55729
fix(memory/holographic): track retrieval counts across retriever paths#55729BongSuCHOI wants to merge 1 commit into
Conversation
Related to the narrower open PR #39664 (increments |
|
Thanks for triaging. Yes, this PR intentionally covers the broader retriever surface via the shared |
teknium1
left a comment
There was a problem hiding this comment.
Verified that the counter gap remains on current main: prefetch() and fact_store route through FactRetriever, whose result paths return without changing retrieval_count (plugins/memory/holographic/retrieval.py:106-112, 189-190, 257-258, 335-336, 478-479; plugins/memory/holographic/__init__.py:206-210, 284-318). The shared helper is a good fit, and current main's shared store lock is re-entrant (plugins/memory/holographic/store.py:153).
Problems
- The new structural parametrization does not exercise
_score_facts_by_vector(): itsprobecase has no category (tests/plugins/memory/test_holographic_retrieval_count.py:66), but that helper is reached only through the category-bank branch (plugins/memory/holographic/retrieval.py:139-152).
Suggested changes
- Add a categorized
proberegression and assert the returned fact IDs increment, covering the category-bank/vector route.
Automated hermes-sweeper review.
| ) | ||
| assert _counts_by_id(provider) == {kept: 2, other: 0} | ||
| finally: | ||
| provider.shutdown() |
There was a problem hiding this comment.
This probe case has no category, so it takes probe's direct scoring path rather than the category-bank branch that calls _score_facts_by_vector() (retrieval.py:139-152). Please add a categorized probe assertion so the new bookkeeping in that helper has regression coverage.
Summary
Fixes #17899.
Holographic memory exposes
retrieval_count, but the production retrieval paths used byprefetch()andfact_storereturned facts without incrementing that usage counter.MemoryStore.search_facts()had inline counter logic, but the tool/provider paths go throughFactRetriever.search(),probe(),related(),reason(), and_score_facts_by_vector()instead.This PR moves the counter update into a shared store helper and records retrievals for every fact list returned by the retriever.
Changes
MemoryStore.mark_retrieved()as the single helper forretrieval_countupdates.search_facts()path.FactRetriever.search()FactRetriever.probe()FactRetriever.related()FactRetriever.reason()_score_facts_by_vector()Related prior work
I searched existing issues/PRs first. Two closed PRs had addressed the same bug class:
search().This PR reintroduces the broad fix with focused tests against current
main.Testing