Split index lifecycle into IndexBuilder trait for maintenance operations

Extracts directory bookkeeping and construction methods into a new IndexBuilder trait, separating maintenance operations from scientific computation logic. Migrates relevant methods to the new trait, updates module imports across obikindex, and narrows the crate's public API surface. Adds an end-to-end CLI smoke test to verify multi-index merge workflows.
2026-08-21 10:46:57 +02:00
parent da3aa5a2cb
commit 02dbdd11aa
10 changed files with 198 additions and 78 deletions
@@ -56,12 +56,16 @@ 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}`. (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
+layer_builder}`. (8) design agreed, item 1 done in (9) —
+`obikindexer::extensions::PrivateBuilder`, private, six construction-only
+`KmerIndex` methods moved out. (10) done, same session — `obikindex::
+IndexBuilder`, public, the four maintenance methods
+(`clear_output_for_create`/`create_skeleton`/`finalize_indexed`/`state`)
+shared with `merge`/`select`/`rebuild`/`reindex`. Item 2 from (8)
+(`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
@@ -1056,14 +1060,18 @@ trait(s), of two kinds:
 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
+## (9) done (2026-08-21): `obikindexer::extensions::PrivateBuilder` — item 1 above, implemented
+
+Renamed from `IndexBuilder` to `PrivateBuilder` immediately after (same
+session), freeing the name `IndexBuilder` for (10)'s public trait — read
+`IndexBuilder` below as `PrivateBuilder` throughout this section.

 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 {
+pub(crate) trait PrivateBuilder {
    fn mark_scattered(&mut self) -> OKIResult<()>;
    fn mark_counted(&self) -> OKIResult<()>;
    fn mark_indexed(&self) -> OKIResult<()>;
@@ -1071,7 +1079,7 @@ pub(crate) trait IndexBuilder {
    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 { ... }
+impl PrivateBuilder for KmerIndex { ... }
 ```

 All six moved bodily out of `obikindex::index::{kmer_index, index_layer}`
@@ -1105,7 +1113,7 @@ 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`
+rename**: `PrivateBuilder` 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
@@ -1141,12 +1149,80 @@ 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).
+Still not done at the time of writing: 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.
+
+## (10) done (2026-08-21): `obikindex::IndexBuilder` — the public counterpart, same session
+
+Immediate correction to (9): the private trait built there was renamed
+`PrivateBuilder` (freeing the name), and the four methods (9) had left
+inherent on `KmerIndex` — `clear_output_for_create`/`create_skeleton`/
+`finalize_indexed`/`state` — got their own trait after all: **`IndexBuilder`**,
+public, defined in `obikindex` itself (not `obikindexer`):
+
+```rust
+pub trait IndexBuilder: Sized {
+    fn clear_output_for_create<P: AsRef<Path>>(output: P, force: bool) -> OKIResult<()>;
+    fn create_skeleton<P: AsRef<Path>>(output: P, meta: &IndexMeta) -> OKIResult<Self>;
+    fn finalize_indexed<P: AsRef<Path>>(output: P, rep: &mut Reporter) -> OKIResult<Self>;
+    fn state(&self) -> IndexState;
+}
+impl IndexBuilder for KmerIndex { ... }
+```
+
+User's framing: these four are "maintenance", not "scientific computation
+on an index" — a different kind of non-generic-ness than (9)'s six
+(`mark_*`/`write_spectrum`/`build_index_layer`/`remove_build_artifacts`,
+exclusive to the 4-stage pipeline). Maintenance is used more broadly
+(`merge`/`select`/`rebuild`/`reindex`), so it gets a real, public trait —
+not folded back into `KmerIndex`'s inherent surface, and not private
+either.
+
+**Where it lives, and why that's not arbitrary**: (9) needed the orphan
+rule to force its trait into `obikindexer`, to achieve genuine
+crate-private visibility. Here the requirement is the opposite:
+`merge.rs`/`select.rs`/`rebuild.rs`/`reindex.rs` — the trait's own
+heaviest users — live *inside* `obikindex`. A trait they need to reach
+must be local to `obikindex` (or a crate `obikindex` itself depends on,
+which doesn't exist for this). So `IndexBuilder` lives in a new
+`obikindex/src/index/builder.rs`, `pub trait` (no orphan-rule tension at
+all here — both trait and type are local to the same crate), re-exported
+from `obikindex`'s crate root alongside `PrivateBuilder`'s sibling
+`obikindexer::extensions::PrivateBuilder` staying where it is. Two
+traits, two crates, two different reasons, not a contradiction.
+
+**Blast radius, all inside `obikindex` plus one external crate**: every
+internal caller of these four methods needs the trait imported now that
+they're no longer inherent — `merge.rs`, `select.rs`, `rebuild.rs`,
+`reindex.rs` (`use crate::index::builder::IndexBuilder;`) and, externally,
+`obikmer::cmd::index::mod` (`use obikindex::IndexBuilder;`, for the three
+`idx.state() < IndexState::X` resumability checks). Call syntax at every
+site is unchanged (`KmerIndex::create_skeleton(...)`,
+`self.state()`) — only trait-in-scope requirements are new, which is
+exactly the point: same ergonomics, less surface baked into `KmerIndex`
+itself.
+
+Verification went one step further than (9): beyond
+`cargo check --workspace --all-targets` + `cargo test --workspace` +
+`scripts/smoke_test_index.sh` (all green, 870 kmers again), ran
+`obikmer merge` end to end on two freshly built indexes (exercises
+`clear_output_for_create`/`finalize_indexed` directly, the two methods
+`scripts/smoke_test_index.sh` itself never touches) — exit 0, `pack`
+stage completed. Test suite alone would not have caught a regression
+here: no existing test builds two real indexes and merges them through
+the CLI.
+
+`KmerIndex` itself now carries only: identity/config accessors
+(`root_path`/`meta`/`kmer_size`/`n_bits`/`evidence_mode`/`genomes`/...),
+path resolution (`partition_dir`/`index_dir`/`layer_dir`/
+`partition_meta`/`n_layers`), and a few index-maintenance operations not
+yet sorted into either trait (`layer_unitigs_path`, `pack_matrices`,
+`upgrade_layer_meta` — see (8)'s "tested and discarded" list; still
+correctly inherent, not construction-only by the semantic criterion) —
+`create`/`open`/`exists` (identity, can't be trait methods needing `Self`
+before one exists) round that out.

 ## The problem

@@ -0,0 +1,93 @@
+//! Index-maintenance operations: preparing an output directory for a new
+//! index, laying out a fresh skeleton, finalizing after construction, and
+//! reading build progress. Not "scientific computation on an index" (kmer
+//! counting, distance, query — those live elsewhere), but genuine
+//! maintenance/bookkeeping around an index's own construction lifecycle —
+//! used by `merge`/`select`/`rebuild`/`reindex` (all in this crate) and by
+//! `obikindexer`'s pipeline algorithms alike.
+//!
+//! Public, and defined here rather than in `obikindexer`, precisely
+//! because it's used by `merge`/`select`/`rebuild`/`reindex` — those live
+//! *inside* `obikindex` itself, so a trait only they could reach would
+//! have to be local to this crate regardless (`obikindexer → obikindex`
+//! is the only direction this dependency ever runs). Distinct from
+//! `obikindexer::extensions::PrivateBuilder`, which stays `pub(crate)`
+//! there and covers a different set of methods — the ones exclusive to
+//! the four-stage build pipeline, never needed by `merge`/`select`/
+//! `rebuild`/`reindex`. See
+//! `DevDocMD/implementation/partition_layer_cache.md`, "(8)"/"(9)".
+
+use std::fs;
+use std::path::Path;
+
+use obisys::Reporter;
+use obisys::Stage;
+
+use crate::index::error::{OKIError, OKIResult};
+use crate::index::kmer_index::KmerIndex;
+use crate::index::meta::IndexMeta;
+use crate::index::state::{IndexState, SENTINEL_INDEXED};
+
+pub trait IndexBuilder: Sized {
+    /// Make `output` ready to receive a newly created index.
+    ///
+    /// If an index already exists there, remove it when `force` is set,
+    /// otherwise fail. A bare directory with no `index.meta` (e.g. one left
+    /// behind by a `DirLock`) is not considered a pre-existing index.
+    fn clear_output_for_create<P: AsRef<Path>>(output: P, force: bool) -> OKIResult<()>;
+
+    /// Lay out a fresh index skeleton at `output`: the root directory,
+    /// `index.meta` (from `meta`), and an empty partition layout.
+    ///
+    /// For construction paths that build partitions from scratch (`select`,
+    /// `rebuild`). `merge` bootstraps by copying a source index instead, so
+    /// it does not use this.
+    fn create_skeleton<P: AsRef<Path>>(output: P, meta: &IndexMeta) -> OKIResult<Self>;
+
+    /// Mark `output` as fully indexed, pack its column matrices, and reopen it.
+    ///
+    /// Shared tail of the `select`/`rebuild` construction paths, once their
+    /// partitions have been written.
+    fn finalize_indexed<P: AsRef<Path>>(output: P, rep: &mut Reporter) -> OKIResult<Self>;
+
+    /// Current construction state, as reported by sentinel files on disk.
+    fn state(&self) -> IndexState;
+}
+
+impl IndexBuilder for KmerIndex {
+    fn clear_output_for_create<P: AsRef<Path>>(output: P, force: bool) -> OKIResult<()> {
+        let output = output.as_ref();
+        if Self::exists(output) {
+            if force {
+                fs::remove_dir_all(output).map_err(OKIError::Io)?;
+            } else {
+                return Err(OKIError::Io(std::io::Error::new(
+                    std::io::ErrorKind::AlreadyExists,
+                    format!("{}: output directory already exists", output.display()),
+                )));
+            }
+        }
+        Ok(())
+    }
+
+    fn create_skeleton<P: AsRef<Path>>(output: P, meta: &IndexMeta) -> OKIResult<Self> {
+        let output = output.as_ref();
+        fs::create_dir_all(output).map_err(OKIError::Io)?;
+        meta.write(output).map_err(OKIError::Io)?;
+        Ok(KmerIndex { root_path: output.to_owned(), meta: meta.clone() })
+    }
+
+    fn finalize_indexed<P: AsRef<Path>>(output: P, rep: &mut Reporter) -> OKIResult<Self> {
+        let output = output.as_ref();
+        fs::File::create(output.join(SENTINEL_INDEXED)).map_err(OKIError::Io)?;
+        let idx = KmerIndex::open(output)?;
+        let t_pack = Stage::start("pack");
+        idx.pack_matrices(false)?;
+        rep.push(t_pack.stop());
+        Ok(idx)
+    }
+
+    fn state(&self) -> IndexState {
+        IndexState::detect(&self.root_path).unwrap_or(IndexState::Empty)
+    }
+}
@@ -2,7 +2,7 @@ use std::fs;
 use std::path::{Path, PathBuf};

 use crate::layer::meta::PartitionMeta;
-use obisys::{Reporter, Stage, progress_bar};
+use obisys::progress_bar;
 use rayon::prelude::*;

 use obikseq::{set_k, set_m};
@@ -10,7 +10,6 @@ use obikseq::{set_k, set_m};
 use crate::index::common::load_meta;
 use crate::index::error::{OKIError, OKIResult};
 use crate::index::meta::{GenomeInfo, IndexConfig, IndexMeta};
-use crate::index::state::{IndexState, SENTINEL_INDEXED};

 pub struct KmerIndex {
    pub(crate) root_path: PathBuf,
@@ -52,61 +51,6 @@ impl KmerIndex {
        IndexMeta::exists(path.as_ref())
    }

-    /// Make `output` ready to receive a newly created index.
-    ///
-    /// If an index already exists there, remove it when `force` is set,
-    /// otherwise fail. A bare directory with no `index.meta` (e.g. one left
-    /// behind by a `DirLock`) is not considered a pre-existing index.
-    pub fn clear_output_for_create<P: AsRef<Path>>(output: P, force: bool) -> OKIResult<()> {
-        let output = output.as_ref();
-        if Self::exists(output) {
-            if force {
-                fs::remove_dir_all(output).map_err(OKIError::Io)?;
-            } else {
-                return Err(OKIError::Io(std::io::Error::new(
-                    std::io::ErrorKind::AlreadyExists,
-                    format!("{}: output directory already exists", output.display()),
-                )));
-            }
-        }
-        Ok(())
-    }
-
-    /// Lay out a fresh index skeleton at `output`: the root directory,
-    /// `index.meta` (from `meta`), and an empty partition layout.
-    ///
-    /// For construction paths that build partitions from scratch (`select`,
-    /// `rebuild`). `merge` bootstraps by copying a source index instead, so
-    /// it does not use this.
-    pub(crate) fn create_skeleton<P: AsRef<Path>>(output: P, meta: &IndexMeta) -> OKIResult<KmerIndex> {
-        let output = output.as_ref();
-        fs::create_dir_all(output).map_err(OKIError::Io)?;
-        meta.write(output).map_err(OKIError::Io)?;
-        Ok(KmerIndex { root_path: output.to_owned(), meta: meta.clone() })
-    }
-
-    /// Mark `output` as fully indexed, pack its column matrices, and reopen it.
-    ///
-    /// Shared tail of the `select`/`rebuild` construction paths, once their
-    /// partitions have been written.
-    pub(crate) fn finalize_indexed<P: AsRef<Path>>(
-        output: P,
-        rep: &mut Reporter,
-    ) -> OKIResult<Self> {
-        let output = output.as_ref();
-        fs::File::create(output.join(SENTINEL_INDEXED)).map_err(OKIError::Io)?;
-        let idx = KmerIndex::open(output)?;
-        let t_pack = Stage::start("pack");
-        idx.pack_matrices(false)?;
-        rep.push(t_pack.stop());
-        Ok(idx)
-    }
-
-    /// Current construction state, as reported by sentinel files on disk.
-    pub fn state(&self) -> IndexState {
-        IndexState::detect(&self.root_path).unwrap_or(IndexState::Empty)
-    }
-
    /// The index's root directory.
    pub fn root_path(&self) -> &Path {
        &self.root_path
@@ -3,6 +3,8 @@ use std::fs;
 use std::io;
 use std::path::Path;

+use crate::index::builder::IndexBuilder;
+
 use obisys::{Reporter, Stage, progress_bar, spinner};
 use tracing::{debug, info};

@@ -2,6 +2,7 @@ pub mod error;
 pub mod meta;
 pub mod predicate;
 pub mod state;
+mod builder;
 mod common;
 mod distance;
 mod dump;
@@ -22,6 +23,7 @@ mod select_layer;
 mod stats;

 pub use error::{OKIError, OKIResult};
+pub use builder::IndexBuilder;
 pub use common::olm_to_sk;
 pub use graph_pipeline::{materialize_layer, write_graph_as_unitigs};
 pub use distance::{DistanceMetric, DistanceOutput};
@@ -1,5 +1,6 @@
 use std::path::Path;

+use crate::index::builder::IndexBuilder;
 use crate::index::{KmerFilter, MergeMode};
 use obisys::{Reporter, Stage, progress_bar};
 use tracing::info;
@@ -1,3 +1,4 @@
+use crate::index::builder::IndexBuilder;
 use crate::layer::{IndexMode, TypedLayer};
 use obisys::{Reporter, Stage, progress_bar};
 use std::fs;
@@ -1,5 +1,6 @@
 use std::path::Path;

+use crate::index::builder::IndexBuilder;
 use crate::index::OutputCol;
 use obisys::{Reporter, Stage, progress_bar};
 use tracing::info;
@@ -14,9 +14,9 @@ pub mod partition;

 pub use index::{
    validate_label, AggOp, DistanceMetric, DistanceOutput, GenomeInfo, GroupFilterParams,
-    GroupQuorumFilter, IndexBitsPerKmer, IndexConfig, IndexMeta, IndexState, KmerDesc, KmerFilter,
-    KmerIndex, MergeMode, MetaPred, OKIError, OKIResult, OutputCol, PartitionRunner, QueryHit,
-    QueryStats, META_FILENAME, SENTINEL_COUNTED, SENTINEL_INDEXED, SENTINEL_SCATTERED,
-    materialize_layer, olm_to_sk, write_graph_as_unitigs, passes_all,
+    GroupQuorumFilter, IndexBitsPerKmer, IndexBuilder, IndexConfig, IndexMeta, IndexState,
+    KmerDesc, KmerFilter, KmerIndex, MergeMode, MetaPred, OKIError, OKIResult, OutputCol,
+    PartitionRunner, QueryHit, QueryStats, META_FILENAME, SENTINEL_COUNTED, SENTINEL_INDEXED,
+    SENTINEL_SCATTERED, materialize_layer, olm_to_sk, write_graph_as_unitigs, passes_all,
 };
 pub use index::{filter, meta};
@@ -6,7 +6,7 @@ use obikindexer::algorithms::counter::Counter;
 use obikindexer::algorithms::dereplicator::Dereplicator;
 use obikindexer::algorithms::layer_builder::LayerBuilder;
 use obikindexer::algorithms::partitionner::PartitionRouter;
-use obikindex::{validate_label, GenomeInfo, IndexConfig, IndexState, KmerIndex};
+use obikindex::{validate_label, GenomeInfo, IndexBuilder, IndexConfig, IndexState, KmerIndex};
 use obikindex::layer::IndexMode;

 fn parse_key_value(s: &str) -> Result<(String, String), String> {