-
Notifications
You must be signed in to change notification settings - Fork 2
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Template merge #15
Template merge #15
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Main points - documentation, template issues and code - input mismatch. Can you please make the changes? Then we can review a cleaner copy.
.nf-core.yml
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add Code of Conduct back in the repository and then you should be able to remove from here? You can get the latest copy from https://github.com/nf-core/tools/blob/master/nf_core/pipeline-template/CODE_OF_CONDUCT.md
LICENSE
Outdated
@@ -1,6 +1,6 @@ | |||
MIT License | |||
|
|||
Copyright (c) 2022 Genome Research Ltd. | |||
Copyright (c) 2023 Genome Research Ltd |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copyright (c) 2023 Genome Research Ltd | |
Copyright (c) 2022-2023 Genome Research Ltd. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has a lot of left over from the older version. It might be easier to make a copy of README from the template version and then fill in the TODOs.
assets/email_template.html
Outdated
<meta name="description" content="sanger-tol/genomeassembly: A bioinformatics best-practice analysis pipeline for genome assembly from P | ||
acBio CCS and HiC reads"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the line break intentional?
## You inject any metadata in the Nextflow '${workflow}' object | ||
data: | | ||
<h4>Methods</h4> | ||
<p>Data was processed using sanger-tol/genomeassembly v${workflow.manifest.version} ${doi_text} of the nf-core collection of workflows (<a href="https://doi.org/10.1038/s41587-020-0439-x">Ewels <em>et al.</em>, 2020</a>).</p> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
<p>Data was processed using sanger-tol/genomeassembly v${workflow.manifest.version} ${doi_text} of the nf-core collection of workflows (<a href="https://doi.org/10.1038/s41587-020-0439-x">Ewels <em>et al.</em>, 2020</a>).</p> | |
<p>Data was processed using sanger-tol/genomeassembly v${workflow.manifest.version} ${doi_text} of the sanger-tol collection of workflows, created using nf-core (<a href="https://doi.org/10.1038/s41587-020-0439-x">Ewels <em>et al.</em>, 2020</a>).</p> |
nextflow.config
Outdated
// References | ||
genome = null | ||
igenomes_base = 's3://ngi-igenomes/igenomes' | ||
igenomes_ignore = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you removed igenomes...
pipeline_template.yml
Outdated
@@ -0,0 +1,2 @@ | |||
prefix: sanger-tol | |||
skip: [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you skipped igenomes
subworkflows/local/input_check.nf
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code in this file does not make sense to me, considering your input and stuff. Can you please explain/fix?
workflows/genomeassembly.nf
Outdated
// Check input path parameters to see if they exist | ||
def checkPathParamList = [ params.input ] | ||
def checkPathParamList = [ params.input, params.multiqc_config, params.fasta ] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You test for params.fasta
both here and in main.nf
but I don't see a params.fasta in your input. I am confused about this.
72409da
to
20f78da
Compare
|
Changes to README according to the TEMPLATE updates
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am unable to test the pipeline at this point. This is just a documentation review.
The nf-core linting
error might be fixed by modules update.
README.md
Outdated
|
||
The pipeline is built using [Nextflow](https://www.nextflow.io), a workflow tool to run tasks across multiple compute infrastructures in a very portable manner. It uses Docker/Singularity containers making installation trivial and results highly reproducible. The [Nextflow DSL2](https://www.nextflow.io/docs/latest/dsl2.html) implementation of this pipeline uses one container per process which makes it much easier to maintain and update software dependencies. Where possible, these processes have been submitted to and installed from [nf-core/modules](https://github.com/nf-core/modules) in order to make them available to all nf-core pipelines, and to everyone within the Nextflow community! | ||
|
||
<!-- TODO nf-core: Add full-sized test dataset and amend the paragraph below if applicable --> | ||
|
||
On release, automated continuous integration tests run the pipeline on a full-sized dataset on the AWS cloud infrastructure. This ensures that the pipeline runs on AWS, has sensible resource allocation defaults set to run on real-world datasets, and permits the persistent storage of results to benchmark between pipeline releases and other analysis sources. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This section should have automatically been removed as part of the template merge. Seems like the merging was just messy.
On release, automated continuous integration tests run the pipeline on a full-sized dataset on the AWS cloud infrastructure. This ensures that the pipeline runs on AWS, has sensible resource allocation defaults set to run on real-world datasets, and permits the persistent storage of results to benchmark between pipeline releases and other analysis sources. |
assets/schema_input.json
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I cannot test this right now.
conf/base.config
Outdated
cpus = { check_max( 1 * task.attempt, 'cpus' ) } | ||
memory = { check_max( 6.GB * task.attempt, 'memory' ) } | ||
time = { check_max( 4.h * task.attempt, 'time' ) } | ||
|
||
errorStrategy = { task.exitStatus in [143,137,104,134,139] ? 'retry' : 'finish' } | ||
errorStrategy = { task.exitStatus in ((130..145) + 104) ? 'retry' : 'finish' } | ||
maxRetries = 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You might want to up this number as it is no longer controlled by the main sanger.config
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, I don't quite get this comment. This is the list of error codes on which the pipelines should be retried. Do you mean to top it up with some other error codes?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Figured it out. Asked Matthieu.
nextflow_schema.json
Outdated
"schema": "assets/schema_input.json", | ||
"description": "Path to YAML file containing information about the samples in the experiment.", | ||
"description": "Path to comma-separated file containing information about the samples in the experiment.", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"description": "Path to comma-separated file containing information about the samples in the experiment.", | |
"description": "Path to YAML file containing information about the samples in the experiment.", |
You can use the nf-core schema build
tool to check the rest of the document via the online GUI.
Updated config for running longranger with and without LSF
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Documentation will be done in separate PR. This is good to go.
PR checklist
nf-core lint
).nextflow run . -profile test,docker --outdir <OUTDIR>
).docs/usage.md
is updated.docs/output.md
is updated.CHANGELOG.md
is updated.README.md
is updated (including new tool citations and authors/contributors).