Remove lib directory and replace with atomic subworkflows #213
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
adamrtalbot
reviewed
Oct 11, 2023
Comment on lines
+60
to
+61
| include { PIPELINE_INITIALISATION } from './subworkflows/local/nf_core_fetchngs_utils' | ||
| include { PIPELINE_COMPLETION } from './subworkflows/local/nf_core_fetchngs_utils' |
There was a problem hiding this comment.
Include statements should be near the top in one consistent place (i.e. like other programming languages).
| ======================================================================================== | ||
| */ | ||
|
|
||
| workflow NEXTFLOW_PIPELINE_UTILS { |
There was a problem hiding this comment.
Much better name than auxiliary 👍
Comment on lines
+74
to
+84
| def dumpParametersToJSON(output_directory) { | ||
| def output_d = new File("${output_directory}/pipeline_info/") | ||
| if (!output_d.exists()) { | ||
| output_d.mkdirs() | ||
| } | ||
|
|
||
| def timestamp = new java.util.Date().format( 'yyyy-MM-dd_HH-mm-ss') | ||
| def output_pf = new File(output_d, "params_${timestamp}.json") | ||
| def jsonStr = JsonOutput.toJson(params) | ||
| output_pf.text = JsonOutput.prettyPrint(jsonStr) | ||
| } |
There was a problem hiding this comment.
How about parameterising the output path directly rather than trying to create a new file with a fixed name:
!! UNTESTED !!
Suggested change
| def dumpParametersToJSON(output_directory) { | |
| def output_d = new File("${output_directory}/pipeline_info/") | |
| if (!output_d.exists()) { | |
| output_d.mkdirs() | |
| } | |
| def timestamp = new java.util.Date().format( 'yyyy-MM-dd_HH-mm-ss') | |
| def output_pf = new File(output_d, "params_${timestamp}.json") | |
| def jsonStr = JsonOutput.toJson(params) | |
| output_pf.text = JsonOutput.prettyPrint(jsonStr) | |
| } | |
| def dumpParametersToJSON(output) { | |
| def output_pf = new File(output) | |
| def output_d = output.getParent() | |
| if (!output_d.exists()) { | |
| output_d.mkdirs() | |
| } | |
| def timestamp = new java.util.Date().format( 'yyyy-MM-dd_HH-mm-ss') | |
| def jsonStr = JsonOutput.toJson(params) | |
| output_pf.text = JsonOutput.prettyPrint(jsonStr) | |
| } |
There was a problem hiding this comment.
You would then need to use
def outfile = ${output_directory}/pipeline_info/params_${timestamp}.json
dumpParametersToJSON(outfile)
Comment on lines
+29
to
+48
| NEXTFLOW_PIPELINE_UTILS ( | ||
| params.version, | ||
| true, | ||
| params.outdir, | ||
| workflow.profile.tokenize(',').intersect(['conda', 'mamba']).size() >= 1 | ||
| ) | ||
|
|
||
| // | ||
| // Validate parameters and generate parameter summary to stdout | ||
| // | ||
| def pre_help_text = nfCoreLogo(getWorkflowVersion()) | ||
| def post_help_text = '\n' + workflowCitation() + '\n' + dashedLine() | ||
| def String workflow_command = "nextflow run ${workflow.manifest.name} -profile <docker/singularity/.../institute> --input ids.csv --outdir <OUTDIR>" | ||
| NF_VALIDATION_PLUGIN_UTILS ( | ||
| params.help, | ||
| workflow_command, | ||
| pre_help_text, | ||
| post_help_text, | ||
| params.validate_params | ||
| ) |
There was a problem hiding this comment.
It feels like there's a lot going on here, it's difficult for my small brain to follow. Can we simplify the interface at all?
Comment on lines
+114
to
+130
| workflow.onComplete { | ||
| if (email || email_on_fail) { | ||
| completionEmail(summary_params) | ||
| } | ||
|
|
||
| completionSummary() | ||
|
|
||
| if (hook_url) { | ||
| imNotification(summary_params) | ||
| } | ||
|
|
||
| if (input_type == 'sra') { | ||
| sraCurateSamplesheetWarn() | ||
| } else if (input_type == 'synapse') { | ||
| synapseCurateSamplesheetWarn() | ||
| } | ||
| } |
There was a problem hiding this comment.
A controversial take, but we could move this to nextflow.config...(see the note at the end): https://www.nextflow.io/docs/edge/metadata.html#decoupling-metadata
| include { CUSTOM_DUMPSOFTWAREVERSIONS } from '../../../modules/nf-core/custom/dumpsoftwareversions' | ||
| include { NEXTFLOW_PIPELINE_UTILS; getWorkflowVersion } from '../nextflow_pipeline_utils' | ||
| include { NF_VALIDATION_PLUGIN_UTILS } from '../nf_validation_plugin_utils' | ||
| include { NF_CORE_PIPELINE_UTILS; workflowCitation; nfCoreLogo; dashedLine; completionEmail; completionSummary; imNotification } from '../nf_core_pipeline_utils' |
There was a problem hiding this comment.
I'm not sure I like the mixing of functions and subworkflows in the same include statement. Since NF_CORE_PIPELINE_UTILS just runs the check Conda function, why not import that directly and use it? Simpler, easier to understand. We can always wrap it back up later.
There was a problem hiding this comment.
Agreeing with @adamrtalbot on that, I'd rather we have each on a single line
Comment on lines
+11
to
+16
| workflow NF_CORE_PIPELINE_UTILS { | ||
|
|
||
| main: | ||
| checkConfigProvided() | ||
|
|
||
| } |
There was a problem hiding this comment.
This feels a bit superfluous.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.