feat(collect): ISA forward-reseed warm-start (PRMI_ISA), byte-identical - #40
Conversation
|
Warning Review limit reached
More reviews will be available in 28 minutes and 59 seconds. 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 (2)
📝 WalkthroughWalkthroughPass-2 SMEM reseeding now optionally uses ISA-based warm-start hints to seed suffix-array search. A new public ChangesISA Warm-Start Hints for SMEM Collection
Sequence DiagramsequenceDiagram
participant Collector as SMEM Collector
participant ReseedSel as Reseed Selection
participant HintCalc as ReseedHint Calculator
participant ResBound as reseed_bounded_fwd
participant WarmStart as mem_search_warmstart
participant ColdSearch as mem_search
Collector->>ReseedSel: Pass-1 SMEMs
ReseedSel->>HintCalc: pivot position, ISA ref
HintCalc->>ReseedSel: Option<ReseedHint>
ReseedSel->>ResBound: (pivot, min_intv, hint)
ResBound->>WarmStart: hint available?
alt Hint Present
WarmStart->>WarmStart: seed from hint, find_boundary
else No Hint
ResBound->>ColdSearch: cold search path
end
WarmStart->>ResBound: (match_len, interval)
ColdSearch->>ResBound: (match_len, interval)
ResBound->>Collector: truncated SMEM
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
3413e30 to
ee2e936
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
prmi/src/index/spectrum.rs (1)
4887-4892: ⚡ Quick winExercise out-of-range hints in this proptest.
hint_raw % sa_num.max(1)keeps random hints in-range, so the documented clamp path (hint >= sa_num) is not directly tested. Add at least one raw out-of-range hint (for exampleu64::MAX) to pin that contract.♻️ Suggested tweak
- let mut hints = vec![hint_raw % sa_num.max(1)]; + let mut hints = vec![hint_raw, u64::MAX]; + if sa_num > 0 { + hints.push(hint_raw % sa_num); + }🤖 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/spectrum.rs` around lines 4887 - 4892, The proptest currently only pushes in-range hints via let mut hints = vec![hint_raw % sa_num.max(1)]; which never exercises the clamp branch for hint >= sa_num; modify the construction of hints in the test (the code around hint_raw, sa_num and the hints vec) to include at least one raw out-of-range hint (e.g. push u64::MAX or another value >= sa_num) alongside the existing modulo-derived hint so the clamp path is exercised; keep the existing logic that conditionally adds near-interval hints when cold.occ > 0 unchanged.prmi/src/index/collect.rs (1)
251-276: ⚡ Quick winSkip ISA hint materialization when a k-mer table is loaded.
mem_search_warmstartignores its hint on thekmtbranch, so this code still payssa_position_for()here andisa_at()later even though it can never skip the tabled launch. Gatinguse_isaonself.kmt.is_none()keeps kmt-backed indexes on the unchanged reseed path instead of adding dead per-reseed work.♻️ Proposed fix
- let use_isa = isa_reseed_enabled() && self.has_isa(); + let use_isa = isa_reseed_enabled() && self.has_isa() && self.kmt.is_none();Based on the
mem_search_warmstartcontract inprmi/src/index/spectrum.rs:1538-1626, hints are ignored whenkmtis loaded.🤖 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 251 - 276, The code constructs ISA reseed hints and calls self.sa_position_for() even when a k-mer table is loaded and mem_search_warmstart will ignore hints; change the gating so hints are only created when ISA reseeding is enabled, an ISA is present, AND there is no k-mer table. Concretely, modify the use_isa boolean (currently set with isa_reseed_enabled() && self.has_isa()) to also require self.kmt.is_none(), or alternatively check self.kmt.is_none() before calling self.sa_position_for() when building the ReseedHint so that sa_position_for() and isa_at() are not invoked for kmt-backed indexes.
🤖 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.
Nitpick comments:
In `@prmi/src/index/collect.rs`:
- Around line 251-276: The code constructs ISA reseed hints and calls
self.sa_position_for() even when a k-mer table is loaded and
mem_search_warmstart will ignore hints; change the gating so hints are only
created when ISA reseeding is enabled, an ISA is present, AND there is no k-mer
table. Concretely, modify the use_isa boolean (currently set with
isa_reseed_enabled() && self.has_isa()) to also require self.kmt.is_none(), or
alternatively check self.kmt.is_none() before calling self.sa_position_for()
when building the ReseedHint so that sa_position_for() and isa_at() are not
invoked for kmt-backed indexes.
In `@prmi/src/index/spectrum.rs`:
- Around line 4887-4892: The proptest currently only pushes in-range hints via
let mut hints = vec![hint_raw % sa_num.max(1)]; which never exercises the clamp
branch for hint >= sa_num; modify the construction of hints in the test (the
code around hint_raw, sa_num and the hints vec) to include at least one raw
out-of-range hint (e.g. push u64::MAX or another value >= sa_num) alongside the
existing modulo-derived hint so the clamp path is exercised; keep the existing
logic that conditionally adds near-interval hints when cold.occ > 0 unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f300c937-a315-4b9b-858c-173d9405610b
📒 Files selected for processing (2)
prmi/src/index/collect.rsprmi/src/index/spectrum.rs
Wire the inverse-SA hint into the fused collect_smems pass-2 forward reseed, behind the PRMI_ISA env gate. Each reseeded pass-1 SMEM carries a ReseedHint (refpos via sa_position_for(k), plus the read span); the reseed's forward extension warm-starts mem_search from isa_at(refpos + offset) instead of a cold model launch + boundary gallop. When the hint is good the insertion search collapses to ~1-2 probes. Byte-identity is the contract, not an aspiration: the hint only seeds the insertion-point search, and find_boundary expands on a miss to the true boundary, so the result is identical to the cold mem_search for ANY hint — a non-maximal/stale projection just costs a few extra probes. This is why every reseeded SMEM is hinted, including partial-match SMEMs whose cached occurrence diverges from the read past the span end. spectrum.rs: - forward_maximal_len_seeded(seed_win): split forward_maximal_len into a delegating cold wrapper + a window-seeded variant (None == cold, verbatim). - mem_search_warmstart(query, hint): mem_search with the no-kmt launch seeded from hint; the kmt branch (tabled trace) ignores the hint, still byte-id. - mem_search_warmstart_equals_cold proptest: warm == cold for ANY hint (far/near/in-interval), the seed-independence contract. collect.rs: - isa_reseed_enabled() (PRMI_ISA) + ReseedHint + reseed_isa_hint projection. - Thread Option<ReseedHint> through zz_step1_reseed -> reseed_bounded_fwd / zz_right_emit_reseed; warm-start the forward search when the hint projects. - collect_smems_isa_equals_cold proptest + collect_smems_isa_warmstart_equals_ cold_partial: ISA-on == ISA-off == cold reference across the opts sweep, including partial-match reads. Test-only ISA_FORCE override + build_mode2_ with_isa scaffolding. The reseed-LEFT/RC warm-start (zz_left_span_reseed) stays cold here; it lands in a follow-up so this PR is forward-only.
ee2e936 to
3e7ab3e
Compare
1 participant
What
Wires the inverse-suffix-array (ISA) hint into the fused
collect_smemspass-2 forward reseed, behind thePRMI_ISAenv gate. Each reseeded pass-1 SMEM carries aReseedHint(refposviasa_position_for(k), plus the read span[m, n]); the reseed's forward extension warm-startsmem_searchfromisa_at(refpos + offset)instead of a cold model launch + boundary gallop. When the hint is good the insertion-point search collapses to ~1–2 probes, skipping the model launch (the cost no model retrain could cut).This is the first of the ISA-reseed re-derivations onto
main. It is forward-only; the reseed-LEFT/RC warm-start (zz_left_span_reseed) stays cold here and lands in a follow-up.Why it's byte-identical (the contract)
The hint only seeds the insertion-point search;
find_boundaryexpands on a miss to the true boundary, so the result is identical to the coldmem_searchfor any hint — a non-maximal/stale projection just costs a few extra probes, never a wrong answer. That is precisely why every reseeded SMEM is hinted, including partial-match SMEMs whose cached occurrence diverges from the read past the span end (the common reseed case). WithPRMI_ISAunset the reseed runs the existing cold path unchanged.Changes
spectrum.rsforward_maximal_len_seeded(seed_win)— splitsforward_maximal_leninto a delegating cold wrapper + a window-seeded variant.Noneis the cold path, verbatim.mem_search_warmstart(query, hint)—mem_searchwith the no-kmtlaunch seeded fromhint; thekmtbranch (tabled trace) ignores the hint and is byte-identical tomem_search'skmtbranch.mem_search_warmstart_equals_coldproptest — warm == cold for ANY hint (random-far, near-interval, in-interval): the seed-independence contract.collect.rsisa_reseed_enabled()(PRMI_ISA) +ReseedHint+reseed_isa_hintprojection (valid inside the cached span).Option<ReseedHint>throughzz_step1_reseed→reseed_bounded_fwd/zz_right_emit_reseed; warm-start the forward search when the hint projects.collect_smems_isa_equals_coldproptest +collect_smems_isa_warmstart_equals_cold_partial— ISA-on == ISA-off == cold reference across the full opts sweep, including partial-match reads. Test-onlyISA_FORCEoverride +build_mode2_with_isascaffolding.Validation
cargo test --release -p prmi --lib— all pass except the pre-existingsa::doubled_text_tests::doubled_text_rejects_out_of_range_base(a#[should_panic]relying on adebug_assert!that release strips; insa.rs, untouched here).cargo clippy --release -p prmi --lib --all-features— clean.mem_search_warmstart_equals_cold,collect_smems_isa_equals_cold,collect_smems_isa_warmstart_equals_cold_partial.Notes for the reviewer
main.main(isa/has_isa/isa_at/sa_position_for, themem_search_from_hintfamily). No index-format/sidecar/build changes.Summary by CodeRabbit
Release Notes
New Features
Tests