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

nf-schema support #3116

Merged
merged 69 commits into from
Aug 27, 2024
Merged

nf-schema support #3116

merged 69 commits into from
Aug 27, 2024

Conversation

nvnieuwk
Copy link
Contributor

@nvnieuwk nvnieuwk commented Aug 14, 2024

PR features:

  • Minimal support for nf-schema (includes validation of draft 2020-12, definition notation checks...)
  • Update template schema
  • Check the pinning of the nf-schema version
  • Check for draft 2020-12 usage when nf-schema is used
  • A linting warning that advises the user to migrate to nf-schema
  • Check the plugin function imports are using nf-schema when nf-schema is specified in the config
  • we should add a "nf-validation" field in the templatefeatures.yml allowing one to opt-out of using nf-validation.
  • support nested parameters (Not doing this since this will no longer be supported in the future)

Features that are nice but not urgent (so probably not for this PR)

  • Check for the usage of the new config options instead of the parameters

@nvnieuwk nvnieuwk changed the title Feat/linting nf-schema nf-schema support Aug 14, 2024
nf_core/pipeline-template/nextflow.config Outdated Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Just a reminder that this should be added to the nf-core repo, maybe we can create a new subworkflow in case someone wants to continue using nf-validation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yep I made a PR to the modules repo with these changes :p these changes are not definitive yet

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Comment on lines 275 to 291
beforeText = """
-${colors.dim}----------------------------------------------------${colors.reset}-
${colors.green},--.${colors.black}/${colors.green},-.${colors.reset}
${colors.blue} ___ __ __ __ ___ ${colors.green}/,-._.--~\'${colors.reset}
${colors.blue} |\\ | |__ __ / ` / \\ |__) |__ ${colors.yellow}} {${colors.reset}
${colors.blue} | \\| | \\__, \\__/ | \\ |___ ${colors.green}\\`-._,-`-,${colors.reset}
${colors.green}`._,._,\'${colors.reset}
${colors.purple} ${manifest.name} ${manifest.version}${colors.reset}
-${colors.dim}----------------------------------------------------${colors.reset}-
"""
afterText = """${manifest.doi ? "* The pipeline\n" : ""}${manifest.doi.tokenize(",").collect { " https://doi.org/${it.trim().replace('https://doi.org/','')}"}.join("\n")}${manifest.doi ? "\n" : ""}
* The nf-core framework
https://doi.org/10.1038/s41587-020-0439-x

* Software dependencies
https://github.com/${manifest.name}/blob/master/CITATIONS.md
"""
Copy link
Member

Choose a reason for hiding this comment

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

should we lint for this in nf-core pipelines?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good idea, but I want to implement this in a better way as it's soooo ugly right now, I'm just not sure how.

I'll also add a chrome color filter to nf-schema if validation.monochromeLogs is set to true. This should simplify the configs a lot already (We can hardcode the monochrome values in the message together with the pipeline name and version, same thing for the citations) I'd love to have these values as plain strings without having to use any external variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think the current way will be the best I can do. This will also stay supported in Nextflow in the future

Copy link
Member

@mirpedrol mirpedrol left a comment

Choose a reason for hiding this comment

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

Nice! Only some last comments 😄

nf_core/pipeline-template/nextflow.config Outdated Show resolved Hide resolved
nf_core/pipelines/create/templatefeatures.yml Outdated Show resolved Hide resolved
nf_core/pipelines/lint/nextflow_config.py Outdated Show resolved Hide resolved
nf_core/pipelines/lint/plugin_includes.py Outdated Show resolved Hide resolved
@@ -36,8 +36,9 @@ def schema_description(self):
warned.append(f"Ungrouped param in schema: `{up}`")

# Iterate over groups and add warning for parameters without a description
for group_key in self.schema_obj.schema["definitions"].keys():
group = self.schema_obj.schema["definitions"][group_key]
defs_notation = self.schema_obj.defs_notation
Copy link
Contributor

Choose a reason for hiding this comment

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

don't see that this can be any other value than $defs in the code

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This can still be definitions if the pipeline still uses nf-validation

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't see that value written to in places different to the initialization and here

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's set everytime the schema of the PipelineSchema class is updated. This option is used heavily in schema.py and a bit in schema_description.py (or do you mean something else?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So it depends on which schema has been used

Copy link
Contributor

Choose a reason for hiding this comment

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

I am talking about self.schema_obj.defs_notation. where is that value set to something different than $defs?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

here:

self.defs_notation = "definitions"

basically if nf-validation is set in the config, then use definitions else use $defs

@nvnieuwk
Copy link
Contributor Author

Not sure what I can do about the snapshotting test 😁

@mirpedrol
Copy link
Member

Not sure what I can do about the snapshotting test 😁

You can update them by running pytest tests/pipelines/test_create_app.py --snapshot-update, only if the update makes sense 😄 which in this case does, I'm not sure why updating the report is failing, but if it were working you could see a report showing the changes like this:
image

@nvnieuwk
Copy link
Contributor Author

Thanks that did it! 🥳

@nvnieuwk nvnieuwk merged commit 7d23535 into nf-core:dev Aug 27, 2024
82 checks passed
@nvnieuwk nvnieuwk deleted the feat/linting-nf-schema branch August 28, 2024 07:41
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