Skip to content
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

Merged
merged 28 commits into from
Sep 20, 2023
Merged

Template merge #15

merged 28 commits into from
Sep 20, 2023

Conversation

ksenia-krasheninnikova
Copy link
Contributor

PR checklist

  • This comment contains a description of changes (with reason).
  • If you've fixed a bug or added code that should be tested, add tests!
  • If you've added a new tool - have you followed the pipeline conventions in the contribution docs
  • If necessary, also make a PR on the sanger-tol/genomeassembly branch on the nf-core/test-datasets repository.
  • Make sure your code lints (nf-core lint).
  • Ensure the test suite passes (nextflow run . -profile test,docker --outdir <OUTDIR>).
  • Usage Documentation in docs/usage.md is updated.
  • Output Documentation in docs/output.md is updated.
  • CHANGELOG.md is updated.
  • README.md is updated (including new tool citations and authors/contributors).

Copy link
Contributor

@priyanka-surana priyanka-surana left a 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
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
Copyright (c) 2023 Genome Research Ltd
Copyright (c) 2022-2023 Genome Research Ltd.

Copy link
Contributor

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.

Comment on lines 7 to 8
<meta name="description" content="sanger-tol/genomeassembly: A bioinformatics best-practice analysis pipeline for genome assembly from P
acBio CCS and HiC reads">
Copy link
Contributor

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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
<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
Comment on lines 17 to 20
// References
genome = null
igenomes_base = 's3://ngi-igenomes/igenomes'
igenomes_ignore = false
Copy link
Contributor

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...

nextflow.config Outdated Show resolved Hide resolved
@@ -0,0 +1,2 @@
prefix: sanger-tol
skip: []
Copy link
Contributor

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

Copy link
Contributor

@priyanka-surana priyanka-surana Jul 18, 2023

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?

// Check input path parameters to see if they exist
def checkPathParamList = [ params.input ]
def checkPathParamList = [ params.input, params.multiqc_config, params.fasta ]
Copy link
Contributor

@priyanka-surana priyanka-surana Jul 18, 2023

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.

@github-actions
Copy link

github-actions bot commented Aug 16, 2023

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 1788133

+| ✅ 131 tests passed       |+
#| ❔  18 tests were ignored |#
!| ❗  15 tests had warnings |!

❗ Test warnings:

  • files_exist - File not found: conf/igenomes.config
  • readme - README contains the placeholder zenodo.XXXXXXX. This should be replaced with the zenodo doi (after the first release).
  • pipeline_todos - TODO string in WorkflowMain.groovy: Add Zenodo DOI for pipeline after first release
  • pipeline_todos - TODO string in methods_description_template.yml: #Update the HTML below to your prefered methods description, e.g. add publication citation for this pipeline
  • pipeline_todos - TODO string in genomeassembly.nf: Add all file path parameters for the pipeline to the list below
  • pipeline_todos - TODO string in usage.md: Add documentation about anything specific to running your pipeline. For general topics, please point to (and add to) the main nf-core website.
  • pipeline_todos - TODO string in output.md: Write this documentation describing your workflow's output
  • pipeline_todos - TODO string in base.config: Check the defaults for all processes
  • pipeline_todos - TODO string in base.config: Customise requirements for specific processes.
  • pipeline_todos - TODO string in ci.yml: You can customise CI pipeline run tests as required
  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • system_exit - System.exit in WorkflowSanger-tol-genomeassembly.groovy: System.exit(1) [line 51]
  • schema_description - No description provided in schema for parameter: timestamp
  • schema_description - No description provided in schema for parameter: hifiasm
  • schema_description - No description provided in schema for parameter: hifiasmhic

❔ Tests ignored:

  • files_exist - File is ignored: assets/nf-core-genomeassembly_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-genomeassembly_logo_light.png
  • files_exist - File is ignored: docs/images/nf-core-genomeassembly_logo_dark.png
  • files_exist - File is ignored: .github/ISSUE_TEMPLATE/config.yml
  • files_exist - File is ignored: .github/workflows/awstest.yml
  • files_exist - File is ignored: .github/workflows/awsfulltest.yml
  • nextflow_config - Config variable ignored: manifest.name
  • nextflow_config - Config variable ignored: manifest.homePage
  • files_unchanged - File ignored due to lint config: LICENSE or LICENSE.md or LICENCE or LICENCE.md
  • files_unchanged - File ignored due to lint config: .github/CONTRIBUTING.md
  • files_unchanged - File ignored due to lint config: .github/ISSUE_TEMPLATE/bug_report.yml
  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • files_unchanged - File ignored due to lint config: assets/sendmail_template.txt
  • files_unchanged - File does not exist: assets/nf-core-genomeassembly_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-genomeassembly_logo_light.png
  • files_unchanged - File does not exist: docs/images/nf-core-genomeassembly_logo_dark.png
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy
  • files_unchanged - File ignored due to lint config: .gitignore or .prettierignore or pyproject.toml

✅ Tests passed:

Run details

  • nf-core/tools version 2.8
  • Run at 2023-09-14 10:32:20

Copy link
Contributor

@priyanka-surana priyanka-surana left a 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.
Copy link
Contributor

@priyanka-surana priyanka-surana Sep 6, 2023

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.

Suggested change
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.

Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor Author

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?

Copy link
Contributor Author

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.

conf/modules.config Outdated Show resolved Hide resolved
"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.",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"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.

@muffato
Copy link
Member

muffato commented Sep 8, 2023

@muffato Does this make sense? Is this too Sanger specific to be here?

See my comments and my suggestion in #19

Updated config for running longranger with and without LSF
Copy link
Contributor

@priyanka-surana priyanka-surana left a 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.

@priyanka-surana priyanka-surana merged commit 3bf48e2 into dev Sep 20, 2023
4 of 6 checks passed
@priyanka-surana priyanka-surana deleted the template_merge branch September 20, 2023 12:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants