Repository navigation
Modularize MAG generation with read grouping and selectable binning strategies - #1454
paulzierep wants to merge 40 commits into
Conversation
…ow as 1.0 CheckM2 omits degenerate bins from its quality table. dRep then ran its bundled CheckM on the unassessed genome, that assessment failed, and dereplication returned zero genomes, which cascaded into the abundance and reporting steps. An identifier extraction plus a filter-from-file step now restrict dRep's genome list to bins that CheckM2 assessed, so the genome list and the genomeInfo table always agree. All 18 exact grouped/hybrid/coverage/refiner cases now complete with no errored jobs; previously failing grouped and hybrid cases dereplicate 24-108 bins. The breaking input change is released as 1.0 with a dated changelog entry.
|
can you please fill in the PR body for all of these ? |
paulzierep
changed the title
Test Results (powered by Planemo)Test Summary
Errored Tests
|
Test Results (powered by Planemo)Test Summary
Errored Tests
|
Reduce every mock-sample test case from five to two samples so the N-squared cross-alignment matrix (25 -> 4 alignments) keeps the parent workflow CI test within the runtime limit now that COMEBin is disabled. Single-coverage depth matrices keep their 5 columns (3 fixed plus 2 for the sample's own reads); multi-coverage depth matrices drop from 13 to 7 columns (3 fixed plus 2 per sample). Fixture URLs are unchanged.
…dry-mags-parent # Conflicts: # workflows/microbiome/mags-building/CHANGELOG.md # workflows/microbiome/mags-building/MAGs-generation.ga # workflows/microbiome/mags-building/README.md
Keep the parent inside the CI runtime budget while preserving the option space that matters: both assemblers, grouped/original/hybrid read selection, both coverage strategies, nanopore long reads and both refiners. All four run with COMEBin disabled, since the binner toggles are covered by the standalone single- and multi-sample binning tests.
bebatut
left a comment
bebatut
left a comment
There was a problem hiding this comment.
Thanks a lot!
For such a complex workflow, it would be nice to have the different steps as (sub)sections in the README with more details in each. The README is the first thing users will see when they are looking for workflows, so we need to make the different steps stand out and make clear of the different strategies inside each step.
It would be also nice to
- remove in
annotationthe text that is already in thelabelof the steps - changing the order in the inputs to have the reads first before going in details
Deduplicate step annotations, lead the input list with reads, rename the binning frame, and rewrite the README to describe each pipeline stage, the grouping and custom-assembly requirements, and the inspectable outputs.
|
Thanks @bebatut. Reorganized the README around grouping, assembly, binning, refinement and MAG selection, with direct sentences and sublists. Clarified original versus grouped inputs and the coverage-depth fields; renamed the binning strategy and put reads before the detailed controls. Step annotations now explain behavior rather than repeat labels. The workflow has separate grouping, assembly, binning, refinement and quality/abundance frames. |
|
Pinned workflow-test jobs to Ubuntu 24.04 in 2031a87, matching the earlier successful run. GitHub reports runner disconnection on the two failed attempts, which used ubuntu-latest (26.04). The workflow and its test assertions are unchanged. |
bebatut
left a comment
bebatut
left a comment
There was a problem hiding this comment.
Thanks @paulzierep. Few new comments and suggestions 😬
|
Updated the README around the analysis stages, moved preprocessing requirements next to the read inputs, clarified the coverage-table columns and workflow composition, and added citation guidance. Input annotations now include their stage to make the run form easier to navigate. |
mvdbeek
left a comment
mvdbeek
left a comment
There was a problem hiding this comment.
Thanks for the updates! Re-reviewed at 26a8247. References are to workflows/microbiome/mags-building/.
On the hold: galaxyproject/galaxy#24665 (duplicate scheduling with mapped pick_value) is fixed by galaxyproject/galaxy#24670, so that shouldn't block this PR anymore once the fix is deployed.
Fixed since my last review
- Embedded binning workflows. The embedded Single-sample binning 1.1, Multi-sample binning 1.1 and Metagenomic Assembly 1.2 now match the merged standalone workflows on
mainstep for step (tools, versions, tool state, connections,when). Keeping the parent's boolean toggles and adapting them withmap_param_value(steps 69-73) is a clean way to do it. Every subworkflow input is connected, and every subworkflow output the parent uses exists. - README corrections: QUAST assesses rather than selects, the completeness threshold also filters dRep, the CheckM2 database scope, the identifier and missing-metadata caveats. The new "Subworkflow dependencies and update order" section is useful.
- CHANGELOG now describes the three test cases.
- MultiQC section titles. Each CheckM/CheckM2 section is now wired to the matching input.
main(0.9) had these titles the wrong way round too.
Still open
1. Content assertions for the new parent logic. The custom-assembly case (MAGs-generation-tests.yml:4-78) and the hybrid / both strategies / both refiners case (:185-292) still only check coverage-table column counts and that the MultiQC report contains QUAST and CheckM, which every run of this workflow produces. Could you add a few assertions? For example: element count or identifiers on Merged refined bins (all branches) (including one _1-suffixed element), one dereplicated bin or Cluster Assignment row in the hybrid case, and one Binette or dRep check in the custom-assembly case. These add no CI time.
2. Stale README paragraph. README.md:178 still says the embedded copies "retain boolean binner controls and a completeness default of 75" while the "pending standalone 1.1 workflows" differ, and that synchronizing is a prerequisite. That is no longer true now that the copies are synced. Please replace it with a note that the parent exposes booleans and maps them to the standalone Yes/No inputs, and that it passes its own completeness default (75) to the subworkflows.
3. CHANGELOG entries.
CHANGELOG.mdhas no entry for re-embedding the released Single-/Multi-sample binning 1.1 and Metagenomic Assembly 1.2, or for the boolean adapters. Line 12 still speaks of an "assembly and assembly selection" subworkflow.- The MultiQC title fix deserves a line under
### Fixed.
Smaller things
test-data/metadata_mock_community.tsvis added but unused. Onlymetadata_mock_community_mocks.tsvis referenced (MAGs-generation-tests.yml:234).Minimum MAG completeness percentageis described twice (README.md:65and:69).:65could just point to:69.- The header comment of
MAGs-generation-tests.yml(lines 1-3) still mentions "the default path", which no case covers now. Run COMEBinis the only toggle without a?, and the MultiQC section nameCoverM Bin coveragehas a leading space.
Test Results (powered by Planemo)Test Summary
Errored Tests
|
Test Results (powered by Planemo)Test Summary
Errored Tests
|
Test Results (powered by Planemo)Test Summary
Errored Tests
|
|
Thanks @mvdbeek, addressed in c202982: 1. Content assertions. All identifiers are taken from the last green run (26a8247).
2. README. The stale paragraph now says that the embedded workflows are the released Single-/Multi-sample binning 1.1 and Metagenomic Assembly 1.2, that the parent maps its booleans to the standalone 3. CHANGELOG.
Smaller things:
|
- Tests: assert merged refined bins (including _1/_2 suffixed elements from both coverage strategies), a dereplicated MAG and dRep cluster rows in the custom-assembly and hybrid cases; fix the header comment. - README: replace the stale embedded-copy paragraph; describe the minimum completeness once. - CHANGELOG: re-embedded Single-/Multi-sample binning 1.1 and Metagenomic Assembly 1.2 with boolean adapters, MultiQC title fix, test changes. - Rename Run COMEBin to Run COMEBin?, drop the leading space in the CoverM MultiQC section, correct the refiner-combine annotations, and remove the unused metadata_mock_community.tsv.
c202982 to
be5ee51
Compare
|
Lifting the hold: galaxyproject/galaxy#24665 is closed, and the fix galaxyproject/galaxy#24670 is merged into |
Test Results (powered by Planemo)Test Summary
Failed Tests
Passed Tests
|
Both refiners produce near-identical Mock1 bins, so which one dRep keeps is arbitrary. Check that dereplication yields bins; Cluster Assignment still checks that the Binette bin entered dereplication.
Test Results (powered by Planemo)Test Summary
Failed Tests
Passed Tests
|
Test Results (powered by Planemo)Test Summary
Errored Tests
Failed Tests
Passed Tests
|
Collection test attributes only accept collection_type, so 'min: 1' failed schema validation and planemo treated the output as a file without a path. Cluster Assignment already checks dereplication.
|
@mvdbeek could you check whether this is a runner issue? The test job (run 42597800693) didn't fail on a test. After about 4 hours the runner was shut down: At that point tests 1 and 2 had finished with all jobs green (about 76 and 36 minutes). Test 3 had been running for about 2 hours with no job errors and was at the QUAST/CheckM stage. If there is a limit of about 4 hours that I'm missing, I'll make test 3 lighter. |
MaxBin2 reads the first number after the contig name as the abundance, so the jgi_summarize_bam_contig_depths table made it bin on contig length instead of coverage. Single coverage now converts the depth table to contig name and totalAvgDepth without header; multi coverage writes one such file per read sample and passes them as multiple abundance files. Same change as IWC galaxyproject#1538 and galaxyproject#1539.
Outputs of conditional steps are optional, and the editor marks an optional output connected to MaxBin2's required abundance input as invalid. The conversion is cheap, so run it unconditionally; MaxBin2 itself stays gated.
The three test cases took about 76 min, 36 min and over 2 h (the runner was shut down after about 4 h). The individual binners are already tested by the standalone single- and multi-sample binning workflows, so the parent tests now only enable MetaBAT2 and keep covering the parent-level choices: assembler, long reads, grouping, coverage strategy and refiner.
Test Results (powered by Planemo)Test Summary
Failed Tests
Passed Tests
|
Test Results (powered by Planemo)Test Summary
Failed Tests
Passed Tests
|
Test Results (powered by Planemo)Test Summary
Failed Tests
Passed Tests
|
Bin names that appear in one or both coverage strategies, and dRep's choice of cluster representative between near-identical Mock1/Mock2 bins, vary between runs. Keep the top-ranked Binette bin checks and verify that Binette bins enter the dRep cluster assignment.
|
@mvdbeek the remaining test failures came from bin identifiers that vary between runs, so I relaxed those assertions in fbd462e:
The parent-logic checks therefore remain: Binette bins reach the merged set with suffixes from both strategies and enter dereplication. The unsuffixed-bin and selected-MAG checks are gone because they can't be made deterministic with this fixture. The CHANGELOG reflects this. The other points from your last review were addressed in the earlier commits. |
Test Results (powered by Planemo)Test Summary
Errored Tests
|
Summary
Release MAGs generation 1.0 with internal grouping, selectable coverage and
refinement strategies, and genuinely embedded grouping/assembly/binning workflows.
This retains the end-to-end analysis while making its stages easier to understand,
maintain and reuse through #1453 and #1452.
Breaking input change
Provide original per-sample short reads, not separate original/grouped
short-read inputs. The parent now performs grouping internally. These same reads
support grouping, assembly, binning coverage and MAG abundance estimation in the
original samples. Existing jobs using the previous interface need to be remapped.
Nanopore and PacBio reads are independently optional; long-read assembly is
supported through metaSPAdes.
Analysis choices
grouped-first union of grouped and original samples. Duplicate identifiers keep
the grouped entry; this is a union, not a failed-binning fallback.
(default), multi coverage using all original samples, or Both. Multi coverage
is not restricted to the members of a co-assembly; that alternative is not
implemented. Both retains the union and suffixes identifier conflicts.
branches. The parent supplies explicit completeness/contamination-weight controls.
optional boolean controls, all default on. Keep at least one enabled.
mean from both mates. Single uses selected/group-merged reads; Multi uses the
original per-sample reads. No manual read-length input remains.
Implementation and usability
assembly selection, and both binning strategies. Preserve the sample structure
when optional long reads are absent.
placeholders, and preserve sample nesting.
metadata/grouping help and the primary short-read input's role.
stage colors. Boolean conditions retain valid workflow-editor connections.
fastq_groupmerge versions.
Dereplication robustness
CheckM2 can omit degenerate bins it cannot assess. Passing those genomes to dRep
alongside an incomplete quality table caused a fallback to bundled CheckM and
could abort dereplication and downstream reporting. The parent now filters the
genome list to bins present in the CheckM2 quality report before dRep. Quality
thresholds are not lowered.
Validation and regression coverage
two short/long-read baselines plus 16 cases covering grouping, coverage,
refinement, all-binners-on and individual binner-disable scenarios.
live checkpoints 64/64, and read-length checks 21/21.
re-baselined to verified redesigned outputs. The long-read paired identifier was
aligned with its nanopore identifier; this is documented in the changelog.
lint passes. Refreshed EU editor audit of Multi and parent/embedded children:
382 connections, zero invalid.
pairs→Bowtie2 (remove zip), and COMEBin 1.1.0+galaxy1. All othercomputational regions and test assertions are unchanged.
validate this revision. Fresh unchanged EU cases cover multi-only, grouped
multi, and both strategies/all-binners-on. Multi-only and all-binners-on failed
at de-interlacer scheduling (
unexpected_failure, no red tool jobs); newlycreated jobs reference cached source jobs. Uncached retries are running to
isolate cache-copy behavior. Root cause is not yet confirmed; this revision is
not runtime-accepted. Grouped and standalone Multi checks remain pending.
funding/publication and relevant documentation. Its unrelated workflows remain
untouched; Update fairymags wfs #1370 is not fully superseded.
dRep 3.7.1→4.0.2 needs separate clustering/representative-genome acceptance;
text-processing, metaSPAdes, QUAST and MultiQC upgrades also remain deferred.
Tool updates (metaSPAdes 4.3.0, MaxBin2, SemiBin)
4.3.0+galaxy3(Update spades allow co / individual assembly tools-iuc#8872) in its co-assembly mode, which takes the same per-sample or per-group read lists as before, so the read nesting and the optional long-read handling stay as they are. (The embedded assembly is Metagenomic Assembly 1.3 from metagenomic-assembly: update metaSPAdes to 4.3.0+galaxy3 #1545, unchanged. The new individual mode cannot be used here: when a long-read type is missing, the parent can only hand the assembly a collection of skipped placeholder datasets, never a null input, and metaSPAdes then receives a placeholder as a read file.)2.2.7+galaxy7(MaxBin2: handle explicit insufficient-marker no-bin outcome tools-iuc#8787) and SemiBin2.5.0+galaxy0(semibin: update to version 2.5.0 tools-iuc#8885), as in single-sample-binning: give MaxBin2 a two-column abundance file #1538 / multi-sample-binning: give MaxBin2 one abundance file per sample #1539.Usage constraints worth reviewing
tool's output-discovery pattern; distinct sample/group names avoid collisions.
read collection and match identifiers/order; grouping reads does not turn
existing per-sample assemblies into a co-assembly.
Context and related PRs
This is part of the FAIRyMAGs workflow modularization: publish reusable binning
workflows alongside an end-to-end MAG-generation parent, rather than maintaining
separate copies of the same binning logic. Each deliverable has its own branch
and PR so it can be reviewed independently.
Suggested review/merge order: Single → Multi → Parent. The parent embeds its
children, so importing it does not require importing the standalone workflows first.
Developed within FAIRyMAGs, with links
to the related workflows and learning resources in the README.
IWC checklist
FOR CONTRIBUTOR:
FOR REVIEWERS:
This workflow does/runs/performs … xyz … to generate/analyze/etc …namefield should be human readable (spaces are fine, no underscore, dash only where spelling dictates it), no abbreviation unless generally understood-) over underscore (_), prefer all lowercase. Folder becomes repository in iwc-workflows organization and is included in TRS id