feat: Install subworkflows with modules from different remotes #3083

jvfe · 2024-07-29T11:33:13Z

PR checklist

This comment contains a description of changes (with reason)
CHANGELOG.md is updated
If you've fixed a bug or added code that should be tested, add tests!
Documentation in docs is updated

Addresses #1927 and #2497

Issue information

Currently, get_components_to_install is the function used to download a subworkflow's modules. It relies on simple text parsing to define where to get a subworkflow/module from and install it.

As outlined in the above issues (and in slack conversations), it is currently not possible to install subworkflows which depend on modules that originate from different remotes. The solution is to copy over required modules to a same remote in order to use them.

Implementation

At this moment the implementation is mostly a work-around/hack. I aimed to make the least possible modifications to the source code in order to make the intended behavior functional. Here's how it works:

It still uses the same text parsing for all modules and subworkflows that don't have a meta.yml file defined.
If a subworkflow defines a meta.yml, they can optionally define the git_remote and org_path keys for a component. This is based on a suggestion given on the original issue. E.g.:

components:
  - name: gatk4/mutect2
  - name: gatk4/genomicsdbimport
    org_path: custom_remote
    git_remote: https://github.com/custom_remote/modules.git

This info is used to grab the respective modules from the specified remote.
I've setup a dummy repository with a subworkflow that showcases this. If no remote is specified, it assumes the module is from the same remote as the subworkflow (as it is currently).

Trying to run nf-core subworkflows --git-remote https://github.com/jvfe/test-subworkflow-remote install get_genome_annotation with this branch of nf-core/tools should work. There's a basic test I've setup to test for this.

Known issues

One of the hackier parts is that I'm having to redefine ModulesRepo from inside ComponentInstall. This is definitely not ideal, so I'd appreciate any suggestions on how to improve this logic.

All in all, I'm eager to hear what everyone thinks. I'm tagging @muffato who has been supervising and helping me in this implementation.

Strangely it is passing at the moment, it seems the install command returns a passing exit code regardless of installing all modules. - Addresses #2

This reverts commit 534da92.

tests: Add test case for cross-organization subwf

fix: Avoid duplicate downloads in nested subworkflows

* dev: (299 commits) Update nf_core/pipelines/create/utils.py [automated] Update CHANGELOG.md allow numbers in custom pipeline name [automated] Update CHANGELOG.md Update pre-commit hook pre-commit/mirrors-mypy to v1.11.1 Fixed linting Updated changelog Added process_high_memory to create/lint list update chagelog use pathlib instead of os.path Apply suggestions from code review add option to skip code linters Update gitpod/workspace-base Docker digest to f189a41 Update pre-commit hook pre-commit/mirrors-mypy to v1.11.0 [automated] Update CHANGELOG.md Update python:3.12-slim Docker digest to 740d94a Update .pre-commit-config.yaml Update nf_core/__main__.py Add default version Automatically add a default version in pipelines_create ...

awgymer · 2024-08-07T15:59:16Z

I'm very wary of increasing the idiosyncrasies in the code and I feel that making it such that the code returns variably either strings or dicts and thus necessitates isinstance checks is liable to be difficult to follow/trace through the code in future.

I understand the desire to simply get something working but I think that if we are going to implement this and support it we need to do it right.

I think this also indicates support only for cross-organisational modules and not cross-organisational subworkflows?

muffato · 2024-08-08T16:17:21Z

I think this also indicates support only for cross-organisational modules and not cross-organisational subworkflows?

Currently, yes

I'm very wary of increasing the idiosyncrasies in the code and I feel that making it such that the code returns variably either strings or dicts and thus necessitates isinstance checks is liable to be difficult to follow/trace through the code in future.

Once get_components_to_install is updated to return dictionaries for sub-workflows too, all the isinstance(..., dict) will evaluate to true, and all their if statements will be simplified / collapse.

I understand the desire to simply get something working but I think that if we are going to implement this and support it we need to do it right.

It's a question of time management (@jvfe is working part time on this project). We're not sure about about the handling of the default org vs the module org in nf_core/components/install.py (cf the creation of ModulesRepo instances). We didn't want to invest deeper without clearing that first. Making the change in get_components_to_install is easy, but finding the other files that need an update and making sure that all the tests still pass takes a bit longer.

mirpedrol

Thanks for your contribution @jvfe!
I think this is a good start 😄 I agree with @awgymer that allowing component to be both a string or a dict is confusing and it will cause problems. But I like the idea of using the information in the meta.yml to obtain the remote form which the component should be installed.
My suggestion would be to convert component to a dictionary in all cases since the beginning, with the default being the current remote.

