From 579cfd8752652076cfc98d0fc08f150fbd32b6db Mon Sep 17 00:00:00 2001 From: Eric Coissac Date: Fri, 28 Aug 2026 14:14:22 +0200 Subject: [PATCH] docs: document partition runner deadlock fix and select CLI updates Update documentation to reflect architectural fixes in the NUMA partition runner and implementation changes in the merge and select modules. The partition runner now wraps task execution in `catch_unwind`, emitting panic events that count toward completion thresholds to prevent controller deadlocks, while re-raising the first payload on return. CLI documentation for select has been updated to mandate output directories, introduce default hard-linking with automatic fallback, and document presence aggregation optimizations. Known issues such as self-merge crashes and sparse-packed index gaps are now documented as failing fast due to these improvements. --- .../numa_partition_runner/index.html | 40 ++++- DevDoc/implementation/merge/index.html | 30 ++++ DevDoc/implementation/select/index.html | 144 +++++++++++------- .../architecture/numa_partition_runner.md | 21 ++- DevDocMD/implementation/merge.md | 11 ++ DevDocMD/implementation/select.md | 90 ++++++----- 6 files changed, 243 insertions(+), 93 deletions(-) diff --git a/DevDoc/architecture/numa_partition_runner/index.html b/DevDoc/architecture/numa_partition_runner/index.html index c25fea11..c79dca08 100644 --- a/DevDoc/architecture/numa_partition_runner/index.html +++ b/DevDoc/architecture/numa_partition_runner/index.html @@ -1367,6 +1367,17 @@ + + +
  • + + + + Panic propagation (fixed, 2026-08-28) + + + +
  • @@ -1553,6 +1564,17 @@ +
    • + +
  • + + + + Panic propagation (fixed, 2026-08-28) + + + +
  • @@ -1836,11 +1858,27 @@ dormant workers have been woken and grows every node by the same amount per step, capped by that node's remaining dormant workers and by the run's total budget (n_total) — balance across nodes is now guaranteed by construction, not incidental to channel implementation details.

    +

    Panic propagation (fixed, 2026-08-28)

    +

    A worker whose f(i) panicked used to never send its WorkerEvent::Completed +— the controller's while completed < n_total loop then waited forever for +an event that partition could no longer produce, since other live +workers/timer threads kept event_rx open. Discovered via obikselect +panicking on an unimplemented Sparse matrix case (see implementation/select.md): +the process hung indefinitely instead of erroring out.

    +

    Fixed: each worker wraps f(i) in std::panic::catch_unwind and sends a new +WorkerEvent::Panicked(i, payload) instead of silently dropping the +partition. The controller counts it toward completed (unblocking the loop) +and keeps the first payload seen; once run returns, panic::resume_unwind +re-raises it on the caller's thread — the original message/backtrace still +surfaces, from the right place, instead of a silent deadlock. Takes priority +over a plain Err (a panic means a bug, not a normal typed failure).

    Open questions

    @@ -1598,6 +1609,17 @@ +
    • + +
  • + + + + Known issue (not yet fixed, 2026-08-28) + + + +
    • @@ -1795,6 +1817,14 @@ spectrums/ <label>.json ← one file per genome, rebuilt from all sources index.meta ← complete genome list + evidence kind written at bootstrap +

    mphf.bin/unitigs.bin/evidence.bin/unitigs.bin.idx/fingerprint.bin/layer_meta.json marked "unchanged" above are hard-linked from the base source's own files during the bootstrap copy (2026-08-28), not copied — merge_partition never rewrites them for pre-existing layers, only the presence/counts subdirectory gets widened in place, so only that subdirectory is a real, independent copy. Falls back to a real copy per file if linking itself fails (different filesystems). Verified: source files are byte-identical (checksummed) before/after a normal merge.

    +

    Known issue (not yet fixed, 2026-08-28)

    +

    Merging an index against itself — literally the same directory passed twice as separate source arguments (e.g. obikmer merge -o out IDX IDX --rename-duplicates) — panics deep in the MPHF's rank-select structure (common_traits::select_in_word, assertion failed: rank < self.count_ones()), inside MphfLayer::find called from merge_partition's "is this source kmer already in dst" check against the bootstrap-copied dst_layers. Root cause not identified; ruled out so far:

    + +

    Only reproduces when sources[0] and sources[1] are the exact same on-disk path opened as two independent KmerIndex handles — an artificial scenario (nobody merges an index with itself intentionally), deprioritized rather than investigated further for now. Does terminate cleanly rather than hang, since PartitionRunner's panic propagation fix (see architecture/numa_partition_runner.md) now surfaces it as a normal process panic/exit 101 instead of a silent deadlock.

    diff --git a/DevDoc/implementation/select/index.html b/DevDoc/implementation/select/index.html index 97f4f7c1..c98f84e1 100644 --- a/DevDoc/implementation/select/index.html +++ b/DevDoc/implementation/select/index.html @@ -1119,17 +1119,6 @@ - - -
  • - - - - In-place: keep only group A - - - -
  • @@ -1157,6 +1146,23 @@ + +
    • @@ -1638,17 +1644,6 @@ - - -
  • - - - - In-place: keep only group A - - - -
  • @@ -1676,6 +1671,23 @@ + +
    • @@ -1728,23 +1740,32 @@ are preserved unchanged; only the data matrices are rewritten.

    Synopsis

    obikmer select <input-index>
-        { --output <dir> | --in-place }
+        --output <dir>
         [--group    <name>:<pred>  ...]
         [--group-op <name>:<op>    ...]
         [--aggregate-by <key>          ]
         [--aggregate-op <op>           ]
         [--select   <col1,col2,...>    ]
         [--presence-threshold <N>      ]
+        [--force-copy                  ]

    Output destination

    -

    Exactly one of --output or --in-place must be specified.

    -

    --output <dir> — writes a new index to <dir>. The source index is -unchanged. The MPHF and unitig files are copied; only the data matrices are -rewritten with the new column layout.

    -

    --in-place — rewrites the data matrices of the source index directly. -Removed or replaced columns are lost. The operation writes to temporary files -first, then renames atomically, so an interrupted run leaves the index intact.

    +

    --output <dir> is required — select always writes a new index; there is no +--in-place mode (2026-08-28: never implemented, removed from the design). The +source index is unchanged.

    +

    Each layer's kmer-identity files (mphf.bin/unitigs.bin/evidence.bin/ +unitigs.bin.idx/fingerprint.bin/layer_meta.json) are never rewritten by a +column projection/aggregation, so they are hard-linked into the output rather +than copied — no extra disk for them even on a large index. Falls back to a +real copy automatically if linking fails (different filesystems); --force-copy +forces a real copy always, for an output that must survive independently of the +source on disk (a hard link shares the same inode — rewriting one path outside +select itself would affect the other). Only the presence/counts +subdirectory is ever a genuinely new, independent file.

    +

    To replace an index with a selected version of itself, select to a temporary +directory and swap it in (rm -rf INDEX && mv INDEX.tmp INDEX) — the case +--in-place used to cover.

    Defining output columns

    Named groups — --group

    @@ -1908,9 +1929,6 @@ is > N.

    obikmer select myindex --output out \
   --select "Betula_nana--TROM-V-149986,Betula_nana--AG-P04-25-01"
    -

    In-place: keep only group A

    -
    obikmer select myindex --in-place --group "A:group=A" --select "A"
-

    Compose with filter

    # Step 1: keep only B. nana-specific k-mers
 obikmer filter myindex --output filtered \
