test+build+ci: LCP-array coverage, release profile, and a Rust workflow (independent) - #11
Conversation
Two prerequisites for the performance work that follows, neither of which changes behaviour. The crate had no test covering the LCP array. That is the riskiest possible gap for this algorithm: the public entry points discard the array, but the *next* merge level consumes it in the three-case decision, so a single wrong LCP entry silently reorders suffixes one level up and the SA comes out subtly wrong. Add four tests that check `lcp[0] == 0` and `lcp[i] == lcp(text[sa[i-1]..], text[sa[i]..])` against a naive oracle, over fixtures, random texts across four alphabet sizes, long runs and periodic text, and finite `max_context`. `bench/README.md` claims the published numbers were taken with fat LTO and one codegen unit, supplied by a parent workspace. That workspace is not in this repo, so every build made from it since the crate went standalone has used `lto = false, codegen-units = 16`. Pin the profile here. In a library crate `[profile.release]` applies only when this crate is the workspace root, so it affects this repo's own tests, examples and benches and is invisible to downstream consumers. Measured on Apple M4 Max, 12 threads, chr21 FASTA (47.5 MB): 27.8 s -> 24.0 s wall. Neutral on N-free DNA. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The repository had no Rust CI at all: the only workflow deployed the docs site, while 73 tests sat in the tree with nothing running them. That is not a safe baseline for changing the sorting kernel. Covers both architectures that matter here, since the LCP kernel and the pooled external-memory bucket path are the parts that diverge per platform: macOS is aarch64/NEON, Ubuntu is x86_64/AVX2. Runs the tests in debug as well as release, because debug is what exercises the `debug_assert`s guarding the unchecked scatter in `radix.rs` and the buffer-length invariants in the merge kernel. Adds fmt, clippy with warnings denied, a check against the declared 1.89 MSRV, and rustdoc with broken links denied. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Public documentation linked to `FilteredSource`, which is private, so `cargo doc` fails under `RUSTDOCFLAGS=-D warnings`. Pre-existing, but it blocks the rustdoc job added in the previous commit. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
A pull request opened from a fork by a first-time contributor does not run workflows until a maintainer approves them, which is why the API showed no check runs at the tips reviewed in COMBINE-lab#7. Building on push means the contributor's own fork produces evidence that can be linked from the PR. Adds `workflow_dispatch` for the same reason. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Accepted and merged as I reviewed this against current Validation performed:
The current GitHub runner-image mapping confirms that I did not run a full annotated-genome performance A/B for this PR because it has no runtime code change in the ruSTAR build. The added source is test-only, documentation-only, or workflow-only. The root One local operational note: on this host, running all external-memory tests concurrently can exceed the process file-descriptor limit; serial execution passes. The fork's Ubuntu and macOS jobs passed the default parallel invocation, so this did not block the merge, but bounding pooled files in concurrent tests may be useful future hardening. |
2 participants
Independent of every other open PR. Branches from
main, no behaviour change.This is #3 reduced to the part @rob-p accepted: the test and build scaffolding, without the packed-key work that PR also carried.
The crate had no test covering the LCP array
That is the riskiest gap in this codebase. The public entry points discard the LCP array, but the next merge level consumes it in the three-case decision, so a single wrong entry silently reorders suffixes one level up and the suffix array comes out subtly wrong — permutation intact, order not.
Four tests, checking
lcp[0] == 0andlcp[i] == lcp(text[sa[i-1]..], text[sa[i]..])against a naive oracle over fixtures, random texts across four alphabet sizes, long runs and periodic text, and finitemax_context.62 tests before, 66 after.
The release profile was not what the benchmarks claimed
bench/README.mdstated the published numbers were taken withlto = "fat"andcodegen-units = 1, supplied by a parent workspace. That workspace is not in this repository. From the commit that made the crate standalone until now, every build made here usedlto = false, codegen-units = 16, so numbers taken in that window are not comparable with numbers taken since. The profile is pinned here and the stale claim corrected.In a library crate
[profile.release]applies only when the crate is the workspace root, so this affects the repo's own tests, examples and benches and is invisible to downstream consumers.CI
The repository had no Rust CI, only a docs deploy, while carrying 62 tests with nothing running them.
Runs on
ubuntu-latestandmacos-latest, since the LCP kernel and the pooled bucket path are what diverge per platform (x86_64/AVX2 against aarch64/NEON). Tests run in debug as well as release — that is not ceremony: debug is what exercises thedebug_asserts guarding the buffer-length invariants, and on a later branch it caught au64shift-by-64 that release masks to zero and silently gets right. Plus fmt, Clippy with warnings denied, an MSRV check against the declared 1.89, and rustdoc with broken links denied.The workflow triggers on every branch rather than only
main. A pull request opened from a fork by a first-time contributor does not run workflows until a maintainer approves them, which is why the API showed no check runs at the tips reviewed in #7; building on push lets the fork produce evidence that can be linked.Also
Fixes a pre-existing broken intra-doc link in
build_ext_mem_for_filter, which pointed at the privateFilteredSourceand would fail the new rustdoc job.Review notes from #7, addressed
You asked to correct the stale statement that ruSTAR has not migrated to
SegmentedTextand to clarify that the verifier is for complete plain SAs — both of those live in the packed-key part of #3 and are not in this PR.verify_sais not here either; it belongs with the work that needed it.🤖 Generated with Claude Code