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.

Sign up for GitHub

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

Jump to bottom

Template update #475

Merged
merged 19 commits into from
Dec 20, 2021
Merged

Template update #475

merged 19 commits into from
Dec 20, 2021

Conversation

FriederikeHanssen
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 - add to the software_versions process and a regex to scrape_software_versions.py
    • 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 nf-core/sarek 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).
  • 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).

@github-actions
Copy link

github-actions bot commented Dec 16, 2021 •
edited
Loading

nf-core lint overall result: Passed ✅ ⚠️

Posted for pipeline commit 35b05c9

+| ✅ 135 tests passed       |+
#| ❔   6 tests were ignored |#
!| ❗   5 tests had warnings |!

❗ Test warnings:

  • pipeline_todos - TODO string in awsfulltest.yml: You can customise AWS full pipeline tests as required
  • pipeline_todos - TODO string in test_full.config: Specify the paths to your full test data ( on nf-core/test-datasets or directly in repositories, e.g. SRA)
  • pipeline_todos - TODO string in test_full.config: Give any required params for the test so that command line flags are not needed
  • pipeline_todos - TODO string in test_full.config: Specify the paths to your full test data ( on nf-core/test-datasets or directly in repositories, e.g. SRA)
  • pipeline_todos - TODO string in test_full.config: Give any required params for the test so that command line flags are not needed

❔ Tests ignored:

  • files_unchanged - File ignored due to lint config: .github/workflows/linting.yml
  • files_unchanged - File ignored due to lint config: assets/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_light.png
  • files_unchanged - File ignored due to lint config: docs/images/nf-core-sarek_logo_dark.png
  • files_unchanged - File ignored due to lint config: lib/NfcoreTemplate.groovy
  • files_unchanged - File ignored due to lint config: assets/multiqc_config.yaml

✅ Tests passed:

Run details

  • nf-core/tools version 2.3.dev0
  • Run at 2021-12-17 19:50:36

@FriederikeHanssen
Copy link
Contributor Author

FriederikeHanssen commented Dec 16, 2021 •
edited
Loading

@maxulysse as I understand it this line

sarek/nextflow.config

Line 186 in 4e5ed7a

includeConfig 'conf/genomes.config'
is mainly needed to get the tests to pass. Could we include the files the same way as RNASeq does, and then use the line from the template

sarek/nextflow.config

Line 187 in 4e5ed7a

//params.genomes = [:]
? Or is the genomes.config needed for anything else?

@FriederikeHanssen FriederikeHanssen marked this pull request as ready for review December 16, 2021 21:16
@maxulysse
Copy link
Member

@maxulysse as I understand it this line

sarek/nextflow.config

Line 186 in 4e5ed7a

includeConfig 'conf/genomes.config'
is mainly needed to get the tests to pass. Could we include the files the same way as RNASeq does, and then use the line from the template

sarek/nextflow.config

Line 187 in 4e5ed7a

//params.genomes = [:]
? Or is the genomes.config needed for anything else?

This genomes.config is indeed needed for the tests, but it can be done differently.
It was kept so far because that was how the files were organized when we started this project. But yeah, we should use a custom config for that. Kill everything

@FriederikeHanssen
Copy link
Contributor Author

OK @ggabernet all the CI tests and the linting tests are passing again. It is really ready for review now 😄

ggabernet
ggabernet reviewed Dec 20, 2021
View reviewed changes
Copy link
Member

@ggabernet ggabernet left a comment

Choose a reason for hiding this comment

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

Looks good to me!

conf/test.config
@@ -97,18 +107,12 @@ profiles {
}
}

//This is apparently useless as it won't overwrite things in the modules.config
Copy link
Member

Choose a reason for hiding this comment

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

yes, this is because test.config is loaded before modules.config. you want it to overwrite the modules.config it needs to be passed as a parameter there, e.g. a new param called --single_fork that is set on the test profile, and if this one is set then the modules.config sets the maxForks to 1 of SNPEFF and ENSEMBLVEP.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes figured it out with Mahesh's help :D I was pretty confused for about an hour there 😆

ggabernet
ggabernet approved these changes Dec 20, 2021
View reviewed changes
Copy link
Member

@ggabernet ggabernet left a comment

Choose a reason for hiding this comment

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

Looks good to me!

marissaDubbelaar
marissaDubbelaar approved these changes Dec 20, 2021
View reviewed changes
Copy link

@marissaDubbelaar marissaDubbelaar left a comment

Choose a reason for hiding this comment

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

LGTM

conf/genomes.config
@@ -5,54 +5,12 @@
* Defines reference genomes, without using iGenome paths
* Can be used by any config that customises the base
* path using $params.genomes_base / --genomes_base
*
* CAREFUL: Some o the files might be reuiqred in the CI tests not yet implemented. They should be gradually moved to the test.config. Until then lets keep this file.
Copy link

Choose a reason for hiding this comment

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

"Some o" and "reuiqred" -> "Some of" and "required", I don't know if this is directly implemented from the template, but might be nice to change?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

no this is not from the template, but a reminder for us to remove this file also as all CI tests are activated

@FriederikeHanssen FriederikeHanssen merged commit f498614 into nf-core:dev Dec 20, 2021
@FriederikeHanssen FriederikeHanssen deleted the merging-template-updates branch December 20, 2021 13:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marissaDubbelaar marissaDubbelaar marissaDubbelaar approved these changes

@ggabernet ggabernet ggabernet approved these changes

@maxulysse maxulysse Awaiting requested review from maxulysse maxulysse is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@FriederikeHanssen @maxulysse @ggabernet @marissaDubbelaar @nf-core-bot