@@ -1921,32 +1939,46 @@ obikmer select

    Implementation notes

    -

    select does not rebuild the MPHF. The 256 partitions are processed in parallel -(rayon), each writing its output independently; results require no synchronisation -because every partition owns a distinct set of files.

    -

    For each layer in each partition:

    +

    select does not rebuild the MPHF. Every partition is processed independently +(PartitionRunner), each writing its own output layers; no cross-partition +synchronisation is needed.

    +

    For each layer in each partition (obikselect::select_layer::select_partition):

      -
    1. The slot count n is read by opening the source data matrix.
      2. -
    2. A new data matrix is built with M columns (M = number of output columns).
      3. -
    3. For each slot s in 0..n:
      4. -
    4. old_row = matrix.fill_row(s) — reads the original N-column row without allocating.
      5. -
    5. For each output column j:
        -
      • new_row[j] = aggregate(op, old_row[group_indices]).
        • -
      • Pass-through columns are represented as single-element groups with the - default operator (any for presence, sum for count) — same code path.
        • -
      -
      6. -
    6. The new row is written slot by slot into each column builder.
      7. -
    7. All plain files in the source layer directory (mphf.bin, unitigs.bin, - evidence files, layer_meta.json) are copied verbatim; only the presence/ - or counts/ subdirectory is rewritten.
      8. +
    8. copy_layer_files hard-links the source layer's kmer-identity files + (mphf.bin/unitigs.bin/evidence.bin/unitigs.bin.idx/ + fingerprint.bin/layer_meta.json) into the destination — never a real + copy unless linking fails or --force-copy is given.
      9. +
    9. A new data matrix is built with M columns (M = number of output columns), + under a fresh presence//counts/ subdirectory (never touching the + source's own).
      10. +
    10. Presence source (2026-08-28: batch_presence_counts): one shared pass + over the source bit matrix computes every output group's presence count at + once — row-major native for a Sparse source (for_each_genome_in_row, + which has no column representation to read a col_view from at all — the + reason this replaced the old per-group loop, not just an optimisation of + it), deduplicated column-major (one col_view per distinct referenced + column, not per group) for Columnar/Packed. Every AggOp for a bit + matrix is then a cheap derivation of that one count vector (sum = the + count itself, any/max = count ≥ 1, all/min = count == group + size, none = count == 0) — see + obikselect::select_layer::agg_result_from_count.
      11. +
    11. Count source: unchanged, one col_view-driven pass per output column + via MatrixGroupOps — sum/min/max are genuine per-value reductions + for a count matrix, not derivable from a single presence count the way + they are for a bit matrix.
    12. index.meta is rewritten with the new genome list and updated with_counts.
    -

    --in-place write strategy: new data is written to a temporary sibling -directory (presence_new/ or counts_new/); on success the old directory is -removed and the temporary one is renamed into place. An interrupted run leaves -at most one stale *_new/ directory; the original data is intact until the -rename step.

    +

    Known gap (not yet fixed, 2026-08-28)

    +

    Step 4 above still panics (col_view() not available on Sparse +PersistentCompactIntMatrix) if the source is a count index packed +sparse — batch_presence_counts' row-major treatment was only ported to the +bit-matrix (Presence) case, since that was the one actually blocking a real +benchmark run. select/filter on a sparse-packed count index still hits +this; the fix would follow the same shape (a PersistentSparseCompactIntMatrix +row-major decode, analogous to for_each_genome_in_row), just not done. Since +obisys::numa::runner::PartitionRunner's panic-propagation fix (see +architecture/numa_partition_runner.md), this at least fails fast (process +panic, exit 101) instead of hanging.

    diff --git a/DevDocMD/architecture/numa_partition_runner.md b/DevDocMD/architecture/numa_partition_runner.md index 06b7765c..994e8c76 100644 --- a/DevDocMD/architecture/numa_partition_runner.md +++ b/DevDocMD/architecture/numa_partition_runner.md @@ -304,10 +304,29 @@ step, capped by that node's remaining dormant workers and by the run's total budget (`n_total`) — balance across nodes is now guaranteed by construction, not incidental to channel implementation details. +## Panic propagation (fixed, 2026-08-28) + +A worker whose `f(i)` panicked used to never send its `WorkerEvent::Completed` +— the controller's `while completed < n_total` loop then waited forever for +an event that partition could no longer produce, since other live +workers/timer threads kept `event_rx` open. Discovered via `obikselect` +panicking on an unimplemented `Sparse` matrix case (see `implementation/select.md`): +the process hung indefinitely instead of erroring out. + +Fixed: each worker wraps `f(i)` in `std::panic::catch_unwind` and sends a new +`WorkerEvent::Panicked(i, payload)` instead of silently dropping the +partition. The controller counts it toward `completed` (unblocking the loop) +and keeps the first payload seen; once `run` returns, `panic::resume_unwind` +re-raises it on the caller's thread — the original message/backtrace still +surfaces, from the right place, instead of a silent deadlock. Takes priority +over a plain `Err` (a panic means a bug, not a normal typed failure). + ## Open questions - **Error handling**: `run` currently returns the first error; remaining errors - are dropped. A `Vec` return would give complete diagnostics. + are dropped. A `Vec` return would give complete diagnostics. (Panics are + now caught and re-raised individually — see above — this only concerns + plain `Err` results.) - **`INITIAL_DIVISOR` / `GROWTH_DIVISOR` tuning**: currently `4` and `8` (start at 1/4 of a node's cores, grow by 1/8 per step), chosen to fix an diff --git a/DevDocMD/implementation/merge.md b/DevDocMD/implementation/merge.md index cd273495..b93332d6 100644 --- a/DevDocMD/implementation/merge.md +++ b/DevDocMD/implementation/merge.md @@ -194,3 +194,14 @@ spectrums/