feat(collect): pass 3 (max_mem_intv strategy) + forward narrowing - #34
Conversation
|
Warning Review limit reached
More reviews will be available in 51 minutes and 1 second. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR enables pass-3 (FMI long-MEM reseed) in the PRMI index collection pipeline by removing FFI guards, implementing pass-3 core logic, adding spectrum-based narrowing helpers, and extending test coverage to validate correctness across the full stack. ChangesPass-3 Collection Pipeline Enablement
Sequence Diagram(s)sequenceDiagram
participant collect_smems_unsorted
participant pass3_seed_one_pivot
participant mem_search
participant forward_narrow_first_below
collect_smems_unsorted->>pass3_seed_one_pivot: per pivot, opts.max_mem_intv > 0
pass3_seed_one_pivot->>mem_search: fast path at Lstart
mem_search-->>pass3_seed_one_pivot: occurrence count
alt occurrence < max_mem_intv immediately
pass3_seed_one_pivot-->>collect_smems_unsorted: fast SMEM result
else fallback to narrowing
pass3_seed_one_pivot->>forward_narrow_first_below: find first deep length below threshold
forward_narrow_first_below-->>pass3_seed_one_pivot: (length, lower, occurrence)
pass3_seed_one_pivot-->>collect_smems_unsorted: narrowed SMEM result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
prmi-sys/tests/spectrum_ffi.rs (1)
1982-2036: ⚡ Quick winStrengthen this test to prove the fixture actually exercises pass 3.
Right now the test validates FFI==Rust for both
max_mem_intv=3andmax_mem_intv=0, but it doesn’t assert that those two modes produce different results for this dataset. Adding an explicit enabled-vs-disabled difference check would make this a true pass-3 coverage test, not just dual plumbing parity.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prmi-sys/tests/spectrum_ffi.rs` around lines 1982 - 2036, The test currently only checks FFI vs Rust parity for max_mem_intv == 3 and == 0 but doesn't assert those two modes produce different outputs; update the test to prove pass-3 is actually exercised by asserting enabled-vs-disabled divergence: after collecting both runs (the existing variables rn / rust or ffi_buf for the enabled run and rn0 / rust_p0 / ffi_buf for the disabled run), add an assertion that the results are not identical (e.g., assert rn != rn0 or assert at least one SMEM tuple differs between rust[0..rn] and rust_p0[0..rn0] or between ffi_buf slices), referencing the existing symbols rn, rn0, rust, rust_p0, ffi_buf so the check compares the already-collected data rather than re-running collection.prmi/src/index/collect.rs (1)
1305-1323: ⚡ Quick winAdd an explicit N-boundary oracle case for pass 3.
pass3_equals_oracleonly generates0..=3reads, so the new pass-3 branches that skip/advance across ambiguous bases are never checked against an independent reference.collect_smems_full_pipelinedoes sweep0..=4, but its reference path appendspass3_walk, so a shared off-by-one at an N boundary would still pass. Please add a targeted read with embedded4s and assert the exact emission/advance behavior against the spectrum oracle or a hand-written expectation.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prmi/src/index/collect.rs` around lines 1305 - 1323, pass3_equals_oracle currently only tests reads with symbols 0..=3 so the new pass-3 branches that handle ambiguous bases (encoded as 4) are untested; add an explicit N-boundary case inside the pass3_equals_oracle property that constructs a read containing embedded 4s (e.g., a short vector like [..,4,..,4,..]) and compare the result of pass3_walk for that read to either pass3_walk_spectrum (spectrum oracle) or a hand-written expected tuple sequence; update the test to call pass3_walk(&idx, &read_with_4s, &fwd, max_mem_intv, min_seed_len) and assert equality with the chosen oracle (pass3_oracle or pass3_walk_spectrum) so the skip/advance behavior at N boundaries is verified (leave the original random-case checks intact).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@prmi-sys/src/lib.rs`:
- Around line 1822-1824: Update the doc comment for the function
prmi_collect_smems to reflect the new pass-3 contract: change the header text
that currently says it runs "passes 1+2" to state it runs passes 1 and 2 and
optionally a pass 3 when the options field max_mem_intv > 0 (i.e., pass-3
long-MEM reseed is enabled), and briefly note that max_mem_intv == 0 disables
pass 3 so the C-facing docs accurately describe the behavior.
---
Nitpick comments:
In `@prmi-sys/tests/spectrum_ffi.rs`:
- Around line 1982-2036: The test currently only checks FFI vs Rust parity for
max_mem_intv == 3 and == 0 but doesn't assert those two modes produce different
outputs; update the test to prove pass-3 is actually exercised by asserting
enabled-vs-disabled divergence: after collecting both runs (the existing
variables rn / rust or ffi_buf for the enabled run and rn0 / rust_p0 / ffi_buf
for the disabled run), add an assertion that the results are not identical
(e.g., assert rn != rn0 or assert at least one SMEM tuple differs between
rust[0..rn] and rust_p0[0..rn0] or between ffi_buf slices), referencing the
existing symbols rn, rn0, rust, rust_p0, ffi_buf so the check compares the
already-collected data rather than re-running collection.
In `@prmi/src/index/collect.rs`:
- Around line 1305-1323: pass3_equals_oracle currently only tests reads with
symbols 0..=3 so the new pass-3 branches that handle ambiguous bases (encoded as
4) are untested; add an explicit N-boundary case inside the pass3_equals_oracle
property that constructs a read containing embedded 4s (e.g., a short vector
like [..,4,..,4,..]) and compare the result of pass3_walk for that read to
either pass3_walk_spectrum (spectrum oracle) or a hand-written expected tuple
sequence; update the test to call pass3_walk(&idx, &read_with_4s, &fwd,
max_mem_intv, min_seed_len) and assert equality with the chosen oracle
(pass3_oracle or pass3_walk_spectrum) so the skip/advance behavior at N
boundaries is verified (leave the original random-case checks intact).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 91cbba33-b147-4843-8f28-799369096b4a
📒 Files selected for processing (4)
prmi-sys/src/lib.rsprmi-sys/tests/spectrum_ffi.rsprmi/src/index/collect.rsprmi/src/index/spectrum.rs
Completes `collect_smems`: ports the pass-3 `max_mem_intv` strategy (the FMI long-MEM reseed round) and removes the FFI's `-5` rejection of `max_mem_intv > 0`. With this the fused collector is byte-identical to FMI seeding for the consumer's full production config, not just passes 1+2. New `forward_narrow_first_below` (spectrum.rs): given a model-seeded length-L0 interval whose occ >= threshold, narrows FORWARD to the first deeper length whose occ < threshold, seeded at [lo0,hi0) instead of the cold [0,sa_num). Self- contained — deps (l_pac, lower/upper_bound_prefix) all on main. `pass3_seed_one_pivot` (collect.rs): one model-seeded mem_search locate at Lstart (the common case), falling back to forward_narrow_first_below only on the rare repetitive case — instead of the cold forward_spectrum trace. Wired into the driver's gated pass-3 loop; the entrypoint/CollectOpts/FFI docs drop the 'not yet ported' caveats and the attrib slot-4 (pass3) bucket is now live. Byte-identity: `pass3_equals_oracle` cross-checks the model-seeded walk vs the `forward_spectrum` reference walk (`pass3_seed_one_pivot_spectrum`) AND an independent `mem_search` oracle; `pass3_disabled_when_zero` isolates the gate; `collect_smems_full_pipeline` now sweeps max_mem_intv>0; the FFI test exercises pass 3 (FFI == Rust through the whole pipeline) plus a pass-3-disabled cross-check.
Add `examples/collect_gate`: run `LearnedIndex::collect_smems` over a FASTQ against a built prmi sidecar, write the emitted SMEMs to a TSV, and report total SA-probes/read with a per-phase breakdown (pass-1 left/right, reseed left/forward, pass 3) from the `attrib` buckets. Now that pass 3 is live (#34) all five buckets populate, giving a complete per-read probe attribution for deciding where any future optimization would actually pay. Gated on `spectrum-probe-count` (the probe counter + `attrib` are compiled out without it); a stub main on the default build prints the feature hint and exits 2. Paths come from env (PRMI_PREFIX / PRMI_FQ / PRMI_PAC required, no machine-specific defaults — mirrors the ffi_overhead example); the CollectOpts knobs default to the bwa-mem values. stdout is just the probes/read number, for scripting. cargo build --all-targets clean (both feature configs); the missing-env path exits 2 with a clear message.
1 participant
v0.3 perf series — PR-D′: completes
collect_smemswith pass 3 (built on #32's collect core + #31's forward-truncate; the last piece gating full-SAM e2e parity).What
Ports the pass-3
max_mem_intvstrategy (the FMI long-MEM reseed round) and removes the FFI's-5rejection ofmax_mem_intv > 0. With this the fused collector is byte-identical to FMI seeding for the consumer's full production config, not just passes 1+2 — so the box-gate can finally measure real end-to-end parity.How
forward_narrow_first_below(spectrum.rs, net-new): given a model-seeded length-L0interval whose occ ≥ threshold, narrows FORWARD to the first deeper length whose occ < threshold, seeded at[lo0, hi0)instead of the cold[0, sa_num). Self-contained — deps (l_pac,lower_bound_prefix,upper_bound_prefix) all already on main.pass3_seed_one_pivot(collect.rs): one model-seededmem_searchlocate atLstart(the common case), falling back toforward_narrow_first_belowonly on the rare repetitive case — instead of the coldforward_spectrumtrace. Wired into the driver's gated pass-3 loop; theattribslot-4 (pass3) profiling bucket is now live.Byte-identity
pass3_equals_oraclecross-checks the model-seeded walk vs theforward_spectrumreference walk (pass3_seed_one_pivot_spectrum) AND an independentmem_searchoracle — three independent code paths, same SMEMs.pass3_disabled_when_zeroisolates the gate;collect_smems_full_pipelinenow sweepsmax_mem_intv > 0; the FFI test exercises pass 3 (FFI == Rust through the whole pipeline) + a pass-3-disabled cross-check.Series status
This is the forward-narrow primitive (
0456819) + pass-3 walk (afda522) from the carving plan. After this, the remaining net-new work is just the LCP-accel comparator (61d7186) + the marginalsa_entry()trim (17d506b) + a trivial inline-attr — the rest of the perf series is subsumed or dropped.Pre-PR
/coderabbitai-review: 0 findings (self-caught + fixed oneattribdoc-drift item before review).Summary by CodeRabbit
Release Notes
New Features
Tests