feat: enable index resumption and enforce directory creation

The command now supports reopening existing indexes instead of failing when the output file exists. Control flow branches between opening an existing index and constructing a new one, moving configuration setup exclusively to the creation path. Directory existence is enforced upfront with proper I/O error propagation. The --force flag retains its original semantics by removing the target directory before proceeding with a fresh build.
This commit is contained in:
Eric Coissac
2026-08-21 05:06:38 +02:00
parent abc51c2add
commit 5c1584967f
161 changed files with 5274 additions and 845 deletions
+176 -40
View File
@@ -731,14 +731,14 @@
<li class="md-nav__item">
<a href="../../implementation/obilayeredmap/" class="md-nav__link">
<a href="../../implementation/layer_tier/" class="md-nav__link">
<span class="md-ellipsis">
obilayeredmap crate
obikindex layer tier
@@ -976,6 +976,34 @@
<li class="md-nav__item">
<a href="../../implementation/partition_layer_cache/" class="md-nav__link">
<span class="md-ellipsis">
Partition and layer caching (discussion)
</span>
</a>
</li>
</ul>
</nav>
@@ -1328,6 +1356,17 @@
</span>
</a>
</li>
<li class="md-nav__item">
<a href="#implemented-2026-08-20" class="md-nav__link">
<span class="md-ellipsis">
Implemented (2026-08-20)
</span>
</a>
</li>
</ul>
@@ -1602,6 +1641,17 @@
</span>
</a>
</li>
<li class="md-nav__item">
<a href="#implemented-2026-08-20" class="md-nav__link">
<span class="md-ellipsis">
Implemented (2026-08-20)
</span>
</a>
</li>
</ul>
@@ -1637,7 +1687,7 @@ one-way function, not an invertible bijection with a stored inverse. Any
method that reconstructs a kmer from a bare slot number is wrong by
construction, regardless of the mechanism used (MPHF re-hash, or evidence
decode + direct unitig read). See <code>MphfLayer::kmer_at</code>
(<code>obilayeredmap/src/mphf_layer.rs</code>) — flagged for removal, currently called
(<code>obikindex/src/layer/mphf_layer.rs</code>) — flagged for removal, currently called
from <code>obikphylo/siblings/build.rs</code> and <code>family_scan.rs</code> (since removed — see
"Pending work" status below).</p>
<h2 id="two-pipelines-never-mixed">Two pipelines, never mixed</h2>
@@ -1700,7 +1750,7 @@ partition is unknown) and must keep going through
<code>index()</code>.</p>
<h2 id="pending-work-done">Pending work — done</h2>
<p>The plan above shipped: <code>obikphylo</code> (a new crate — phylo-domain extension
traits over <code>obikindex::KmerIndex</code>/<code>obilayeredmap::Layer&lt;D&gt;</code>, replacing the
traits over <code>obikindex::KmerIndex</code>/<code>obikindex::layer::Layer&lt;D&gt;</code>, replacing the
old <code>obikindex::siblings</code> module) builds and reads the annex purely in
iteration order (<code>SiblingLayerExt::iter_siblings</code>/<code>iter_minorants</code>, both with
batch variants, mirroring <code>Layer&lt;D&gt;</code>'s own <code>KmerIter</code>/<code>KmerBatchIter</code>
@@ -2268,13 +2318,13 @@ BRWT-style column-correlation exploitation.</p>
<p><code>PersistentSparseBitMatrix</code> went from a validated but unused type to a
real, selectable on-disk format:</p>
<ul>
<li><strong>Generic <code>Layer&lt;D&gt;</code></strong>: <code>obilayeredmap::Layer&lt;D&gt;</code>'s presence-only methods
<li><strong>Generic <code>Layer&lt;D&gt;</code></strong>: <code>obikindex::layer::Layer&lt;D&gt;</code>'s presence-only methods
(<code>n_cols</code>, <code>sub_matrix</code>, <code>fill_sub_matrix</code>) are generic over any
<code>D: LayerData&lt;Item = Box&lt;[bool]&gt;&gt; + BinaryMatrix</code>, not hardcoded to
<code>PersistentBitMatrix</code><code>PersistentSparseBitMatrix</code> implements
<code>LayerData</code> (<code>open</code>/<code>read</code>) the same way. <code>find_slot</code>/<code>index_batch</code> were
already generic over any <code>D: LayerData</code>, so they needed no change.
Verified by <code>obilayeredmap</code>'s
Verified by <code>obikindex::layer</code>'s
<code>presence_layer_generic_over_sparse_matches_dense</code> test: build a dense
presence layer, convert it to sparse via <code>build_from_dense</code>, open both
as <code>Layer&lt;PersistentBitMatrix&gt;</code>/<code>Layer&lt;PersistentSparseBitMatrix&gt;</code> on
@@ -2284,7 +2334,7 @@ real, selectable on-disk format:</p>
<code>AtomicUsize</code>s in test builds, not thread-local, so a test using a
different <code>k</code> races every other test in the same crate binary; a k=11
version of this test passed alone but failed under the full
<code>obilayeredmap</code> suite for exactly that reason before being fixed.)</li>
<code>obikindex::layer</code> suite for exactly that reason before being fixed.)</li>
<li><strong><code>obikphylo::siblings::cache::Mat</code></strong> gained a third variant,
<code>SparsePresence(Layer&lt;PersistentSparseBitMatrix&gt;)</code>, alongside <code>Count</code>
and <code>Presence</code> — every method (<code>find_slot</code>, <code>index_batch</code>,
@@ -2314,7 +2364,7 @@ real, selectable on-disk format:</p>
dense-path test (<code>sibling_annex_one_sibling_each</code>) exactly — proves the
sparse format round-trips through the real build pipeline
(<code>PartitionCache</code> sparse-detection included), not just the
<code>obicompactvec</code>/<code>obilayeredmap</code> unit layers below it.</li>
<code>obicompactvec</code>/<code>obikindex::layer</code> unit layers below it.</li>
</ul>
<p>Full workspace <code>cargo test</code> (all crates, unit + doc tests) green after
this change.</p>
@@ -2360,7 +2410,7 @@ this change.</p>
(activation: either given; defaults <code>1.0</code>/<code>0.5</code> for whichever is unset).</li>
</ul>
<p>Full workspace <code>cargo test</code> green after this change (167 unit tests in
<code>obicompactvec</code>+<code>obilayeredmap</code>+<code>obikphylo</code> alone, plus every other
<code>obicompactvec</code>+<code>obikindex::layer</code>+<code>obikphylo</code> alone, plus every other
crate's suite, no regressions).</p>
<p><strong>Still open, not part of this change</strong> (per "Correction to the 'single
pass' framing" above): <code>--raw-snp-distance</code>/<code>--raw-snp-counts</code> (the
@@ -2380,7 +2430,7 @@ reproducible across two runs with warm disk cache), the opposite of
<code>pack --sparse</code>'s stated intent ("faster for single-row access... like
query").</p>
<p><strong>Root cause, read from source, not measured in isolation:</strong>
<code>KmerPartition::query_partition_with</code> (<code>obikpartitionner/src/query_layer.rs:155-220</code>)
<code>KmerPartition::query_partition_with</code> (<code>obikindex/src/partition/query_layer.rs:155-220</code>)
is architecturally column-major: stage 2 walks <code>for g in 0..n_cols { for
slot in hit_slots { layer.col_value(g, slot) } }</code>, documented (correctly)
as the right locality strategy for the packed/columnar formats, where
@@ -2407,45 +2457,131 @@ consumes as <code>QueryHit::Value</code> — not a materialized sub-matrix.</p>
<p><strong>Proposed primitive</strong> (design only, not implemented — explicit ask: keep
count matrices <em>not excluded</em>, even though effort right now is
presence/absence only):</p>
<div class="highlight"><pre><span></span><code><span class="sd">/// Visit every nonzero cell among `slots`. Order unspecified.</span>
<span class="k">fn</span><span class="w"> </span><span class="nf">for_each_nonzero</span><span class="p">(</span><span class="o">&amp;</span><span class="bp">self</span><span class="p">,</span><span class="w"> </span><span class="n">slots</span><span class="p">:</span><span class="w"> </span><span class="kp">&amp;</span><span class="p">[</span><span class="kt">usize</span><span class="p">],</span><span class="w"> </span><span class="n">f</span><span class="p">:</span><span class="w"> </span><span class="nc">impl</span><span class="w"> </span><span class="nb">FnMut</span><span class="p">(</span><span class="kt">usize</span><span class="w"> </span><span class="cm">/*idx into slots*/</span><span class="p">,</span><span class="w"> </span><span class="kt">usize</span><span class="w"> </span><span class="cm">/*col*/</span><span class="p">,</span><span class="w"> </span><span class="kt">u32</span><span class="w"> </span><span class="cm">/*value*/</span><span class="p">));</span>
<p>Not a closure-driven <code>for_each</code> — a real <code>Iterator</code>, one concrete struct
per matrix format, so the traversal state (current position in the sorted
slot list, current column, permutation, sparse-row decode cursor…) lives
in named struct fields instead of being threaded implicitly through
recursion or a captured closure. RPITIT (stable since 1.75, and this
workspace is edition 2024) means the trait method can return it without
naming or boxing the concrete type:</p>
<div class="highlight"><pre><span></span><code><span class="sd">/// Yields every nonzero cell among `slots`, in implementation-defined order.</span>
<span class="k">fn</span><span class="w"> </span><span class="nf">nonzero_iter</span><span class="o">&lt;&#39;</span><span class="na">a</span><span class="o">&gt;</span><span class="p">(</span><span class="o">&amp;&#39;</span><span class="na">a</span><span class="w"> </span><span class="bp">self</span><span class="p">,</span><span class="w"> </span><span class="n">slots</span><span class="p">:</span><span class="w"> </span><span class="kp">&amp;</span><span class="o">&#39;</span><span class="na">a</span><span class="w"> </span><span class="p">[</span><span class="kt">usize</span><span class="p">])</span><span class="w"> </span><span class="p">-&gt;</span><span class="w"> </span><span class="nc">impl</span><span class="w"> </span><span class="nb">Iterator</span><span class="o">&lt;</span><span class="n">Item</span><span class="w"> </span><span class="o">=</span><span class="w"> </span><span class="p">(</span><span class="kt">usize</span><span class="p">,</span><span class="w"> </span><span class="kt">usize</span><span class="p">,</span><span class="w"> </span><span class="kt">u32</span><span class="p">)</span><span class="o">&gt;</span><span class="w"> </span><span class="o">+</span><span class="w"> </span><span class="o">&#39;</span><span class="na">a</span><span class="p">;</span>
<span class="c1">// item: (idx into `slots`, col, value)</span>
</code></pre></div>
<p>This is the one foundational primitive per format — both <code>fill_sub_matrix</code>
and a <code>for_each</code>-style callback become trivial, free consumers of it
(<code>.for_each(f)</code> is already <code>Iterator::for_each</code> from std; <code>fill_sub_matrix</code>
becomes "drain the iterator, scatter into <code>out[][]</code>"), instead of two
independently-maintained traversals that can silently diverge (see the bug
below — this is exactly how it happened).</p>
<ul>
<li>On <code>PersistentSparseBitMatrix</code>: native override, one pass per slot via
the existing (currently private) <code>for_each_genome_in_row</code> — O(Σ row
nnz), zero n_cols-wide allocation. This is the whole point: expose code
that already exists rather than write anything new for the sparse side.</li>
<li>On <code>PersistentBitMatrix::{Packed,Columnar,Implicit}</code>: <strong>provided
default</strong>, derived from <code>fill_sub_matrix</code> (materialize, then filter to
true cells) — reuses the already-optimal column-major/mmap path for
those formats, no new code needed there either.</li>
<li><strong>On <code>PersistentCompactIntMatrix</code> (counts)</strong>: same provided-default
treatment, derived from its own existing <code>fill_sub_matrix</code> (<code>u32</code>-typed
already, <code>intmatrix.rs:387</code>) — not hand-optimized (no sparse count
format exists — "Explicitly deferred" per <code>traits.rs:9-12</code>), but not
excluded either: it gets a working, not-pathological implementation for
free today, on the same trait, ready for a native override the day a
sparse count format lands. This is why the signature carries <code>u32</code>
rather than <code>bool</code> — presence is <code>0</code>/<code>1</code>, counts are <code>u32</code>, one trait
covers both without a bool/u32 split forcing counts out of the design.</li>
<li><strong>On <code>PersistentSparseBitMatrix</code></strong>: the struct is nearly free to write —
it wraps the existing (currently private) <code>for_each_genome_in_row</code>
per-row decode, advancing to the next <code>slots</code> entry on exhaustion. O(Σ
row nnz), zero <code>n_cols</code>-wide allocation.</li>
<li><strong>On <code>PersistentBitMatrix::{Packed,Columnar}</code></strong>: revised — cheaper than
first thought, by reusing the same split already used for
<code>fill_matrix</code>'s own implementation instead of hand-writing a resumable
state machine at the matrix level. The base-vector layer
(<code>BitSliceView</code>, <code>views.rs</code>) already separates the two concerns:
<code>fill_batch_sorted</code> (<code>views.rs:55-60</code>, sorted-slot batch lookup) and a
genuine per-bit <code>Iterator</code> (<code>BitSliceIter</code>, <code>views.rs:94+</code>) sit side by
side there, one level below the matrix. Adding a
"positions among <code>sorted_slots</code> where the bit is set" iterator at that
same vector level is a <code>filter</code> over the existing <code>get()</code> — no new
state machine, since <code>std::iter::Filter</code> already <em>is</em> one, generated by
the compiler:
<div class="highlight"><pre><span></span><code><span class="k">fn</span><span class="w"> </span><span class="nf">nonzero_among_sorted</span><span class="o">&lt;&#39;</span><span class="na">s</span><span class="o">&gt;</span><span class="p">(</span><span class="o">&amp;&#39;</span><span class="na">s</span><span class="w"> </span><span class="bp">self</span><span class="p">,</span><span class="w"> </span><span class="n">sorted_slots</span><span class="p">:</span><span class="w"> </span><span class="kp">&amp;</span><span class="o">&#39;</span><span class="na">s</span><span class="w"> </span><span class="p">[</span><span class="kt">usize</span><span class="p">])</span><span class="w"> </span><span class="p">-&gt;</span><span class="w"> </span><span class="nc">impl</span><span class="w"> </span><span class="nb">Iterator</span><span class="o">&lt;</span><span class="n">Item</span><span class="w"> </span><span class="o">=</span><span class="w"> </span><span class="kt">usize</span><span class="o">&gt;</span><span class="w"> </span><span class="o">+</span><span class="w"> </span><span class="o">&#39;</span><span class="na">s</span><span class="w"> </span><span class="p">{</span>
<span class="w"> </span><span class="n">sorted_slots</span><span class="p">.</span><span class="n">iter</span><span class="p">().</span><span class="n">copied</span><span class="p">().</span><span class="n">filter</span><span class="p">(</span><span class="k">move</span><span class="w"> </span><span class="o">|&amp;</span><span class="n">slot</span><span class="o">|</span><span class="w"> </span><span class="bp">self</span><span class="p">.</span><span class="n">get</span><span class="p">(</span><span class="n">slot</span><span class="p">))</span>
<span class="p">}</span>
</code></pre></div>
The matrix-level <code>nonzero_iter</code> then composes these per column with
<code>flat_map</code> over <code>0..n_cols</code> (each column's hits, tagged with <code>c</code>,
slot mapped back through the sort permutation <code>fill_batch</code>/
<code>fill_batch_sorted</code> already carry) — again a combinator chain, not a
hand-rolled struct. Same algorithm, same mmap/sort locality as today's
<code>fill_sub_matrix</code>; just assembled from <code>std</code> iterator adaptors instead
of a loop body writing into a buffer, mirroring the vector/matrix split
the codebase already uses for <code>fill_batch_sorted</code> rather than
introducing a new shape.</li>
<li><strong><code>Implicit</code></strong>: trivial (<code>slots.iter().map(|&amp;i| (i, 0, 1))</code>, one column,
always present).</li>
<li><strong>On <code>PersistentCompactIntMatrix</code> (counts)</strong>: same treatment as
<code>Packed</code>/<code>Columnar</code> — no sparse count format exists yet ("Explicitly
deferred" per <code>traits.rs:9-12</code>), so no native low-effort case the way
<code>Sparse</code> has one, but not excluded either: the iterator's <code>Item</code> is
already <code>(usize, usize, u32)</code>, not <code>bool</code>, specifically so presence
(<code>0</code>/<code>1</code>) and counts (arbitrary <code>u32</code>) share one primitive instead of a
bool/u32 split forcing counts out of the design. Ready for a native
sparse-count struct later without a signature change.</li>
</ul>
<p>Would let <code>query_partition_with</code>'s stage 2 collapse to one
<code>layer.matrix().for_each_nonzero(&amp;hit_slots, |i, g, v| on_event(...))</code>
call per layer, format-agnostic, with each backend's existing (or
default-derived) implementation deciding the actual access pattern.</p>
<p><strong>Separately, an existing bug in the generic path</strong> (found while tracing
this, itself not yet fixed): <code>PersistentBitMatrix::fill_sub_matrix</code>
(<code>persistent.rs:190-215</code>, the enum wrapper backing <code>BinaryMatrix</code>'s
default trait impl) does <em>not</em> delegate to
<code>for (i, g, v) in layer.matrix().nonzero_iter(&amp;hit_slots) { on_event(...) }</code>
per layer, format-agnostic, each backend's struct deciding the actual
traversal.</p>
<p><strong>This also closes the existing dispatch bug for free, by construction</strong>:
<code>PersistentBitMatrix::fill_sub_matrix</code> (<code>persistent.rs:190-215</code>, the enum
wrapper backing <code>BinaryMatrix</code>'s trait impl) today does <em>not</em> delegate to
<code>PersistentSparseBitMatrix::fill_sub_matrix</code> for <code>Self::Sparse</code> — it
reimplements the same naive per-(column, slot) <code>fill_row_bool</code> loop
instead, bypassing the efficient native method one file over
(<code>sparse.rs:249-258</code>). <code>obikphylo::siblings::cache::Mat</code>
(<code>cache.rs:138-145</code>) independently built its own parallel enum wrapper
that dispatches correctly — a sign this was worked around rather than
fixed at the source. Any future <code>for_each_nonzero</code> work should fix this
dispatch too (or route through it), rather than adding a third
independently-dispatching wrapper.</p>
that dispatches correctly — evidence this was worked around rather than
fixed at the source: two hand-written traversals for the same format,
free to drift apart, and they did. If <code>fill_sub_matrix</code> itself is
rewritten as "drain <code>nonzero_iter</code>, scatter into <code>out[][]</code>", there is only
one traversal per format left to get right — the bug class doesn't just
get fixed once, it stops being possible to reintroduce.</p>
<h2 id="implemented-2026-08-20">Implemented (2026-08-20)</h2>
<p>Built as designed above, with one deviation from the original sketch:
<code>nonzero_iter</code> ended up <code>Box&lt;dyn Iterator&lt;...&gt;&gt;</code>, not a bare <code>impl
Iterator</code>, because <code>Columnar</code>/<code>Packed</code>/<code>Sparse</code>/<code>Implicit</code> are genuinely
different concrete types and this method isn't on a trait (kept off
<code>BinaryMatrix</code> deliberately — that trait is used as <code>dyn BinaryMatrix</code> in
<code>tests/sparse.rs</code>, and RPITIT methods aren't dyn-compatible). One <code>Box</code>
per <code>nonzero_iter</code> call, not per cell — negligible next to what it
replaces.</p>
<ul>
<li><code>BitSliceView::nonzero_among_sorted</code> / <code>IntSliceView::nonzero_among_sorted</code>
(<code>obicompactvec/src/views.rs</code>): the vector-level <code>filter</code>/<code>filter_map</code>
primitive, exactly as sketched — no new state machine, <code>std</code>'s own.</li>
<li><code>PersistentSparseBitMatrix::nonzero_iter</code> (<code>bitmatrix/sparse.rs</code>): native,
<code>std::iter::from_fn</code> over one buffered row at a time via the existing
<code>for_each_genome_in_row</code> — no <code>n_cols</code>-wide allocation, ever.</li>
<li><code>PersistentBitMatrix::nonzero_iter</code> (<code>bitmatrix/persistent.rs</code>): dispatches
to the above for <code>Sparse</code>; for <code>Columnar</code>/<code>Packed</code>, loops columns,
collects each column's <code>nonzero_among_sorted</code> hits via <code>.extend()</code> (not
<code>flat_map</code> — a <code>flat_map</code> closure can't lazily return something
borrowing its own captured sort permutation across separate calls
without either boxing per-column or fighting the borrow checker; eager
collection into one <code>Vec</code> sidesteps it, at zero cost since
<code>fill_sub_matrix</code> already fully materialized anyway). <code>Implicit</code> trivial.</li>
<li><code>PersistentBitMatrix::fill_sub_matrix</code> and <code>sub_matrix</code> rewritten to
drain <code>nonzero_iter</code> — the dispatch bug is gone because there is now
only one traversal per format, not because the old one was patched.
<code>PersistentCompactIntMatrix::nonzero_iter</code> added the same way (counts
not excluded, per the earlier ask) — no native low-effort case, since no
sparse count format exists, but on the same primitive, ready for one.</li>
<li><code>KmerPartition::query_partition_with</code> (<code>obikindex/src/partition/query_layer.rs</code>):
stage 2's column-major <code>for g { for slot { col_value } }</code> replaced by one
<code>layer.nonzero_iter(&amp;slot_list)</code> call per layer, format-agnostic.</li>
<li>Tests: <code>nonzero_iter_matches_dense</code>, <code>nonzero_iter_matches_row</code>, and —
the one that actually targets the dispatch bug rather than each type's
own correctness — <code>enum_wrapper_dispatches_to_native_sparse</code> (builds
<code>PersistentBitMatrix::Sparse(...)</code> directly, not through <code>open</code>, since
<code>open</code> only auto-detects <code>Sparse</code> from a <code>presence/</code> dir layout).
<code>cargo test --workspace</code>: green, no regressions.</li>
</ul>
<p><strong>Measured</strong>: re-ran the <code>benchmark/</code> query branch (100k reads × 2
specimens, same setup as the original finding). Correctness still 0
mismatches. The dense/sparse performance gap is gone — previously sparse
~30-50% slower than dense, reproducibly; now within ~1-3% either way
(7.42s dense vs 7.60s sparse for <code>Escherichia_coli--K-12_MG1655</code>; 5.25s vs
5.30s for <code>Saccharolobus_islandicus--M.16.4</code>) — noise-level, not a
systematic gap. <code>pack --sparse</code>'s claimed query win isn't confirmed
outright by this (sparse should arguably now <em>beat</em> dense on truly sparse
real data, not just tie), but the pathological regression is fixed.</p>