refact: Add suggestions from the third round of reviews

nf_core/components/install.py

@@ -44,6 +44,7 @@ def __init__(
         self.force = force
         self.prompt = prompt
         self.sha = sha
+        self.current_sha = sha
         if installed_by is not None:
             self.installed_by = installed_by
         else:
@@ -57,6 +58,14 @@ def install(self, component: Union[str, Dict[str, str]], silent: bool = False) -
             self.modules_repo = ModulesRepo(remote_url, branch)
             component = component["name"]
 
+        if self.current_remote is None:
+            self.current_remote = self.modules_repo.remote_url
+
+        if self.current_remote == self.modules_repo.remote_url and self.sha is not None:
+            self.current_sha = self.sha
+        else:
+            self.current_sha = None
+
         if self.repo_type == "modules":
             log.error(f"You cannot install a {component} in a clone of nf-core/modules")
             return False
@@ -80,8 +89,8 @@ def install(self, component: Union[str, Dict[str, str]], silent: bool = False) -
             return False
 
         # Verify SHA
-        if not self.modules_repo.verify_sha(self.prompt, self.sha):
-            err_msg = f"SHA '{self.sha}' is not a valid commit SHA for the repository '{self.modules_repo.remote_url}'"
+        if not self.modules_repo.verify_sha(self.prompt, self.current_sha):
+            err_msg = f"SHA '{self.current_sha}' is not a valid commit SHA for the repository '{self.modules_repo.remote_url}'"
             log.error(err_msg)
             return False
 
@@ -124,7 +133,7 @@ def install(self, component: Union[str, Dict[str, str]], silent: bool = False) -
             modules_json.update(self.component_type, self.modules_repo, component, current_version, self.installed_by)
             return False
         try:
-            version = self.get_version(component, self.sha, self.prompt, current_version, self.modules_repo)
+            version = self.get_version(component, self.current_sha, self.prompt, current_version, self.modules_repo)
         except UserWarning as e:
             log.error(e)
             return False
@@ -209,15 +218,17 @@ def collect_and_verify_name(
         if component is None:
             component = questionary.autocomplete(
                 f"{'Tool' if self.component_type == 'modules' else 'Subworkflow'} name:",
-                choices=sorted(modules_repo.get_avail_components(self.component_type, commit=self.sha)),
+                choices=sorted(modules_repo.get_avail_components(self.component_type, commit=self.current_sha)),
                 style=nf_core.utils.nfcore_question_style,
             ).unsafe_ask()
 
         if component is None:
             return ""
 
         # Check that the supplied name is an available module/subworkflow
-        if component and component not in modules_repo.get_avail_components(self.component_type, commit=self.sha):
+        if component and component not in modules_repo.get_avail_components(
+            self.component_type, commit=self.current_sha
+        ):
             log.error(f"{self.component_type[:-1].title()} '{component}' not found in available {self.component_type}")
             print(
                 Panel(
@@ -233,9 +244,10 @@ def collect_and_verify_name(
 
             raise ValueError
 
-        if not modules_repo.component_exists(component, self.component_type, commit=self.sha):
-            warn_msg = f"{self.component_type[:-1].title()} '{component}' not found in remote '{modules_repo.remote_url}' ({modules_repo.branch})"
-            log.warning(warn_msg)
+        if self.current_remote == modules_repo.remote_url:
+            if not modules_repo.component_exists(component, self.component_type, commit=self.current_sha):
+                warn_msg = f"{self.component_type[:-1].title()} '{component}' not found in remote '{modules_repo.remote_url}' ({modules_repo.branch})"
+                log.warning(warn_msg)
 
         return component
 

nf_core/components/update.py

@@ -1,5 +1,6 @@
 import logging
 import os
+import re
 import shutil
 import tempfile
 from pathlib import Path
@@ -41,6 +42,8 @@ def __init__(
         limit_output=False,
     ):
         super().__init__(component_type, pipeline_dir, remote_url, branch, no_pull)
+        self.current_remote = remote_url
+        self.branch = branch
         self.force = force
         self.prompt = prompt
         self.sha = sha
@@ -92,6 +95,13 @@ def update(self, component=None, silent=False, updated=None, check_diff_exist=Tr
         Returns:
             (bool): True if the update was successful, False otherwise.
         """
+        if isinstance(component, dict):
+            # Override modules_repo when the component to install is a dependency from a subworkflow.
+            remote_url = component.get("git_remote", self.current_remote)
+            branch = component.get("branch", self.branch)
+            self.modules_repo = ModulesRepo(remote_url, branch)
+            component = component["name"]
+
         self.component = component
         if updated is None:
             updated = []
@@ -868,7 +878,17 @@ def get_components_to_update(self, component):
         if self.component_type == "modules":
             # All subworkflow names in the installed_by section of a module are subworkflows using this module
             # We need to update them too
-            subworkflows_to_update = [subworkflow for subworkflow in installed_by if subworkflow != self.component_type]
+            git_remote = self.current_remote
+            for subworkflow in installed_by:
+                if subworkflow != component:
+                    for remote_url, content in mods_json["repos"].items():
+                        if (all_subworkflows := content.get("subworkflows")) is not None:
+                            for _, details in all_subworkflows.items():
+                                if subworkflow in details:
+                                    git_remote = remote_url
+                if subworkflow != self.component_type:
+                    subworkflows_to_update.append({"name": subworkflow, "git_remote": git_remote})
+
         elif self.component_type == "subworkflows":
             for repo, repo_content in mods_json["repos"].items():
                 for component_type, dir_content in repo_content.items():
@@ -879,9 +899,9 @@ def get_components_to_update(self, component):
                             # We need to update it too
                             if component in comp_content["installed_by"]:
                                 if component_type == "modules":
-                                    modules_to_update.append(comp)
+                                    modules_to_update.append({"name": comp, "git_remote": repo, "org_path": dir})
                                 elif component_type == "subworkflows":
-                                    subworkflows_to_update.append(comp)
+                                    subworkflows_to_update.append({"name": comp, "git_remote": repo, "org_path": dir})
 
         return modules_to_update, subworkflows_to_update
 
@@ -896,7 +916,7 @@ def update_linked_components(
         Update modules and subworkflows linked to the component being updated.
         """
         for s_update in subworkflows_to_update:
-            if s_update in updated:
+            if s_update["name"] in updated:
                 continue
             original_component_type, original_update_all = self._change_component_type("subworkflows")
             self.update(
@@ -908,7 +928,7 @@ def update_linked_components(
             self._reset_component_type(original_component_type, original_update_all)
 
         for m_update in modules_to_update:
-            if m_update in updated:
+            if m_update["name"] in updated:
                 continue
             original_component_type, original_update_all = self._change_component_type("modules")
             try:
@@ -931,28 +951,42 @@ def update_linked_components(
     def manage_changes_in_linked_components(self, component, modules_to_update, subworkflows_to_update):
         """Check for linked components added or removed in the new subworkflow version"""
         if self.component_type == "subworkflows":
-            subworkflow_directory = Path(self.directory, self.component_type, self.modules_repo.repo_path, component)
+            org_path_match = re.search(r"(?:https://|git@)[\w\.]+[:/](.*?)/", self.current_remote)
+            if org_path_match:
+                org_path = org_path_match.group(1)
+
+            subworkflow_directory = Path(self.directory, self.component_type, org_path, component)
             included_modules, included_subworkflows = get_components_to_install(subworkflow_directory)
             # If a module/subworkflow has been removed from the subworkflow
             for module in modules_to_update:
-                if module not in included_modules:
+                module = module["name"]
+                included_modules_names = [m["name"] for m in included_modules]
+                if module not in included_modules_names:
                     log.info(f"Removing module '{module}' which is not included in '{component}' anymore.")
                     remove_module_object = ComponentRemove("modules", self.directory)
                     remove_module_object.remove(module, removed_by=component)
             for subworkflow in subworkflows_to_update:
-                if subworkflow not in included_subworkflows:
+                subworkflow = subworkflow["name"]
+                included_subworkflow_names = [m["name"] for m in included_subworkflows]
+                if subworkflow not in included_subworkflow_names:
                     log.info(f"Removing subworkflow '{subworkflow}' which is not included in '{component}' anymore.")
                     remove_subworkflow_object = ComponentRemove("subworkflows", self.directory)
                     remove_subworkflow_object.remove(subworkflow, removed_by=component)
             # If a new module/subworkflow is included in the subworklfow and wasn't included before
             for module in included_modules:
-                if module not in modules_to_update:
-                    log.info(f"Installing newly included module '{module}' for '{component}'")
+                module_name = module["name"]
+                module["git_remote"] = module.get("git_remote", self.current_remote)
+                module["branch"] = module.get("branch", self.branch)
+                if module_name not in modules_to_update:
+                    log.info(f"Installing newly included module '{module_name}' for '{component}'")
                     install_module_object = ComponentInstall(self.directory, "modules", installed_by=component)
                     install_module_object.install(module, silent=True)
             for subworkflow in included_subworkflows:
-                if subworkflow not in subworkflows_to_update:
-                    log.info(f"Installing newly included subworkflow '{subworkflow}' for '{component}'")
+                subworkflow_name = subworkflow["name"]
+                subworkflow["git_remote"] = subworkflow.get("git_remote", self.current_remote)
+                subworkflow["branch"] = subworkflow.get("branch", self.branch)
+                if subworkflow_name not in subworkflows_to_update:
+                    log.info(f"Installing newly included subworkflow '{subworkflow_name}' for '{component}'")
                     install_subworkflow_object = ComponentInstall(
                         self.directory, "subworkflows", installed_by=component
                     )
@@ -971,3 +1005,5 @@ def _reset_component_type(self, original_component_type, original_update_all):
         self.component_type = original_component_type
         self.modules_json.pipeline_components = None
         self.update_all = original_update_all
+        if self.current_remote is None:
+            self.current_remote = self.modules_repo.remote_url

tests/subworkflows/test_install.py

@@ -7,7 +7,6 @@
 
 from ..test_subworkflows import TestSubworkflows
 from ..utils import (
-    CROSS_ORGANIZATION_BRANCH,
     CROSS_ORGANIZATION_URL,
     GITLAB_BRANCH_TEST_BRANCH,
     GITLAB_REPO,
@@ -87,16 +86,29 @@ def test_subworkflows_install_different_branch_fail(self):
 
     def test_subworkflows_install_across_organizations(self):
         """Test installing a subworkflow with modules from different organizations"""
-        install_obj = SubworkflowInstall(
-            self.pipeline_dir, remote_url=CROSS_ORGANIZATION_URL, branch=CROSS_ORGANIZATION_BRANCH
-        )
-        # The hic_bwamem2 subworkflow contains modules from different organizations
-        install_obj.install("get_genome_annotation")
+        # The fastq_trim_fastp_fastqc subworkflow contains modules from different organizations
+        self.subworkflow_install_cross_org.install("fastq_trim_fastp_fastqc")
+        # Verify that the installed_by entry was added correctly
+        modules_json = ModulesJson(self.pipeline_dir)
+        mod_json = modules_json.get_modules_json()
+        assert mod_json["repos"][CROSS_ORGANIZATION_URL]["modules"]["nf-core-test"]["fastqc"]["installed_by"] == [
+            "fastq_trim_fastp_fastqc"
+        ]
+
+    def test_subworkflow_install_with_same_module(self):
+        """Test installing a subworkflow with a module from a different organization that is already installed from another org"""
+        # The fastq_trim_fastp_fastqc subworkflow contains the cross-org fastqc module, not the nf-core one
+        self.subworkflow_install_cross_org.install("fastq_trim_fastp_fastqc")
         # Verify that the installed_by entry was added correctly
         modules_json = ModulesJson(self.pipeline_dir)
         mod_json = modules_json.get_modules_json()
-        assert mod_json["repos"][CROSS_ORGANIZATION_URL]["modules"]["jvfe"]["wget"]["installed_by"] == [
-            "get_genome_annotation"
+
+        assert mod_json["repos"]["https://github.com/nf-core/modules.git"]["modules"]["nf-core"]["fastqc"][
+            "installed_by"
+        ] == ["modules"]
+
+        assert mod_json["repos"][CROSS_ORGANIZATION_URL]["modules"]["nf-core-test"]["fastqc"]["installed_by"] == [
+            "fastq_trim_fastp_fastqc"
         ]
 
     def test_subworkflows_install_tracking(self):

tests/subworkflows/test_remove.py

@@ -1,6 +1,7 @@
 from pathlib import Path
 
 from nf_core.modules.modules_json import ModulesJson
+from tests.utils import CROSS_ORGANIZATION_URL
 
 from ..test_subworkflows import TestSubworkflows
 
@@ -99,3 +100,27 @@ def test_subworkflows_remove_included_subworkflow(self):
         assert Path.exists(samtools_index_path) is True
         assert Path.exists(samtools_stats_path) is True
         self.subworkflow_remove.remove("bam_sort_stats_samtools")
+
+    def test_subworkflows_remove_subworkflow_keep_installed_cross_org_module(self):
+        """Test removing subworkflow and all it's dependencies after installing it, except for a separately installed module from another organisation"""
+        self.subworkflow_install_cross_org.install("fastq_trim_fastp_fastqc")
+        self.mods_install.install("fastqc")
+
+        subworkflow_path = Path(self.subworkflow_install.directory, "subworkflows", "jvfe")
+        fastq_trim_fastp_fastqc_path = Path(subworkflow_path, "fastq_trim_fastp_fastqc")
+        fastqc_path = Path(self.subworkflow_install.directory, "modules", "jvfe", "fastqc")
+        nfcore_fastqc_path = Path(self.subworkflow_install.directory, "modules", "nf-core", "fastqc")
+
+        mod_json_before = ModulesJson(self.pipeline_dir).get_modules_json()
+        assert self.subworkflow_remove_cross_org.remove("fastq_trim_fastp_fastqc")
+        mod_json_after = ModulesJson(self.pipeline_dir).get_modules_json()
+
+        assert Path.exists(fastq_trim_fastp_fastqc_path) is False
+        assert Path.exists(fastqc_path) is False
+        assert Path.exists(nfcore_fastqc_path) is True
+        assert mod_json_before != mod_json_after
+        # assert subworkflows key is removed from modules.json
+        assert CROSS_ORGANIZATION_URL not in mod_json_after["repos"].keys()
+        assert (
+            "fastqc" in mod_json_after["repos"]["https://github.com/nf-core/modules.git"]["modules"]["nf-core"].keys()
+        )

tests/subworkflows/test_update.py

@@ -11,10 +11,11 @@
 from nf_core.components.components_utils import NF_CORE_MODULES_NAME, NF_CORE_MODULES_REMOTE
 from nf_core.modules.modules_json import ModulesJson
 from nf_core.modules.update import ModuleUpdate
+from nf_core.subworkflows.install import SubworkflowInstall
 from nf_core.subworkflows.update import SubworkflowUpdate
 
 from ..test_subworkflows import TestSubworkflows
-from ..utils import OLD_SUBWORKFLOWS_SHA, cmp_component
+from ..utils import CROSS_ORGANIZATION_URL, OLD_SUBWORKFLOWS_SHA, cmp_component
 
 
 class TestSubworkflowsUpdate(TestSubworkflows):
@@ -372,3 +373,22 @@ def test_update_change_of_included_modules(self):
         assert "ensemblvep" not in mod_json["repos"][NF_CORE_MODULES_REMOTE]["modules"][NF_CORE_MODULES_NAME]
         assert "ensemblvep/vep" in mod_json["repos"][NF_CORE_MODULES_REMOTE]["modules"][NF_CORE_MODULES_NAME]
         assert Path(self.pipeline_dir, "modules", NF_CORE_MODULES_NAME, "ensemblvep/vep").is_dir()
+
+    def test_update_subworkflow_across_orgs(self):
+        """Install and update a subworkflow with modules from different organizations"""
+        install_obj = SubworkflowInstall(
+            self.pipeline_dir,
+            remote_url=CROSS_ORGANIZATION_URL,
+            sha="9627f4367b11527194ef14473019d0e1a181b741",
+        )
+        # The fastq_trim_fastp_fastqc subworkflow contains the cross-org fastqc module, not the nf-core one
+        install_obj.install("fastq_trim_fastp_fastqc")
+
+        update_obj = SubworkflowUpdate(
+            self.pipeline_dir,
+            remote_url=CROSS_ORGANIZATION_URL,
+            update_all=False,
+            update_deps=True,
+            show_diff=False,
+        )
+        assert update_obj.update("fastq_trim_fastp_fastqc") is True

tests/test_subworkflows.py

@@ -12,6 +12,8 @@
 import nf_core.subworkflows
 
 from .utils import (
+    CROSS_ORGANIZATION_BRANCH,
+    CROSS_ORGANIZATION_URL,
     GITLAB_SUBWORKFLOWS_BRANCH,
     GITLAB_SUBWORKFLOWS_ORG_PATH_BRANCH,
     GITLAB_URL,
@@ -103,10 +105,17 @@ def setUp(self):
             force=False,
             sha="8c343b3c8a0925949783dc547666007c245c235b",
         )
+        self.subworkflow_install_cross_org = nf_core.subworkflows.SubworkflowInstall(
+            self.pipeline_dir, remote_url=CROSS_ORGANIZATION_URL, branch=CROSS_ORGANIZATION_BRANCH
+        )
+
         self.mods_install = nf_core.modules.install.ModuleInstall(self.pipeline_dir, prompt=False, force=True)
 
         # Set up remove objects
         self.subworkflow_remove = nf_core.subworkflows.SubworkflowRemove(self.pipeline_dir)
+        self.subworkflow_remove_cross_org = nf_core.subworkflows.SubworkflowRemove(
+            self.pipeline_dir, remote_url=CROSS_ORGANIZATION_URL, branch=CROSS_ORGANIZATION_BRANCH
+        )
 
     @pytest.fixture(autouse=True)
     def _use_caplog(self, caplog):

tests/utils.py

@@ -20,7 +20,7 @@
 OLD_TRIMGALORE_SHA = "9b7a3bdefeaad5d42324aa7dd50f87bea1b04386"
 OLD_TRIMGALORE_BRANCH = "mimic-old-trimgalore"
 GITLAB_URL = "https://gitlab.com/nf-core/modules-test.git"
-CROSS_ORGANIZATION_URL = "https://github.com/jvfe/test-subworkflow-remote.git"
+CROSS_ORGANIZATION_URL = "https://github.com/nf-core-test/modules.git"
 CROSS_ORGANIZATION_BRANCH = "main"
 GITLAB_REPO = "nf-core-test"
 GITLAB_DEFAULT_BRANCH = "main"