1042 lines
60 KiB
Markdown
1042 lines
60 KiB
Markdown
# Partition and layer caching (discussion)
|
|
|
|
**Superseded (2026-08-21):** `obikpartition` and `obilayeredmap` are no
|
|
longer separate workspace crates — both were folded back into `obikindex`
|
|
as submodules (`obikindex::partition`, `obikindex::layer`), alongside the
|
|
crate's original content as `obikindex::index`, purely to reduce the
|
|
crate count (no behavior change). Every mention of `obikpartition`/
|
|
`obilayeredmap` as a *crate* below, and every dependency-direction
|
|
argument phrased in terms of "which crate depends on which" (e.g. "this
|
|
crate depends only on `obilayeredmap` and below, never on `obikindex`"),
|
|
describes that now-superseded split-crate architecture and is kept as-is
|
|
for historical context — read `obikpartition::X` as `obikindex::
|
|
partition::X` and `obilayeredmap::X` as `obikindex::layer::X` throughout.
|
|
The underlying module boundary and its rationale (Layer tier / Partition
|
|
tier / Index tier, each depending only downward) are unchanged; only the
|
|
crate-vs-module packaging changed. See [obikindex::layer](layer_tier.md)
|
|
for the current module doc.
|
|
|
|
**Superseded, second event, same day (2026-08-21):** `obikpartitionner`
|
|
and `obikderep`, the two algorithm crates, are also gone — but unlike
|
|
`obikpartition`/`obilayeredmap` above, they were **not** folded into
|
|
`obikindex`. They were first (mistakenly) merged into `obikindex` as an
|
|
`algorithms` submodule, then corrected into a new sibling crate,
|
|
**`obikindexer`**, holding `obikindexer::algorithms::{partitionner,
|
|
dereplicator}` and depending on `obikindex` — never the reverse, same
|
|
dependency direction `obikpartitionner`/`obikderep` already had. Read
|
|
`obikpartitionner::X` as `obikindexer::algorithms::partitionner::X` and
|
|
`obikderep::X` as `obikindexer::algorithms::dereplicator::X` throughout
|
|
what follows. The distinction the mistake surfaced, worth keeping: data
|
|
crates (`obikindex`, holding the `index`/`partition`/`layer` model) merge
|
|
naturally into one crate as submodules; algorithm crates that operate on
|
|
that model from outside stay separate, so the dependency only ever runs
|
|
one way.
|
|
|
|
Status (2026-08-20, latest pass): (1) done — `obilayeredmap::Layer`
|
|
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. (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. (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
|
|
merged into `KmerIndex` — see "Major restructuring" below), and never
|
|
retracted that usage before this section was rewritten. An agent working
|
|
from that stale wording built the wrong thing. **If you are about to
|
|
implement (2), read "Definitions: `obikpartition` and `KmerPartition`"
|
|
below — it is the current, authoritative naming — before touching any
|
|
other section of this file, some of which still describe superseded
|
|
states of the code and are kept only as dated history.**
|
|
|
|
## Definitions: `obikpartition` and `KmerPartition` (not yet created)
|
|
|
|
**`obikpartition`** — a new workspace crate, not created yet. Holds the
|
|
**Partition** tier of the `Index { Partition { Layer } }` model, the same
|
|
way `obilayeredmap` already holds the **Layer** tier as its own crate
|
|
rather than living inside `obikindex`. Depends only on `obilayeredmap`
|
|
(for `Layer`) and lower (`obikseq`, `obiskio`). Does **not** depend on
|
|
`obikindex`, `obikpartitionner`, or `obikphylo`. Dependency direction:
|
|
`obikindex → obikpartition → obilayeredmap`; `obikphylo → obikindex`
|
|
(and/or `obikpartition` directly if it ends up needing it without going
|
|
through `KmerIndex`).
|
|
|
|
**`KmerPartition`** (singular) — the one type this crate exists for.
|
|
Represents **one partition's already-open layers** — a read cache, built
|
|
once per partition and held for the run, not rebuilt per lookup. Shape:
|
|
|
|
```rust
|
|
pub struct KmerPartition {
|
|
layers: Vec<obilayeredmap::Layer>,
|
|
}
|
|
```
|
|
|
|
Nothing else. In particular:
|
|
- **No path computation.** `KmerPartition::open` takes an already-resolved
|
|
`index_dir: &Path` (plus `mode: &IndexMode`, `n_layers: usize`,
|
|
`with_counts: bool` — whatever it needs, as plain arguments), the same
|
|
discipline `obikpartitionner::PartitionRouter::open` already follows.
|
|
Computing `index_dir`/`layer_dir` from a partition number is
|
|
`KmerIndex`'s job (`obikindex`, which owns that already — see "Major
|
|
restructuring" below); `KmerPartition` never reaches back into
|
|
`KmerIndex` to get it (would require `obikpartition → obikindex`, the
|
|
wrong direction).
|
|
- **No routing/write state.** Writing raw superkmers, `dereplicate`,
|
|
`count_kmer` stay in `obikpartitionner::PartitionRouter` — a completely
|
|
different crate, a completely different phase (pre-layer, whereas
|
|
`KmerPartition` only makes sense once layers exist).
|
|
- **No multi-partition collection baked in.** `KmerPartition` is *one*
|
|
partition. Whatever ends up caching several of them (replacing
|
|
`obikphylo::siblings::cache::PartitionCache`'s `Vec<Vec<Layer>>` and
|
|
`obikindex::query_layer`'s per-call reopen) holds `Vec<KmerPartition>` —
|
|
that collection can live in `obikpartition` too, or in `obikindex`
|
|
alongside `KmerIndex`; not yet decided, secondary to getting
|
|
`KmerPartition` itself right first.
|
|
|
|
**Do not confuse with `KmerPartitions`** (plural — note the `s`): that
|
|
type is **gone**. It used to be `obikpartitionner`'s (nee `obikpartition`,
|
|
briefly — see the crate-rename history below, itself a separate rename
|
|
from this one) do-everything struct — routing, dereplication, *and* path
|
|
lookups all in one. It was deleted on 2026-08-20; its read-side (paths,
|
|
`n_layers`, `partition_meta`) was absorbed into `KmerIndex`, its
|
|
write-side became `PartitionRouter`. `KmerPartition` (this section,
|
|
singular, no final `s`) is a brand-new type with a different job, in a
|
|
crate that doesn't exist yet — not a revival, not a renaming, of
|
|
`KmerPartitions`.
|
|
|
|
## Type-to-concept mapping: Index / Partition / Layer
|
|
|
|
The conceptual nesting `Index { Partition { Layer { MPHF, Evidence, Matrix
|
|
} } } }`, current state:
|
|
|
|
- **Index** = `obikindex::KmerIndex` — `{ root_path, meta: IndexMeta }`.
|
|
Also directly exposes the partition-path/metadata accessors
|
|
(`partition_dir(i)`, `index_dir(i)`, `layer_dir(i, l)`,
|
|
`partition_meta(i)`, `n_layers(i)`, `partition_mode(i)`,
|
|
`n_partitions()`) since `KmerPartitions` merged into it (see "Major
|
|
restructuring" below) — `KmerIndex` today *is* "index + collection of
|
|
partitions' paths & metadata," just without a `Vec` of open layers.
|
|
- **Partition, the collection** = no dedicated type today; the closest
|
|
thing is `KmerIndex` itself (previous bullet). Once `KmerPartition`
|
|
(singular, see Definitions above) exists, a `Vec<KmerPartition>`
|
|
somewhere would be this — still open, see "Direction agreed" below.
|
|
- **Partition, one of them** = `obikpartition::KmerPartition` — **to be
|
|
built**, see Definitions above. Nothing plays this role today;
|
|
`obikphylo::siblings::cache::PartitionCache` and
|
|
`obikindex::query_layer::QueryLayer` each independently reinvent a
|
|
fragment of it.
|
|
- **Layer** = `obilayeredmap::Layer` (format-erased: `Count`/`Presence`,
|
|
each wrapping a `TypedLayer<D>`) — see "(1) done" below for how this
|
|
came to be; `TypedLayer<D>` (`{ mphf: MphfLayer, data: D }`, monomorphic)
|
|
is the lower-level, `D`-fixed building block `Layer` is built on, not
|
|
what other crates should reach for directly.
|
|
- **MPHF** = `MphfLayer.mphf: MemCase<MphfEps>` — kmer → slot.
|
|
- **Evidence** = `MphfLayer.ev: LayerEvidence` (`Exact`/`Approx`/
|
|
`Hybrid` — `evidence.bin`/`fingerprint.bin`; see `EvidenceKind`).
|
|
- **Matrix** = `TypedLayer<D>.data: D` — `PersistentBitMatrix` /
|
|
`PersistentCompactIntMatrix`.
|
|
|
|
Target nesting once `KmerPartition` exists:
|
|
|
|
```
|
|
KmerIndex (obikindex)
|
|
└─ (opened on demand, per i) KmerPartition (obikpartition — not yet built)
|
|
└─ layers: Vec<Layer> (obilayeredmap)
|
|
└─ Layer::Count/Presence(TypedLayer<D>)
|
|
└─ TypedLayer<D> { mphf: MphfLayer, data: D }
|
|
├─ mphf.mphf → MPHF
|
|
├─ mphf.ev → Evidence
|
|
└─ data → Matrix
|
|
```
|
|
|
|
## Major restructuring (2026-08-20): `KmerPartitions` merged into `KmerIndex`
|
|
|
|
Prompted by a direct question: why keep `KmerIndex`/`KmerPartitions` split
|
|
when, one level down, `KmerPartitions` is going to directly hold
|
|
`Vec<KmerPartition>` rather than being split again into
|
|
"collection-holder" + "collection"? Investigating the actual justification
|
|
("`KmerPartitions` has an independent lifecycle, used before an index
|
|
exists") turned out to be **false** — `KmerPartitions::create` was called
|
|
in exactly one place, inside `KmerIndex::create`, and every
|
|
`open_with_config` reopen outside `KmerIndex`'s own constructors was a
|
|
redundant re-derivation of a `KmerPartitions` already reachable via
|
|
`index.partition()` (the exact kind of duplication this whole doc has been
|
|
tracking). Once that was gone, so was the reason to keep them separate.
|
|
|
|
Second correction, from the same conversation: `obikpartitionner` had
|
|
accumulated query/merge/select/rebuild/dump/distance logic that has
|
|
nothing to do with partitioning super-kmers — it operates on *layers*,
|
|
which don't exist yet at the phase `obikpartitionner` is actually
|
|
responsible for (scatter → dereplicate → count, all pre-layer). That
|
|
logic moved to `obikindex`, which already depends on `obilayeredmap` and
|
|
never needed `obikpartitionner` for it. No crate-dependency inversion was
|
|
needed — `obikindex → obikpartitionner` stays the same direction as before.
|
|
|
|
**Result:**
|
|
- `obikpartitionner` (renamed back from `obikpartition`) now contains only
|
|
`PartitionRouter` (superkmer routing: `write`/`write_batch`/`flush`/
|
|
`close`, `dereplicate`, `count_kmer`, `KmerSpectrum`) and the
|
|
`partition_dir(root, i)` naming primitive both `PartitionRouter` and
|
|
`KmerIndex` build on. `KmerPartitions` no longer exists as a type.
|
|
- `KmerIndex` (`obikindex`) absorbed `KmerPartitions`'s read-side entirely:
|
|
`partition_dir`/`index_dir`/`layer_dir`/`partition_meta`/`n_layers`/
|
|
`partition_mode`/`n_partitions` (the last now derived from
|
|
`2^config.n_bits`, no longer a stored, independently-set duplicate field
|
|
— `kmer_size`/`minimizer_size` used to be double-stored, in both
|
|
`KmerPartitions` and `IndexMeta.config`, a latent-drift risk flagged
|
|
earlier in this doc; now single-sourced from `IndexMeta.config`). Seven
|
|
whole files moved from `obikpartitionner` into `obikindex` verbatim as
|
|
`impl KmerIndex` blocks, kept as separate files (not merged into
|
|
existing same-topic files): `index_layer.rs`, `query_layer.rs`,
|
|
`merge_layer/`, `select_layer.rs`, `rebuild_layer.rs`, `dump_layer.rs`,
|
|
plus `distance.rs`'s `count_store`/`presence_store` (renamed
|
|
`matrix_store.rs` to avoid colliding with `obikindex`'s own pre-existing
|
|
`distance.rs`), and their shared support (`common.rs`'s `load_meta`/
|
|
`olm_to_sk`, `filter.rs`, `graph_pipeline.rs`).
|
|
- `obikphylo::siblings::cache::PartitionCache::build` now takes `&KmerIndex`
|
|
directly instead of a separately-opened `&KmerPartitions` — this deleted
|
|
the redundant-reopen pattern at all 8 call sites
|
|
(`alignment`/`build`/`cardinality`/`distance`/`entropy`×2/
|
|
`sankoff_bundle`/`stats`), the same bug flagged earlier in this
|
|
conversation as a side effect of investigating the false "independent
|
|
lifecycle" claim.
|
|
- `KmerIndex::partition()`/`partition_mut()` are gone; `scatter()`
|
|
(`obikmer`) and any write-side code get a transient `PartitionRouter` via
|
|
`KmerIndex::partition_router()`.
|
|
- A real bug caught by the test suite during this move:
|
|
`PartitionRouter::open` initially defaulted to `closed: true` (inherited
|
|
from `KmerPartitions::open_with_config`'s old read-only-reopen
|
|
semantics), which broke every write through a router obtained via
|
|
`partition_router()`. Fixed — `PartitionRouter` is exclusively a
|
|
write/processing tool now, so `open` always starts open.
|
|
|
|
Full workspace test suite green (0 failed) after, including all 27
|
|
`obikphylo::siblings` tests.
|
|
|
|
## (1) done (2026-08-20): `Layer` is now the heterogeneous handle, `Mat` is gone
|
|
|
|
Resolved the naming question left open above. `Layer<D>` (the old
|
|
generic/monomorphic type) renamed to `TypedLayer<D>` throughout
|
|
(`obilayeredmap`, `obikindex`, `obikphylo` — 12 files, mechanical) to free
|
|
`Layer` for the type that's actually meant to be everyone's default
|
|
handle. `obilayeredmap::content_layer::Layer` (re-exported at the crate
|
|
root) is that type — `Count(TypedLayer<PersistentCompactIntMatrix>)`/
|
|
`Presence(TypedLayer<PersistentBitMatrix>)`, `Layer::open` doing the same
|
|
disk probe `Mat::open` used to, `find_slot`/`index_batch`/`n_cols`/
|
|
`fill_sub_matrix_carries` dispatching per variant exactly as `Mat` did.
|
|
|
|
`obikphylo::siblings::cache::Mat` deleted outright — `PartitionCache` now
|
|
holds `Vec<Vec<obilayeredmap::Layer>>` directly. The one sibling-specific
|
|
method `Mat` carried (`iter_minorants_batch`) is not on `obilayeredmap::
|
|
Layer` (phylo concepts don't belong in `obilayeredmap`) — it's an
|
|
`impl SiblingLayerExt for obilayeredmap::Layer` in `iter.rs`, dispatching
|
|
to each variant's existing `impl<D: LayerData> SiblingLayerExt for
|
|
TypedLayer<D>`.
|
|
|
|
Full workspace suite green (0 failed) after, including all 27
|
|
`obikphylo::siblings` tests.
|
|
|
|
Still not built: (2) — `KmerPartition` (singular, one partition's open
|
|
`Vec<Layer>`) and a multi-partition cache in `obikpartitionner` to replace
|
|
`obikphylo::siblings::cache::PartitionCache` and `obikindex::query_layer`'s
|
|
still-separate `QueryLayer` (which still independently bundles MPHF+matrix,
|
|
2-way not using `Layer` at all). Both remaining consumers now sit one
|
|
`Layer::open` call away from unifying onto (2) once it exists.
|
|
|
|
## (1b) done (2026-08-20): `Layer::Empty` — the first non-ready-to-read state
|
|
|
|
First step toward `Layer` representing a layer's whole life, not just the
|
|
open-for-reading end of it (see "Definitions" above: `KmerPartition` will
|
|
hold `Vec<Layer>` regardless of each layer's state, states in between
|
|
included). Added one variant:
|
|
|
|
```rust
|
|
pub enum Layer {
|
|
Empty { dir: PathBuf },
|
|
Count(TypedLayer<PersistentCompactIntMatrix>),
|
|
Presence(TypedLayer<PersistentBitMatrix>),
|
|
}
|
|
```
|
|
|
|
`Layer::create(dir)` makes the directory and returns `Empty { dir }` —
|
|
nothing else; no MPHF/unitigs/evidence construction yet (that's the
|
|
deferred next step: `build_mphf()`/`build_unitigs()`/`build_evidence()`
|
|
methods to progress `Empty` → eventually `Count`/`Presence`). `Empty`
|
|
carries path accessors so builder code has one place to get
|
|
`mphf_path()`/`unitigs_path()`/`evidence_path()`/`fingerprint_path()`/
|
|
`counts_dir()`/`presence_dir()` from, instead of redeclaring the
|
|
`mphf.bin`/`unitigs.bin`/… filenames at each write site — reusing the
|
|
constants `layer.rs`/`mphf_layer.rs` already own (`COUNTS_DIR`/
|
|
`PRESENCE_DIR` widened from private to `pub(crate)`, file-name constants
|
|
already were).
|
|
|
|
Every read method (`content`/`evidence_kind`/`n`/`find_slot`/
|
|
`index_batch`/`n_cols`/`fill_sub_matrix_carries`) panics on `Empty` with a
|
|
one-line message naming the method — confirmed as the right behaviour:
|
|
calling any of them on an `Empty` layer means the caller assumed a layer
|
|
was ready when it wasn't, an implementation error to surface loudly, not
|
|
a case to design around (`Option`/`Result` would let it silently
|
|
propagate instead of failing at the actual mistake). Same panic added to
|
|
`obikphylo::siblings::iter.rs`'s `impl SiblingLayerExt for
|
|
obilayeredmap::Layer` (4 methods), the one other place that exhaustively
|
|
matched `Layer`'s variants.
|
|
|
|
Full workspace suite green (`cargo check --workspace --all-targets` then
|
|
`cargo test --workspace`, exit code 0) after.
|
|
|
|
Still deferred, per explicit instruction: `build_mphf()`/
|
|
`build_unitigs()`/`build_evidence()` to progress `Empty` further, and (2)
|
|
— `KmerPartition` itself — unchanged from above (see "(2a) done" below,
|
|
added next).
|
|
|
|
## (2a) done (2026-08-20): `obikpartition` crate + `KmerPartition`
|
|
|
|
Built exactly the shape "Definitions" (top of file) specifies, nothing
|
|
more — deliberately scoped down from the full "Direction agreed" plan
|
|
below: only steps 1–2 (`open`/`n_layers`/`layer`/`layers`/`find`), not 3–4
|
|
(migrating `PartitionCache`/`QueryLayer` onto it), per explicit
|
|
instruction to implement `KmerPartition` first and decide the wiring
|
|
("comment on branche tout ça dans la construction") separately, later.
|
|
|
|
```rust
|
|
pub struct KmerPartition {
|
|
layers: Vec<obilayeredmap::Layer>,
|
|
}
|
|
|
|
impl KmerPartition {
|
|
pub fn open(index_dir: &Path, mode: &IndexMode, n_layers: usize, with_counts: bool) -> OLMResult<Self>;
|
|
pub fn n_layers(&self) -> usize;
|
|
pub fn layer(&self, i: usize) -> &Layer;
|
|
pub fn layers(&self) -> &[Layer];
|
|
pub fn find(&self, kmer: CanonicalKmer) -> Option<usize>;
|
|
}
|
|
```
|
|
|
|
`open` takes `index_dir`/`mode`/`n_layers`/`with_counts` as plain
|
|
arguments — no reach-back into `KmerIndex` (would need `obikpartition →
|
|
obikindex`, the wrong direction) — and builds each layer's path via
|
|
`obilayeredmap::layer_dir(index_dir, l)`, the same shared naming
|
|
primitive `KmerIndex::layer_dir` itself delegates to, not a second copy of
|
|
the `layer_N` convention. `find` mirrors `PartitionCache::find`'s
|
|
semantics (first layer that carries the kmer wins) but doesn't yet cover
|
|
`find_presence_batch`/`find_presence_batch_fast` — those exist only to
|
|
serve `PartitionCache`, so they're part of the (2b) migration, not this
|
|
step; building them now against the current sibling-specific tuple shape
|
|
`(CanonicalKmer, usize, u8, u8)` would either bake phylo vocabulary
|
|
(`family_idx`, `base`) into `obikpartition` or require deciding a generic
|
|
payload shape — a real design fork, deferred to when (2b) is actually
|
|
tackled rather than guessed at here.
|
|
|
|
Crate deps: `obikseq`, `obilayeredmap` only (dev-deps add `obiskio`,
|
|
`obicompactvec`, `tempfile` for tests) — matches the "Definitions"
|
|
constraint (`obikpartition` depends on `obilayeredmap` and below, never
|
|
`obikindex`/`obikpartitionner`/`obikphylo`). Registered as a new workspace
|
|
member (`src/Cargo.toml`). 3 new tests (`open_reads_every_layer_in_order`,
|
|
`find_reports_the_first_layer_that_carries_the_kmer`,
|
|
`find_returns_none_for_an_absent_kmer`). Full workspace suite green
|
|
(`cargo check --workspace --all-targets` then `cargo test --workspace`)
|
|
after.
|
|
|
|
Still not done: (2b) — migrating `obikphylo::siblings::cache::
|
|
PartitionCache` (currently `Vec<Vec<Layer>>`) and
|
|
`obikindex::query_layer::QueryLayer` (currently uncached, bypasses `Layer`
|
|
entirely) onto `KmerPartition`/`Vec<KmerPartition>`; deciding whether that
|
|
collection lives in `obikpartition` or `obikindex`; deciding the
|
|
batch-lookup surface's exact shape (generic payload vs. as-is sibling
|
|
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.
|
|
|
|
## (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
|
|
`mphf.bin` plus (`evidence.bin`/`fingerprint.bin` + `unitigs.bin`), and the
|
|
matrix side mmaps `matrix.pbmx`/`matrix.pcmx` (or one file per genome column
|
|
if not yet packed). Any code path that reopens a layer per lookup instead of
|
|
once per run pays this cost repeatedly.
|
|
|
|
`obikphylo::siblings::cache::PartitionCache` was built to avoid exactly this
|
|
for `build_sibling_annex`/`sibling_annex_stats`: those commands probe many
|
|
partitions, once per source layer, over the whole run. Profiling a real run
|
|
showed wall-clock time dominated by repeated `open()`/mmap syscalls, not
|
|
computation — parallelising the naive per-lookup opens spread the cost
|
|
across cores without reducing it. `PartitionCache::build` opens every
|
|
partition's every layer once, up front, in parallel, and keeps the handles
|
|
alive for the run.
|
|
|
|
## Three independent implementations of the same bundle (historical — (1) fixed this)
|
|
|
|
**As of 2026-08-20 this table describes the pre-(1) state.** `Mat` no
|
|
longer exists (deleted when `obilayeredmap::Layer` replaced it — see "(1)
|
|
done" above); `Layer<D>` in the table below is what's now called
|
|
`TypedLayer<D>`. `QueryLayer` is unaffected and still stands as described —
|
|
still uncached, still not using `Layer` at all — which is exactly what (2)
|
|
needs to fix. Kept for the original motivation, not as current fact:
|
|
|
|
Searching the codebase for "who bundles MPHF + matrix, with per-layer format
|
|
auto-detection" turned up three unrelated implementations:
|
|
|
|
| | lives in | scope | cached? |
|
|
|---|---|---|---|
|
|
| `Layer<D>` (now `TypedLayer<D>`) | `obilayeredmap` | one layer, `D` fixed at compile time | held alive by whoever owns the `Layer`, no policy of its own |
|
|
| `Mat` (now deleted; superseded by `obilayeredmap::Layer`) | `obikphylo::siblings::cache` | one layer, format resolved per instance from an enum of 3 `Layer<D>` variants | yes, via `PartitionCache` |
|
|
| `QueryLayer` (unchanged, still current) | `obikindex::query_layer` (moved crates since this was written — see "Major restructuring") | one layer, `(MphfLayer, PersistentBitMatrix\|PersistentCompactIntMatrix)` pair, bypasses `TypedLayer<D>`/`Layer` entirely | **no** — opened fresh inside `query_partition_with` on every call |
|
|
|
|
`query_partition_with` is `obikmer query`'s normal query path — the one
|
|
most exposed to repeated cross-partition lookups — and it is still the one
|
|
with no cache at all. `obikphylo` built a cache first only because
|
|
sibling-annex construction hits the cost hardest, not because the need is
|
|
sibling-specific.
|
|
|
|
## The gap in `obilayeredmap`'s existing cache (historical — (1) fixed this)
|
|
|
|
`obilayeredmap::LayeredMap<D>` already caches correctly at the granularity
|
|
of one partition: `open(root)` opens every layer once, keeps
|
|
`Vec<TypedLayer<D>>` alive for the `LayeredMap`'s lifetime. But it is
|
|
monomorphic — every layer in the `Vec` must share the same concrete `D`.
|
|
In practice this was false: layers in the same partition are packed
|
|
independently over time (`pack --sparse` converts one layer's presence
|
|
matrix at a time). This motivated (1) — `obilayeredmap::Layer`, done — but
|
|
note the specific `PersistentSparseBitMatrix`-mixing scenario described
|
|
here turned out to be moot: `PersistentBitMatrix` itself absorbed sparse
|
|
storage as a 4th internal variant before (1) was built (see "One bug found
|
|
… one earlier claim retracted" below), so the only heterogeneity `Layer`
|
|
actually needs to represent is `Count` vs. `Presence`, not dense-vs-sparse
|
|
presence. `LayeredMap<D>` itself is unaffected by any of this — it's still
|
|
monomorphic, still not used by `Layer`/`KmerPartition` (which bypass it
|
|
entirely, opening each `TypedLayer<D>` directly, the same way `Mat` did).
|
|
|
|
## Resource cost: mmap does not hold a file descriptor
|
|
|
|
Before deciding how many layers/partitions a cache may hold open
|
|
simultaneously, the binding constraint needs to be identified correctly.
|
|
|
|
Confirmed against upstream documentation, not inferred from behaviour:
|
|
|
|
> "After the mmap() call has returned, the file descriptor, fd, can be
|
|
> closed immediately without invalidating the mapping."
|
|
> — [mmap(2), man7.org](https://man7.org/linux/man-pages/man2/mmap.2.html)
|
|
|
|
> "The close(2) function does not unmap pages"
|
|
> — [mmap(2), Apple Developer](https://developer.apple.com/library/archive/documentation/System/Conceptual/ManPages_iPhoneOS/man2/mmap.2.html)
|
|
|
|
> "A file backed Mmap ... will remain valid even after the File is dropped.
|
|
> ... the Mmap handle is completely independent of the File used to create
|
|
> it."
|
|
> — [memmap2::Mmap, docs.rs](https://docs.rs/memmap2/latest/memmap2/struct.Mmap.html)
|
|
|
|
Every read-only mmap in this codebase already follows this: `Mmap::map(&File::open(path)?)?`
|
|
— the `File` is a temporary, dropped (fd closed) immediately after the
|
|
mapping is established; every persistent struct (`PersistentBitVec`,
|
|
`PersistentCompactIntVec`, `PackedBitMatrix`, `Evidence`, `FingerprintVec`,
|
|
...) stores only the `Mmap`, never the `File`. So a cache built on these
|
|
types does **not** consume the process's open-file-descriptor budget
|
|
(`ulimit -n`, notoriously low by default on macOS) proportionally to how
|
|
many mmapped files it holds.
|
|
|
|
It does consume a different resource — the process's virtual-memory mapping
|
|
table (one entry per active `mmap()` region). Linux exposes this as
|
|
`vm.max_map_count` (default 65530). No documented macOS equivalent (fixed
|
|
numeric ceiling) was found; the constraint there appears to be virtual
|
|
address space rather than an explicit mapping counter, but this is not
|
|
sourced and should not be assumed. This is the resource actually worth
|
|
measuring before deciding on cache size, not fd count — and it is why
|
|
*packing* (`matrix.pbmx`/`matrix.pcmx`, one mmap for all columns) matters
|
|
independently of any caching decision: an unpacked `Columnar` matrix opens
|
|
one mmap **per genome column**, multiplying the mapping count a cache would
|
|
have to hold by `n_genomes`.
|
|
|
|
## Layering: who owns what (superseded — see Definitions above)
|
|
|
|
This section used to argue nobody owned "the collection of partitions."
|
|
That's resolved: `KmerIndex` (`obikindex`) owns it now, directly (see
|
|
"Major restructuring" below). What's still genuinely unowned is *one
|
|
partition's open layers* — `KmerPartition`, in the not-yet-created
|
|
`obikpartition` — see "Definitions" at the top of this file for the
|
|
current, authoritative answer. Left here only so old links/references to
|
|
this heading don't 404; don't read this section for current facts.
|
|
|
|
## Direction agreed, not yet implemented
|
|
|
|
Only (2) remains — (1) shipped as `obilayeredmap::Layer` (see "(1) done"
|
|
above). Concretely, in order:
|
|
|
|
1. Create the `obikpartition` crate (`obikindex → obikpartition →
|
|
obilayeredmap`, no other edges — see "Definitions" above for the exact
|
|
constraint and why).
|
|
2. `KmerPartition { layers: Vec<obilayeredmap::Layer> }` — `open`,
|
|
`n_layers`, `layer(i)`, `find`, plus whatever batch-lookup surface
|
|
`obikphylo::siblings::cache::PartitionCache` currently needs
|
|
(`find_presence_batch`/`find_presence_batch_fast`; `fast_mode` is
|
|
sibling-specific bookkeeping and should probably stay in `obikphylo`,
|
|
wrapping a `KmerPartition`/`Vec<KmerPartition>` rather than living
|
|
inside it — same "generic vs. domain-specific" split `iter_minorants_batch`
|
|
already went through for `Layer` in (1)).
|
|
3. Migrate `obikphylo::siblings::cache::PartitionCache` to hold
|
|
`Vec<KmerPartition>` instead of `Vec<Vec<Layer>>`.
|
|
4. Migrate `obikindex::query_layer::QueryLayer`/`query_partition_with` to
|
|
use `KmerPartition` too, closing the "no cache at all" gap on
|
|
`obikmer`'s normal query path (see "Three independent implementations,"
|
|
historical, above).
|
|
|
|
Open before implementing: exact API shape of `KmerPartition` (propose,
|
|
confirm before coding — non-trivial), and whether the multi-partition
|
|
`Vec<KmerPartition>` needs an eviction policy or can simply hold every
|
|
partition open for the process lifetime (revisit once the
|
|
VM-mapping-count question above has a real number behind it for this
|
|
codebase's scale — still not measured).
|
|
|
|
## Preparatory work done (2026-08-20)
|
|
|
|
Groundwork for (1)/(2), landed ahead of the design itself. **Note**: at
|
|
the time this was written, the collection type these bullets describe was
|
|
named `KmerPartition` (singular) in this doc; it was renamed
|
|
`KmerPartitions` (plural) shortly after, then deleted entirely and merged
|
|
into `KmerIndex` (see "Major restructuring" above). The bullets below are
|
|
edited to say `KmerPartitions` throughout, to not collide with the
|
|
unrelated, brand-new singular `KmerPartition` defined at the top of this
|
|
file — the accessors described here live on `KmerIndex` today, not on
|
|
any type called `KmerPartition`.
|
|
|
|
- `KmerPartitions` (`obikpartitionner`, at the time) gained
|
|
`partition_dir`/`index_dir`/`layer_dir` as the single source of truth
|
|
for a partition's on-disk layout, replacing per-module duplicated
|
|
`const INDEX_SUBDIR: &str = "index"` (7 copies) and ad hoc path joins —
|
|
including one found duplicated *inside the struct itself*
|
|
(`ensure_writer` rebuilt `part_dir`'s own logic by hand).
|
|
- Same struct gained `partition_meta`/`n_layers`/`index_mode`, wrapping
|
|
`obilayeredmap::meta::PartitionMeta::load` (via the existing
|
|
`common::load_meta`, which also recovers indexes built before
|
|
`meta.json` existed). Before this, `obikphylo` and `obikindex` imported
|
|
`obilayeredmap::meta::PartitionMeta` directly and called `::load()`
|
|
themselves at 21 call sites, each redoing its own error-mapping —
|
|
every one of those crates knew the on-disk metadata format instead of
|
|
going through an interface. Fixed everywhere except one remaining spot
|
|
(below). Caught as a side effect: `dump_layer.rs`/`query_layer.rs` had
|
|
been calling `PartitionMeta::load` directly, bypassing `load_meta`
|
|
entirely — they never got the missing-`meta.json` recovery the other
|
|
callers did.
|
|
- Layer introspection API discussed but **not yet implemented** — three
|
|
axes, deliberately kept separate after an initial draft conflated them:
|
|
- `LayerContent { Count, Presence }` — what the layer stores; a `const`
|
|
on `LayerData` (compile-time, zero-cost), not a runtime field.
|
|
- `StorageKind { Implicit, Columnar, Packed, Sparse }` — how it's
|
|
stored; only meaningful for `D` that actually carry data (`Layer<()>`
|
|
has neither this nor `LayerContent` — it's a write-time-only state,
|
|
never a queryable content: once a layer is closed, "no matrix file"
|
|
reads back as `Presence`/`Implicit` via `PersistentBitMatrix::open`'s
|
|
own fallback, not as some third "empty" content).
|
|
- `EvidenceKind { Exact, Approx, Hybrid }` — from `MphfLayer`'s own
|
|
already-in-memory `LayerEvidence` discriminant.
|
|
- Not all `(LayerContent, StorageKind)` pairs are legal: `Count` never
|
|
has `Implicit` or `Sparse`.
|
|
|
|
**Implemented (2026-08-20).** `LayerContent`/`StorageKind`/`EvidenceKind`
|
|
now exist, each with two forms:
|
|
- A runtime accessor on an already-open value (`Layer<D>::content()`/
|
|
`storage_kind()`/`evidence_kind()`, `PersistentBitMatrix::storage_kind()`,
|
|
`PersistentCompactIntMatrix::storage_kind()`, `MphfLayer::evidence_kind()`)
|
|
— reads a discriminant already in memory, zero disk access.
|
|
- A lightweight `detect()`/`detect_storage()` disk probe that mirrors the
|
|
corresponding `open()`'s own priority order by hand (file-existence
|
|
checks only, no mmap) — usable *before* committing to a `D`, unlike the
|
|
runtime accessors. Exposed per-layer on `LayeredMap<D>` as
|
|
`detect_layer_content`/`detect_layer_storage`/`detect_layer_evidence`
|
|
(work regardless of `D`, since they only use `self.root` + the layer
|
|
index).
|
|
|
|
`StorageKind` lives in `obicompactvec` (owner of `PersistentBitMatrix`/
|
|
`PersistentCompactIntMatrix`); `LayerContent`/`EvidenceKind` live in
|
|
`obilayeredmap`. `HasLayerContent`/`HasStorageKind` gate `Layer<()>` out of
|
|
`content()`/`storage_kind()` (no matrix, nothing to report), matching the
|
|
"empty is transitional" conclusion above. 42 new tests across
|
|
`obilayeredmap`'s `tests/layer.rs` and `tests/map.rs`; full workspace
|
|
suite green (0 failed) after.
|
|
|
|
Not done: these `detect()` probes don't yet replace `Mat::open`'s or
|
|
`QueryLayer::open`'s own hand-rolled equivalents (still duplicated content/
|
|
storage decisions, now a *third* copy of the same logic to keep in sync)
|
|
— that consolidation is (1)/(2)'s job, not this prep step's.
|
|
|
|
## One bug found while reading around this (signalled, not fixed); one earlier claim retracted
|
|
|
|
- `obicompactvec::bitmatrix::sparse.rs`'s module doc says
|
|
"Not used by any production code path yet" — false since `obikmer pack
|
|
--sparse` (`cmd/pack/mod.rs`) is wired to `pack_sparse_bit_matrix` and
|
|
`Mat::open` already reads the result back in the sibling-annex path.
|
|
Stale comment, not corrected.
|
|
- **Retracted (2026-08-20)**: an earlier pass through this doc claimed
|
|
`obikpartition::query_layer::QueryLayer::open` had no sparse-format
|
|
detection and would silently corrupt reads on a `pack --sparse`d layer.
|
|
False — `PersistentBitMatrix` (`obicompactvec::bitmatrix::persistent`)
|
|
is a 4-way enum (`Columnar`/`Packed`/`Sparse`/`Implicit`), not 3-way as
|
|
first read; its `open()` already detects `Sparse` via
|
|
`presence/sparse_meta.json`, and every method on the type (`row`,
|
|
`fill_row`, `nonzero_iter`, …) already dispatches all 4 arms.
|
|
`QueryLayer::open`'s `PersistentBitMatrix::open(layer_dir)` call was
|
|
never the bug. Root cause of the false claim: a `grep -n
|
|
"Implicit\|Columnar\|Packed"` used to read the enum definition silently
|
|
skipped the `Sparse(...)` line because it matched none of those three
|
|
words — a self-inflicted blind spot from a filtered read, not a fact
|
|
about the code. Lesson: for a `pub enum` whose variant list matters,
|
|
read the definition unfiltered, don't grep for the variant names you
|
|
expect to find.
|
|
- One real consequence of that same correction, **fixed (2026-08-20)**:
|
|
`obikphylo::siblings::cache::Mat::SparsePresence(Layer<
|
|
PersistentSparseBitMatrix>)` was redundant — `Mat::Presence(Layer<
|
|
PersistentBitMatrix>)` alone already handles sparse layers
|
|
transparently, since `PersistentBitMatrix` absorbs `Sparse` internally.
|
|
Removed the variant, the `presence/is_multi.prsb` probe in `Mat::open`
|
|
(now just opens `Layer::<PersistentBitMatrix>` unconditionally for the
|
|
non-count case — sparse-vs-dense is `PersistentBitMatrix::open`'s own
|
|
concern), and every now-single-armed match in `find_slot`/`index_batch`/
|
|
`iter_minorants_batch`/`n_cols`/`fill_sub_matrix_carries`. Full
|
|
workspace test suite green after, including the 27 `obikphylo::siblings`
|
|
tests that exercise `pack_matrices(true)`/sparse through `Mat`.
|
|
|
|
## Remaining instance of the PartitionMeta-encapsulation problem
|
|
|
|
`obikphylo::siblings::family_scan::scan_layer_families` still re-derives
|
|
`index_dir` from `layer_dir.parent()` and calls `PartitionMeta::load`
|
|
itself, purely to get `.mode` for `obilayeredmap::Layer::open` (was
|
|
`Mat::open`, same gap, survived the `Mat` → `Layer` swap in (1) unchanged).
|
|
Fixing it the way the 21 other call sites were fixed needs more than a 1:1
|
|
swap: `scan_layer_families` only receives a bare `layer_dir: &Path`, not a
|
|
`(partition, part, layer)` triple, and its single upstream source of layer
|
|
paths, `sibling_layer_dirs`, returns a flat `Vec<PathBuf>` with the
|
|
partition/layer indices already discarded. Fixing it properly means either
|
|
having `sibling_layer_dirs` return `(PathBuf, IndexMode)` (or `(part,
|
|
layer)`) pairs, or threading `&KmerIndex` + indices through instead of
|
|
paths (not `&KmerPartition` — that type doesn't exist yet, and once it
|
|
does it still won't know `IndexMode`, which lives on `KmerIndex`/
|
|
`PartitionMeta`) — and touching every one of `scan_layer_families`'s 8
|
|
callers (`distance.rs`, `alignment.rs`, `cardinality.rs`, `entropy.rs` ×2,
|
|
`sankoff_bundle.rs` ×2, `stats.rs`). Left alone this round; worth doing as
|
|
part of the same pass that builds `KmerPartition`, since those callers are
|
|
exactly the sibling-annex consumers it's meant to serve.
|