Refactor indexing pipeline and partition path resolution
Invert dependencies by moving partition path primitives to a dedicated module and updating the index crate accordingly. Reshape the partition router to accept a mutable index reference, enabling chainable configuration and resolving lifetime issues with explicit drops. Shift orchestration logic from the index crate to the CLI, replacing monolithic scatter calls with discrete dereplication and counting steps. Introduce a generic progress callback API to decouple rate calculation from UI rendering, and correct file I/O paths to route layer-0 artifacts under the partition index directory.
This commit is contained in:
@@ -5,7 +5,11 @@ exists, `Mat` is gone. (1b) done — `Layer::Empty`, the first non-ready
|
||||
state, added (panics on every read method). (2a) done — the
|
||||
`obikpartition` crate and `KmerPartition` itself exist (`open`/`n_layers`/
|
||||
`layer`/`layers`/`find`). (2b) — migrating `PartitionCache`/`QueryLayer`
|
||||
onto it — **not started**, deliberately deferred. Earlier mix-up, for
|
||||
onto it — **not started**, deliberately deferred. (3) done — the
|
||||
`obikindex ↔ obikpartitionner` dependency inverted: `PartitionRouter` now
|
||||
takes `&mut KmerIndex` and produces `Layer::Empty` shells directly, closing
|
||||
the gap `Layer::Empty` was built for in (1b) — see "(3) done" below.
|
||||
Earlier mix-up, for
|
||||
context: an earlier
|
||||
version of this doc used the name `KmerPartition` (singular) for what was
|
||||
actually the *collection* type (later renamed `KmerPartitions`, later
|
||||
@@ -315,6 +319,134 @@ tuple moved in wholesale); `scan_layer_families`'s still-independent
|
||||
`PartitionMeta::load` (see "Remaining instance…" below) — all explicitly
|
||||
deferred to whenever wiring is tackled next.
|
||||
|
||||
## (3) done (2026-08-20): `obikindex ↔ obikpartitionner` dependency inverted, `PartitionRouter` now fills `Layer::Empty` shells
|
||||
|
||||
Resolved a question left implicit since "Major restructuring": that pass
|
||||
set the direction `obikindex → obikpartitionner` (so `KmerIndex` could
|
||||
delegate `partition_dir` to it) without questioning whether that was the
|
||||
right direction at all. Challenged directly: `obikpartitionner` is an
|
||||
*algorithm* (superkmer routing/dereplication/counting) operating on an
|
||||
*index* (`KmerIndex`, the data structure) — algorithms depend on the data
|
||||
types they need, not the other way around. [[feedback_no_precedent_defense]]
|
||||
applied here: "that's the direction we already picked" was not treated as
|
||||
a justification for keeping it.
|
||||
|
||||
**New direction**: `obikpartitionner → obikindex` (+ `obilayeredmap`,
|
||||
`obipipeline`, `obiread` directly, for what `run`'s pipeline itself needs).
|
||||
`obikindex → obikpartitionner` is gone entirely — `KmerIndex` no longer
|
||||
imports `PartitionRouter`/`KmerSpectrum` in any form. Two path-naming
|
||||
primitives that used to make this edge necessary moved down a tier instead
|
||||
of staying put:
|
||||
- `partition_dir`/`PARTITIONS_SUBDIR` moved from `obikpartitionner` into
|
||||
`obikpartition` (the Partition-tier crate `KmerPartition` already lives
|
||||
in), alongside a new `index_dir(root, i)` — both free functions,
|
||||
mirroring `obilayeredmap::layer_dir` one tier down. `KmerIndex::
|
||||
partition_dir`/`index_dir` now delegate here instead of to
|
||||
`obikpartitionner`/an inline `.join("index")`.
|
||||
- `KmerIndex::create`/`create_skeleton` no longer call
|
||||
`PartitionRouter::create` to lay out an empty `partitions/` skeleton
|
||||
upfront — turned out to be dead weight once traced: `select_layer.rs`/
|
||||
`rebuild_layer.rs` already `create_dir_all` their own partition/layer
|
||||
directories on demand, and `Layer::create`'s directory-creation covers
|
||||
the scatter path the same way. Partitions and their layer-0 shells now
|
||||
come into existence lazily, on first write, with nothing to pre-create.
|
||||
`KmerIndex::create`'s now-unused `force: bool` parameter was dropped
|
||||
(4 call sites updated) rather than left as a dead parameter.
|
||||
|
||||
**`PartitionRouter` reshaped** (`obikpartitionner/src/partition/router.rs`)
|
||||
around the "création, paramétrage, run()" shape agreed on: `new(index:
|
||||
&mut KmerIndex) -> Self` (no disk access), chainable setters
|
||||
(`level_max`/`theta`/`workers`/`max_open`, defaults matching the CLI's old
|
||||
hardcoded values), then `run(path_source, on_progress)`. `write`/
|
||||
`write_batch`/`flush`/`close`/`dereplicate`/`count_kmer` stay public,
|
||||
unconsumed (`&self`/`&mut self`, not `self`) — callers needing fine-grained
|
||||
control (tests, `obikphylo`'s test harness) still get it, `run` is a
|
||||
convenience layered on top, not the only way in.
|
||||
|
||||
`run` absorbs the entire body of what used to be the free function
|
||||
`obikmer::steps::scatter` (now deleted, along with the `steps` module
|
||||
entirely) — the `obipipeline::make_pipe!` two-stage pipeline
|
||||
(file→pages→superkmers), throttling, per-file logging. What changed:
|
||||
- Every `ensure_writer(partition)` call now does `Layer::create(&layer0_dir)`
|
||||
(`layer0_dir = obilayeredmap::layer_dir(&index.index_dir(i), 0)`) before
|
||||
opening `raw.{ext}` inside it — raw/dereplicated superkmer files and the
|
||||
provisional `mphf1.bin`/`counts1.bin`/`kmer_spectrum_raw.json` now live
|
||||
under `<partition>/index/layer_0/`, not flat under `<partition>/` as
|
||||
before. This is `Layer::Empty` actually being used as the "builder code
|
||||
holding an `Empty` layer" its own (1b) docs anticipated, not just a shell
|
||||
with no consumer.
|
||||
- **Caught by an end-to-end smoke test, not by `cargo test`**: this path
|
||||
move broke `obikindex::index_layer::build_index_layer` and
|
||||
`remove_build_artifacts`, both of which still read/deleted
|
||||
`dereplicated.skmer.zst`/`mphf1.bin`/`counts1.bin` from
|
||||
`self.partition_dir(i)` (the old flat location) — no test in the
|
||||
workspace suite exercises the real CLI's file-reading `scatter` path
|
||||
end-to-end (`obikphylo`'s test harness and `obikpartitionner`'s own
|
||||
tests both call `write_batch` directly, bypassing `run`/file discovery
|
||||
entirely), so the whole suite stayed green while `obikmer index` on
|
||||
real FASTA silently indexed 0 kmers. Found by running the actual CLI
|
||||
against a small FASTA and noticing `count.json`'s `f0` (870, correct)
|
||||
didn't match "0 total kmers indexed" at the final stage. Fixed by
|
||||
retargeting both functions to `self.layer_dir(i, 0)`. **Lesson,
|
||||
consistent with the retracted-claim lesson above**: a green test suite
|
||||
is not proof a refactor is correct when no test in it exercises the
|
||||
specific path that changed — for anything touching the CLI's own
|
||||
file-driven entry point, running the CLI for real is not optional
|
||||
verification.
|
||||
- The internal `obisys::spinner("scatter")` + hand-rolled EMA-rate display
|
||||
is gone from the library entirely, replaced by an `Option<impl
|
||||
FnMut(obisys::Progress)>` parameter — a new, deliberately generic
|
||||
progress-reporting type (`obisys::Progress { position: u64, total:
|
||||
Option<u64> }`, alongside the existing `TracedBar`/`spinner`/
|
||||
`progress_bar`) added specifically so every future algo crate's `run()`
|
||||
reports progress the same shape, once, rather than each inventing its
|
||||
own. `total: None` here (bases processed isn't knowable without
|
||||
pre-scanning every input file) — deliberately simpler than the old
|
||||
in-library rate/file-count/thread-count message; the caller can
|
||||
recompute a Mbp/s rate from consecutive `position` values +
|
||||
wall-clock time itself, which is exactly what `cmd/index/mod.rs` now
|
||||
does to reproduce the old spinner message. This is a real, intentional
|
||||
restriction of the library's job: it reports raw ticks, the CLI decides
|
||||
what a human sees — same "generic vs. domain-specific" split applied
|
||||
again, this time to progress reporting rather than to Layer content.
|
||||
Explicitly **not** the same mechanism as `Stage`/`Reporter` (per
|
||||
[[feedback_stage_reporter_in_cmd_layer]]): `Stage`/`Reporter` measures a
|
||||
whole call's wall time from outside it; a progress callback has to fire
|
||||
*from inside* a loop mid-call, which wrapping from outside cannot
|
||||
express — two different needs, not the same rule reapplied under a new
|
||||
name. `Stage::start("scatter")`/`rep.push(...)` stayed in
|
||||
`cmd/index/mod.rs`, wrapping the whole `run()` call, unchanged in kind.
|
||||
- `dereplicate`/`count_kmer` keep their existing internal
|
||||
`obisys::progress_bar(...)` calls as-is (unconverted to the callback) —
|
||||
explicitly out of scope for this pass, by agreement.
|
||||
|
||||
**Forced, not optional, consequence of the dependency inversion**:
|
||||
`KmerIndex::dereplicate_and_count`/`partition_router`/`write_spectrum(&
|
||||
KmerSpectrum)` could not stay on `KmerIndex` at all once `obikindex` can no
|
||||
longer name `obikpartitionner::{PartitionRouter, KmerSpectrum}` in any
|
||||
position — not a design choice, a mechanical requirement of severing the
|
||||
edge. Replaced by: `KmerIndex::write_spectrum(f0: u64, f1: u64, counts:
|
||||
&BTreeMap<u32, u64>)` (plain values, no `KmerSpectrum` dependency) and a
|
||||
new `KmerIndex::mark_counted()` (symmetric to the already-existing
|
||||
`mark_scattered`), with the orchestration itself (`router.dereplicate()` →
|
||||
`router.count_kmer()` → `write_spectrum` → `mark_counted()`) now living in
|
||||
`cmd/index/mod.rs`, not `obikindex`.
|
||||
|
||||
Every `PartitionRouter::new(&mut index)` call in this codebase runs into
|
||||
the same NLL trap once: `PartitionRouter` has a `Drop` impl (auto-`close`
|
||||
on scope exit), which extends its `&mut KmerIndex` borrow to the end of
|
||||
the enclosing scope even after its last real use — `idx.mark_scattered()`
|
||||
right after `router.run(...)` (or `idx.write_spectrum(...)` right after
|
||||
`router.count_kmer(...)`) fails to borrow-check unless the router is
|
||||
`drop()`-ed explicitly first. Hit and fixed identically at all three call
|
||||
sites that needed it (`cmd/index/mod.rs` ×2, `obikphylo`'s test harness,
|
||||
`obikpartitionner`'s own tests).
|
||||
|
||||
Full workspace suite green (`cargo check --workspace --all-targets` +
|
||||
`cargo test --workspace`, exit code 0) both before and after the
|
||||
`index_layer.rs` fix above — the smoke test is what actually caught the
|
||||
regression the suite missed.
|
||||
|
||||
## The problem
|
||||
|
||||
Reading a layer's data (MPHF + matrix) is not free: `MphfLayer::open` mmaps
|
||||
|
||||
Reference in New Issue
Block a user