Skip to content

Add sativaepang/* modules: reference, lootasks, looplace, looscore - #12910

Open
erikrikarddaniel wants to merge 7 commits into
nf-core:masterfrom
erikrikarddaniel:sativaepang-modules
Open

Add sativaepang/* modules: reference, lootasks, looplace, looscore#12910
erikrikarddaniel wants to merge 7 commits into
nf-core:masterfrom
erikrikarddaniel:sativaepang-modules

Conversation

@erikrikarddaniel

Copy link
Copy Markdown
Member

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the module conventions in the contribution docs
  • If necessary, include test data in your PR.
  • Remove all TODO statements.
  • Broadcast software version numbers to topic: versions - See version_topics
  • Follow the naming conventions.
  • Follow the parameters requirements.
  • Follow the input/output options guidelines.
  • Add a resource label
  • Use BioConda and BioContainers if possible to fulfil software requirements.
  • Ensure that the test works with either Docker / Singularity. Conda CI tests can be quite flaky:
    • For modules:
      • nf-core modules test <MODULE> --profile docker
      • nf-core modules test <MODULE> --profile singularity
      • nf-core modules test <MODULE> --profile conda

Description

Adds four new modules exposing sativa-epang's staged
leave-one-out CLI, one nf-core module per natural stage, replacing what would otherwise be a single
monolithic-CLI wrapper:

  • sativaepang/reference — builds a SATIVA reference (refjson + model) from an alignment and
    taxonomy, optionally handing off a tree built elsewhere (e.g. RAxML-NG, see
    raxmlng/search) via -reftree/-refmodel
    instead of running sativa-epang's own constrained RAxML search.
  • sativaepang/lootasks — deals the refjson into leave-one-out folds via -stage loo-tasks, one
    self-contained task directory per fold plus a manifest.json.
  • sativaepang/looplace — places every leave-one-out fold with EPA-ng via -stage loo-place,
    writing a jplace (plus logs) into each fold directory.
  • sativaepang/looscore — reads the placed folds and reports mislabelled sequences via
    -stage loo-score, completing the chain.

These four modules will back a fasta_tax_detectmislabels_sativaepang subworkflow (opened
separately once this and raxmlng/search have landed) used by
nf-core/taxmarker.

Notable implementation details

  • lootasks works around a real sativa-epang bug: -stage loo-tasks -r (a refjson built from
    an externally-supplied -reftree/-refmodel, as ours always is) leaves no RAxML_info file for
    the tool to recover its model from, and embeds the .model file's raw content — including
    RAxML-NG's own trailing partition clause (, name = range) — verbatim into manifest.json.
    loo-place then rejects every fold ("Wrong model specification"), since inline-mode EPA-ng
    doesn't accept that clause the way file-mode does at the reference stage. Worked around by
    exporting SATIVA_EPANG_MODEL ourselves, trimmed to the bare model expression. Reported
    upstream; not yet fixed as of this PR.
  • looplace stages a private, writable copy of its input taskdir before running loo-place,
    rather than mutating the staged directory from lootasks in place — the same class of
    resume-breaking bug fixed in transdecoder/predict cannot be resumed: it writes checkpoints into its staged input, then fails on re-execution #12799 (TRANSDECODER_PREDICT). Verified by hand: staging a
    read-only unplaced taskdir through the same mv+mkdir+symlink script confirmed the original
    directory gained zero new files.
  • reference's refjson output embeds its own build invocation (working-directory-relative
    paths and all), so it isn't byte-reproducible across engines — masked via sanitizeOutput's
    unstableKeys, with explicit JSON-structure assertions covering what the module actually
    guarantees instead.
  • sativa-epang has no --version flag and its own internal version string is stale (inherited
    from upstream SATIVA); the installed conda package version is read back out of the share/
    path baked into the sativa-epang wrapper script at build time.

Test data

Both tiers now live in nf-core/test-datasets#2266 (merged): the tool's own bundled 38-sequence
tiny example, and a curated 121-sequence GTDB archaeal 16S dataset (which carries a deliberate
swapped-label pair used as a real detection assertion in looscore's tests, not just
process.success).

🤖 Generated with Claude Code

https://claude.ai/code/session_014eSqkuYZXfNiWy87y3srup

erikrikarddaniel and others added 5 commits September 9, 2026 15:32
Builds a SATIVA reference (refjson + model) from an alignment and
taxonomy, optionally handing off a tree built elsewhere (e.g.
RAxML-NG) via -reftree/-refmodel instead of running sativa-epang's own
constrained RAxML search. First of four sativaepang/* modules exposing
sativa-epang's staged leave-one-out CLI as separate nf-core modules.

Tested against two tiers on both Docker and Singularity: the tool's
own bundled 38-sequence example, and a curated 121-sequence GTDB
archaeal 16S dataset. refjson embeds its own build invocation
(working-directory-relative paths and all) so it isn't
byte-reproducible across engines; masked via sanitizeOutput's
unstableKeys, with explicit JSON-structure assertions covering what
the module actually guarantees instead.

sativa-epang has no --version flag and its own internal version
string is stale (inherited from upstream SATIVA); the installed conda
package version is instead read back out of the share/ path baked
into the sativa-epang wrapper script at build time.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014eSqkuYZXfNiWy87y3srup
Deals a sativaepang refjson into leave-one-out folds via -stage
loo-tasks, one self-contained task directory per fold plus a
manifest.json, ready for independent EPA-ng placement.

Works around a real sativa-epang bug: -stage loo-tasks with -r (a
refjson built from an externally-supplied -reftree/-refmodel, as ours
always is) leaves no RAxML_info file for the tool to recover its model
from, and embeds the .model file's raw content -- including RAxML-NG's
own trailing partition clause (", name = range") -- verbatim into
manifest.json. loo-place then rejects every fold ("Wrong model
specification"), since inline-mode EPA-ng doesn't accept that clause
the way file-mode does at the reference stage. Worked around by
exporting SATIVA_EPANG_MODEL ourselves, trimmed to the bare model
expression. Reported upstream; not yet fixed as of this commit.

Tested against both tiers on Docker and Singularity.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014eSqkuYZXfNiWy87y3srup
Places every leave-one-out fold with EPA-ng via -stage loo-place,
writing a jplace (plus logs) into each fold directory.

Unlike a typical module, this one takes a whole directory and mutates
it -- but naively writing into the staged taskdir would risk mutating
sativaepang/lootasks's own output in place (a symlink under local
staging, or a real copy under stageInMode 'copy'), the same class of
-resume-breaking bug fixed in nf-core#12799 (TRANSDECODER_PREDICT).
Fixed the same way: stage a private, writable directory tree (real
new directories for each fold, symlinked read-only contents) before
running loo-place, so the upstream task's output is never touched.

Verified this actually holds, not just assumed: manually staged a
read-only unplaced taskdir through the exact mv+mkdir+symlink script
this module runs and confirmed the original directory gained zero new
files while the restaged local copy received real jplace/log files.

Tested against both tiers on Docker and Singularity -- unlike
sativaepang/reference's refjson, the jplace outputs here are
byte-reproducible across engines, no unstableKeys masking needed.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014eSqkuYZXfNiWy87y3srup
Reads the placed leave-one-out folds and reports mislabelled sequences
via -stage loo-score, completing the four-module chain
(reference -> lootasks -> looplace -> looscore) that replaces
sativa-epang's monolithic CLI with one nf-core module per natural
stage.

loo-score only ever reads taskdir (jplace files, manifest.json) and
writes its own output elsewhere, so unlike looplace this one takes
taskdir as a plain read-only input -- confirmed by hand before relying
on it (ran loo-score against a taskdir and diffed its contents
before/after).

Tested against both tiers on Docker and Singularity. The gtdb test is
a real detection assertion, not just process.success: that dataset
carries a deliberate swapped-label pair (DupHaloA/DupSulfoA), and both
are asserted present in the .mis output at the confidence the full
chain actually produces.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014eSqkuYZXfNiWy87y3srup
CI's conda job failed: EPA-ng's placement likelihoods aren't
byte-reproducible between the conda environment and the container
builds, even though the underlying inputs and logs otherwise match.
epa_info.log/epang.log also carry a wall-clock "Elapsed Time: Ns" line
that flips between runs under load on any engine, causing the same
class of flake on Docker/Singularity too.

Mask all three (epa_result.jplace, epa_info.log, epang.log) via
sanitizeOutput's unstablePatterns, matching the existing
sigprofiler/snapaligner precedent in this repo. The structural asserts
already in place (every fold directory contains a jplace) remain the
actual correctness guarantee.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014eSqkuYZXfNiWy87y3srup
@erikrikarddaniel

Copy link
Copy Markdown
Member Author

The conda CI job failed: EPA-ng's placement likelihoods aren't byte-reproducible between the conda environment and the container builds (identical inputs/logs, differing epa_result.jplace content). epa_info.log/epang.log also carry a wall-clock "Elapsed Time: Ns" line that can flip between runs under load, on any engine.

Pushed df09b4f masking all three via sanitizeOutput's unstablePatterns (same precedent as sigprofiler/snapaligner elsewhere in this repo). The structural asserts already in the tests (every fold directory contains a jplace) remain the real correctness guarantee. Verified stable across repeated runs on Docker, Singularity, and conda.

🤖 Generated with Claude Code

@erikrikarddaniel
erikrikarddaniel marked this pull request as ready for review September 9, 2026 15:05

@SPPearce SPPearce left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you have a pass to trim out a bunch of the LLM comments, both in the modules and the tests.
Any # comment you put into the script block will be present in the .command.sh I think.

Can you also use sanitizeOutput basically everywhere in the nf-test files.

Comment thread modules/nf-core/sativaepang/looplace/main.nf Outdated
Comment on lines +23 to +45
# \$taskdir is staged from sativaepang/lootasks's own output -- a symlink under
# local/shared-filesystem staging, but a real copy under stageInMode 'copy' (the
# default for cloud storage without Fusion). loo-place writes a jplace (and logs)
# inside every fold directory, which would mutate that other task's output in place
# and make this task uncacheable across -resume (same class of bug as
# nf-core/modules#12799). Give it a private, writable directory tree instead: fold
# subdirectories are real (new) directories here, their read-only contents
# (ref.nwk/ref.fasta/query.fasta) stay symlinks to avoid copying large alignments,
# and manifest.json at the top level is a plain symlink since loo-place never writes
# there. mv handles both staging modes: it moves a symlink as a symlink, and renames
# a real directory in place, so nothing is deleted either way.
mv "$taskdir" "${taskdir}.staged"
real_taskdir=\$(readlink -f "${taskdir}.staged")
mkdir "$taskdir"
find "\$real_taskdir" -mindepth 1 -maxdepth 1 | while read -r entry; do
name=\$(basename "\$entry")
if [ -d "\$entry" ]; then
mkdir "$taskdir/\$name"
find "\$entry" -mindepth 1 -maxdepth 1 -exec ln -s {} "$taskdir/\$name"/ \\;
else
ln -s "\$entry" "$taskdir/\$name"
fi
done

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a lot of code, is it really required?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some form of private writable staging is required: sativa-epang writes results into every fold directory, and taskdir can be staged as a plain symlink straight into lootasks' own task directory (default local-executor staging) — writing into it in place would mutate that other task's output and break -resume the same way #12799 did.

You're right that the generic version was overkill though: it was walking an arbitrary file/directory tree, but lootasks' output schema is actually fixed (manifest.json + fold_*/{ref.nwk,ref.fasta,query.fasta}, confirmed against a real multi-fold run — nothing else ever appears). Pushed in adf8590: hardcoded to that shape instead, about a third of the size.

