-
Notifications
You must be signed in to change notification settings - Fork 709
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
Prototype implementation for new Nextflow native DSL2 syntax on nf-core #701
Conversation
Tested with:
Directory tree is exactly the same as Major changes to remember/check for rebasing:
Additional suggestion.
|
Ready for review. |
Dev -> Master for 3.4 release
Updates from 3.4 incorporated. |
|
Mannn all of these Related to nextflow-io/nextflow#2072 |
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.
Having now looked at it properly. This really looks awesome @mahesh-panchal 🤩 Let's wait for some more opinions. Maybe we can get this PR as it is into a separate branch on this repo? Seeing the diff
with stuff commented out may be useful for others that want to switch.
Once this PR is in then we can systematically strip out all of the comments and tidy stuff up to get it release ready. I don't see any limitations at the moment providing we have ticked all of the boxes with the older implementation in terms of publishing files and passing options around. You also mentioned that the tree
looks the same before and after so that suggests we are on the right track.
Lemme know what you think. I am feeling optimistic that we may even be able to start the proess of converting modules/pipelines to this format during the Hackathon.
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.
@mahesh-panchal, you're a genius!
This addresses most of the points I raised in nf-core/tools#1163.
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.
Awesome @mahesh-panchal ! 😍
I really think this PR fits perfectly Nextflow philosophy of keeping separated configuration from the workflow code which is one of the reasons that makes Nextflow so powerful. I agree that we should move to this new implementation 🚀
The only thing left we need to decide is whether to use the |
@mahesh-panchal are we able to dump the more "controversial" aspects of this implementation that have been raised here somewhere so we can maybe have more focussed discussions around those? e.g. this, this, this etc. A TODO list of things we need to get sorted to implement it would be useful too (e.g. updating nf-core/modules, module template, pipeline template, |
Use publish dir but with no functions.nf
I got an idea that we can keep passing opts using params too if we like:
Example:
output:
|
Add args override from params
Can someone check the json schema please for the |
New bug. Not sure if it's my fault in configuration.
Run with:
|
Test profile still throws these warnings with the fix in
These process are not run in the test profile, but there is configuration for them. |
What if we use conditional process selectors? 🤔 Is that even possible in a config file? So for example,
|
Yes. That's possible. That's effectively how we're allowing args from the command line:
I need to take a closer look at the code to see what conditionals are needed though. |
Absolutely fantastic effort @mahesh-panchal ! 👏🏽 |
Looks like this may have been a one-off fluke. I haven't been able to reproduce. |
Suggestion
Swap from the current method of publishing to the using publishDir directly in configs.
Benefits:
Additionally, use the
process.ext
directive to pass custom tool parameters and suffixes to processes.Code snippet also added to allow this to be updated via params. e.g.,
--process.FASTQC.args '--quiet'
.Also suggest removing
functions.nf
in every module and either replacing withOr removing entirely.
getSoftwareName
seems to be only really needed for the default publishing directory, andgetProcessName
could be included at the bottom of a module main.nf as it's such as a small function.PR checklist
nf-core lint
).nextflow run . -profile test,docker
).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).