Incorporate Feedback Review

Snakefile

@@ -210,8 +210,6 @@ rule reconstruct:
         # Remove the default placeholder parameter added for algorithms that have no parameters
         if 'spras_placeholder' in params:
             params.pop('spras_placeholder')
-        # TODO consider the best way to pass global configuration information to the run functions
-        # This approach requires that all run functions support a singularity option
         params['container_framework'] = FRAMEWORK
         runner.run(wildcards.algorithm, params)
 

src/allpairs.py

@@ -54,7 +54,7 @@ def run(nodetypes=None, network=None, output_file=None, container_framework="doc
         Run All Pairs Shortest Paths with Docker
         @param nodetypes: input node types with sources and targets (required)
         @param network: input network file (required)
-        @param singularity: if True, run using the Singularity container instead of the Docker container
+        @param container_framework: choose the container runtime framework, currently supports "docker" or "singularity" (required)
         @param output_file: path to the output pathway file (required)
         """
         if not nodetypes or not network or not output_file:

src/analysis/cytoscape.py

@@ -6,12 +6,12 @@
 from src.containers import prepare_volume, run_container
 
 
-def run_cytoscape(pathways: List[Union[str, PurePath]], output_file: str) -> None:
+def run_cytoscape(pathways: List[Union[str, PurePath]], output_file: str, container_framework="docker") -> None:
     """
     Create a Cytoscape session file with visualizations of each of the provided pathways
     @param pathways: a list of pathways to visualize
     @param output_file: the output Cytoscape session file
-    @param singularity: whether to run in a Singularity container
+    @param container_framework: choose the container runtime framework, currently supports "docker" or "singularity"
     """
     work_dir = '/spras'
 
@@ -50,7 +50,6 @@ def run_cytoscape(pathways: List[Union[str, PurePath]], output_file: str) -> Non
     print('Running Cytoscape with arguments: {}'.format(' '.join(command)), flush=True)
 
     # TODO consider making this a string in the config file instead of a Boolean
-    container_framework = config.config.container_framework
     container_prefix = "py4cytoscape:v1"
     out = run_container(container_framework,
                         container_prefix,

src/config.py

@@ -69,7 +69,7 @@ def process_config(self, raw_config):
         if "container_framework" in raw_config:
             container_framework = raw_config["container_framework"].lower()
             if container_framework not in ("docker", "singularity"):
-                msg = "SPRAS was configured to run with an unknown container framework: '" + raw_config["container_framework"] + "'. Accepted values are 'docker' or 'singularity"
+                msg = "SPRAS was configured to run with an unknown container framework: '" + raw_config["container_framework"] + "'. Accepted values are 'docker' or 'singularity'."
                 raise ValueError(msg)
             self.container_framework = container_framework
         else:

src/containers.py

@@ -216,9 +216,6 @@ def prepare_volume(filename: Union[str, PurePath], volume_base: Union[str, PureP
     if isinstance(filename, PurePath):
         filename = str(filename)
 
-    # There's no clear way to get DEFAULT_HASH_LENGTH from config without a circular import...
-    # For now, hardcoding the value to 7, since it appeared the value wasn't updated by
-    # config.yaml before anyway.
     filename_hash = hash_filename(filename, config.config.hash_length)
     dest = PurePosixPath(base_path, filename_hash)
 

src/domino.py

@@ -64,7 +64,7 @@ def run(network=None, active_genes=None, output_file=None, slice_threshold=None,
         @param output_file: path to the output pathway file (required)
         @param slice_threshold: the p-value threshold for considering a slice as relevant (optional)
         @param module_threshold: the p-value threshold for considering a putative module as final module (optional)
-        @param singularity: if True, run using the Singularity container instead of the Docker container (optional)
+        @param container_framework: choose the container runtime framework, currently supports "docker" or "singularity" (required)
         """
 
         if not network or not active_genes or not output_file:

src/meo.py

@@ -95,7 +95,7 @@ def run(edges=None, sources=None, targets=None, output_file=None, max_path_lengt
         Only the edge output file is retained.
         All other output files are deleted.
         @param output_file: the name of the output edge file, which will overwrite any existing file with this name
-        @param singularity: if True, run using the Singularity container instead of the Docker container
+        @param container_framework: choose the container runtime framework, currently supports "docker" or "singularity" (required)
         """
         if edges is None or sources is None or targets is None or output_file is None:
             raise ValueError('Required Maximum Edge Orientation arguments are missing')

src/mincostflow.py

@@ -51,7 +51,7 @@ def run(sources=None, targets=None, edges=None, output_file=None, flow=None, cap
         @param output_file: output file name (required)
         @param flow: amount of flow going through the graph (optional)
         @param capacity: amount of capacity allowed on each edge (optional)
-        @param singularity: if True, run using the Singularity container instead of the Docker container (optional)
+        @param container_framework: choose the container runtime framework, currently supports "docker" or "singularity" (required)
         """
 
         # ensures that these parameters are required

src/omicsintegrator1.py

@@ -89,7 +89,7 @@ def run(edges=None, prizes=None, dummy_mode=None, mu_squared=None, exclude_terms
         All other output files are deleted.
         @param output_file: the name of the output sif file for the optimal forest, which will overwrite any
         existing file with this name
-        @param singularity: if True, run using the Singularity container instead of the Docker container
+        @param container_framework: choose the container runtime framework, currently supports "docker" or "singularity" (required)
         """
         if edges is None or prizes is None or output_file is None or w is None or b is None or d is None:
             raise ValueError('Required Omics Integrator 1 arguments are missing')

src/pathlinker.py

@@ -55,7 +55,7 @@ def run(nodetypes=None, network=None, output_file=None, k=None, container_framew
         @param network:  input network file (required)
         @param output_file: path to the output pathway file (required)
         @param k: path length (optional)
-        @param singularity: if True, run using the Singularity container instead of the Docker container
+        @param container_framework: choose the container runtime framework, currently supports "docker" or "singularity" (required)
         """
         # Add additional parameter validation
         # Do not require k

test/AllPairs/test_ap.py

@@ -7,6 +7,9 @@
 import src.config as config
 from src.allpairs import AllPairs
 
+# Note that we don't directly use the config in the test, but we need the config
+# to be initialized under the hood nonetheless. Initializing the config has implications
+# like setting hash length behaviors, container registries, etc.
 config.init_from_file("config/config.yaml")
 
 TEST_DIR = 'test/AllPairs/'

test/analysis/test_cytoscape.py

@@ -6,6 +6,8 @@
 import src.config as config
 from src.analysis.cytoscape import run_cytoscape
 
+config.init_from_file("config/config.yaml")
+
 INPUT_DIR = 'test/analysis/input/example/'
 INPUT_PATHWAYS = [INPUT_DIR + 'data0-meo-params-GKEDDFZ_pathway.txt',
                   INPUT_DIR + 'data0-omicsintegrator1-params-RQCQ4YN_pathway.txt',

test/test_config.py

@@ -0,0 +1,109 @@
+import pytest
+
+import src.config as config
+
+
+# Set up a dummy config for testing. For now, only include things that MUST exist in the dict
+# in order for the config init to complete. To test particular parts of the config initialization,
+# individual values of the dict can be changed and the whole initialization can be re-run.
+def get_test_config():
+    test_raw_config = {
+        "container_framework": "singularity",
+        "container_registry": {
+            "base_url": "docker.io",
+            "owner": "reedcompbio",
+        },
+        "hash_length": 7,
+        "reconstruction_settings": {
+            "locations": {
+                "reconstruction_dir": "my_dir"
+            }
+        },
+        "datasets": [{"label":"alg1"}, {"label":"alg2"}],
+        "algorithms": [{"params": ["param2", "param2"]}],
+        "analysis": {
+            "summary": {
+                "include": False
+            },
+            "ml": {
+                "include": False
+            },
+            "graphspace": {
+                "include": False
+            },
+            "cytoscape": {
+                "include": False
+            },
+        },
+    }
+
+    return test_raw_config
+
+class TestConfig:
+    """
+    Tests various parts of the configuration mechanism
+    """
+    def test_config_hash_length(self):
+        # Initialize the configuration
+        test_config = get_test_config()
+        config.init_global(test_config)
+        assert (config.config.hash_length == 7)
+
+        test_config["hash_length"] = ""
+        config.init_global(test_config)
+        assert (config.config.hash_length == config.DEFAULT_HASH_LENGTH)
+
+        # Initialize the configuration
+        test_config["hash_length"] = "12"
+        config.init_global(test_config)
+        assert (config.config.hash_length == 12)
+
+    def test_config_container_framework_normalization(self):
+        # Test singularity
+        test_config = get_test_config()
+
+        test_config["container_framework"] = "singularity"
+        config.init_global(test_config)
+        assert (config.config.container_framework == "singularity")
+
+        # Test singularity with capitalization
+        test_config["container_framework"] = "Singularity"
+        config.init_global(test_config)
+        assert (config.config.container_framework == "singularity")
+
+        # Test docker
+        test_config["container_framework"] = "docker"
+        config.init_global(test_config)
+        assert (config.config.container_framework == "docker")
+
+        # Test docker with capitalization
+        test_config["container_framework"] = "Docker"
+        config.init_global(test_config)
+        assert (config.config.container_framework == "docker")
+
+        # Test unknown framework
+        test_config["container_framework"] = "badFramework"
+        with pytest.raises(ValueError):
+            config.init_global(test_config)
+
+        test_config["container_framework"] = "badFramework"
+
+    def test_config_container_registry(self):
+        test_config = get_test_config()
+        test_config["container_registry"]["base_url"] = "docker.io"
+        test_config["container_registry"]["owner"] = "reedcompbio"
+        config.init_global(test_config)
+        assert (config.config.container_prefix == "docker.io/reedcompbio")
+
+        test_config["container_registry"]["base_url"] = "another.repo"
+        test_config["container_registry"]["owner"] = "different-owner"
+        config.init_global(test_config)
+        assert (config.config.container_prefix == "another.repo/different-owner")
+
+        test_config["container_registry"]["base_url"] = ""
+        test_config["container_registry"]["owner"] = ""
+        config.init_global(test_config)
+        assert (config.config.container_prefix == config.DEFAULT_CONTAINER_PREFIX)
+
+
+

test/test_util.py

@@ -2,9 +2,11 @@
 
 import pytest
 
+import src.config as config
 from src.containers import convert_docker_path, prepare_path_docker, prepare_volume
 from src.util import hash_params_sha1_base32
 
+config.init_from_file("config/config.yaml")
 
 class TestUtil:
     def test_prepare_path_docker(self):