# and manifest.json at the top level is a plain symlink since loo-place never writes
# there. mv handles both staging modes: it moves a symlink as a symlink, and renames
# a real directory in place, so nothing is deleted either way.
mv "$taskdir" "${taskdir}.staged"

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Surely just stage the original with this name instead.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same reasoning as in my reply above — happy to take another look if you still see room to trim further once you've seen the shrunk version.

- Simplify the version-extraction eval line in all four modules (one
  grep call instead of a sed one-liner), same output verified.
- Shrink looplace's private-writable-taskdir staging: the schema is
  fixed (manifest.json + fold_*/{ref.nwk,ref.fasta,query.fasta}, never
  anything else -- confirmed against a real 25-fold run), so the
  generic find-based file/dir walk isn't needed. Kept the symlink
  approach over stageInMode 'copy': lootasks already writes each
  fold's ref.fasta as a real per-fold copy, so a second full copy here
  would double an already GTDB-scale-relevant cost.
- Trim comments that were sitting inside script blocks (leaking into
  .command.sh) or otherwise over-explaining, in both modules and tests.
- Add sanitizeOutput to looscore's two non-stub tests, matching the
  other three modules -- also drops the redundant positional (0/1)
  snapshot entries that duplicated the named outputs.

Addresses review from SPPearce on nf-core#12910.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014eSqkuYZXfNiWy87y3srup
@erikrikarddaniel

