fix(intelligence): integrate retention fields into runtime paths - #1084
Conversation
…anbase#1082) reinforcement_factor, initial_retention, and current_retention were written during process_memory_metadata() but never consumed in the core runtime paths (should_forget, on_get, search ranking). This commit: - should_forget() now computes effective_retention = initial_retention * decay_factor so higher-importance memories survive longer - New reinforce() method boosts current_retention with diminishing returns when a memory is accessed at or after its next_review time - on_get() triggers reinforcement on review-due access and preserves current_retention during periodic reprocessing (access_count % 5) - New calculate_current_retention() provides unified real-time retention for search ranking and display - process_search_results() uses effective_retention instead of raw decay Fixes oceanbase#1082
|
I found one runtime issue that I think should be addressed before merging.
In Could we either apply the reinforcement result before the forget check and make |
Address review feedback from oceanbase#1084: current_retention was written by reinforce() but never consumed by should_forget() or search ranking. - should_forget() now uses max(initial_retention * decay_factor, current_retention) so reinforced memories are protected from premature forgetting - on_get() applies reinforcement result to normalized memory before the forget check, preventing the same-call reinforce-then-forget race - calculate_current_retention() returns max(base, stored current_retention) so search ranking reflects reinforcement boosts - 4 new tests covering reinforcement-protects-from-forgetting and search-ranking-reflects-reinforcement scenarios
|
@wayyoungboy Thanks for the thorough review! You're absolutely right, current_retention was being written but never read by the runtime consumers. I've pushed a follow-up commit (dcaf714) that addresses the issue: Changes:
Tests added (4 new, 76 total intelligence tests pass):
All 76 intelligence-related unit tests pass with zero regressions. |
wayyoungboy
left a comment
There was a problem hiding this comment.
Requesting changes because the latest follow-up makes current_retention a permanent floor for all memories created through the normal metadata path. process_memory_metadata() initializes current_retention to initial_retention, and this code then uses max(initial_retention * decay_factor, current_retention). Since the stored current_retention is not itself decayed, a realistic working memory with initial_retention=0.5 remains at an effective retention of 0.5 even after its time-decayed base retention has fallen below the 0.3 forget threshold, so should_forget() returns false indefinitely. The same floor is exposed through calculate_current_retention() and search ranking.
The new tests miss this because the should-forget and base retention cases hand-build metadata without the current_retention field that real memories always get. I think the fix needs to decay stored current_retention, store only review boosts separately, or otherwise avoid treating the initialized current_retention as a non-decaying lower bound.
Treat stored current_retention as a timestamped retention snapshot instead of a permanent floor, so normal memories can still decay while recent review boosts remain effective.
|
@wayyoungboy Thank you for the careful review! Fixed in b740973. What changed:
Regression coverage added:
Verification:
Please take another look when convenient. |
wayyoungboy
left a comment
There was a problem hiding this comment.
Rechecked the current head after the latest retention fix. The previous blocker appears addressed in the diff:
current_retentionis now treated as a timestamped snapshot and decays fromlast_reviewed/ creation time instead of acting as a permanent floor.should_forget()now uses the unified real-time retention calculation.reinforce()boosts the decayed real-time value, andon_get()applies the full reinforcement update before the same-call forget check.- The added tests cover initialized retention decaying below the forget threshold, search ranking not treating initialized retention as fixed, and reinforcement using the decayed snapshot before boosting.
I did not find a new blocking issue in this static re-review. I am not turning this into an approval in this pass because I did not run the PR test plan locally.
…anbase#1084) * fix(intelligence): integrate retention fields into runtime paths (oceanbase#1082) reinforcement_factor, initial_retention, and current_retention were written during process_memory_metadata() but never consumed in the core runtime paths (should_forget, on_get, search ranking). This commit: - should_forget() now computes effective_retention = initial_retention * decay_factor so higher-importance memories survive longer - New reinforce() method boosts current_retention with diminishing returns when a memory is accessed at or after its next_review time - on_get() triggers reinforcement on review-due access and preserves current_retention during periodic reprocessing (access_count % 5) - New calculate_current_retention() provides unified real-time retention for search ranking and display - process_search_results() uses effective_retention instead of raw decay Fixes oceanbase#1082 * fix(intelligence): make current_retention a runtime-effective field Address review feedback from oceanbase#1084: current_retention was written by reinforce() but never consumed by should_forget() or search ranking. - should_forget() now uses max(initial_retention * decay_factor, current_retention) so reinforced memories are protected from premature forgetting - on_get() applies reinforcement result to normalized memory before the forget check, preventing the same-call reinforce-then-forget race - calculate_current_retention() returns max(base, stored current_retention) so search ranking reflects reinforcement boosts - 4 new tests covering reinforcement-protects-from-forgetting and search-ranking-reflects-reinforcement scenarios * fix(intelligence): decay current retention snapshots Treat stored current_retention as a timestamped retention snapshot instead of a permanent floor, so normal memories can still decay while recent review boosts remain effective.
Summary
Fixes #1082
reinforcement_factor,initial_retention, andcurrent_retentionwere written duringprocess_memory_metadata()but never consumed in the core runtime paths (should_forget(),on_get(), search ranking). This meant:memory_typewere forgotten at the same time regardless of importancecurrent_retentionwas a static display value that never changedChanges
should_forget()now computeseffective_retention = initial_retention * decay_factorso higher-importance memories survive longer than low-importance onesreinforce()method boostscurrent_retentionwith diminishing returns when a memory is accessed at or after itsnext_reviewtime, and advancesreview_count/next_review/last_reviewedon_get()integration triggers reinforcement on review-due access and preservescurrent_retentionduring periodic reprocessing (access_count % 5) instead of resetting itcalculate_current_retention()provides a unified real-time effective retention calculation for search ranking and displayprocess_search_results()useseffective_retention(=initial_retention * decay_factor) instead of rawdecay_factorfor rankingFiles Changed
src/powermem/intelligence/ebbinghaus_algorithm.py_resolve_initial_retention(),reinforce(),calculate_current_retention(); modifiedshould_forget()src/powermem/intelligence/plugin.pyon_get()now triggers review reinforcement and preserves retention during reprocessingsrc/powermem/intelligence/intelligent_memory_manager.pyprocess_search_results()useseffective_retentiontests/unit/intelligence/test_retention_runtime.pytests/unit/intelligence/test_ebbinghaus_decay_rate.pydecay_factor->effective_retention)Test plan
test_retention_runtime.py