fix(memory/holographic): track retrieval_count on all retrieval methods - #21022
Closed
zons-zhaozhy wants to merge 2 commits into
Closed
fix(memory/holographic): track retrieval_count on all retrieval methods#21022zons-zhaozhy wants to merge 2 commits into
zons-zhaozhy wants to merge 2 commits into
Conversation
The retrieval_count column in the facts table was never incremented by
FactRetriever, even though store.search_facts() had this logic. Since the
production code path uses FactRetriever.search/probe/related/reason (not
store.search_facts()), every fact's retrieval_count stayed at 0 forever.
This broke the trust scoring model's ability to factor in usage frequency
— frequently retrieved facts should gain trust over time, but the signal
was always zero.
Changes:
- Extract store.increment_retrieval_count() from search_facts() for reuse
- Add FactRetriever._record_retrieval() that delegates to the store method
- Call _record_retrieval() at every return point in:
search(), probe(), related(), reason(), _score_facts_by_vector()
- contradict() intentionally excluded: it returns fact pairs, not
individual fact retrievals, so retrieval_count semantics don't apply
- store.search_facts() now calls the shared method (no duplication)
Performance: ~0.4ms overhead per retrieval on a 500-fact store (in-process
SQLite), which is negligible for the typical holographic store size.
Test coverage: 8 new tests covering search/probe/related/reason
incrementing, empty-result safety, and accumulation across calls.
212 existing tests pass with zero regressions.
alt-glitch
added
type/bug
Collaborator
…e_auto _resolve_auto() Step 1 only passed explicit_base_url when main_provider was 'custom', causing named providers (zai, anthropic, gemini, etc.) to lose model.base_url from config.yaml in the auxiliary auto-chain. This forced Z.AI users to hit _resolve_zai_base_url() probe results which randomly hop between api.z.ai and open.bigmodel.cn on every call, triggering intermittent 'Initializing agent...' rebuilds. The CLI main-model path (resolve_runtime_provider L1277-1285) already respects cfg_base_url; this makes the auxiliary path consistent. Ref: NousResearch#16719, NousResearch#17737, NousResearch#19437
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #17899
The
retrieval_countcolumn in thefactstable was never incremented during normal operation. Whilestore.search_facts()had the correct UPDATE logic, the production code path goes throughFactRetriever.search/probe/related/reason(notstore.search_facts()), so every fact'sretrieval_countstayed at 0 forever.This broke the trust scoring model's ability to factor in usage frequency — frequently retrieved facts should gain trust over time, but the signal was always zero.
Root Cause
Two search paths exist in the holographic memory plugin:
retrieval_count?store.search_facts()FactRetriever.search/probe/related/reason()Changes
1. Extract shared
store.increment_retrieval_count()(store.py)Pulls the SQL UPDATE out of
search_facts()into a dedicated method for reuse:fact_id IN (...)) — safe against SQL injectionNoneIDs (defensive, shouldn't happen but costs nothing)2. Add
FactRetriever._record_retrieval()(retrieval.py)Single helper that delegates to
store.increment_retrieval_count(). Called at every return point in:search()— full-text + vector hybrid searchprobe()— entity recall (falls back tosearch())related()— structural adjacency searchreason()— compositional multi-entity search_score_facts_by_vector()— HRR vector fallback scoringcontradict()is intentionally excluded: it returns fact pairs (not individual retrievals), soretrieval_countsemantics don't apply.3. Deduplicate
store.search_facts()(store.py)Now calls
increment_retrieval_count()instead of inline SQL — single source of truth for the UPDATE logic.Relationship to #17910
@luyao618's #17910 correctly identified and fixed the same issue in
search(). I independently arrived at the same root cause and fix for that method, but noticed thatprobe(),related(),reason(), and_score_facts_by_vector()have the same gap. This PR extends the fix to all five retrieval entry points and extracts a shared helper to avoid duplicating the UPDATE SQL.If the maintainers prefer, the changes here could be folded into #17910 instead — happy to close this PR in that case.
Test Coverage
8 new tests in
tests/plugins/memory/test_holographic_retrieval_count.py:test_search_increments_countsearch()increments matched factstest_search_accumulatessearch()calls accumulatetest_search_empty_results_nooptest_probe_increments_countprobe()(entity recall) incrementstest_related_increments_countrelated()(adjacency) incrementstest_reason_increments_countreason()(compositional) incrementstest_record_empty_list_noop_record_retrieval([])is safetest_record_accumulatesRegression: 212 existing tests pass with zero failures.
Performance
~0.4ms overhead per retrieval on a 500-fact store (in-process SQLite, single UPDATE with IN clause). Negligible for the typical holographic store size.