nf_core/components/components_utils.py

nf_core/components/install.py

tests/utils.py

* dev: (35 commits) don't remove creating a pipeline to test it 😶 update textual snapshots update textual snapshots fix indentation Update nf_core/pipelines/create/create.py [automated] Update CHANGELOG.md [automated] Update CHANGELOG.md add option to exclude changelog from custom pipeline template add option to exclude codespaces from pipeline template remove multiqc images from the template docs and the same for subworkflows run tests also after pytest-migrate update textual snapshots add multiqc potion to test data update textual snapshots fix tests [automated] Update CHANGELOG.md add option to exclude multiqc from pipeline template rename regenerae-snapshots to update-textual-snapshots Apply suggestions from code review ...

mashehu

Great work! Does it also work with nf-core subworkflows patch?

CHANGELOG.md

nf_core/components/components_utils.py

nf_core/components/install.py

nf_core/components/update.py

nf_core/modules/modules_json.py

Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>

mirpedrol · 2025-03-07T11:27:53Z

Does it also work with nf-core subworkflows patch?

I have tested that it works 👍 patch is independent of the included components so it should not be affected by these changes

awgymer · 2025-03-07T11:40:42Z

As I can see this PR is looking good and probably heading towards a merge I just want to gently raise my previous point

I'm unclear on what level of commitment to maintain and work around issues this introduces there is from an nf-core standpoint.

This PR adds functionality to make cross-repo subworkflows work but we are still lacking a serious CI testing solution and I'm worried about other footguns the functionality potentially opens up. I'm conscious that greater private repo support is something many (including myself!) want but I also want to ensure we achieve it with stability and long-term viability.

muffato · 2025-03-07T13:30:54Z

@awgymer What CI testing is missing ? This includes does include tests for the new cross-repo functionality. That's in fact how it was developed: @jvfe first wrote (failing) test cases and then implemented the code so that they pass.

awgymer · 2025-03-07T14:51:50Z

@awgymer What CI testing is missing ?

It's a fundamental limitation of this style of subworkflows that you cannot test them easily in their remotes on CI right now and this tools functionality does nothing to address that. Once again my issue is not so much with the actual code of this PR but rather the wider implications this PR has.

muffato · 2025-03-07T15:59:55Z

Ah right. Well, it sounds like it is a good idea for another internship/mentorship ;) a nf-core command to install missing cross-org modules so that tests can then run.

mashehu · 2025-03-10T12:09:41Z

how about a CI test, where we create a test pipeline and build a mixed subworkflow there?

muffato · 2025-03-10T13:20:32Z

It's there already: https://github.com/nf-core/tools/pull/3083/files#diff-60cea8a0cf3a284ef26219e6dc9b9b2b55ebb25214cb520b0ae38ef6179436d7R87

It uses modules/subworkflows across nf-core/modules and https://github.com/nf-core-test/modules

mirpedrol · 2025-03-18T16:53:20Z

Hello!
After a chat with the #infrastructure team, we think that this PR can be merged, as the missing CI testing is not affecting the nf-core modules repo.
A solution for this can be discussed afterwards. For instance, one option would be to create a pipeline, install the subworkflow, and test it there.

jvfe · 2025-03-19T13:09:56Z

Hi everyone sorry for my absence as of late, but I'm glad to see the feature getting some traction and coming closer to a merge!

A solution for this can be discussed afterwards. For instance, one option would be to create a pipeline, install the subworkflow, and test it there.

This seems like an adequate solution to me, although it'd be better implemented in a separate PR.

The current CI failures seem to be caused by the nf-schema plugin and don't seem related to the feature, so not sure if there's a proper solution from this side.

mirpedrol · 2025-03-19T15:22:38Z

The current CI failures seem to be caused by the nf-schema plugin and don't seem related to the feature, so not sure if there's a proper solution from this side.

Hi @jvfe, the fail is due to the edge release of Nextflow, the Nextflow team is working on a fix. Let's wait to see if they fix it soon, otherwise, I will consider merging without these tests (as they were passing before). Before the hackathon would be a good timing for people to test it out and find possible bugs

muffato · 2025-03-20T21:51:11Z

Thank you everyone !

jvfe added 19 commits June 21, 2024 15:42

tests: Add test case for cross-organization subwf

9825d89

