ADR: Workflow modules#7342
Conversation
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
Signed-off-by: Ben Sherman <bentshermann@gmail.com>
✅ Deploy Preview for nextflow-docs ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
Addressing @pditommaso 's feedback in more detail:
ADR updated -- both process modules and workflow modules are now stored in the
The ADR does specify this, but I added an example of a workflow module's directory tree to make it more clear
Agreed. I added an example of inputs/outputs in the module spec. At the very least we need
ADR updated -- workflow modules now get their own
I kept this section here for now so that I can continue to refine the mapping rules. I will probably just move it back to #7208 |
|
|
||
| Note that `fastq_align_star` declares only its *direct* dependencies (`star/align` and `bam_sort_stats_samtools`) in `requires.modules`. The `samtools` processes are transitive dependencies of `bam_sort_stats_samtools` and are vendored within *its* own `modules` directory, not directly under `fastq_align_star`. Because each workflow module vendors its own dependencies, the same module may appear more than once in the tree when it is used by multiple workflows. | ||
|
|
||
| ### Workflow execution |
There was a problem hiding this comment.
TODO: clarify mapping rules for CLI params -> workflow takes, beyond the one example
Consider moving back to #7208
|
|
||
| Currently, nf-core enforces a flat module structure. nf-core modules are currently versioned by commit hash and workflows pin exact versions. There is no way to resolve conflicts between workflows. Instead, all nf-core workflows are kept in sync at publish time. This approach places the maintenance burden on *module developers* -- when updating a module, you must also update all consuming workflows. | ||
|
|
||
| Instead, the ADR avoids this problem entirely by allowing each workflow to have its own `modules` directory. The downside is that modules will be duplicated in the project, making the project larger and increasing the number of scripts to be parsed. |
There was a problem hiding this comment.
TODO: evaluate the impact on project size
It should be minimal if we install only the script+spec and exclude tests. In principle you don't need the tests in your pipeline if they were verified when the module was published
There was a problem hiding this comment.
We might also consider the impact on compile time, since Nextflow will end up parsing more scripts when duplication is allowed. But this should be minimal compared to overall runtime
| // workflow | ||
| include { FASTQ_ALIGN_STAR } from 'nf-core/fastq_align_star' |
There was a problem hiding this comment.
Transitive dependencies also complicates how we sync nf-core subworkflows to the registry. They have includes based on nf-core's directory structure (flat, separate modules vs subworkflows) whereas ideally they should use the canonical module name as shown here
Perhaps we can be clever and re-write these include statements when importing to the registry? Should be possible with some regex, or an AST transform is always an option
|
I have some differences about how modules are managed in workflows and pipelines. In modules, we propose to use the My doubt is why we do not apply the pipelines way for workflows? So, the version is pinned if the module is included in the folder; otherwise, use the latest. In this way, we will not need the The problem that I could see is if there are a big amount modules pinned that could pass the 1MB limit for the upload compressed artifact. |
|
Are you talking about bundling the transitive modules in the workflow module? i suppose we could do that with just a directory tree of The version pinning approach we took for modules was mostly a way to avoid depending on the pipeline spec since we don't have it yet. Once we have the pipeline spec, I could see us moving the version pins to there. So I wouldn't worry too much about consistency there |
Yes, I was thinking it also to simplify the import of nf-core subworkflow. But it can make publisehd subworkflows unreliable. I think we can extract the nf-core modules versions from the git commit. |
Yeah... my experience of this from working closely with If it's about concordance with the way modules operate currently I'd argue it would be better to just make people have to pin modules too.
FWIW I tested running a simple EDIT: Of course this would probably require the top-level manifest to carry the full nested module tree of dependencies because otherwise it would become tricky to update a nested workflow module. |
Not sure if I have really get your point. Do you want to update the dependencies of an already installed workflow (locally) or update the dependencies of a published workflow in the remote server? For the first one, I think you can do it with the current |
| requires: | ||
| nextflow: ">=24.04.0" | ||
| modules: | ||
| - nf-core/star/align@0.0.0-4e3e10e |
There was a problem hiding this comment.
is requires.modules just allowing to pin a version or are we going to support ranges or constraints (< 2.0.0)
There was a problem hiding this comment.
My inclination is to not implement version ranges for v1, since we can always add it later
It depends on whether other module namespaces will want to use version ranges or if they will want to be like nf-core
|
The first. One issue right now is that My issue with needing to do
|
| license: MIT | ||
| requires: | ||
| nextflow: ">=24.04.0" | ||
| modules: |
There was a problem hiding this comment.
Regarding this, is it required to validate if the required modules have been published before, or is it a workflow developer's responsibility? Ideally, it we will not publish incomplete workflows or with mistakes in the dependencies. However, it could be tricky because there are workflows that can be published in a registry depending on modules stored in other registries, and the dependency resolution will depend on the local configuration os the user (repositories set in registry.urls). For instance, an organization that is hosting its own registry used to store workflows that depend on modules in the public registry.
There was a problem hiding this comment.
Asking Claude, seems both options are used and depends on the registries architecture. It seems cargo checks because they have the central (crates.io). However, Maven does not check dependencies and the error is discovered at install time.
There was a problem hiding this comment.
As a user I think it would be a much worse experience if the validation didn't happen at publish time. I think it's also worth considering that nextflow has so far been a primarily bioinformatics platform and that many users are not necessarily coming from engineering backgrounds and the friction of failing installs and/or not providing publish-time checks could carry more risk than in some languages
There was a problem hiding this comment.
I figured we would validate this at publish time. Publishing a workflow module should require that all module dependencies are already published in the same registry
Cross-registry deps can be handled for now by mirroring the public registry into the private one
|
|
||
| When a workflow is included, it is vendored into the including project under `modules/<scope>/<name>/`. Included workflows should be committed to the including repository. | ||
|
|
||
| Each workflow module should store its dependencies within its own `modules` directory. This way, two workflows can use different versions of the same module without introducing a version conflict. Dependencies are not bundled with the workflow module -- they are installed when the workflow module is installed. |
There was a problem hiding this comment.
@awgymer pulling your question into a thread:
One issue right now is that
nf-coredoes not have "real" versioning of modules. Its arbitrary git commits and a different "version" might mean a logic change or it might mean formatting, or a container which can be overridden in config anyway - or any combination of these.
My issue with needing to do
modules installin arbitrarily nested locations is:
- highly manual per-module
- not traceable
- prone to human error
In theory, I think you would be able to edit the requires.modules section of an installed workflow module, changing versions to your liking, then run nextflow modules install/update once in the project root and Nextflow would recursively install modules to align it with the module specs. Nextflow would just warn about locally-modified modules for certain operations
This is different from how we currently manage modules though, since we don't have a pipeline spec to pin module versions right now but workflow modules will pin their dependencies. So we'll have to think about this more
|
|
||
| Workflow modules, unlike process modules, can include other modules (both processes and workflows). As a result, we must address the *diamond dependency problem* -- what happens when two workflows depend on different versions of the same module? | ||
|
|
||
| Currently, nf-core enforces a flat module structure. nf-core modules are currently versioned by commit hash and workflows pin exact versions. There is no way to resolve conflicts between workflows. Instead, all nf-core workflows are kept in sync at publish time. This approach places the maintenance burden on *module developers* -- when updating a module, you must also update all consuming workflows. |
There was a problem hiding this comment.
@awgymer this is the main assumption I wanted to check from the nf-core side
My sense is that module developers will not want to deal with semantic versioning and version ranges across thousands of modules, which is why we are going down this route of just duplicating the modules in every workflow
I'm curious which trade-off you think folks would prefer?
3 participants
Spun out from #7213
This PR proposes an ADR for workflow modules -- extending the module system to support both processes and workflows.
Updated based on latest round of feedback