Extract dereplication logic into obikderep crate and centralize paths

Move partition dereplication logic into a dedicated `obikderep` crate that implements a two-phase hash-split/merge strategy with Rayon for parallel processing. Centralize superkmer file path construction in `obilayeredmap` and update dependent crates to use the new pipeline and shared path helpers. Adjust test suites to explicitly invoke the dereplication step.
2026-08-20 22:30:03 +02:00
parent 616cb76af3
commit abc51c2add
17 changed files with 534 additions and 97 deletions
@@ -8,8 +8,18 @@ state, added (panics on every read method). (2a) done — the
 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
+the gap `Layer::Empty` was built for in (1b) — see "(3) done" below. (4)
+done — dereplication split out into its own crate, `obikderep`, first step
+of an incremental "one algorithm at a time" split of `obikpartitionner`'s
+remaining bundle (`count_kmer`/`build_layers` not yet moved) — see "(4)
+done" below. **(5) — full design agreed, not yet implemented** —
+`KmerPartition` was found to be wired into nothing (`KmerIndex` never
+calls it; every path is still computed via free functions), and the fix
+turned out to be bigger than `KmerPartition` alone: `Layer`'s own
+constructors don't self-name either. Full redesign of both, agreed in
+detail, session ended (budget) before implementation — see "(5) design
+agreed" below; **read it before touching `KmerPartition`/`Layer`
+signatures**, the shape is fully specified. 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
@@ -447,6 +457,289 @@ Full workspace suite green (`cargo check --workspace --all-targets` +
 `index_layer.rs` fix above — the smoke test is what actually caught the
 regression the suite missed.

+## (4) done (2026-08-20): `obikderep` — dereplication split out of `obikpartitionner`, one algorithm at a time
+
+Follow-on question after (3): the indexing pipeline has 4 stages (scatter,
+dereplicate, count_kmer, index-build — see the CLI's own `Reporter` output,
+one line per stage), but `obikpartitionner` — a name that says
+*partitioning* — owned three of them (routing, dereplication, counting).
+Challenged directly, same as (3)'s dependency-direction question: a crate
+should hold what its name says, not accumulate unrelated stages just
+because they happened to land there first. Two ways to fix it — one crate
+renamed to hold all remaining stages, or one crate per stage — decided in
+favour of the latter, explicitly **incremental**: build the *second* algo
+crate first (`obikderep`, dereplication only), only then look at what it
+and `PartitionRouter` actually have in common, and factor a shared
+`Algorithm` trait (future `obikalgorithm` crate) from that real overlap —
+not guessed at from a single example. `count_kmer` and `build_layers`
+(currently `KmerIndex` inherent methods — itself flagged as inconsistent
+with "`KmerIndex` is a data structure, not a compute structure") are left
+alone this round, on purpose — one stage moves at a time.
+
+**`obikderep`** (new crate): `Dereplicator<'a> { index: &'a KmerIndex, n_partitions, level }`
+— `new(index: &KmerIndex)` (shared borrow, not `&mut`: dereplication never
+writes index metadata), no setters yet (nothing to configure), `run(on_progress)`
+does the two-phase split+merge dereplication in parallel across partitions,
+ported unchanged from `PartitionRouter::dereplicate` (moved wholesale:
+`optimal_buckets`/`dereplicate_partition`/`load_bucket`/`flush_map`/
+`remove_skmer_file`, now private to this crate in `dereplicate.rs`).
+`obikpartitionner::PartitionRouter::dereplicate` is gone; `count_kmer`
+stays.
+
+**A real signature difference from `PartitionRouter::run`, not an
+inconsistency**: `Dereplicator::run` takes `Option<impl Fn(Progress) +
+Sync>`, not `FnMut`. `PartitionRouter::run`'s callback is invoked from one
+sequential loop (`FnMut` is fine); `Dereplicator::run`'s work is
+`rayon::par_iter`, so the callback can be invoked concurrently from
+multiple worker threads — same reason `obisys::TracedBar`'s own methods
+take `&self`, not `&mut self`. Progress position is tracked with an
+`AtomicU64`, incremented from inside the parallel closure so each
+completed partition reports immediately — collecting all results first and
+reporting after (the first draft of this) would have delivered every tick
+in one burst at the very end, defeating the point of a live progress bar.
+`total: Some(n_partitions)` (known up front, unlike scatter's bases count)
+— `cmd/index/mod.rs` renders a real `progress_bar`, not a spinner, driven
+by the callback exactly like scatter's spinner is.
+
+**A second, pre-existing instance of the exact bug (3) fixed, caught
+before it shipped**: `dereplicated.skmer.zst` was hand-built as a string
+literal independently in three places — `obikpartitionner`'s
+`dereplicate.rs`/`count.rs` *and* `obikindex`'s `index_layer.rs` (a literal
+that already predated this session, never caught until now). Splitting
+dereplication into its own crate turns this from "two places, still
+matching by luck" into "three independent crates that must agree on a
+filename with no shared dependency forcing them to" — no longer
+deferrable. Fixed by adding `obilayeredmap::{raw_superkmers_path,
+dereplicated_superkmers_path}` (free functions, `layer_dir: &Path ->
+PathBuf`, mirroring `layer_dir` itself) — the filename lives in one place,
+in the Layer-tier crate every consumer here already depends on
+(`obikpartitionner`, `obikderep`, `obikindex` all reach it without a new
+edge), and no external crate ever sees the literal `"skmer.zst"` again.
+This reverses (3)'s own earlier call to keep `SK_EXT` private to
+`obikpartitionner` — that call assumed a single owner; a second owner
+appearing (`obikderep`) removed the assumption it rested on, so the
+decision changed with it, not out of inconsistency.
+
+Every `count_kmer` call site that used to run after `router.dereplicate()`
+on the same `PartitionRouter` now runs after a separate
+`Dereplicator::new(&idx).run(...)` call, on a freshly-constructed
+`PartitionRouter` — `PartitionRouter` no longer offers a combined
+"dereplicate then count" path. Updated at all three call sites that had
+one: `cmd/index/mod.rs`, `obikphylo`'s test harness, `obikpartitionner`'s
+own tests.
+
+Full workspace suite green (`cargo check --workspace --all-targets` +
+`cargo test --workspace`, exit code 0), plus an end-to-end CLI smoke test
+against real FASTA data (scatter → dereplicate → count → index-build →
+query, same numbers as (3)'s smoke test: 870 kmers) — required this time
+too, per (3)'s own lesson: no test in the suite exercises `obikmer index`'s
+real file-driven path.
+
+Still not done: `count_kmer`/`build_layers` staying where they are, the
+`obikalgorithm` shared-trait extraction (deliberately deferred until a
+third data point exists), and everything already listed under (2b).
+
+## (5) design agreed, not yet implemented (2026-08-20): `KmerPartition` rewritten, `Layer` gains self-naming, a future cache crate over `KmerIndex`
+
+Session ended (out of budget) before any of this was coded. Everything
+below is a **fully specified plan**, agreed sentence by sentence with the
+user — not a sketch to re-derive, not a proposal to re-litigate. Implement
+it as written; if something here turns out to be wrong once coded, fix it
+and update this section, don't restart the design conversation.
+
+### How this was found
+
+Direct question from the user: "tu as bien créé une structure
+`KmerPartition` ?" — yes (2a), but investigating exposed that it is
+**wired into nothing**. `KmerIndex` has no `partition(i)` method at all;
+`partition_dir`/`index_dir`/`layer_dir` still call `obikpartition::
+partition_dir`/`index_dir` and `obilayeredmap::layer_dir` as bare free
+functions directly, never touching a `KmerPartition`/`Layer` object to get
+there. The end result on disk is identical (same paths), which is exactly
+why no test caught it — but the *responsibility* is in the wrong place:
+one function (on `KmerIndex`) knows the whole three-tier naming
+convention, instead of each tier asking the one below it for its own
+path. User's framing, verbatim, now saved as [[feedback_no_spaghetti_petits_pois]]:
+"spaghetti" (logic untraceable, split across too many unrelated crates)
+and "petits pois" (small bits of naming logic dispersed with no owning
+object) are **strictly forbidden** — this was a live example of both.
+
+Pushed further, twice:
+1. First correction: `Layer::create(&obilayeredmap::layer_dir(&dir, 0))` —
+   still a free-function call from *outside* `Layer` to compute where it
+   should live. "Le layer n'est pas con, c'est lui qui dit où est-ce qu'il
+   doit être sauvé" (the layer isn't stupid, it says itself where it
+   should be saved).
+2. Second correction, the general principle: **"une partition est juste
+   identifiée par un numéro, tout se calcule à partir du numéro, et un
+   layer est identifié à partir d'un numéro et tout se calcule à partir de
+   ce numéro."** Concretely: each object stores its own local identifying
+   number *plus* its immediate parent's path (captured once, at
+   construction) — never a path handed in again later by a caller, and
+   never a free function outside the object that can compute that path
+   independently. Explicitly rejected along the way: making users pass
+   "the partition's path that contains the layer" to open a layer — the
+   parent path is captured once, at the child's construction, not
+   re-supplied at every call.
+
+### The agreed shape
+
+**`Layer`** (`obilayeredmap`) — identified by `l` + its parent partition's
+directory, both captured at construction, never received again:
+
+```rust
+pub enum Layer {
+    Empty { partition_dir: PathBuf, l: usize },   // pure identification, no disk I/O
+    Count(TypedLayer<PersistentCompactIntMatrix>),
+    Presence(TypedLayer<PersistentBitMatrix>),
+}
+
+impl Layer {
+    pub fn at(partition_dir: &Path, l: usize) -> Self;                 // identify only
+    fn dir(&self) -> PathBuf;                                          // private — layer_dir() no longer a public free function, folded in here
+    pub fn create(self) -> io::Result<Self>;                           // creates the directory if needed; no path parameter anymore
+    pub fn open(self, mode: &IndexMode, with_counts: bool) -> OLMResult<Self>;  // no path parameter anymore
+    // mphf_path()/unitigs_path()/evidence_path()/fingerprint_path()/counts_dir()/presence_dir()
+    // unchanged in spirit, implemented via self.dir() instead of a stored `dir` field read directly
+}
+```
+
+Note this **replaces** `Layer::Empty { dir: PathBuf }` from (1b) — `dir`
+becomes a computed value (`partition_dir.join(format!("layer_{l}"))`), not
+a stored field. `obilayeredmap::layer_dir`/`raw_superkmers_path`/
+`dereplicated_superkmers_path` (currently public free functions,
+introduced in (3)/(4)) stop being called from outside `obilayeredmap`
+entirely once this lands — they were the right fix for their moment (a
+second crate, `obikderep`, needed to agree on a filename with no owner),
+but the *real* fix, now visible with a third data point, is that `Layer`
+itself should be the only thing anyone asks.
+
+**`KmerPartition`** (`obikpartition`) — same principle, one tier up:
+
+```rust
+pub struct KmerPartition {
+    index_root: PathBuf,   // the parent KmerIndex's root, captured once
+    i: usize,
+}
+
+impl KmerPartition {
+    pub fn new(index_root: PathBuf, i: usize) -> Self;                 // identify only, no disk I/O
+    pub fn create(index_root: PathBuf, i: usize) -> io::Result<Self>;  // creates this partition's directory + an empty layer 0 (a partition is never born without one — that knowledge lives here, not in whoever calls create)
+    pub fn partition_dir(&self) -> PathBuf;                            // part_{i:05}
+    pub fn index_dir(&self) -> PathBuf;                                // part_{i:05}/index
+    pub fn layer(&self, l: usize) -> Layer;                            // Layer::at(&self.index_dir(), l) — caller never touches a path
+    pub fn meta(&self) -> SKResult<PartitionMeta>;                     // n_layers + mode; must absorb the recovery-on-missing-file logic
+                                                                         // currently private in obikindex::common::load_meta (obikpartition
+                                                                         // can't depend on obikindex to reuse it — this logic moves down)
+    pub fn n_layers(&self) -> SKResult<usize>;                         // meta()?.n_layers
+    pub fn mode(&self) -> SKResult<IndexMode>;                         // meta()?.mode — "exact/approximatif"
+    pub fn is_filled(&self) -> bool;                                   // does this partition's directory exist at all
+    pub fn n_kmers(&self) -> io::Result<usize>;                        // LayerMeta::load(&self.layer(0).dir()).n — reads layer 0's count as
+                                                                         // a representative figure, same "read the first one" trick
+                                                                         // n_layers_per_partition() already uses at the KmerIndex level
+}
+```
+
+This **replaces** (2a)'s `KmerPartition { layers: Vec<Layer> }` entirely
+— no eagerly-opened `Vec<Layer>`, no `find()` (both belong to the future
+cache, see below, which is the thing that actually holds opened layers
+alive across many lookups). (2a)'s version is safe to delete outright: it
+was never wired into anything (confirmed above), so nothing depends on
+its current shape. New dependencies needed: `obikpartition` gains
+`obiskio` (for `SKResult`) and `obicompactvec` (for `LayerMeta`).
+
+Deliberately **not built this round**: cross-level consistency checks
+("verify everything below me is in the same state") — a real idea, raised
+by the user, but nothing concrete needs it yet; building it speculatively
+would be exactly the premature-abstraction pattern this project avoids.
+
+**`KmerIndex`** (`obikindex`) — becomes the sole entry point:
+
+```rust
+pub fn partition(&self, i: usize) -> KmerPartition;   // KmerPartition::new(self.root_path.clone(), i)
+```
+
+`partition_dir(i)`/`index_dir(i)`/`layer_dir(i, l)` **stay** as public
+methods (≈30 existing call sites across `obikindex`/`obikphylo` — see (3)'s
+option A, applied identically here) but become pure delegations:
+`self.partition(i).partition_dir()`, `self.partition(i).index_dir()`,
+`self.partition(i).layer(l).dir()` (needs `Layer::dir()` to be visible
+enough for this — likely `pub(crate)` in `obilayeredmap` plus a thin
+public wrapper, or a public accessor on `Layer` itself; not fully nailed
+down, decide while implementing). No caller outside `obikindex` changes.
+
+### Known blast radius (why this wasn't done in the same session)
+
+- **14 files** call `Layer::open`/`Layer::create` directly today
+  (`obikphylo/siblings/{cache,build,family_scan,tests}.rs`,
+  `obikpartitionner/partition/router.rs`, `obikpartition/src/lib.rs`,
+  `obikindex/{rebuild_layer,dump_layer,index,query_layer}.rs`,
+  `obilayeredmap/{mphf_layer,layer,map,content_layer}.rs`) — every one
+  loses its path parameter and gains a `(partition_dir, l)` or an
+  already-identified `Layer` to call `.create()`/`.open()` on instead.
+- **≈30 files** call `KmerIndex::partition_dir`/`index_dir`/`layer_dir` —
+  unaffected in their own code (same public signatures), but worth
+  re-checking once (5) lands that none of them were relying on the old
+  free-function-based implementation in a way the new delegation breaks.
+- `obikpartitionner::PartitionRouter::ensure_writer` and `obikderep`'s
+  `run` both currently call `obilayeredmap::{layer_dir, raw_superkmers_path,
+  dereplicated_superkmers_path}` directly (from (3)/(4)) — both need to
+  switch to going through `index.partition(i).layer(0)` instead.
+
+### Also agreed, separately: `PartitionRouter::new` never needed `&mut KmerIndex`
+
+Verified by reading the code: every call `PartitionRouter` makes on
+`index` is `&self` (`index.kmer_size()`, `index.index_dir(i)`). The `&mut`
+in its current signature (from (3)) was inherited from the original
+"the router writes to the partitions" reasoning, never actually required
+by any method call. This is *exactly* what caused every `drop(router)`
+workaround needed throughout (3)/(4) (`cmd/index/mod.rs` ×2, `obikphylo`'s
+test harness, `obikpartitionner`'s own tests) — `PartitionRouter` holds a
+`Drop` impl, which extends a `&mut` borrow to the end of its scope even
+past its last real use. **Fix alongside (5)**: change
+`PartitionRouter::new(index: &'a mut KmerIndex)` to `&'a KmerIndex`, and
+remove the now-unnecessary `drop(router)` calls at all four sites.
+
+### Also discussed: a future cache crate, not part of (5), not `obikalgorithm` either
+
+Separate idea, explicitly **not** part of this design and **not** started:
+a new crate whose only job is to cache open `KmerPartition`s (and their
+opened `Layer`s) across one run — replacing both `obikphylo::siblings::
+cache::PartitionCache` (today, sibling-specific, holds `Vec<Vec<Layer>>`)
+and `obikindex::query_layer::QueryLayer` (today, uncached, bypasses
+`Layer` entirely) — the two consumers (2b) already identified as each
+reinventing a fragment of the same thing.
+
+User's framing: this is **not** a third `obikalgorithm` data point — an
+algorithm has a `new → run → done` shape; a cache has a fundamentally
+different one (open, stay alive for a whole run, serve lookups, maybe
+evict) — "on crée un cache sur un index, ça consomme un index." Two
+distinct crate *roles* in this ecosystem (data crates: `obikpartition`/
+`obilayeredmap`; algorithm crates: `obikpartitionner`/`obikderep`/future
+`obikalgorithm` implementors; and now a cache/service crate), not one
+unified shape to force everything into.
+
+Depends on (5) being done first: the cache crate's whole job is holding
+`Vec<KmerPartition>`/opened `Layer`s alive, built via `KmerIndex::
+partition(i)` as its factory — nothing to build it on top of until (5)
+lands. Still open once (5) is done: eviction policy vs. holding everything
+open for the process lifetime (the never-measured mmap/VM-mapping-count
+question from earlier in this doc), and whether it lives in `obikpartition`
+itself or a new crate.
+
+### Order of remaining work, as currently understood
+
+1. **(5)** — `Layer`/`KmerPartition`/`KmerIndex` rewrite described above,
+   plus the `PartitionRouter` `&mut` → `&` fix (same root cause, same
+   session, do together).
+2. The future cache crate (name not chosen), consuming `KmerIndex::
+   partition(i)` — unblocks migrating `PartitionCache`/`QueryLayer` (2b).
+3. `obikalgorithm` — still deliberately waiting for a third `run()`-shaped
+   data point (`count_kmer` or `build_layers` migrating out of
+   `KmerIndex`/`PartitionRouter`) before extracting a shared trait; two
+   examples were judged not enough to be sure of the shape (`Fn+Sync` vs
+   `FnMut` callback bound already diverged between the two that exist).
+
 ## The problem

 Reading a layer's data (MPHF + matrix) is not free: `MphfLayer::open` mmaps