Strangely it is passing at the moment, it seems the install command returns a passing exit code regardless of installing all modules. - Addresses #2

tests: Check module on module list instead

568363a

chore: Dummy commit to test action

534da92

Revert "chore: Dummy commit to test action"

286ab86

This reverts commit 534da92.

tests: Change installed subworkflow

423517e

Merge pull request #6 from sanger-tol/cross-org-test

6816cc1

tests: Add test case for cross-organization subwf

feat: Read from meta.yml in get_components_to_install

af816fd

refact: Change module return type to always be dict

d0e6031

fix: Add correct return type

1737c88

fix: Use any for values

f7aaea2

test: Fix module key in across orgs

d1c490d

refact: Reset modules repo when git_remote not defined

6fa5cf4

refact: Copy parent attribute

d903282

refact: Keep old strategy as fallback

7f095fa

refact: Check component not in subwf list

012e9d6

refact: Change return type to optional

d77fccb

refact: Change way of handling dicts in modulesjson

faa7faf

refact: Handle dicts in meta_yml lint

0f08bbe

fix: Pass branch in install too

6d052de

jvfe changed the base branch from master to dev July 29, 2024 11:33

jvfe added 3 commits August 5, 2024 10:13

fix: Check if module in meta.yml is imported

c6ce67f

Merge pull request #9 from sanger-tol/fix/duplicated-downloads

792f68b

fix: Avoid duplicate downloads in nested subworkflows

nf-core deleted a comment from github-actions bot Aug 5, 2024

mirpedrol reviewed Aug 9, 2024

View reviewed changes

jvfe added 3 commits August 10, 2024 14:35

refact: Define org path based on git remote

fca6f27

feat: Allow defining branches in meta.yml

092bf91

Merge branch 'dev' into fix/1927

96139ac

mashehu modified the milestones: 3.4.0, 3.3.0 Feb 24, 2025

mashehu approved these changes Feb 24, 2025

View reviewed changes

Apply suggestions from code review

d513cca

Co-authored-by: Matthias Hörtenhuber <mashehu@users.noreply.github.com>

Merge branch 'dev' of https://github.com/nf-core/tools into fix/1927

ea4fbaa

use ruamel.yaml

2acd2ff

mirpedrol added 4 commits March 7, 2025 14:35

fix typing

f6a6ab2

fix using ruamel.yaml

b98ac7e

fix test_environment_variables_override test

bb2e433

fix typing for python 3.9

822fe08

Merge branch 'dev' into fix/1927

9c9a2a7

mirpedrol merged commit 6de7d93 into nf-core:dev Mar 20, 2025
94 checks passed

github-project-automation bot moved this to Done in nf-core infrastructure projects Mar 20, 2025

github-project-automation bot moved this from In Progress to Done in nf-core module & sub-workflow sharing across organizations Mar 20, 2025

buehlere mentioned this pull request Apr 3, 2025

Subworkflows with nf-core / msk modules mskcc-omics-workflows/modules#199

Open

2 tasks

eparisis mentioned this pull request May 27, 2025

Is this usable for subworkflows too? nf-core/modules-template#4

Closed

muffato deleted the fix/1927 branch June 2, 2025 13:50

prototaxites mentioned this pull request Jun 23, 2025

Update instructions for cross-org components sanger-tol/nf-core-modules#44

Open

feat: Install subworkflows with modules from different remotes #3083

feat: Install subworkflows with modules from different remotes #3083

Uh oh!

Conversation

jvfe commented Jul 29, 2024 • edited by anoronh4 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Issue information

Implementation

Known issues

Uh oh!

awgymer commented Aug 7, 2024

Uh oh!

muffato commented Aug 8, 2024

Uh oh!

mirpedrol left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mashehu left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mirpedrol commented Mar 7, 2025

Uh oh!

awgymer commented Mar 7, 2025

Uh oh!

muffato commented Mar 7, 2025

Uh oh!

awgymer commented Mar 7, 2025

Uh oh!

muffato commented Mar 7, 2025

Uh oh!

mashehu commented Mar 10, 2025

Uh oh!

muffato commented Mar 10, 2025

Uh oh!

mirpedrol commented Mar 18, 2025

Uh oh!

jvfe commented Mar 19, 2025

Uh oh!

mirpedrol commented Mar 19, 2025

Uh oh!

Uh oh!

muffato commented Mar 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

jvfe commented Jul 29, 2024 •

edited by anoronh4

Loading

muffato commented Mar 20, 2025 •

edited

Loading