Submit tasks as job arrays and fix RNAs in distill summaries#472
Open
tpall wants to merge 62 commits intoWrightonLabCSU:devfrom
Open
Submit tasks as job arrays and fix RNAs in distill summaries#472tpall wants to merge 62 commits intoWrightonLabCSU:devfrom
tpall wants to merge 62 commits intoWrightonLabCSU:devfrom
Conversation
…nctionality - Update groupby_column default value to "input_fasta" in distill.py - Adjust input paths in distill.nf for consistency - Enhance argument handling in SUMMARIZE process
…rid of FASTA_COLUMN environment variable
Member
|
Hey @tpall Thanks for this. The job array addition it nice. There is a larger planned update to batch a lot of the inputs into singular jobs to reduce the burden on the queue, since running DRAM with lots of inputs can overwhelm a SLURM scheduler, but adding in job arrays, which weren't supported on the version of Nextflow we initially developed DRAM2 on, but we recently moved to >=24 (we should lock in >=24.04.0 since there are early 24.* prereleases out there if we add in job arrays). I will have to do some testing on utilizing job arrays and their implication. Because from my initial testing it seems like it stops the next stop from proceeding until their are enough inputs to fill an array. Which might be ok. But if we are going to be doing batching anyway, it might not be that important and not worth it. Also thanks for some of the other QoL updates like updating some of the syntax to DSL2 (Channel -> channel, etc.). I will have to more fully review the code, which I can get to in a couple weeks. I have deadline for next week, and probably won't be able to review much before then. But I will leave just a couple of quick thoughts. Thanks again |
| } | ||
|
|
||
| withName: 'DRAM:ANNOTATE:CALL:.*|DRAM:ANNOTATE:DB_SEARCH:.*' { | ||
| array = params.array_size |
Member
There was a problem hiding this comment.
I would like to support people running DRAM2 with local executor (such as on their own computer if they want), which doesn't support array. So the array should only be used with executors that support it.
| maxRetries = 2 | ||
| } | ||
|
|
||
| withName: 'DRAM:ANNOTATE:CALL:.*|DRAM:ANNOTATE:DB_SEARCH:.*' { |
Member
There was a problem hiding this comment.
jobs under DRAM:ANNOTATE:QC:COLLECT.* could also have a job array
Comment on lines
+68
to
+71
| withName: 'DRAM:ANNOTATE:CALL:.*|DRAM:ANNOTATE:DB_SEARCH:.*' { | ||
| array = params.array_size | ||
| } | ||
|
|
Member
There was a problem hiding this comment.
this code here in base.config for the job array should probably be in modules.config
Added COLLECT_RNA steps to array
Adds a DECOMPRESS_FASTA module (bbtools reformat.sh in the existing bbmap container) and routes only .gz inputs through it via a branch on the fasta channel. Basename stripping is unified so sample.fa and sample.fa.gz produce the same downstream name, keeping outputs identical regardless of input compression. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
It was already defined in nextflow.config (default 10) and consumed by conf/base.config, but absent from nextflow_schema.json, so runs emitted a schema-validation warning. Added alongside queue_size under Process Options. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drops the never-included trees module and all scripts only it referenced (parse_annotations.py, update_annots_trees.py, color_labels.R), plus update_tree.py which had no references at all. Also removes assets/trees/ refpkgs (only consumed by the dropped module) and the DRAM-v1 standalone DB setup scripts under assets/internal/ which were never wired into the DSL2 pipeline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Both are unreferenced DRAM-v1 helpers under bin/assets/forms/. They shell out to a DRAM-setup.py CLI that isn't part of the DSL2 pipeline, and upstream has already removed them. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Eight references used DB_CHANNEL_SETUP (all caps) but the workflow is defined as DB_channel_SETUP. Groovy is case-sensitive so the references failed at runtime with "No such variable: DB_CHANNEL_SETUP". Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Ports TRANSPOSON_PFAMS, CELL_ENTRY_CAZYS, and VIRAL_PEPTIDASES_MEROPS verbatim from DRAM v1 (mag_annotator/annotate_vgfs.py at upstream 6cd68f9) into a shared module so the upcoming dramv_flags step and later code can import them without re-declaring. Cross-checked byte-equal to the v1 source via AST literal eval (17 / 22 / 210 entries). VirSorter category constants (HALLMARK / VIRAL_LIKE) intentionally omitted; they only feed the auxiliary_score path which is deferred to Phase 2. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Phase 1 port of the FASTA-only branch of v1's add_dramv_scores_and_flags / get_metabolic_flags (annotate_vgfs.py at upstream 6cd68f9). Adds two columns to a combined annotations TSV: is_transposon (bool): row's pfam ids intersect TRANSPOSON_PFAMS. amg_flags (str): concatenated single-letter flags in v1 order M / K / E / A / P / T / F / B (V is deferred until VOGdb is wired). Implementation notes: - All set lookups go through the rule_parser ID_EXPR_DICT, with one local override: polars str.extract_all returns full matches not capture groups, so the bracket-wrapping pfam expressions in rule_parser produce '[PF01609.1]' instead of 'PF01609' and never intersect TRANSPOSON_PFAMS. Override pulls the bare id. Same quirk affects EC entries upstream; flagged for a follow-up rule_parser fix. - B flag uses a 3-window rolling sum on _M over scaffold (sorted by start_position), checking the three windows that include each row. Faithfully reproduces v1's behaviour, including for genes at scaffold boundaries. - Scaffold lengths read directly from the catalog FASTA so the F flag doesn't depend on QUAST / call_genes outputs. - metabolic_genes set is built at runtime from existing distill_*.tsv rows where potential_amg == TRUE — no separate distill_amg.tsv vendored or maintained. Smoke-tested locally: M / K / E / A / P / T / F / B all fire correctly; B does not cross scaffold boundaries; <3-gene scaffolds get no B. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 4 of Phase 1: plumbing for the dramv_flags.py compute added in 0984ec2. - modules/local/dramv/dramv_flags.nf: process that concatenates the staged input FASTAs into a single catalog file and feeds it to dramv_flags.py alongside the combined annotations TSV. Uses the same conda env recipe as SUMMARIZE (polars / click / lark / numpy). - modules/local/dramv/environment.yml: matching env spec. - subworkflows/local/annotate.nf: when params.use_dramv is on, run DRAMV_FLAGS after QC (or DB_SEARCH if QC is off) and rebind ch_combined_annotations to the augmented TSV so SUMMARIZE sees the amg_flags / is_transposon columns. - nextflow.config: introduce params.use_dramv (default false) and params.amg_length_from_end (default 5000 bp) for the F-flag window. Smart viral-mode defaults (forcing groupby_column=scaffold and skipping TRNA/RRNA scan + QUAST when use_dramv is true) are step 6. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 5 of Phase 1. When --amg_only is set, distill.py:
1. Requires the amg_flags column on the input annotations and
filters to rows with 'M' present and none of 'A','P','T'
(M-and-not-A/P/T, per the agreed default for Phase 1).
2. Keeps the potential_amg column from each distill sheet, filters
the concatenated form to potential_amg=TRUE, and overrides
topic_ecosystem to 'AMG' so all retained rows collapse into a
single 'AMG' sheet in metabolism_summary.xlsx.
3. Errors out with a clear message if amg_flags is missing on the
annotations or if no selected distill sheet carries
potential_amg.
End-to-end smoke-tested locally on a 5-row synthetic annotations TSV:
the 'AF' row is dropped, 'AMG' is the only topic_ecosystem in the
output, and counts match for the K01647 / K00033 rows that
participate in multiple AMG pathway contexts.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 6 of Phase 1. When params.use_dramv is true: - subworkflows/local/call.nf: skip QUAST. Catalog-level QUAST stats collapse all viral contigs into one row, which doesn't align with the per-scaffold (per-vMAG) unit of analysis we use in viral mode. Substitute the dummy sheet so SUMMARIZE still gets its expected optional input. - subworkflows/local/qc.nf: skip COLLECT_RNA. The rRNA / tRNA collectors group by input_fasta and the catalog ships as one fasta, so the collected output would be a single uninformative row. Mirrors v1's --skip_trnascan, extended to rRNA. (Phase 2 can add a per-scaffold collector if anyone asks for phage tRNAs.) - modules/local/distill/distill.nf: pass --groupby_column (forcing 'scaffold' under use_dramv, otherwise params.groupby_column) and append --amg_only so distill.py runs in DRAM-v mode. - workflows/dram.nf: same scaffold override at the PRODUCT_HEATMAP call site. No new params; viral-mode behaviour is fully derived from params.use_dramv. Tests + docs + smoke-run on real geNomad+CheckV data are step 7+. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Step 7 of Phase 1 (option a only - pytest unit, no nf-test fixture). Seven focused tests covering: - each individual flag firing on a single scaffold (M / K / A / T / F / B + is_transposon column) - B not crossing scaffold boundaries - sub-3-gene scaffolds correctly get no B - K forces M (when only the amgs set, not metabolic_genes, matches) - E (verified AMG) - F window boundary behaviour at start vs end of contig - multi-record FASTA parsing including IDs with embedded spaces tests/unit/conftest.py adds bin/ to sys.path so the test file can import dramv_flags the same way Nextflow stages it. Drive-by: switch rolling_sum min_periods to min_samples in compute_flags to clear a polars 1.21+ deprecation warning. Run with: pytest tests/unit/ -- all seven pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Without these in nextflow_schema.json the launcher prints "WARN: invalid input values" for both params on every viral-mode run. Cosmetic only - no behaviour change.
…nputs In DRAM-v viral mode all three of rrna_collected / trna_collected / quast_stats resolve to the same default_sheet placeholder, which gives them the same basename when Nextflow tries to stage them and triggers "input file name collision". Tag each path input with stageAs so the work directory always sees three distinct staged names. Bacterial mode is unaffected: the natural filenames (collected_rrnas.tsv, collected_trnas.tsv, collected_quast.tsv) are already distinct, but stageAs simply renames the staged copy and distill.py opens whatever path it is given. Empty / NULL placeholder files are still handled by the read-and-fall-back-to-None logic already in distill.py. Reproduces in feature/dramv-phase1 smoke run on the eluring-virome catalog at SUMMARIZE step.
conf/modules.config was appending --groupby_column ${FASTA_COLUMN} to
SUMMARIZE via task.ext.args. After f255dde the distill.nf script
template already passes --groupby_column with the use_dramv-aware
value, so the ext.args line ends up doubling the flag. click takes
the last occurrence on the command line, which silently overrides
"scaffold" with "input_fasta" in viral mode and breaks per-vMAG
aggregation.
Removing the redundant ext.args line lets distill.nf own the flag.
Bacterial behaviour is unchanged because params.groupby_column
defaults to params.CONSTANTS.FASTA_COLUMN.
Caught during the eluring-virome smoke run on
feature/dramv-phase1: 58 viral contigs, but summarized_genomes.tsv
had a single "smoke_filtered" sample column instead of one column
per scaffold.
Closes Phase 1 of DRAM-v support. README gets an "8) Viral mode" example mirroring the bacterial usage block and explaining what --use_dramv toggles. CHANGELOG adds an Unreleased section enumerating the feature, the bundled assets, the unit-test surface, and the ride-along distill.py rewrite + Nextflow plumbing fixes that landed on the same branch.
DRAMV_FLAGS' output never had a publishDir entry, so its annotations_with_flags.tsv only existed inside the work directory and downstream tools / users had to dig with find to read it. Mirror the COMBINE_ANNOTATIONS publish pattern: copy the TSV to outdir/ANNOTATE and route any process logs into pipeline_info/logs. Cosmetic; no behaviour change for existing runs.
is_transposon (and therefore the T flag in amg_flags) is built from pfam_hits / pfam_id ∩ TRANSPOSON_PFAMS. Without --use_pfam the annotations carry no Pfam columns at all, is_transposon is false on every row, and T silently never fires across the whole catalog. Caught by spot-checking the eluring-virome smoke run: 3352 annotations, all is_transposon=false, even though several proviral contigs would be expected to carry transposon Pfams. When --use_dramv is set and use_pfam wasn't already enabled, flip it on with a log.warn so the user knows. Anyone who explicitly wants viral mode without Pfam (e.g. for runtime reasons) can pass --use_pfam false. Note: the workflow's --anno_dbs parsing path still leaves PFAM commented out due to a separate documented DRAM2 bug (see workflows/dram.nf:108). This change only flips the direct params.use_pfam path and so doesn't conflict with that.
feat(dramv): Phase 1 — FASTA-only AMG flags and viral distillate
The Phase 1 viral-mode example was added as the 8th usage example in the merged feature/dramv-phase1 branch but isn't visible to anyone skimming the top of the README. Add a one-paragraph intro mention of --use_dramv right after the bacterial database paragraph, and a Quick Links bullet that points at the example. No new feature work, just findability.
Why: vOTU catalogs and similar workflows often produce one concatenated fasta living alongside other unrelated fastas; previously --input_fasta only accepted a directory and globbed via --fasta_fmt, forcing users to stage a clean directory or symlink. Detect via isFile() and pass the path directly to fromPath; fall back to the existing dir-glob behavior otherwise. n_fastas is 1 for file inputs. Schema description and error message updated to reflect both modes. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Why: indexing a vOTU catalog's called_genes.faa exceeded the 1 GB ceiling of process_tiny and was OOM-killed. errorStrategy 'finish' also disabled the task.attempt memory multiplier, so the failure was unrecoverable without a manual config override. Now uses process_small (12 GB base) and retries up to 2 times, letting the multiplier scale to 24/36 GB if needed for very large catalogs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop errorStrategy 'finish' so SLURM exit codes 130-145 trigger the base config's retry-with-doubled-resources. KOFam, the largest HMM database, gets its own withName block at process_medium (36 GB) to avoid OOM on the first attempt without inflating the smaller HMM searches. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add optional --kofam_chunk_size and --vog_chunk_size params that split the
called-protein FASTA into N-record chunks via splitFasta, run hmmsearch per
chunk as parallel array tasks, and concat the per-chunk hits back into the
canonical ${sample}___formatted_${db_name}_hits.csv filename that
combine_annotations.py expects. 0 disables chunking (default), preserving
the existing single-task behaviour.
Restores parallelism for cases where many contigs are submitted as one
input fasta — previously a single 2-cpu task chewed through everything
and OOM'd on big virome catalogs.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Drop errorStrategy 'finish' so SLURM exit codes 130-145 trigger the base config's retry-with-doubled-resources, and switch the label from process_small (12 GB) to process_big (72 GB). The merged annotations frame grows with gene count x number of DBs joined, so 12 GB OOMs on large virome catalogs. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Nextflow 26.04 V2 config parser does strict scope checking and rejects cross-block references at parse time. The validation.help block interpolated manifest.name / manifest.version / manifest.doi from the sibling manifest block, and validation.summary referenced validation.help.beforeText/afterText. V1 resolved both at runtime; V2 errors out with "manifest is not defined" and "validation is not defined" before the workflow even launches. Inlines literal values (WrightonLabCSU/dram, 2.0.0-beta24) and duplicates the help banner into summary so each block is self-contained. The manifest.doi branch was always empty (doi = "") so dropping it is a no-op. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Three V2-compat issues triggered when launching with Nextflow 26.04:
1. conf/constants.config used a nested closure block inside params:
params { CONSTANTS { FASTA_COLUMN = "input_fasta" } }
V1 promoted nested closures inside params to a Map; V2 leaves
params.CONSTANTS as null, and every nextflow.config / .nf reference
to params.CONSTANTS.FASTA_COLUMN crashed with "Cannot get property
FASTA_COLUMN on null object". Switched to a Map literal so the
shape is identical under both parsers.
2. nf-schema@2.1.1 does not register the validation { ... } config
scope under V2, producing 11 "Unrecognized config option
validation.*" warnings. Bumped to 2.6.1 (same version as aftekas).
3. boost { cleanup = false } block referenced a scope from an
nf-boost plugin that is not declared in the plugins block, so V2
warned. Removed the orphan block.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
V2 config parser does not merge params from `includeConfig` before
evaluating inline references in subsequent `params { ... }` blocks,
so `params.CONSTANTS.FASTA_COLUMN` at nextflow.config:156 resolved
to null and crashed with "Cannot get property FASTA_COLUMN on null
object" -- visible during `nextflow pull` because manifest read
parses the full config.
Hardcoding the literal "input_fasta" sidesteps the parse-time lookup.
Runtime usages in .nf modules continue to work since params are fully
merged before processes execute.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Bare CLI flags like --call / --annotate / --use_kofam arrive at the nf-schema validator as the string "true" under Nextflow 26.x + nf-schema 2.6.x (V1 implicitly coerced, V2 + 2.6.x do not). Schema declares them "type": "boolean", so validation rejected every flag with 'Value is [string] but should be [boolean]'. Setting validation.lenientMode = true tells nf-schema to attempt type coercion before failing, restoring the previous CLI ergonomics without forcing users into a -params-file YAML. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Set array_size = 0 to sidestep a Nextflow concurrency bug in BashWrapperBuilder.createContainerBuilder where the container bind/mount list is iterated by one TaskArrayCollector while another concurrently mutates it, raising ConcurrentModificationException mid-run. Reproduced on HMM_SEARCH_CAMPER but the race is generic to any DB_SEARCH process matched by the array directive in base.config. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Holds papers and notes that should stay out of git. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Add essential_viral_function column to bin/assets/amg_database.tsv. Mark TRUE for 23 rows that Martin et al. 2025 (Nat Microbiol 10:2122-2129, doi:10.1038/s41564-025-02095-4) either explicitly call out as misleading AMG annotations (DsrC, QueC/QueF/QueDEF, folA/folB/folK, THF dehydrogenase, formyl transferases) or implicate as essential viral function via the "exclude nucleotide metabolism" recommendation (RNR, pur*, pyrE, mazG, adenylate/cytidylate/guanylate kinase, FGAM synthase, SAICAR synthetase). Metadata only — no flag-logic changes in this commit. The column is consumed by a follow-up that wires an N flag in dramv_flags.py. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- L155 EC: 2.1.2.33.5.4.10 → EC:2.1.2.3; EC:3.5.4.10 (purH bifunctional, AICARFT + IMPCHase, missing semicolon between ECs). - L167 EC: 1.6.1.27.1.1.1 → EC:1.6.1.2; EC:7.1.1.1 (NAD(P)+ trans- hydrogenase, old + new EC, same concatenation bug). - L270 PFAM: PF0232 → PF02324 (Pfam GH70 / Glyco_hydro_70; valid Pfam accessions are 5 digits). The malformed tokens silently failed exact-match lookups in build_amg_id_sets and compute_flags. With these corrected, all_amgs gains one identifier (L167 contributes two valid ECs in place of one bogus token). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Wire up an N letter in amg_flags that fires when any of a gene's annotation IDs match a row in amg_database.tsv where essential_viral_function=TRUE. - build_amg_id_sets() returns (all, verified, essential). Tolerates the column being absent for backward compat. - compute_flags() takes an optional essential_amgs kwarg (default empty), so existing callers don't have to change. - N is appended after B in amg_flags. It does NOT propagate to M (unlike K) and is not yet consumed by any downstream filter — purely informational metadata until a strict-AMG filter decision is made. Five new unit tests cover N firing alone, N combining with K/E (MKEN), backward-compat default, and loader behavior with/without the column. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
The --amg_only filter now also drops rows whose amg_flags contain N (essential viral function per Martin et al. 2025). This implements the recommendation to exclude essential viral processes — nucleotide metabolism, queuosine biosynthesis, folate biosynthesis, DsrC/TusE- ambiguous genes — from strict AMG calls. Help text updated to document the new exclusion and cite the paper. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Update the DRAM-v Phase 1 viral mode description: amg_flags now includes a non-v1 N letter, and the strict-AMG distillate filter excludes N as well as A/P/T. Cite Martin et al. 2025 inline. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
array_size = 10parameter to nextflow.config andarrayto conf/base.config for more efficient cluster execution.Channeltochannel) for consistency across workflows and improve readability + usage of implicit variable within closures (e.g.it.nametoit -> it.name)Computing environment and command