feat(index): expose KmerTable::parts for out-of-crate .kmt serialization (+ measurement harnesses) - #52
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (4)
WalkthroughAdds two example binaries: ChangesKMT tooling and public API
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 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 |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@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: 2
🤖 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/examples/realread_dump.rs`:
- Around line 41-42: The stride parameter parsed at line 41 can accept a value
of 0, which causes an infinite loop at line 85 because the loop counter i never
advances when stride is 0 at line 107. Add validation immediately after parsing
the stride variable to ensure stride is greater than 0, and exit the program
with an error message if stride is invalid or zero. This validation should
happen before any read processing logic begins.
- Around line 69-70: The assignments to _plus and _qual variables do not
validate that these lines exist or are properly formatted, allowing malformed
FASTQ records to be processed. After obtaining the _plus and _qual lines via
lines.next(), verify that both return Some values (indicating the lines exist)
and specifically check that the _plus line starts with the `+` character; if
either validation fails, return an error to halt processing instead of
continuing with incomplete records.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 211853e6-410b-4206-ad06-14e1106003a0
📒 Files selected for processing (3)
prmi/examples/build_kmt.rsprmi/examples/realread_dump.rsprmi/src/index/spectrum.rs
143b996 to
bd7b087
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/examples/build_kmt.rs`:
- Around line 55-69: The digest and PAC file inputs are not validated against
each other before writing the KMT file, which could result in a logically
incorrect table if a mismatched PAC file is used. Between reading the pac
content with std::fs::read and calling KmtFileWriter::write, add validation that
hashes the pac content and verifies the computed hash matches the digest value
obtained from hex_decode_32. This ensures the PAC file corresponds to the
intended digest before writing the KMT file.
In `@prmi/examples/realread_dump.rs`:
- Around line 99-105: The quality line validation in the match block only checks
for the presence of the quality line but does not verify that its length matches
the sequence length. After successfully reading the quality line into _qual, add
a validation check that compares qual.len() with seq.len() and exit with an
appropriate error message if they do not match. This ensures malformed FASTQ
records with mismatched sequence and quality lengths are properly rejected
before being processed by window metrics.
- Around line 72-75: The match statement on lines.next() uses a catch-all
pattern `_ => break` that treats both None (EOF) and Some(Err(e)) (read errors)
identically, causing silent truncation when errors occur. Replace the `_ =>
break` pattern with explicit handling: keep `None => break` for EOF, but add a
separate `Some(Err(e))` arm that panics or returns an error to properly surface
read failures instead of silently skipping them.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 9f674be3-73d3-4b6a-9275-c6a389e61058
📒 Files selected for processing (2)
prmi/examples/build_kmt.rsprmi/examples/realread_dump.rs
bd7b087 to
df46512
Compare
There was a problem hiding this comment.
♻️ Duplicate comments (3)
prmi/examples/realread_dump.rs (2)
99-105: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winReject FASTQ records with sequence/quality length mismatch.
The code requires quality presence but not length parity with
seq; malformed records can still feed window metrics. Validatequal.len() == seq.len()and fail otherwise.Suggested patch
- let _qual = match lines.next() { + let qual = match lines.next() { Some(Ok(l)) => l, _ => { eprintln!("malformed FASTQ: missing quality line for record {rid}"); std::process::exit(1); } }; + if qual.len() != seq.len() { + eprintln!( + "malformed FASTQ: sequence/quality length mismatch at record {rid} (seq={}, qual={})", + seq.len(), + qual.len() + ); + std::process::exit(1); + }🤖 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/examples/realread_dump.rs` around lines 99 - 105, After successfully reading the quality line into _qual from the match expression with lines.next(), add a validation check to ensure the quality string length matches the sequence length. If qual.len() does not equal seq.len(), print a descriptive error message (similar to the existing "malformed FASTQ" message for the missing quality line) and call std::process::exit(1) to reject the malformed record.
72-75: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFail on FASTQ read errors instead of treating them as EOF.
_ => breakcurrently swallowsSome(Err(_)), causing silent truncation of output. HandleNoneas EOF andSome(Err(e))as a hard failure.Suggested patch
- let _hdr = match lines.next() { - Some(Ok(l)) => l, - _ => break, - }; + let _hdr = match lines.next() { + None => break, + Some(Ok(l)) => l, + Some(Err(e)) => { + eprintln!("failed reading FASTQ header at record {rid}: {e}"); + std::process::exit(1); + } + };🤖 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/examples/realread_dump.rs` around lines 72 - 75, In the match statement for lines.next() in the realread_dump.rs file, replace the catch-all pattern `_` with explicit patterns to distinguish between EOF and read errors. Keep the `None => break` case for EOF handling, but add a separate `Some(Err(e)) => { }` case that explicitly handles read errors instead of silently treating them as EOF. This ensures that FASTQ read failures are properly reported rather than silently truncating the output.prmi/examples/build_kmt.rs (1)
55-69: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winDigest/PAC identity is not enforced, so an invalid
.kmtcan still load.At Line 55 and Line 58, the digest and PAC source are independent inputs. At Line 68,
writepersists the user-supplied digest, and Line 73-77 only verifies loadability (has_kmt), not PAC/digest consistency. A wrongpac_pathwith a copied index digest can produce a logically incorrect table that still loads.One-line fix: hash
pacand require it to equaldigest(or derive expected digest from index metadata) beforeKmtFileWriter::write.Also applies to: 73-77
🤖 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/examples/build_kmt.rs` around lines 55 - 69, The code does not validate that the user-supplied digest matches the actual PAC file contents, allowing inconsistent data to be written to the kmt file. Before calling KmtFileWriter::write, compute the hash of the pac variable and verify it equals the digest variable. If they do not match, raise an error instead of proceeding with the write operation. This ensures the digest stored in the kmt file is consistent with the PAC that was actually used.
🤖 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.
Duplicate comments:
In `@prmi/examples/build_kmt.rs`:
- Around line 55-69: The code does not validate that the user-supplied digest
matches the actual PAC file contents, allowing inconsistent data to be written
to the kmt file. Before calling KmtFileWriter::write, compute the hash of the
pac variable and verify it equals the digest variable. If they do not match,
raise an error instead of proceeding with the write operation. This ensures the
digest stored in the kmt file is consistent with the PAC that was actually used.
In `@prmi/examples/realread_dump.rs`:
- Around line 99-105: After successfully reading the quality line into _qual
from the match expression with lines.next(), add a validation check to ensure
the quality string length matches the sequence length. If qual.len() does not
equal seq.len(), print a descriptive error message (similar to the existing
"malformed FASTQ" message for the missing quality line) and call
std::process::exit(1) to reject the malformed record.
- Around line 72-75: In the match statement for lines.next() in the
realread_dump.rs file, replace the catch-all pattern `_` with explicit patterns
to distinguish between EOF and read errors. Keep the `None => break` case for
EOF handling, but add a separate `Some(Err(e)) => { }` case that explicitly
handles read errors instead of silently treating them as EOF. This ensures that
FASTQ read failures are properly reported rather than silently truncating the
output.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c3ffe675-2651-4f44-84a2-cc448e6adf00
📒 Files selected for processing (3)
prmi/examples/build_kmt.rsprmi/examples/realread_dump.rsprmi/src/index/spectrum.rs
df46512 to
1f7c534
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/examples/build_kmt.rs`:
- Around line 70-85: Add index-identity validation before the
KmtFileWriter::write call to prevent overwriting the KMT file with incorrect
data. After building the kmer_table with idx.build_kmer_table, compare args[4]
(the ref_digest provided as input) against the digest variable from the index
metadata. If they do not match, log an error message and exit the process before
calling KmtFileWriter::write on the kmt_path. This ensures the KMT file is only
written after confirming the PAC/digest pair belongs to the intended prefix.
In `@prmi/examples/realread_dump.rs`:
- Around line 52-56: The PAC file is read into memory and a PacEncoding is
created based on num_bases, but there is no validation that the actual PAC byte
length matches the expected size for num_bases. This can cause corrupted
measurements or panics later when mem_search is called. Add a validation check
immediately after creating the PacEncoding::Packed that verifies the pac vector
length matches the expected byte size for num_bases, and fail fast with an error
if the validation fails.
- Around line 74-81: The FASTQ header reading logic in the match statement
starting with `let _hdr = match lines.next()` does not validate that the header
line starts with the required `@` character. After successfully reading the
header line in the `Some(Ok(l)) => l` branch, add a validation check to ensure
the line begins with `@`. If the header does not start with `@`, treat it as a
malformed record by logging an error and skipping to the next iteration with
`continue` rather than allowing the invalid header to proceed.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: f1c11121-dc5f-4f8e-9a15-05f87e6a3fe2
📒 Files selected for processing (2)
prmi/examples/build_kmt.rsprmi/examples/realread_dump.rs
…lization Lets an external harness build a .kmt from an already-trained index and write it via KmtFileWriter without rebuilding the suffix array. The method already existed for in-crate serialization; only its visibility changes.
…nesses realread_dump: for every 32-mer of a FASTQ, queries an on-disk index and emits per-query prediction, last-mile probe count (spectrum-probe-count feature), converged SA interval, and multiplicity — to join last-mile cost against genome multiplicity on real reads. build_kmt: builds a .kmt shallow-band table from an existing index (no SA rebuild) and writes it bound to the reference digest. Measurement tooling only; gated behind the example target.
1f7c534 to
f9d3e46
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Makes
KmerTable::partspublic so a.kmtshallow-band accelerator can be serialized from outside the crate, and adds two measurement-only example harnesses:build_kmt— build a.kmtfor an existing on-disk index without rebuilding the suffix array, bound bysa_num+ref_digest, with a reopen-and-verify check.realread_dump— per-32-mer-window last-mile probe/prediction/interval dump over a FASTQ, for joining last-mile cost against genome multiplicity on real reads.Examples are measurement-only (not shipped).
cargo build/clippy/testgreen.Summary by CodeRabbit
Release Notes
New Features
.kmtsidecar for an existing index prefix using an on-disk.pacfile, including verification against a provided reference digest.API Changes