feat(exploration): seeded uniform exploration draw (#1176) - #1248
Conversation
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: 9 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 (3)
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 |
Reviewer's GuideImplements the library-side, deterministic uniform exploration draw mechanism using a SplitMix64 RNG, deterministic seed derivation, and cadence predicate, plus comprehensive tests and changelog entry; no retrieval wiring or schema changes are included. File-Level Changes
Possibly linked issues
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:Setr:2026-07-31T18:23:28Z] |
|
[claim:review:Gylf:2026-07-31T18:23:34Z] |
|
[release:review:Gylf:2026-07-31T18:23:39Z] |
|
[claim:review:Garsecg:2026-07-31T18:26:51Z] |
|
[release:review:Garsecg:2026-07-31T18:26:56Z] |
Review — three docstring-accuracy defects, no correctness defectThe implementation is right and I could not break it. Every number in the body Verified independently
1.
|
|
[claim:review:Garsecg:2026-07-31T18:27:52Z] |
|
[release:review:Garsecg:2026-07-31T18:27:57Z] |
|
[release:review:Setr:2026-07-31T18:27:59Z] |
|
[claim:review:Garsecg:2026-07-31T18:28:43Z] |
A belief that starts underranked is never retrieved, never referenced, and so never acquires evidence. Measured on the live store, that loop covers 37,489 of 44,586 active beliefs (84.1%) with no feedback or injection row, and only 1,352 (3.0%) have ever been injected at all. Pure library half: splitmix64, seed derivation from logged state only (scope_id, monotonic fire_idx, query), a cadence predicate, and an unbiased uniform draw without replacement. The draw is uniform rather than the specified A-Res weighting because the weight is invalid and inert. uncertainty_score is Beta differential entropy, so it is <= 0 and exactly 0 for Beta(1,1) -- key = u ** (1/w) divides by zero on 39 pool beliefs and exceeds 1 for the rest. After either sign repair the draw sits 0.0586 (affine) / 0.0890 (exp) total-variation from uniform, because two entropy values cover 88.1% of the pool.
…ct (#1176) Review found three docstrings asserting coverage the tests do not have. On a change whose stated standard is "each test fails if the specific property is removed", that is the claim worth getting right. All three verified by mutation; no code or assertion changed. 1. `test_the_draw_is_actually_uniform` claimed a biased `_bounded` shows up as a lopsided histogram. It does not -- naive `word % bound` leaves it green and reddens only `test_bounded_rejects_the_biased_tail_of_the_word_range`, which is what that test's own docstring says. Two adjacent docstrings asserted opposite things about one mutation. The off-by-one half is true and is kept, having been confirmed separately. 2. `test_widening_the_slot_count_keeps_the_earlier_slots` claimed it rules out "shuffle the whole pool then take `count`". It does not, and neither the review's proposed replacement (a reverse Durstenfeld shuffle) nor the original wording survives measurement: full forward Fisher-Yates then slice -> 25 passed reverse Durstenfeld then slice -> 25 passed size-`take` reservoir (Algorithm R) -> 2 failed Any permutation computed independently of `take` is prefix-stable for free, in either direction. What the test actually rules out is an implementation whose *early* choices depend on `take` -- a size-`take` reservoir, which is precisely the A-Res reservoir this proposal originally specified. The docstring now names that, so it is both accurate and relevant to the alternative someone might reach for. 3. `derive_seed` illustrated the separator with `("ab", 1)` vs `("a", "b1")`. `fire_idx: int`, so that is not a call anyone can make. Replaced with the reachable pair the test already uses, `("a", 11)` vs `("a1", 1)`, and the note that an arbitrary pair would let the mutation through. Also restores the "active, unlocked" qualifier on the 37,489 figure in the module docstring. The PR body and CHANGELOG carry it; the docstring dropped it, attributing an unlocked-filtered numerator to the whole active set.
|
[claim:merge:Gylf:2026-07-31T18:31:57Z] |
|
[release:merge:Gylf:2026-07-31T18:33:00Z] |
4a21610 to
e185124
Compare
Applied the three docstring corrections — but one of them needed correcting tooThe review above is right that all three docstrings overstate what their tests Re-verified
The correction to the correctionThe review proposes naming a reverse (Durstenfeld) shuffle as the class the The reason is structural rather than incidental: any permutation computed What the test does rule out is an implementation whose early choices depend Naming a class the test does not catch is the same defect as claiming coverage Landed —
|
|
[release:review:Garsecg:2026-07-31T18:35:09Z] |
|
[claim:review:Setr:2026-07-31T18:39:58Z] |
Re-verified
|
|
[release:review:Setr:2026-07-31T18:41:34Z] |
|
merge-train: merged e185124 → |
Refs #1176 (proposal 5). Library half only — no hot-path edit, no schema change.
Why
A belief that starts underranked is never retrieved, therefore never referenced, therefore never acquires evidence, therefore stays underranked. Nothing in the retrieval path breaks that loop. I measured how much of the store is inside it, on a fresh
.backupof the live 44,586-belief store:feedback_history, zeroinjection_eventsfeedback_historyrow92,685 feedback rows and 21,328 injection events land on 7,450 and 1,352 beliefs. The feedback is concentrated on the beliefs that were already winning, which is the loop stated as a measurement.
The draw is uniform — a correction to the proposal, not a simplification
The proposal specifies Efraimidis–Spirakis A-Res weighted reservoir sampling keyed on
scoring.uncertainty_score. That weighting is invalid as written and inert after repair. Both were measured before writing any code.Invalid.
uncertainty_scoreis Beta differential entropy, so on[0, 1]it is≤ 0, and exactly0forBeta(1, 1). A-Res requires strictly positive weights:key = u ** (1 / w)divides by zero on the 39Beta(1, 1)beliefs in the pool, and every other key lands> 1— not a reservoir. This is the same sign trap already on record for the log-domain rerank score.Inert. After either natural sign repair, the weighted draw is indistinguishable from uniform on this corpus:
H − H_minexp(H)Two entropy values cover 88.1% of the pool (four cover 98.2%), so the dominant classes draw at 0.99× and 1.15× their pool share. The proposal's own kill criterion — "if it is 2-valued, drop A-Res for plain uniform and save ~100 LOC" — is met. If the posterior ever becomes informative (it is currently a two-valued function of
type × originassigned atderive(), not a learned quantity) a weighted key drops back in behind the same seeded stream with no other change.Determinism
The seed is
blake2boverscope_id, the monotonicfire_idx, and the query — logged state only, no clock, norandom.randomis deliberately unused: its Mersenne state and float conversion are not a stable contract across CPython versions, and replay has to hold across upgrades.Two details that are easy to get wrong and are pinned rather than asserted:
\x1f. Without it,("a", 11, q)and("a1", 1, q)both rendera11qand seed the same draw. The colliding pair in the test is chosen deliberately — an arbitrary pair like("ab", 1)vs("a", 11)still differs and lets the mutation through._boundedrejects the biased tail rather than takingword % bound. No black-box test can see this — at a pool size of 40 against a 2⁶⁴ word range the skew is ~1e-18, and the statistical uniformity test passes with naive modulo. So it is pinned directly, with a hand-made word stream where the two paths provably disagree (bound = 3: rejection returns 2, naive returns 0). An exploration mechanism whose justification is "the ranker is systematically skewed" should not introduce its own skew.splitmix64_streamis pinned to Vigna's publishedsplitmix64.cvectors for seed 0 — the published values, not values captured from this implementation, so the generator is a contract rather than whatever the code happens to do.Verification
6815 passed, 69 skipped, 71 xfailed\x1fseparator and replacing the rejection limit with1 << 64both left the suite green on the first pass. Both now fail exactly one test each.takeswaps consume the same words), only slower. That is the prefix-stability propertydraw_uniformdocuments, so the escape confirms the docstring rather than exposing a gap.Not in this PR
The store-side pool query, the
exploration_eventsledger, and the retrieval wiring with its flag-off byte-parity gate. Threading an exploration slot into the pack changes the result shape and needs a migration, so it is its own reviewable change — and this module can be read and benched without a hot-path edit. Same splitclustering.pyandlock_consistency.pyshipped under.Summary by Sourcery
Introduce a deterministic, seeded uniform exploration draw mechanism to surface underranked beliefs without changing retrieval hot paths.
New Features:
Documentation:
Tests: