diff --git a/DevDocMD/implementation/partition_layer_cache.md b/DevDocMD/implementation/partition_layer_cache.md index ee42564a..5f16e20a 100644 --- a/DevDocMD/implementation/partition_layer_cache.md +++ b/DevDocMD/implementation/partition_layer_cache.md @@ -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, with_counts: bool, mode: &IndexMode, block_bits: u8) -> Result; 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>(output: P, force: bool) -> OKIResult<()>; + fn create_skeleton>(output: P, meta: &IndexMeta) -> OKIResult; + fn finalize_indexed>(output: P, rep: &mut Reporter) -> OKIResult; + 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 diff --git a/src/obikindex/src/index/builder.rs b/src/obikindex/src/index/builder.rs new file mode 100644 index 00000000..52d271da --- /dev/null +++ b/src/obikindex/src/index/builder.rs @@ -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>(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>(output: P, meta: &IndexMeta) -> OKIResult; + + /// 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>(output: P, rep: &mut Reporter) -> OKIResult; + + /// Current construction state, as reported by sentinel files on disk. + fn state(&self) -> IndexState; +} + +impl IndexBuilder for KmerIndex { + fn clear_output_for_create>(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>(output: P, meta: &IndexMeta) -> OKIResult { + 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>(output: P, rep: &mut Reporter) -> OKIResult { + 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) + } +} diff --git a/src/obikindex/src/index/kmer_index.rs b/src/obikindex/src/index/kmer_index.rs index 8153f034..b27c7837 100644 --- a/src/obikindex/src/index/kmer_index.rs +++ b/src/obikindex/src/index/kmer_index.rs @@ -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>(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>(output: P, meta: &IndexMeta) -> OKIResult { - 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>( - output: P, - rep: &mut Reporter, - ) -> OKIResult { - 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 diff --git a/src/obikindex/src/index/merge.rs b/src/obikindex/src/index/merge.rs index f9e328f1..3c580fba 100644 --- a/src/obikindex/src/index/merge.rs +++ b/src/obikindex/src/index/merge.rs @@ -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}; diff --git a/src/obikindex/src/index/mod.rs b/src/obikindex/src/index/mod.rs index 6803cb4f..97d29f79 100644 --- a/src/obikindex/src/index/mod.rs +++ b/src/obikindex/src/index/mod.rs @@ -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}; diff --git a/src/obikindex/src/index/rebuild.rs b/src/obikindex/src/index/rebuild.rs index ce30f39d..e2ff2df7 100644 --- a/src/obikindex/src/index/rebuild.rs +++ b/src/obikindex/src/index/rebuild.rs @@ -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; diff --git a/src/obikindex/src/index/reindex.rs b/src/obikindex/src/index/reindex.rs index b5b5503c..c06e26a8 100644 --- a/src/obikindex/src/index/reindex.rs +++ b/src/obikindex/src/index/reindex.rs @@ -1,3 +1,4 @@ +use crate::index::builder::IndexBuilder; use crate::layer::{IndexMode, TypedLayer}; use obisys::{Reporter, Stage, progress_bar}; use std::fs; diff --git a/src/obikindex/src/index/select.rs b/src/obikindex/src/index/select.rs index 9e39ad87..7fab0be5 100644 --- a/src/obikindex/src/index/select.rs +++ b/src/obikindex/src/index/select.rs @@ -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; diff --git a/src/obikindex/src/lib.rs b/src/obikindex/src/lib.rs index 7309fdc0..00bc29da 100644 --- a/src/obikindex/src/lib.rs +++ b/src/obikindex/src/lib.rs @@ -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}; diff --git a/src/obikmer/src/cmd/index/mod.rs b/src/obikmer/src/cmd/index/mod.rs index 7203e017..59ad9c4d 100644 --- a/src/obikmer/src/cmd/index/mod.rs +++ b/src/obikmer/src/cmd/index/mod.rs @@ -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> {