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

modify template to use nf-validation plugin #1771

Closed
wants to merge 66 commits into from

Conversation

mirpedrol
Copy link
Member

@mirpedrol mirpedrol commented Aug 29, 2022

Closes #1543

The validation of nextflow_schema.json files from nf-core pipelines is now part of a core Nextflow plugin: nextflow-io/nf-validation#5

Test in nf-core/testpipeline

Functions can be imported to a pipeline with include { paramsHelp, paramsSummaryMap, paramsSummaryLog } from 'plugin/nf-validation'

Schema validation code from the pipeline template NfcoreSchema.groovy and MainWorkflow.groovy is removed.

Validate a sample sheet with a JSON schema and create a channel from it. Replaces check_samplesheet subworkflow and code.

PR checklist

  • This comment contains a description of changes (with reason)
  • CHANGELOG.md is updated
  • If you've fixed a bug or added code that should be tested, add tests!
  • Documentation in docs is updated

mirpedrol added a commit to nf-core/testpipeline that referenced this pull request Feb 23, 2023
mirpedrol added a commit to nf-core/testpipeline that referenced this pull request Feb 23, 2023
@maxulysse
Copy link
Member

<3

@codecov
Copy link

codecov bot commented May 23, 2023

Codecov Report

Merging #1771 (15e3ec2) into dev (9d809b6) will decrease coverage by 0.18%.
The diff coverage is 100.00%.

❗ Current head 15e3ec2 differs from pull request most recent head d53a975. Consider uploading reports for the commit d53a975 to get more accurate results

@@            Coverage Diff             @@
##              dev    #1771      +/-   ##
==========================================
- Coverage   72.93%   72.75%   -0.18%     
==========================================
  Files          77       77              
  Lines        8446     8446              
==========================================
- Hits         6160     6145      -15     
- Misses       2286     2301      +15     
Impacted Files Coverage Δ
nf_core/lint/files_exist.py 78.00% <ø> (ø)
nf_core/lint/files_unchanged.py 72.61% <ø> (ø)
nf_core/lint/nextflow_config.py 77.45% <ø> (ø)
nf_core/schema.py 78.84% <100.00%> (ø)

... and 4 files with indirect coverage changes

@mirpedrol mirpedrol marked this pull request as ready for review May 26, 2023 11:14
@@ -11,6 +11,8 @@
- Move registry definitions out of profile scope ([#2286])(https://github.com/nf-core/tools/pull/2286)
- Remove `aws_tower` profile ([#2287])(https://github.com/nf-core/tools/pull/2287)
- Fixed the Slack report to include the pipeline name ([#2291](https://github.com/nf-core/tools/pull/2291))
- Remove shcema validation from `lib` folder and use Nextflow nf-validation plugin instead ([#1771](https://github.com/nf-core/tools/pull/1771/))
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
- Remove shcema validation from `lib` folder and use Nextflow nf-validation plugin instead ([#1771](https://github.com/nf-core/tools/pull/1771/))
- Remove schema validation from `lib` folder and use Nextflow nf-validation plugin instead ([#1771](https://github.com/nf-core/tools/pull/1771/))

// Schema validation default options
validationFailUnrecognisedParams = false
validationLenientMode = false
validate_params = true
Copy link
Contributor

Choose a reason for hiding this comment

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

Should we also make them camel-case?

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm doubting with validate_params. For schema_ignore_params you are right, it should be camelCase as it's part of the nf-validation plugin. But validate_params is only used in nf-core pipelines, so I think it should be snake_case?

Copy link
Member

Choose a reason for hiding this comment

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

I'd go for all snake_case

Copy link
Member

Choose a reason for hiding this comment

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

if it's params, then snake_case

Copy link
Member Author

Choose a reason for hiding this comment

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

Unfortunately the nf-validation plugin is using camelCase as nextflow uses this notation, and it's ment to be for all NF pipelines and not only nf-core.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, then ok for camelCase

@mirpedrol mirpedrol marked this pull request as draft June 1, 2023 10:40
@mirpedrol
Copy link
Member Author

Converting it to a draft again as, after some Slack discussions, we've decided to not include fromSamplesheet() yet.

MatthiasZepper and others added 28 commits June 5, 2023 11:18
…link from singularity to apptainer on the system.
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>
…onal for Tower downloads. Given that there is the option to provide the list of remote containers to skip their download, I agree that this is reasonable.
…revisions may also be branches. Therefore, I rewrote this function to account for revisions that are not releases.
@mirpedrol
Copy link
Member Author

I messed up this PR too much, so I've decided to open a new one #2300

@mirpedrol mirpedrol closed this Jun 5, 2023
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.

None yet

5 participants