feat(retrieval): ACT-R fan-effect ranking for the entity lane, default off (#1176) - #1234
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Sorry @robotrocketscience, you have reached your weekly rate limit of 500000 diff characters.
Please try again later or upgrade to continue using Sourcery
|
Warning Review limit reached
Next review available in: 14 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
robotrocketscience
added
the
attn:review
Reviewer's GuideImplements an ACT-R fan-effect–based ranking mode for the L2.5 entity lane, wires it through both retrieval paths behind a tri-state flag resolved from env/kwargs, and adds a focused test suite and changelog entry to support an A/B experiment with default off. Sequence diagram for ACT-R fan-effect ranking flow in retrievalsequenceDiagram
actor Hook
participant retrieve_v2
participant is_fan_effect_enabled
participant _env_fan_effect_override
participant retrieve_with_tiers
participant _cost
participant _l25_hits
participant MemoryStore
Hook->>retrieve_v2: retrieve_v2(use_fan_effect=None)
retrieve_v2->>is_fan_effect_enabled: is_fan_effect_enabled(kwarg=use_fan_effect)
is_fan_effect_enabled->>_env_fan_effect_override: _env_fan_effect_override()
_env_fan_effect_override-->>is_fan_effect_enabled: env_override
is_fan_effect_enabled-->>retrieve_v2: fan_effect_enabled
retrieve_v2->>retrieve_with_tiers: retrieve_with_tiers(use_fan_effect=fan_effect_enabled)
retrieve_with_tiers->>_cost: _cost(use_fan_effect)
_cost->>_l25_hits: _l25_hits(use_fan_effect)
_l25_hits->>MemoryStore: lookup_entities(keys, limit, origin_tiebreak, fan_effect=use_fan_effect)
alt fan_effect is True
MemoryStore->>MemoryStore: _lookup_entities_fan(keys, limit, origin_tiebreak)
else fan_effect is False
MemoryStore->>MemoryStore: lookup_entities overlap SQL path
end
MemoryStore-->>_l25_hits: [(belief_id, overlap)]
_l25_hits-->>_cost: l25_hits
_cost-->>retrieve_with_tiers: beliefs
retrieve_with_tiers-->>retrieve_v2: RetrievalResult
retrieve_v2-->>Hook: RetrievalResult
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
PR-size soft capThis PR is over the advisory size threshold:
Bigger PRs collide with more open work, which under the parallel-session workflow tends to produce repeated This is advisory only — nothing is blocked. If the size is intentional (large refactor, module removal, generated code), apply the |
|
[claim:review:Toug:2026-07-31T05:13:00Z] |
|
Reviewed against a build of the branch head, on the live 44,584-belief store. The performance claim is wrong, and it is my faultThe body says "Cost is below the lane it replaces — 0.04 ms p50 against
So the fan lane is ~25× slower than the lane it replaces, not faster, and Verified fix, one line of caching.
Parity, and I confirmed memoising I would take the corrected numbers into the changelog too — a "cost is below What I verified, and one correction to my own review
On the reported escapeReporting the eighth mutation rather than dropping it from the table is the Not blockingFull suite on the branch head: 6559 passed, 69 skipped, 71 xfailed — matches the body exactly. CI green. Discretion grep clean on Approving on the mechanism. I would like the latency numbers corrected and the |
robotrocketscience
added
attn:unblock
|
[release:review:Toug:2026-07-31T05:21:48Z] |
|
Reviewer note — #1233 was filed after this PR opened and is the producer-side half of the same surface. It measures Two things follow for this PR, neither of them a change request. It confirms the mechanism. The fan term discounts It also names a behaviour change this PR does not test. Fan rewards rarity, so a five-word fragment attached to exactly one belief gets maximal activation. Where the count lane treats a fragment match and a real identifier match identically — both I have left the tests as they are rather than adding a fragment fixture, because what the right behaviour is is a disposition question, not a coverage gap — and inventing an assertion for it here would pin an answer nobody has ratified. The sequencing consequence is on #1233 in full; short version: fan makes the lane strictly more sensitive to whatever |
99fd3f5 to
c9c00eb
Compare
|
You are right, and the finding is bigger than "the body quotes a stale number" — the implementation was genuinely slow. Fixed in I re-measured independently rather than taking the numbers, on the live 44,584-belief store over the 38 distinct non-empty query-entity sets from real user turns (harness blocks excluded using the hook's own
Reproduces your result: ~35x on my run, and the count is the whole of it. Your diagnosis of the cause is exactly right — the only partial index on Keyed on One case declines the memo. Three new tests, each mutation-checked to fail alone:
Plus The changelog line is corrected to the measured parity figure and now names the memoisation it depends on — you were right that a number 35x out is exactly what gets quoted into a default-flip argument later, so it should not sit there as a bare claim. Also worth recording against this lane while it is in view: I ran the #1177 Roaring-postings kill experiment on the same store afterwards and measured the shipped Re-flagging |
robotrocketscience
added
attn:review
|
Pre-flight result for the A/B this PR exists to enable, posted in full on #1176: the L2.5 lane fires on 93.5% of real user turns against the live store but only 10.5% of real LoCoMo questions against a LoCoMo-shaped one, and the sweep is flat in corpus size (9.5% at 232 beliefs, 10.5% at 622). The mechanism is the one #1233 documents, seen from the query side: the extractor emits whole noun-phrase fragments, so a match needs the same five-word span verbatim on both sides. The live store supplies that because it ingests its own sessions — which is #1174's echo pathology showing up in a second place, not the entity lane doing retrieval work. Nothing here blocks this PR. It ships default-off, the mechanism is correct, and its tests are synthetic by construction, so none of them depend on the fire rate. What it blocks is running the A/B on LoCoMo or LongMemEval and believing the answer — with the treatment inert on ~90% of queries, a null result would be a measurement failure rather than evidence about fan weighting. Recording it here so the connection between this PR and its A/B is not lost: the corpus has to be a replay of real production prompts against the live store, and whatever corpus is chosen should have its lane fire rate measured first. |
|
[claim:review:Toug:2026-07-31T05:56:34Z] |
|
Re-reviewed The memo is exact, and I checked the path that would break itKeying on That only holds if every write that changes the active count bumps the
Exact at every step. No path leaves the memo behind. Numbers reproduceLive 44,584-belief store, 150 distinct real user-turn entity sets:
Parity with the lane it replaces, and Mutations
19 pass unmutated. Both new guards are covered by a test that distinguishes On the generation-0 carve-outDeclining the memo when Closing my own findingThe corrected changelog line is what I asked for. To be explicit about No further findings. Full suite on the branch head: 6577 passed, 69 skipped, 71 xfailed. CI green, discretion Still true and worth carrying into the A/B, unchanged from my first review: this |
robotrocketscience
added
ready-to-merge
|
[release:review:Toug:2026-07-31T06:02:14Z] |
`lookup_entities` gains `fan_effect` (default off), which orders by Anderson's ACT-R fan effect — `A_i = Sum_j ln((N + 1) / fan_j)` over the query entities a belief carries — instead of `COUNT(DISTINCT entity_lower)`. The count prices every matched entity the same, but `tmp` sits in 1,480 beliefs and 86% of entities sit in exactly one, so a match on a ubiquitous token buys the rank of a match on a unique symbol. Written as a log ratio the term is algebraically IDF, so it reuses a calibration already in the system. Every term is non-negative, so an extra match cannot demote, and equal fans degenerate to the overlap count exactly. No `entity_fan` table and no migration, per the operator disposition: fan is counted inline over the query's own keys and the log taken in Python, since SQL LN() needs SQLITE_ENABLE_MATH_FUNCTIONS. Fan counts active beliefs only, matching the lane's own valid_to filter. The returned tuple still carries the overlap count — this changes the ordering, not the interface.
`AELFRICE_FAN_EFFECT` resolves through `is_fan_effect_enabled` (env -> kwarg -> default False) and threads `retrieve_v2` -> `retrieve_with_tiers` -> `_l25_hits` -> `lookup_entities`. `retrieve()` passes it too: the hooks call `retrieve()`, not `retrieve_v2`, and a lane reachable only from the latter is inert on the path the A/B has to measure (#1107). Default off. The kill gate cleared and the measured cost is below the lane it replaces, but that the reorder ranks *better* is what the A/B decides; the default flip is a separate operator call. Tests pin the reorder, the retired-belief fan guard, the degenerate equal-fan case, limit-after-sort, composition with the #1089 origin tie-break, and reachability from `retrieve()` — each with a distinguishing assert. One deliberate negative result is recorded in the docstrings rather than asserted: spelling the `retrieve()` kwarg `False` instead of `None` leaves the file green, because the resolver is env-first. `None` is convention there, not a guard, and the code comment now says so instead of claiming otherwise.
Insert-only under [Unreleased] -> Added. Records the kill-gate figure (337 sole-high-fan displacements on the user arm), the refuted stopword alternative, the no-migration disposition, the measured cost, and the arm-split requirement the bench has to honour.
Review finding: the lane recomputed `count_active_beliefs()` per query. That is `SELECT COUNT(*) FROM beliefs WHERE valid_to IS NULL`, and the only partial index on that predicate carries `(origin, created_at)` and does not cover a bare count — so it scans every row. Measured on a 44,584-belief store over 38 real user-turn query-entity sets, it costs 1.315 ms and made the fan lane ~35x slower than the overlap lane it replaces, not faster. Memoised on `store_generation()`, a keyed `schema_meta` lookup at 0.0031 ms that is bumped inside the same transaction as every content mutation — so the memo is exact rather than merely fresh. Result is parity: 0.039 ms p50 against the shipped lane's 0.045 ms, with ordering identical memoised or not on all 38 sets. Generation 0 declines the memo. It reads 0 both for an unmutated store and for a pre-v4.2 DB not yet reopened, and those are indistinguishable from here; caching under an unmoving key would pin a stale N, which is not free because N multiplies each belief's overlap count and so moves the ranking between beliefs of differing overlap. Each of the three behaviours has a distinguishing test: reverting the memo, dropping the generation check, and removing the generation-0 fall-through each turn exactly one red.
The entry quoted 0.04 ms against 0.09 ms, taken from the R&D round, which measured an implementation shape without a per-query `count_active_beliefs()`. The shipped shape has one. Corrected to the measured parity figure and the memoisation it depends on, since a performance line that is out by 35x is the kind that gets quoted into a default-flip argument later.
c9c00eb to
30ce1e5
Compare
|
merge-train: merged 30ce1e5 → |
github-actions
Bot
removed
the
ready-to-merge
1 participant
Builds proposal 3 of #1176 — the ACT-R fan effect on the L2.5 entity lane — behind a flag, default off. The operator disposition of 2026-07-30 ratified proceed to the A/B; this is the mechanism the A/B needs, built to the three constraints that disposition carried forward.
Refs #1176.
What it does
MemoryStore.lookup_entitiesgainsfan_effect. Off, it is the shippedCOUNT(DISTINCT entity_lower)ordering, byte for byte. On, it orders by Anderson's fan-effect activationover the query entities the belief carries. Written as a log ratio this is algebraically IDF — Anderson (1993) makes that identification explicitly — so it lands in a system that already computes idf rather than introducing a second calibration to tune.
The count lane prices every matched entity the same. The corpus does not:
tmpsits in 1,480 beliefs andandin 1,026, while 86% of entities sit in exactly one. So a match on a corpus-ubiquitous token buys the same rank as a match on a unique symbol — on the one lane that holds unconditional budget precedence.Two properties make the change conservative rather than a new axis. Every term is non-negative (
fan_j <= N + 1), so an additional match can never demote a belief. And with all fans equal the activation is a constant multiple of the overlap, so the ordering degenerates exactly to the lane it replaces — that is a test, not a claim.The three carried-forward constraints
entity_fantable, no migration. Fan is counted inline over the query's own keys (at most 512, in practice at mostquery_entity_cap). This resolves the issue's internal contradiction — summary line "~25 LOC, no migration" against a proposal body asking for a table — in favour of the summary. Given [Umbrella] Deployment and operational hardening #1161, dropping the migration is the whole risk profile.LN(). It requiresSQLITE_ENABLE_MATH_FUNCTIONS, which is not guaranteed across the support matrix. The logarithm is taken in Python.<task-notification>blocks, 100% high-fan-bearing by construction, and pooling them fabricates the result in the proposal's favour.Cost is below the lane it replaces — 0.04 ms p50 against 0.09 ms, tail 102 ms down to 8 ms — because the shipped path sorts every matching row while this one sorts only the grouped beliefs. Two indexed reads replace one.
Fan counts active beliefs only, matching the lane's own
valid_to IS NULLfilter. Otherwise a retired belief would keep damping every term it once carried.Reachable from the path that will be measured
retrieve()— notretrieve_v2— is what the hooks call, and #1107 is the standing example of a lane that existed only in the latter. The flag threadsretrieve_v2→retrieve_with_tiers→_l25_hits→lookup_entities, andretrieve()passes it through. Dropping the wiring at any of those joints turnstest_env_var_reaches_the_production_retrieve_pathred; both joints were checked by mutation, not by inspection.Verification
Full suite: 6559 passed, 69 skipped, 71 xfailed. Eight mutations, seven caught:
fan_effectdispatchlimitbefore the sortvalid_to IS NULLtest_retired_beliefs_do_not_inflate_fantest_origin_tiebreak_composes_with_fan_l25_hits→lookup_entitiesretrieve_with_tiers→_l25_hitsretrieve()passesFalseinstead ofNoneThe escape is reported rather than papered over, and it corrected the code. I had written both a comment and a docstring claiming that
Noneat that call site is load-bearing — that a hardFalsewould leaveAELFRICE_FAN_EFFECT=1inert on the production path. It would not: the resolver is env-first, so the env var overrides either spelling.Noneis still the right spelling, because a.aelfrice.tomltier would have to read through it and that tier is the natural companion to a default flip — but it is a convention, not a guard. The comment and the test docstring now say that, and the test asserts only what it can: that the env var reachesretrieve()at all.The fixtures are built so the two lanes must disagree — the rare-matching belief carries the alphabetically later id, so under the shipped overlap/id ordering it always loses, and every ordering assertion reverts to id order if the fan weighting is dropped.
test_the_two_lanes_actually_disagree_on_this_fixtureis the control that keeps the byte-identical assertions from passing against a fixture the reorder never touches.One fixture detail worth flagging: the live high-fan entities are bare tokens (
tmp,and,pr), but the end-to-end arm drives the real query-side extractor, which does not key on those shapes — baretmpextracts to nothing, and that arm would have silently measured a one-entity query. It uses a path-shaped entity instead. The lane arithmetic is identical; only query-side extraction differs.Discretion grep on added lines clean; CHANGELOG edit insert-only (verified
^-minus^---);scripts/check_changelog_dupes.pyclean.What this does not establish
That the reorder ranks better. Everything measured so far shows only that the lane is not inert with respect to fan (337 sole-high-fan displacements on the user arm against a "near-zero" kill condition) and that a fifth of its slot spend rides on corpus-ubiquitous terms. The A/B is what decides, and flipping the default is a separate operator call.
Summary by Sourcery
Gate an ACT-R fan-effect ranker for the L2.5 entity lane behind a feature flag and thread it through the production retrieval path, with tests and documentation for the new behavior.
New Features:
Enhancements:
Documentation:
Tests: