refactor: extract index construction state tracking into extension trait
Moves pipeline state bookkeeping, including sentinel file marking and spectrum persistence, into the algorithms' run and close methods. Introduces a crate-private extension trait to satisfy Rust's orphan rule while implementing construction-only operations on KmerIndex. Updates helper function visibility for cross-crate access and removes the now-empty index_layer module.
This commit is contained in:
@@ -56,8 +56,12 @@ algorithm, extracted from `PartitionRouter` the same way `Dereplicator`
|
||||
was in (4). (7) done — `LayerBuilder`, the fourth and last pipeline
|
||||
algorithm; the indexing pipeline is now fully decomposed into
|
||||
`obikindexer::algorithms::{partitionner, dereplicator, counter,
|
||||
layer_builder}`. (5) itself still not implemented, still first on the
|
||||
"order of remaining work" list — see "(7) done" below. Earlier mix-up, for
|
||||
layer_builder}`. (8) design agreed, item 1 (`obikindexer::extensions::
|
||||
IndexBuilder`) done in (9) — private extension trait, six construction-only
|
||||
`KmerIndex` methods moved out; item 2 (`obikalgorithm::Algorithm`) still
|
||||
not started. Note: `Layer` renamed `KmerLayer` (2026-08-21, outside this
|
||||
conversation). (5) itself still not implemented, still first on the
|
||||
"order of remaining work" list. 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
|
||||
@@ -952,6 +956,198 @@ variants observed, worth revisiting whether a single trait can express
|
||||
all three or whether that's itself the answer: it can't, and the trait
|
||||
should not force it).
|
||||
|
||||
## (8) design agreed, not yet implemented (2026-08-21): `obikalgorithm::Algorithm` trait + `obikindexer::extensions` — private/public extension-trait split, `KmerLayer` rename
|
||||
|
||||
Session note: `Layer` was renamed `KmerLayer` (user, outside this
|
||||
conversation, alongside other naming homogenisation with `KmerIndex`/
|
||||
`KmerPartition`) — every reference to `Layer` in this doc from before
|
||||
2026-08-21 means today's `obikindex::layer::KmerLayer`.
|
||||
|
||||
### Why this came up
|
||||
|
||||
Verifying "does `cmd/index` now rest entirely on the algorithm structs"
|
||||
(it doesn't quite — see below) led to sorting `KmerIndex`'s own methods by
|
||||
a criterion the user was explicit is **semantic, not mechanical**: "les
|
||||
méthodes qui, sémantiquement, n'ont pas d'intérêt hors de la construction
|
||||
de l'index" (methods that have no semantic interest outside index
|
||||
construction) — not "methods only called from `cmd/index` today," which
|
||||
a grep could answer but would miss methods construction-adjacent code
|
||||
elsewhere (`merge`/`select`/`rebuild`/`reindex`) also depends on for the
|
||||
same reason.
|
||||
|
||||
**Checked, not assumed** (grepped every call site before classifying):
|
||||
|
||||
- **Construction-only, real candidates for a private extension trait**:
|
||||
`KmerIndex::{mark_scattered, mark_counted, mark_indexed, write_spectrum,
|
||||
build_index_layer, remove_build_artifacts, clear_output_for_create,
|
||||
create_skeleton, finalize_indexed, state}`. The last four are called
|
||||
from `merge.rs`/`select.rs`/`rebuild.rs`/`reindex.rs` too (as
|
||||
precondition checks — "is my source `Indexed`?" — or shared
|
||||
skeleton/finalize machinery), not just from the 4-stage pipeline — so
|
||||
this extension trait's scope is "construction of any kind," not
|
||||
narrowly "the initial build pipeline."
|
||||
- **Looked construction-only by name, checked, and kept on `KmerIndex`**:
|
||||
`layer_unitigs_path` (unitigs are the only way to recover a built
|
||||
index's kmer sequences — read by `rebuild_layer.rs` and others, well
|
||||
beyond construction — see [[project_unitigs_always_kept]]),
|
||||
`pack_matrices` (re-runnable maintenance on an already-finished index
|
||||
via `obikmer pack`, not just a pipeline step), `upgrade_layer_meta`
|
||||
(migration, runnable on any existing index at any time).
|
||||
|
||||
### The general pattern (not obikindexer-specific)
|
||||
|
||||
`KmerIndex`/`KmerPartition`/`KmerLayer` stay generic, in `obikindex` —
|
||||
every domain-specific consumer crate gets to attach its own extension
|
||||
trait(s), of two kinds:
|
||||
|
||||
- **Private** (`pub(crate)`, invisible outside the defining crate) — for
|
||||
plumbing only that crate's own algorithms need. `obikindexer` gets
|
||||
exactly one of these (see below); no public counterpart makes sense for
|
||||
it — "l'index est tellement central que le second trait n'a pas
|
||||
vraiment d'intérêt" for construction specifically: nothing external
|
||||
should ever want to call `mark_scattered` or `build_index_layer`.
|
||||
- **Public** — for a genuinely reusable domain extension. The user's own
|
||||
example, found while discussing this, not hypothetical: `obikindex/src/
|
||||
index/distance.rs` (phylogenetic distance metrics) is currently an
|
||||
`impl KmerIndex` block **inside `obikindex` itself** — under this
|
||||
principle it should be a public extension trait owned by `obikphylo`
|
||||
instead (distance metrics are a phylo concept, `obikindex` has no more
|
||||
business defining them than `obikindex::layer` has defining
|
||||
"family"/"minorant", the reasoning `SiblingLayerExt` already followed
|
||||
for `KmerLayer` — see `obikphylo/src/siblings/iter.rs`). **Explicitly
|
||||
deferred** — noted here so it isn't lost, not part of this round.
|
||||
- The future cache-manager crate (still blocked on (5), see above) will
|
||||
add its own **public** extension trait mirroring part of `KmerIndex`'s/
|
||||
`KmerPartition`'s own read API in cached form (e.g. a cached
|
||||
`.partition(i)` that doesn't re-touch disk) — same pattern, third data
|
||||
point once built.
|
||||
|
||||
### Concretely, next to implement (two items, in order)
|
||||
|
||||
1. **`obikindexer::extensions`** — a private (`pub(crate)`) extension
|
||||
trait, most likely named something like `IndexBuildExt` (final name
|
||||
not yet chosen), implemented for `KmerIndex`, carrying the ten methods
|
||||
listed above, moved out of `obikindex::index::{kmer_index,
|
||||
index_layer}`. Every algorithm in `obikindexer::algorithms::*` that
|
||||
currently calls `idx.mark_scattered()`/etc. keeps the same call syntax
|
||||
(extension trait methods are called the same way as inherent ones,
|
||||
just need the trait in scope) — `cmd/index/mod.rs` itself would need
|
||||
`use obikindexer::extensions::IndexBuildExt;` (or the module re-exports
|
||||
it) to keep compiling, since it's the one place outside `obikindexer`'s
|
||||
own algorithms that currently calls `mark_scattered`/`write_spectrum`/
|
||||
`mark_counted`/`mark_indexed` directly. **Not yet decided**: exact
|
||||
trait name, whether it's one trait or split further (e.g. sentinel
|
||||
marking vs. skeleton/finalize machinery), and whether `merge`/`select`/
|
||||
`rebuild`/`reindex` (not yet extracted into algorithms themselves) move
|
||||
onto it now too or keep calling the soon-to-be-inherent-no-longer
|
||||
methods some other way in the meantime — **ask before implementing**,
|
||||
this changes the blast radius significantly (4 more `obikindex`
|
||||
internal files depend on `clear_output_for_create`/`create_skeleton`/
|
||||
`finalize_indexed`/`state`).
|
||||
2. **`obikalgorithm::Algorithm` trait** — new crate, the shared trait
|
||||
`obikpartitionner`→`obikindexer` merge (session start of 2026-08-21)
|
||||
and (6)/(7) were deliberately building toward, now with four real
|
||||
`new`/(setters)/`run` examples and three distinct callback-bound
|
||||
shapes to reconcile (plain `FnMut` for `PartitionRouter`, `FnMut +
|
||||
Send` for `LayerBuilder`, `Fn + Sync` for `Dereplicator`/`Counter` —
|
||||
see (7)). Exact shape not yet drafted in this doc — do that as its own
|
||||
design pass before coding, same discipline as everything above.
|
||||
|
||||
Both items: **design only, nothing implemented yet** — this section is
|
||||
the record to resume from, not a plan already executed.
|
||||
|
||||
## (9) done (2026-08-21): `obikindexer::extensions::IndexBuilder` — item 1 above, implemented
|
||||
|
||||
Scoped down from (8)'s six-method list to the concrete set that's
|
||||
genuinely movable without further ripple — checked, not assumed, before
|
||||
writing anything:
|
||||
|
||||
```rust
|
||||
pub(crate) trait IndexBuilder {
|
||||
fn mark_scattered(&mut self) -> OKIResult<()>;
|
||||
fn mark_counted(&self) -> OKIResult<()>;
|
||||
fn mark_indexed(&self) -> OKIResult<()>;
|
||||
fn write_spectrum(&self, f0: u64, f1: u64, counts: &BTreeMap<u32, u64>) -> OKIResult<()>;
|
||||
fn build_index_layer(&self, i: usize, min_ab: u32, max_ab: Option<u32>, with_counts: bool, mode: &IndexMode, block_bits: u8) -> Result<usize, SKError>;
|
||||
fn remove_build_artifacts(&self, i: usize);
|
||||
}
|
||||
impl IndexBuilder for KmerIndex { ... }
|
||||
```
|
||||
|
||||
All six moved bodily out of `obikindex::index::{kmer_index, index_layer}`
|
||||
into `obikindexer::extensions` (new module, `pub(crate)`) — `index_layer.rs`
|
||||
is now empty and deleted outright.
|
||||
`clear_output_for_create`/`create_skeleton`/`finalize_indexed`/`state`
|
||||
stayed inherent on `KmerIndex`, per (8)'s reasoning: `merge`/`select`/
|
||||
`rebuild`/`reindex` — living *inside* `obikindex` itself — call them too,
|
||||
and `obikindex` can never depend on `obikindexer` to reach a trait defined
|
||||
there. Moving those four is real future work (extract
|
||||
merge/select/rebuild/reindex into algorithms first), not part of this
|
||||
step.
|
||||
|
||||
**One new, small, deliberate API widening in `obikindex`**: `build_index_layer`
|
||||
depends on three helpers that were `pub(crate)` to `obikindex`
|
||||
(`graph_pipeline::{write_graph_as_unitigs, materialize_layer}`,
|
||||
`common::olm_to_sk`) — widened to `pub` (re-exported from `obikindex`'s
|
||||
crate root) so `obikindexer` could reach them. This is exactly the
|
||||
"enrich shared/lower-level APIs instead of ad hoc local code" call the
|
||||
project's own rules ask for, made explicitly rather than routed around:
|
||||
three functions, already generically written (no rewrite needed), now
|
||||
serve a second caller instead of being duplicated.
|
||||
|
||||
**Why the trait had to be defined in `obikindexer`, not `obikindex`**:
|
||||
Rust's orphan rule — implementing a trait for a foreign type requires
|
||||
either the trait or the type to be local to the current crate. `KmerIndex`
|
||||
is foreign to `obikindexer`, so the trait must be the local half; if it
|
||||
were defined in `obikindex` instead, `pub(crate)` there would make it
|
||||
invisible to `obikindexer` too (crate-private means private to *that*
|
||||
crate, not "private except to one named dependent") — the opposite of
|
||||
what was wanted.
|
||||
|
||||
**A real design decision made while wiring callers up, not a mechanical
|
||||
rename**: `IndexBuilder` being genuinely `pub(crate)` to `obikindexer`
|
||||
means `obikmer::cmd::index` (a different crate) can no longer call
|
||||
`mark_scattered`/`mark_counted`/`mark_indexed`/`write_spectrum` directly —
|
||||
it never could have, once privacy was real rather than aspirational. Each
|
||||
algorithm now marks its own completion as part of `run()`/`close()`
|
||||
instead of leaving it to the caller:
|
||||
- `PartitionRouter::close()` (not `run()`) calls `mark_scattered()` —
|
||||
`close()`, not `run()`, is the actual shared completion point between
|
||||
the file-driven `run()` path and the manual `write`/`write_batch`+
|
||||
`close()` path low-level callers (tests) use; putting it in `run()`
|
||||
alone would have silently skipped marking for every caller that never
|
||||
calls `run()`. `run()` already calls `self.close()` at its own end, so
|
||||
this covers both paths through one line, not two.
|
||||
- `Counter::run` calls `write_spectrum` then `mark_counted` before
|
||||
returning.
|
||||
- `LayerBuilder::run` calls `mark_indexed` before returning.
|
||||
|
||||
`cmd/index/mod.rs` lost all four direct calls (`mark_scattered`/
|
||||
`write_spectrum`/`mark_counted`/`mark_indexed`) — each stage's `if
|
||||
idx.state() < IndexState::X { ... }` block is now purely "run the
|
||||
algorithm," no separate bookkeeping call after it. Confirms, precisely
|
||||
this time (checked by re-reading the whole file, not assumed): `cmd/index`
|
||||
now rests on the four algorithms for every read/write of pipeline state
|
||||
except `KmerIndex::{exists, create, state, n_partitions}`, which are
|
||||
genuinely index-identity concerns, not construction bookkeeping — the
|
||||
original question this whole design pass started from.
|
||||
|
||||
Same fix applied to `obikphylo`'s test harness (its four explicit
|
||||
`mark_*`/`write_spectrum` calls removed, relying on the algorithms now
|
||||
doing it themselves) — `obikindexer::algorithms::partitionner`'s own
|
||||
`pipeline_counts` test needed no change (never called `mark_*` directly).
|
||||
|
||||
Full workspace suite green (`cargo check --workspace --all-targets` +
|
||||
`cargo test --workspace`, exit code 0), plus the CLI smoke test — 870
|
||||
kmers, same as (6)/(7).
|
||||
|
||||
Still not done: item 2 from (8) (`obikalgorithm::Algorithm`), (5), the
|
||||
future cache crate, the `distance.rs` → `obikphylo` relocation (noted in
|
||||
(8), explicitly deferred), and extracting
|
||||
`merge`/`select`/`rebuild`/`reindex` into algorithms (which would unblock
|
||||
moving `clear_output_for_create`/`create_skeleton`/`finalize_indexed`/
|
||||
`state` the same way).
|
||||
|
||||
## The problem
|
||||
|
||||
Reading a layer's data (MPHF + matrix) is not free: `MphfLayer::open` mmaps
|
||||
|
||||
Reference in New Issue
Block a user