Copy link
Copy Markdown
Member Author

Thanks for the review! Pushed adf8590 addressing all four points:

  • Version eval: simplified to one grep -m1 -oE '[0-9]+\.[0-9]+\.[0-9]+\.[0-9]+' call across all four modules, same output.
  • looplace's restaging (the three inline comments): shrunk considerably. It no longer walks an arbitrary file/directory tree -- lootasks' output schema is fixed (manifest.json + fold_*/{ref.nwk,ref.fasta,query.fasta}, confirmed against a real multi-fold run, nothing else ever appears), so the private-writable-copy logic is now ~6 lines instead of ~20.
    Kept the symlink-based approach rather than switching to stageInMode 'copy': lootasks already writes each fold's ref.fasta/ref.nwk as a real per-fold copy (not shared), so at GTDB scale (hundreds of thousands of genomes, thousands of folds) a second full copy on top of that in looplace isn't affordable -- happy to discuss further if you still think a full-copy approach is preferable for simplicity.
  • LLM-style comments: trimmed throughout, including the two that were inside script: blocks and leaking into .command.sh.
  • sanitizeOutput everywhere: added to looscore's two non-stub tests (the other three modules already had it) -- as a bonus it also dropped the redundant positional (0/1) snapshot entries that duplicated the named outputs.

Re-verified all 12 tests (4 modules) pass under Docker, Singularity, and conda.

🤖 Generated with Claude Code

https://claude.ai/code/session_014eSqkuYZXfNiWy87y3srup

nf-core modules lint's meta_yml checks compare the eval expression
recorded in meta.yml against main.nf verbatim -- adf8590 simplified
the eval command in main.nf but left meta.yml stale, failing lint on
all four modules. Regenerated via `nf-core modules lint --fix`.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014eSqkuYZXfNiWy87y3srup
@Aaramis

Aaramis commented Sep 11, 2026

Copy link
Copy Markdown

Hi Daniel,

Small correction to "not yet fixed": the loo-tasks model bug is fixed in 5c6bd1e on
epa-ng. It just is not in any tagged release. v0.9.3.6 is the commit right before it,
and bioconda is on 0.9.3.4, so the pending bump to 0.9.3.6 would not help either.

I can tag v0.9.3.7 and push it to bioconda if that is useful. Happy to do it now if
you want to drop the workaround in this PR, or after it lands as a follow-up. Your call

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants