Release 1.0.0 (dev branch)#52
Open
vinisalazar wants to merge 259 commits into
Open
Conversation
- Remove log file from snapshot check, as it contains timestamps and
will always fail
- Same as last commit, remove log and biom files from snapshot check
(they are not stable)
Improve humann tests
- Add missing schema to allOf - Run 'nf-core pipelines lint --fix files_unchanged' to fix linting
- This will be reverted back to the canonical repo after datasets are merged
- Add/edit params in nextflow.config - Remove outdated params from nextflow_schema.json - Remove template string from humann_v4 test - Apply pre-commit
- Fix formatting - Add introduction - Fix steps list - Add myself to contributors
Match nf-core/tools@3.5.2
- Don't use references to 'master' branch
Merge `get-build-artifacts` to `dev`
|
jfy133
requested changes
Apr 28, 2026
Member
jfy133
left a comment
jfy133
left a comment
There was a problem hiding this comment.
First pass:
- Biggest blocker here is the number of local modules, which should all be fully fledged official modules.
- There are a lot of TODO comments too
- As a general thing, for many files , the code formatting is very inconsistent (particularity with indents) and it's made it very hard for me to read quickly. For some of the
.nffiles this would be fixed by runningnextflow lint --format(I think it is, I use the VSCode extension to do it for me by default)
| "tool": { | ||
| "type": "string", | ||
| "enum": ["humann_v3", "humann_v4", "fmhfunprofiler", "rgi", "mifaser", "diamond", "eggnogmapper"], | ||
| "errorMessage": "Invalid tool name. Please see documentation for all supported profilers. Currently these classifers are included: humann_v3, humann_v4, metaphlan, fmhfunprofiler, mifaser, diamond, eggnogmapper.", |
Member
There was a problem hiding this comment.
I don't think you need to list the tools in the error message, I believe nf-validation should do this for oyu.
| container = "ghcr.io/vdblab/biobakery-profiler:4.0.5--3.6.1_smaller-pt2" | ||
| } | ||
| withName: METAPHLAN_METAPHLAN { // to trick nexflow inspect | ||
| container = "ghcr.io/vdblab/biobakery-profiler:4.0.5--3.6.1_smaller-pt2" |
Member
There was a problem hiding this comment.
This seems like a weird hack, what is the difference between this and the official metaphlan module's container? (same for MPAHUMANN3
| run_fmhfunprofiler = true | ||
| run_humann_v3 = true | ||
| run_mifaser = true | ||
| run_diamond = false |
Member
There was a problem hiding this comment.
What is the reason for turning this off? Maybe make separate config files for testing the tools that are all false here to make sure all tools are at least tested to produce output at least once
| } | ||
| withName: FMHFUNPROFILER { | ||
| ext.args = { "${meta.db_params}" } | ||
| ext.prefix = params.perform_runmerging ? { "${meta.id}_${meta.db_name}.fmhfunprofiler" } : { "${meta.id}_${meta.run_accession}_${meta.db_name}.fmhfunprofiler" } |
Member
There was a problem hiding this comment.
Suggested change
| ext.prefix = params.perform_runmerging ? { "${meta.id}_${meta.db_name}.fmhfunprofiler" } : { "${meta.id}_${meta.run_accession}_${meta.db_name}.fmhfunprofiler" } | |
| ext.prefix = { params.perform_runmerging ? "${meta.id}_${meta.db_name}.fmhfunprofiler" : "${meta.id}_${meta.run_accession}_${meta.db_name}.fmhfunprofiler" } |
To make the config file strict syntax compliant
Member
There was a problem hiding this comment.
When I run nf-core pipeline schema build this reports inconsistent parameter defaults for some of the parameters, please check
| show_hidden = false | ||
| version = false | ||
| pipelines_testdata_base_path = 'https://raw.githubusercontent.com/nf-core/test-datasets/' | ||
| pipelines_testdata_base_path = 'https://raw.githubusercontent.com/nf-core/test-datasets/funcprofiler/' |
Member
There was a problem hiding this comment.
Why this change? Again could be annjoying with template merges
| genome = null | ||
| igenomes_base = 's3://ngi-igenomes/igenomes/' | ||
| igenomes_ignore = false | ||
| // genome = null |
Member
There was a problem hiding this comment.
Can remove entirely if not to be used
| apptainer.enabled = false | ||
| executor.cpus = 4 | ||
| executor.memory = 16.GB | ||
| process { |
Member
There was a problem hiding this comment.
You should not define any resource limits in nextflow.config ! they should go in conf/base.config!
|
|
||
| // Load igenomes.config if required | ||
| includeConfig !params.igenomes_ignore ? 'conf/igenomes.config' : 'conf/igenomes_ignored.config' | ||
| // includeConfig !params.igenomes_ignore ? 'conf/igenomes.config' : 'conf/igenomes_ignored.config' |
| dag { | ||
| enabled = true | ||
| file = "${params.outdir}/pipeline_info/pipeline_dag_${params.trace_report_suffix}.html" | ||
| verbose = true |
Collaborator
Author
|
Thanks for the detailed review @jfy133, will get on it soon! |
runs nextflow lint on all nf files
Member
|
Warning Newer version of the nf-core template is available. Your pipeline is using an old version of the nf-core template: 3.5.2. For more documentation on how to update your pipeline, please see the nf-core documentation and Synchronisation documentation. |
Updated DIAMOND and eggNOG-mapper entries to indicate beta status and added a note about their support.
jonasscheid
requested changes
Apr 30, 2026
jonasscheid
left a comment
jonasscheid
left a comment
There was a problem hiding this comment.
Alongside @jfy133 s thorough review, I have major comments that need to be adressed / fixed before a 1.0.0 release. I guess splitting into multiple PRs to fix these issues, might make sense.
| @@ -0,0 +1,79 @@ | |||
| graph LR | |||
| %%metro line: qc | Preprocessing & QC | #4CAF50 | |||
There was a problem hiding this comment.
The 8 lanes per tool feels redundant and crowded since you are specifying them in the trainstop icon already. I think 1 lane per input type might be more valuable. Also consider using file type icons per different input type
|
|
||
|
|
||
| // Genome references | ||
| genome = 'R64-1-1' |
There was a problem hiding this comment.
Try to add all tools here that can be activated with run_xyz, if they can be combined into one run
|
|
||
| Enabled with `--run_humann_v3` or `--run_humann_v4`. Each sample is first run through MetaPhlAn to generate a taxonomic profile, which guides HUMANn functional profiling. | ||
|
|
||
| <details markdown="1"> |
There was a problem hiding this comment.
not sure how well the drop-down renders on nf-core website, but I would suggest to remove them. It's good to directly see the output structure of all tools
|
|
||
| ### HUMANn v3 / v4 | ||
|
|
||
| Enabled with `--run_humann_v3` or `--run_humann_v4`. Each sample is first run through MetaPhlAn to generate a taxonomic profile, which guides HUMANn functional profiling. |
There was a problem hiding this comment.
What is humann_v3 and v4? I would like to have a short 1,2 sentence summary of the underlying concept such that one understands the following output description
|
|
||
| ### FMH FunProfiler | ||
|
|
||
| Enabled with `--run_fmhfunprofiler`. Uses FracMinHash sketching to rapidly assign reads to functional categories. |
| @@ -14,16 +14,16 @@ nextflow_pipeline { | |||
|
|
|||
| then { | |||
| // stable_name: All files + folders in ${params.outdir}/ with a stable name | |||
There was a problem hiding this comment.
In general: You need more tests for your variations of input samplesheet/databases. Preferrably 1 test per path in your metro map
| @@ -0,0 +1,139 @@ | |||
| nextflow_workflow { | |||
|
|
|||
| name "Test Subworkflow CONCAT_ALL" | |||
There was a problem hiding this comment.
Double check if you sure you need a test for a 1-module subworkflow
There was a problem hiding this comment.
If yes, please make this concatall a folder and put the testfolder in it
| @@ -0,0 +1,188 @@ | |||
| def Adef Samplesheet() { | |||
There was a problem hiding this comment.
I dont see where this is used, but maybe I cannot read anymore at this stage :D
| include { SEQKIT_FQ2FA } from '../../../modules/nf-core/seqkit/fq2fa/main' | ||
| include { GUNZIP } from '../../../modules/nf-core/gunzip/main' | ||
|
|
||
| // Custom Functions |
There was a problem hiding this comment.
Nitpick: I think putting custom functions at the end is more appealing
| includeConfig 'conf/base.config' | ||
|
|
||
| profiles { | ||
| run_conda_free_steps{ |
There was a problem hiding this comment.
Not sure if this is the right way. Could you use the template conda profile and add run_fmhfunprofiler = false to it?
move Enabling profiles section before database download instructions add note that --run_ flags require a database indicate database versions for all download instructions.
11 tasks
Clarify beta status and database guidance in docs
Address trivial review changes from PR #52
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
7 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.
Investigating RGI test problem
PR checklist
nf-core pipelines lint).nextflow run . -profile test,docker --outdir <OUTDIR>).nextflow run . -profile debug,test,docker --outdir <OUTDIR>).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).