ADR: Pipeline composition - #7213
Conversation
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
|
Great write up, thanks for this Ben! As you might expect, I'm most concerned about the params. You characterise it as a one-off cost which is mitigated by LLMs, however that doesn't take into account updates to included pipelines (a core functionality with included modules). The I'd still love to look into how we could bulk import nested config and apply it at root level. Even if it is a separate import + apply mechanism (eg. like config profiles in a sense?). I think without it, the use of the meta pipeline functionality is substantially limited. |
Agreed. Feel like we need some sort of auto-import of the params of child workflows, so e.g. they appear automatically in Platform, and I could say e.g. Then some auto-assembly of docs as well. Basically we need to standardise at the nextflow level where a bunch of the non-nextflow pieces need to live. |
|
|
||
| The Nextflow-in-Nextflow approach treats the included pipeline as a *black box* -- it preserves the exact pipeline behavior (core workflow + entry workflow + config) while forfeiting dataflow composition (separate dataflow graphs). | ||
|
|
||
| An ideal solution might combine the best of both: compose pipelines into a single dataflow graph (white box) while inheriting each pipeline's params, outputs, and config so they need not be replicated (black box). We considered such a model, where an included pipeline contributes its shell as namespaced, overridable defaults, but rejected it. Dataflow composition fundamentally requires exposing the core workflow as a set of channel ports, so the white-box mechanism is unavoidable; inheritance would only layer implicit behavior on top of it. That behavior comes at a steep cost: it relocates a one-time *write* cost (boilerplate) into a recurring *read* cost (hidden defaults, auto-bound arguments, auto-published outputs), burdens every tool that must now understand it (linter, type checker, config resolution, resume), and conflicts with the frozen-island philosophy that otherwise governs vendored code. |
There was a problem hiding this comment.
I agree with this. The added complexity is enormous.
There was a problem hiding this comment.
@ewels @pinin4fjords @adamrtalbot
Pulling everyone into this thread to talk about auto-inheritance
As you might expect, I'm most concerned about the params. You characterise it as a one-off cost which is mitigated by LLMs, however that doesn't take into account updates to included pipelines (a core functionality with included modules). The params drift with updates would be dangerous and a constant source of dev work.
That's fair, but not my main point. The core problem is this -- if you want to preserve dataflow concurrency between pipelines, then you can't really just auto-import params into the meta-pipeline. You have to define which params are replaced with inter-pipeline wiring vs exposed to the top-level. That amounts to just writing the meta-workflow.
The development overhead is what it is. I suggest the AI skill just as an idea. I'm sure it could also handle updates. All of that is better than having loads of hidden behavior that makes the meta-pipeline impossible to reason about
I'd still love to look into how we could bulk import nested config and apply it at root level. Even if it is a separate import + apply mechanism (eg. like config profiles in a sense?). I think without it, the use of the meta pipeline functionality is substantially limited.
Not sure I understand this point. Most of the config is just standard boilerplate, so it doesn't make sense to auto-import it because you will just get lots of duplicate config
Unless you are talking about ext config. That will depend on whether we can move the default ext settings into the process definition
There was a problem hiding this comment.
Building on what Adam said:
In a scenario where I update my workflow from v1.1 to v1.2, an update to params should be explicit in the input block, not implicit and I hope it doesn't change too much.
The nice thing about an explicit meta-pipeline definition is that when I update the included pipeline, the linter / language server will immediately pick up on any inconsistencies, because it's just regular code. I'm not sure the tooling would be able to do that if there was a lot of implicit behavior
| } | ||
|
|
||
| // perform RNAseq analysis | ||
| multiqc_report = NFCORE_RNASEQ( ch_samples ) |
There was a problem hiding this comment.
Side note - I would remove MultiQC from all nf-core pipelines and put them in the metapipelines, i.e. no MultiQC repeats, but that's a matter of opinion.
FETCHNGS(ch_inputs)
RNASEQ(fetchngs.out)
MULTIQC(RNASEQ.out.qc_files)There was a problem hiding this comment.
I was wondering about that. Wasn't sure if you would want a meta-pipeline to produce one multiqc report per pipeline or just one for the whole thing
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
@adamrtalbot agreed, I never said global. I would love it if the pipeline config is imported within a dedicated scope and treated as a baseline default. Then the import-ing pipeline can override anything, but doesn't need to duplicate config that isn't being changed. Doing this would not be trivial. The only way I can think of is to do something fairly radical like rendering the config at import time and saving that to a locked config file somewhere. Or some other crazy mechanism. |
Config or params? In my mind they are very different concepts, I was referring to parameters here. |
I agree with this. They're all workflows*, the only thing that separates a "pipeline" from a subworkflow is perception. *except the anonymous entry workflow, which is where the sticky point about params and config comes in 😉 |
Ideally params, but might need to be config for all the
Yeah as it stands I think this basically boils down to the functionality we already have with |
Can the nf-core tooling install a workflow from a pipeline repo? e.g. NFCORE_RNASEQ from nf-core/rnaseq? I think that is the main thing that this ADR adds |
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Version 1.1
Next stepsGonna keep exploring ways to minimize overhead for params and publishing (thread) |
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
meta-pipeline boilerplate Signed-off-by: Ben Sherman <bentshermann@gmail.com>
| ### Reducing params/output boilerplate | ||
|
|
||
| In the example above, the meta-pipeline re-declares the params and outputs from each included pipeline. This boilerplate can be avoided by importing each pipeline's `params` block and `output` block as *record types*: |
There was a problem hiding this comment.
@pinin4fjords @adamrtalbot @ewels
Here is my latest attempt at a "best of both worlds" where the meta-pipeline is a unified Nextflow pipeline but doesn't require tons of developer overhead
Basically we allow the params block and output block to be imported like record types so that you only have to redeclare one param per pipeline, and you get all the defaults baked in
There was a problem hiding this comment.
YES. Sweet sweet sugary goodness.
There was a problem hiding this comment.
So params become a special type of input? Syntactic sugar over this:
take:
RnaseqParams: Map = params
fastqs: List[path]
// etcThere was a problem hiding this comment.
given a params block like this:
params {
input: Channel<Sample>
aligner: String = 'star_salmon'
fasta: Path
}importing it as RnaseqParams essentially gives you this:
record RnaseqParams {
input: Channel<Sample>
aligner: String = 'star_salmon'
fasta: Path
}importing the entry workflow gives you something like this:
workflow NFCORE_RNASEQ {
take:
params: RnaseqParams
// ...
emit:
output: RnaseqOutput
}Signed-off-by: Ben Sherman <bentshermann@gmail.com>
This comment was marked as resolved.
This comment was marked as resolved.
|
Nice work on this — the design is coherent and the doc is refreshingly honest about its own sharp edges (the alternatives section and the best-practices caveats do a lot of the reviewer's work). Reusing 1. The config gap needs real exploration, not a footnote. 2. 3. The params→channel binding needs a general model. 4. Consider a dedicated General suggestions
Overall I'm in favor of the direction — dataflow composition over nested execution is the right choice. The above are mostly about making the contract explicit and being honest that config/environment is the hard part. |
|
It looks like we're all trending towards two ADRs: workflow inclusion and pipeline composition. |
|
Thanks @pditommaso for the reviews. I think we have converged on an approach, and I mostly just need to clarify some details in the ADRs At this point I will move the workflow modules ADR into a separate PR, refine it based on your feedback, and begin implementation I will also keep the workflow execution piece separate in a third PR (#7208) since it is not essential to meta-pipelines and deserves more dedicated treatment on the params -> take mapping The syntax for pipeline composition can also be implemented independently of everything else, since a user could just copy+paste a pipeline into a meta-pipeline. But the full solution will require (1) workflow modules in the registry and (2) pipelines in the registry |
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
|
Moved workflow modules ADR to #7342 Will update pipeline composition ADR soon based on latest feedback |
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
|
ADR updated to focus on "pipeline composition" as the core feature. Remote pipeline inclusion is treated as a secondary goal. Responding to Paolo's feedback in detail:
I added a blurb about which pieces of config likely need to be reconstructed. I can try to lay out a more concrete example later. The main prerequisite is that a pipeline will need to adopt the Even The rest of the config -- anything outside of
Updated the appendix example to use
As the ADR states, this behavior is described in the workflow modules ADR. I will make sure to expand on the binding rules there. But it is the same behavior as used for executing a named workflow.
I don't think users will want to have to define their params twice. But let's see how this evolves. |
|
What are your thoughts on this kind of structure where the container is dictated by the input. https://github.com/mahesh-panchal/nextflow-quarto-website/blob/main/main.nf In this example the module is the same, but one aliased module requires a container with R, while the other requires a container with Julia. The default container can't be set in the process definition and must be via the config in this case. |
| **Configuration** | ||
|
|
||
| Since each included pipeline is just part of the dataflow graph, configuration works like normal. Processes in an included pipeline can be targeted via config selector: | ||
|
|
||
| ```groovy | ||
| process { | ||
| withName: 'NFCORE_FETCHNGS:.*:SRATOOLS_FASTERQDUMP' { | ||
| cpus = 6 | ||
| memory = 24.GB | ||
| } | ||
| withName: 'NFCORE_RNASEQ:.*:STAR_ALIGN' { | ||
| cpus = 12 | ||
| memory = 72.GB | ||
| } | ||
| } | ||
| ``` |
There was a problem hiding this comment.
If we copy+paste the pipeline configuration into the repo on import and add an includeConfig "pipelines/nf-core/rnaseq/nextflow.config statement, this would automatically include the config from nf-core/rnaseq and make a lot of people happy?
There was a problem hiding this comment.
One thing that may cause issues here is then the priority of patterns and simple names potentially causing issues applying to other processes (and where those includeConfigs are placed will also determine priority), and there's also that issue where the actual process name configuration applies too all processes even if they've been aliased. It's a can of worms.
There was a problem hiding this comment.
I think Adam basically covered it. You can either provide the container as a process input, or you can copy/include the config manually in the meta-pipeline (and make sure the selectors are specific enough, etc). Pick your poison
Either way, I don't think we can make config travel automatically with a pipeline/module because the config is a parallel system
There was a problem hiding this comment.
Either way, I don't think we can make config travel automatically with a pipeline/module because the config is a parallel system
Would it be possible to restrict withName and withLabel directives to a certain workflow prefix when doing composition ?
A (sub/)workflow could ship its own nextflow.config with all the process {} config it needs for itself, possibly with very loose withName: SAMTOOLS_SORT rules.
When imported, even via include {worflow as IMPORTED_WF} ..., its nextflow.config would be considered and only applied to the subset of the entire pipeline.
It seems like we always end up battling two conflicting things:
In this example, the workflow isn't self contained but reliant on a config file. So any Nextflow based solution could either magically auto import the config file in, or the author has to fully encapsulate the workflow before it can be cleanly imported. I think this would be an effective compromise, allowing you to automatically include a config file from a repo, but it would often break things 😱
It's a little unrelated to the main ADR, but I disagree with this, you could:
For whatever reason, you may not want to do one of these but the tradeoff is not being fully importable. |
But this is basically what we're allowed to do. We use what's given to us, and implement what's simplest/makes sense to us. |
Sorry, I'm not following, isn't that agreeing what I said? We want the ease of use of the config system, but the flexibility of the config system makes it much harder to include a pipeline within another pipeline? |
@mahesh-panchal you can set the container based on a process input and supply it from a param: process HELLO {
container container_image
input:
val container_image
// ...
}include { HELLO as HELLO_1 } from './hello.nf'
include { HELLO as HELLO_2 } from './hello.nf'
workflow {
HELLO_1( params.container_image_1 )
HELLO_2( params.container_image_2 )
} |
This PR adds an ADR for remote pipeline inclusion, aka "meta-pipelines".
It describes an approach for including remote pipelines into a meta-pipeline in a way that preserves dataflow concurrency between pipeline inputs/outputs.
It discusses alternative approaches such as pipeline chaining / nf-cascade and why they don't satisfy certain use cases (preserving dataflow concurrency).
It also walks through a basic example of fetchngs -> rnaseq.