fix(determinism): total-order tiebreak and seeded stochastic steps (#1370) - #1393
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR makes belief search, enum extraction, and spectral eigenbasis computation deterministic. It adds total-order tie-breaking, sorted enum indexing, content-derived ARPACK seeds, explicit solver limits, and regression tests across insertion orders and Python hash seeds. ChangesDeterministic computation and ordering
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
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 |
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 |
Reviewer's GuideIntroduces deterministic behavior across three subsystems (BM25 FTS search truncation, enum value extraction, and ARPACK eigensolver) by enforcing total-order tie‑breaks, hash-seed–independent iteration, and a content-derived seeded start vector, along with targeted tests that adversarially validate these guarantees. Sequence diagram for deterministic ARPACK eigenbasis computationsequenceDiagram
participant compute_eigenbasis
participant deterministic_start_vector
participant _content_seed
participant default_rng as np_random_default_rng
participant eigsh as spla_eigsh
compute_eigenbasis->>compute_eigenbasis: check n, k_eff, dense/sparse
alt sparse_path
compute_eigenbasis->>deterministic_start_vector: deterministic_start_vector(L)
deterministic_start_vector->>_content_seed: _content_seed(L)
_content_seed-->>deterministic_start_vector: seed:int
deterministic_start_vector->>default_rng: default_rng(seed)
default_rng-->>deterministic_start_vector: rng
deterministic_start_vector->>default_rng: rng.standard_normal(n)
deterministic_start_vector-->>compute_eigenbasis: v0
compute_eigenbasis->>eigsh: eigsh(L, k_eff, which=SM, v0, tol=0.0, maxiter)
eigsh-->>compute_eigenbasis: eigvals, eigvecs
compute_eigenbasis->>compute_eigenbasis: order = np.argsort(eigvals, kind=stable)
compute_eigenbasis-->>compute_eigenbasis: return eigvals[order], eigvecs[:, order]
else dense_path
compute_eigenbasis->>compute_eigenbasis: np.linalg.eigh(L_dense)
end
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- If you want to keep
np.argsort(..., kind="stable")for exactly-degenerate eigenvalues, consider adding a small targeted test that exercises a graph with known eigenvalue degeneracy so the behavior change is explicitly validated; otherwise it may be clearer to drop thekindoverride as a non-essential rider. - In
test_value_compare_hashseed_1370.py,_SUBPROCESS_TIMEOUT_Sis defined but thesubprocess.runcall still uses a hard-codedtimeout=120; wiring this through the shared constant would make future tuning or debugging easier.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- If you want to keep `np.argsort(..., kind="stable")` for exactly-degenerate eigenvalues, consider adding a small targeted test that exercises a graph with known eigenvalue degeneracy so the behavior change is explicitly validated; otherwise it may be clearer to drop the `kind` override as a non-essential rider.
- In `test_value_compare_hashseed_1370.py`, `_SUBPROCESS_TIMEOUT_S` is defined but the `subprocess.run` call still uses a hard-coded `timeout=120`; wiring this through the shared constant would make future tuning or debugging easier.
## Individual Comments
### Comment 1
<location path="src/aelfrice/value_compare.py" line_range="149-153" />
<code_context>
+# `extract_values` would violate its own "same input → byte-identical
+# output" contract. Same bug class as the `MUTABLE_FIELDS` iteration in
+# `replay._mutable_field_diff`.
_ENUM_MEMBER_INDEX: Final[dict[str, tuple[str, str]]] = {
member: (category, sorted(group)[0])
for category, groups in ENUM_VOCAB.items()
for group in groups
- for member in group
+ for member in sorted(group)
}
</code_context>
<issue_to_address>
**suggestion (performance):** The comprehension sorts each group twice; you can avoid redundant work by reusing the sorted list.
`sorted(group)` is computed twice here, which can significantly increase cost for large or numerous groups.
You can compute it once per group and reuse it, e.g.:
```python
_ENUM_MEMBER_INDEX: Final[dict[str, tuple[str, str]]] = {
member: (category, members[0])
for category, groups in ENUM_VOCAB.items()
for group in groups
for members in (sorted(group),)
for member in members
}
```
This preserves deterministic ordering while avoiding redundant sorting.
</issue_to_address>
### Comment 2
<location path="tests/test_value_compare_hashseed_1370.py" line_range="24" />
<code_context>
+# One interpreter start per seed. The suite's 5s default is sized for
+# in-process unit tests, so under parallel load this would report as a
+# hang rather than as slowness (#1307).
+_SUBPROCESS_TIMEOUT_S = 120
+
+# Every group below has two or more members, which is the only place the
</code_context>
<issue_to_address>
**issue:** Align the subprocess timeout in the helper with the `_SUBPROCESS_TIMEOUT_S` constant.
The decorator and docstring both use `_SUBPROCESS_TIMEOUT_S`, but `_render` still hard-codes `timeout=120` in `subprocess.run`. Please use `_SUBPROCESS_TIMEOUT_S` (or pass it through) so the pytest timeout and subprocess timeout stay in sync when the constant changes.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
robotrocketscience
added
the
attn:review
|
Overlap notice — commit 2 of 3 here ( Content-scoped duplicate work: #1370 bundles that fix as §8 of four determinism sections, so no issue number or claim collided. Flagging rather than silently racing. Proposal: #1391 takes the fix; I drop commit 2 from this branch. The remaining two commits — I have asked #1391 to check whether its test is distinguishing before it lands: an in-process test cannot detect this defect, because the hash seed is fixed for the life of the interpreter. The test on this branch spawns six subprocesses under Reviewer: if you would rather this PR keep §8 and #1391 stand down, say so and I will reverse it. Not blocking on the answer — the other two commits are reviewable now. |
|
Reviewed the Your test is stronger than mine, verified rather than assumed. I ran and against the fixed version → One correction to a warning I posted on #1370, which I got wrong. I flagged that Still worth your attention (unchanged): #1370's AC6 ( I have not reviewed the |
|
[claim:review:Garsecg:2026-08-06T04:55:27Z] |
|
[claim:review:Idnn:2026-08-06T04:56:56Z] |
|
[release:review:Idnn:2026-08-06T04:57:01Z] |
2035074 to
b3f2d90
Compare
Review — the fixes are right, the self-criticism is accurate, and the gap you flagged is now closed in
|
robotrocketscience
added
attn:unblock
|
[release:review:Garsecg:2026-08-06T05:00:56Z] |
|
[claim:review:Setr:2026-08-06T15:55:19Z] |
|
[claim:review:Kulili:2026-08-06T15:55:24Z] |
|
[release:review:Kulili:2026-08-06T15:55:28Z] |
|
[claim:review:Idnn:2026-08-06T15:56:24Z] |
|
[release:review:Idnn:2026-08-06T15:56:29Z] |
b3f2d90 to
987ab90
Compare
Decisions made, branch rebuilt at
|
|
Both open Sourcery threads were real, so both are fixed rather than resolved-as-noise. Two commits at
Discretion grep on added lines clean; all six commits signed. |
Review — decisions independently reproduced, one residual left in the shipped surfaceI worked the two riders from scratch before reading your
The one thing still outstanding: the docstring makes the claim the commit message just retracted
The commit message now says Suggested replacement, which also promotes I built and verified this on top of Nothing else blocks. Everything else on this PR is verified and I am ready to drive it |
|
The red checks here are a GitHub Actions outage, not this diff. All of them —
Locally the branch is green — 7389 passed, 72 skipped, 71 xfailed. Re-running the failed |
The seven red checks here are a GitHub Actions outage, not this diffBefore anyone spends a cycle on them: From the GitHub could not fetch the action tarballs. Seven unrelated jobs failing together Do not "fix" anything for these, and do not FF-push around them. The Otherwise this PR is clean: zero unresolved review threads, and the substantive |
30cea94 to
c211ce9
Compare
`_ORDER_BY_BM25` had no secondary sort key, so `search_beliefs` and `search_beliefs_scored` truncated a bm25-ranked scan on SQLite's scan order whenever rows tied at the cut. That tail is the one production uses: `origin_tiebreak` defaults False and `retrieve()` hard-forces it False, with `l1_limit` at 50. Appending `, b.id` makes the order total, matching `list_unexplored_belief_ids`, which already argued the case. Refs #1370 (§5, §6), #1157.
`_ENUM_MEMBER_INDEX` was built by iterating the frozensets in `ENUM_VOCAB`, so its insertion order — and therefore the order `_extract_enums` emits slots — was keyed on string hash randomisation. `extract_values` documents "same input -> byte-identical output"; it was producing a different slot order in every process. Same bug class as the `MUTABLE_FIELDS` iteration already fixed in `replay`. Latent today because consumers only use the conflict set, but #1365 will render these slots into the injected prompt. Refs #1370 (§8), #1157.
`compute_eigenbasis` called `eigsh` with no `v0`, so ARPACK drew a start vector from its own RNG, whose state advances per call. Two solves of the same Laplacian returned different eigenvectors — numerically equivalent, but the heat-kernel authority ranking built on them was not reproducible from the write log, and no test asserted bit identity. `v0` is now derived from the matrix's own bytes (a constant vector is not usable — it is near an eigenvector of the normalized Laplacian and degenerates the Krylov subspace), and `maxiter` is explicit rather than inherited from SciPy's default. `tol=0.0` is passed for the same reason as `maxiter` — to pin a value the module depends on against a future SciPy default change — but unlike `maxiter` it changes nothing today: SciPy 1.17.1 already defaults `tol=0` (verified against the installed version), so it is a no-op guard, not a tightening. The earlier claim that it "pins tol to machine precision" overstated it. The eigenvalue sort is left as it was. A stable `argsort` was tried and dropped: on the bit-identical eigenvalues the seeded `v0` now guarantees, quicksort is already deterministic, so it does not serve #1370 — all it changes is which index wins an exactly-degenerate tie, a behaviour change with no test riding on a diff whose every other line is mutation-checked. If it is wanted it belongs in its own commit with a test that pins the tie. The lane is default-off, so this is not on the hot path; the flip is a config change away and the failure would be silent. Refs #1370 (§10), #1157.
…sses The §10 coverage was four repetitions inside one interpreter, which the PR body correctly names as the weaker of the two available assertions. It is weaker in a way that matters: the defect was ARPACK drawing `v0` from an RNG whose state advances *per call*, so a within-process repetition is exactly the shape a per-call RNG breaks — while a *per-process* source of variation would repeat identically four times in one interpreter and still differ between runs. #1157's contract is about reproducing a ranking from the write log in another process on another machine. The sibling §8 test already goes out-of-process for this reason; §10 had no equivalent. Six children under PYTHONHASHSEED 0-5, each building the graph itself so any set or dict ordering upstream of the solver is inside the blast radius rather than hidden by constructing it in the parent. The property does hold — the seed is blake2b over the CSR bytes feeding default_rng, with no hash randomisation involved — but "expected to hold" is what the pre-#1370 code also looked like. Mutation-verified: dropping the `v0=` argument makes this emit several distinct digests. A second assertion rejects empty child output, since a digest of nothing is also "one distinct result".
`sorted(group)[0]` sat in the dict comprehension's value position, so the group was re-sorted for every member it contained rather than once. Hoisting the sort into its own `for` clause makes it one sort per group and leaves the emitted order — the whole point of #1370 §8 — identical. Refs #1370 (§8).
`_render` hard-coded `timeout=120` while the pytest marker and the docstring both read `_SUBPROCESS_TIMEOUT_S`. Same value today, so the drift is silent — moving the constant would have left the subprocess cap behind. Refs #1370 (§8).
c211ce9 to
5be6840
Compare
robotrocketscience
added
the
ready-to-merge
github-actions
Bot
removed
the
ready-to-merge
|
merge-train: merged 5be6840 → |
1 participant
Closes #1370. Parent #1157 §5, §6, §8, §10.
Four sections, one theme: a ranked cut or a derived value that was not a function of the write log, because something in the path was unordered or unseeded. #1157's determinism contract is compositional — one non-deterministic step destroys the property for the whole pipeline.
Three commits, one per file plus its own tests.
§5 + §6 — one fix, two sections
_ORDER_BY_BM25had no secondary key, so ties at the top-K cut were broken by SQLite's scan order. NowORDER BY bm25(beliefs_fts), b.id. Both consumers covered —search_beliefsandsearch_beliefs_scored, each selecting_ORDER_BY_BM25_ORIGIN if origin_tiebreak else _ORDER_BY_BM25. The fix and its rationale already existed in the same file atstore.py:3958; this brings the default path in line.§8 —
extract_valuesoutput order varied per process_ENUM_MEMBER_INDEXiterated afrozenset, so member order differed between processes under hash randomisation — against a contract that says "Pure function. Same input → byte-identical output." Nowfor member in sorted(group).The test spawns six out-of-process children under
PYTHONHASHSEED0-5 and asserts one distinct rendering. The issue asked for two.grep -rn PYTHONHASHSEED tests/now hits only this file — it is genuinely the repo's first hash-seed test.§10 — ARPACK random start vector
eigshwas called with nov0, so the heat-kernel authority ranking was not reproducible run to run. Now seeded from a blake2b digest over the CSR shape/indptr/indices/data with explicit int64/float64 casts, feedingnp.random.default_rng— deterministic across processes, not merely across calls in one.Verification
The mutation check was re-run independently rather than taken on report, in a clean isolated clone:
, b.idfrom_ORDER_BY_BM25sorted()in_ENUM_MEMBER_INDEXeigshcallRestored: 24 passed. Full suite in the isolated clone: 7239 passed, 70 skipped, 71 xfailed, 0 failed.
The fixtures are adversarially adequate, which is the part worth checking. The bm25 fixture inserts in reverse-id order so rowid order and id order share no members at the cut, and a companion test asserts
len(set(scores)) == 1— proving the tiebreak, not bm25 itself, is what orders them. The hash-seed fixture is guarded by an assertion that ≥4 multi-member groups are actually hit, since single-member groups cannot expose the bug. None of these would have passed before the fix.Three riders, flagged not hidden
tol=0.0is a no-op. SciPy 1.17.1 already defaultstol=0. It is defensive against a future default change; the load-bearing part of §10 isv0alone. The commit message overstates it as "pinned to machine precision".np.argsort(..., kind="stable")is not required by the issue and does not contribute to determinism — quicksort is already deterministic given bit-identical input, which the seededv0now guarantees. It is a real (tiny) behaviour change for exactly-degenerate eigenvalues that no test distinguishes; reverting it alone leaves the suite green. An unverified rider on an otherwise mutation-checked diff.default_rng, no hash randomisation involved — so this is a coverage gap rather than a defect, but it is the weaker of the two assertions available.Reviewer may reasonably ask for the
kind="stable"rider to be dropped or given its own test.Deliberately left undone
clustering.py:215(equal-seed clusters ordered only by sort stability) andretrieval.py:2923(HRR pack consumes probe order with no tiebreak) are the same class and are named but untouched, which is what the issue's closing paragraph permits. Folding them in would have made the diff span a fifth and sixth file.Summary by Sourcery
Enforce deterministic, total-order behaviour across search ranking, value extraction, and spectral graph computations to uphold the pipeline’s determinism contract.
Bug Fixes:
Enhancements:
Tests:
Summary by CodeRabbit
Bug Fixes
Improvements