Merging template updates 2.2#269
Merged
d4straub merged 18 commits intonf-core:devfrom Feb 10, 2022
Merged
Conversation
|
jfy133
approved these changes
Feb 9, 2022
Member
jfy133
left a comment
There was a problem hiding this comment.
Everything looks fine to me, only minor issue are:
bin_summaryis possibly missing frommodules.config(for consistency)sedused for command reporting for a lot of utils (cat, tar). I don't remember what version of coreutils are in the biocontainers, but might be worth to checkcattaretc. don't report their own versions (if you have not done so already of course)
| } | ||
|
|
||
| withName: CENTRIFUGE { | ||
| ext.args = '--quiet' |
Member
There was a problem hiding this comment.
This wasn't in the syntax v1 DSL2 modules.config, and I don't see it in the old local module - is it correct this has been added here?
Collaborator
Author
There was a problem hiding this comment.
Good catch! This is incorrect I think, I'll remove it!
Comment on lines
+237
to
+253
| withName: GTDBTK_SUMMARY { | ||
| ext.args = "--extension fa" | ||
| publishDir = [ | ||
| path: { "${params.outdir}/Taxonomy/GTDB-Tk" }, | ||
| mode: 'copy', | ||
| saveAs: { filename -> filename.equals('versions.yml') ? null : filename } | ||
| ] | ||
| } | ||
|
|
||
| withName: PROKKA { | ||
| ext.args = "--metagenome" | ||
| publishDir = [ | ||
| path: { "${params.outdir}/Prokka/${meta.assembler}" }, | ||
| mode: 'copy', | ||
| saveAs: { filename -> filename.equals('versions.yml') ? null : filename } | ||
| ] | ||
| } |
Member
There was a problem hiding this comment.
Original modules.config had a module called bin_summary between these, is it correct this is not now here?
Collaborator
Author
There was a problem hiding this comment.
Thats now a little aggregated with other output declaration in https://github.com/d4straub/mag/blob/b103f13a06a9154b32c6eb90d611830463defc8c/conf/modules.config#L171-L177.
| max_memory = 6.GB | ||
| max_time = 48.h | ||
| max_memory = '6.GB' | ||
| max_time = '6.h' |
Member
There was a problem hiding this comment.
Correct this has been droped to 48.h? (Same goes for all subsequent test profiles)
Collaborator
Author
There was a problem hiding this comment.
Yes, the template specifies '6.h' since 2.0 already... see https://github.com/nf-core/tools/blob/e563811adb292b07ec933a52611e937ce9ef5481/nf_core/pipeline-template/conf/test.config#L18-L20
|
|
||
| cat <<-END_VERSIONS > versions.yml | ||
| "${task.process}": | ||
| sed: \$(sed --version 2>&1 | sed -n 1p | sed 's/sed (GNU sed) //') |
|
|
||
| cat <<-END_VERSIONS > versions.yml | ||
| "${task.process}": | ||
| sed: \$(sed --version 2>&1 | sed -n 1p | sed 's/sed (GNU sed) //') |
|
|
||
| cat <<-END_VERSIONS > versions.yml | ||
| "${task.process}": | ||
| sed: \$(sed --version 2>&1 | sed -n 1p | sed 's/sed (GNU sed) //') |
|
|
||
| cat <<-END_VERSIONS > versions.yml | ||
| "${task.process}": | ||
| sed: \$(sed --version 2>&1 | sed -n 1p | sed 's/sed (GNU sed) //') |
|
|
||
| script: | ||
| def software = getSoftwareName(task.process) | ||
| def spades_options = params.spades_options ?: '' |
Member
There was a problem hiding this comment.
Shouldn't this be passed via ext.args? If you are evaluating spades_options somewhere 'higher' (i.e. workflow, or subworkflow) this can also be done in modules.config
Collaborator
Author
There was a problem hiding this comment.
You are right, that is not evaluated at any other point, so I moved it so modules.config.
|
|
||
| script: | ||
| def software = getSoftwareName(task.process) | ||
| def spades_options = params.spades_options ?: '' |
Collaborator
Author
There was a problem hiding this comment.
same as above, moved it so modules.config.
Comment on lines
+10
to
+12
| def hasExtension(it, extension) { | ||
| it.toString().toLowerCase().endsWith(extension.toLowerCase()) | ||
| } |
Member
There was a problem hiding this comment.
Could this go in a custom functions file in lib and be imported? Although maybe overkill for a single tiny function
Collaborator
Author
There was a problem hiding this comment.
I kept it where it is now, I agree that this is not the place it is supposed to be but readability is sort of better this way I think.
1 task
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
3 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.
This is nf-core template update 2.2. I attempted to retain the folder structure, I hope I didn't miss anything.
I only did
-profile testto make sure the output is identical to the dev branch.Linting is failing with
files_unchanged.github/workflows/linting.yml does not match the templatebecause I changed to node version 16, because the previous node version 10 is not working any more with Markdown linting. nf-core template will get an update soon.PR checklist
nf-core lint).nextflow run . -profile test,docker).docs/usage.mdis updated.docs/output.mdis updated.CHANGELOG.mdis updated.README.mdis updated (including new tool citations and authors/contributors).