From 7779ff9ab7b054517de40f70b51e48b42a46982f Mon Sep 17 00:00:00 2001
From: Steve Bachmeier <sbachmei@uw.edu>
Date: Wed, 11 Dec 2024 14:03:44 -0800
Subject: [PATCH] add test against combined step and implementation mismatch

---
 src/easylink/implementation_metadata.yaml     |  4 +--
 src/easylink/pipeline_graph.py                |  5 ++-
 .../pipeline_schema_constants/__init__.py     |  1 +
 tests/unit/conftest.py                        | 31 +++++++++++++++++++
 tests/unit/test_pipeline_graph.py             |  5 +++
 5 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/src/easylink/implementation_metadata.yaml b/src/easylink/implementation_metadata.yaml
index 8adde53d..55b7b491 100644
--- a/src/easylink/implementation_metadata.yaml
+++ b/src/easylink/implementation_metadata.yaml
@@ -77,7 +77,7 @@ step_4a_python_pandas:
 step_4b_python_pandas:
   steps: 
     - step_4b
-  image_path: /mnt/team/simulation_science/priv/engineering/er_ecosystem/images/r-image.sif
+  image_path: /mnt/team/simulation_science/priv/engineering/er_ecosystem/images/python_pandas.sif
   script_cmd: python /dummy_step.py
   env:
     INPUT_ENV_VARS: "DUMMY_CONTAINER_MAIN_INPUT_FILE_PATHS,DUMMY_CONTAINER_SECONDARY_INPUT_FILE_PATHS"
@@ -86,7 +86,7 @@ step_4b_python_pandas:
 step_4b_r:
   steps: 
     - step_4b
-  image_path: /mnt/team/simulation_science/priv/engineering/er_ecosystem/images/python_pandas.sif
+  image_path: /mnt/team/simulation_science/priv/engineering/er_ecosystem/images/r-image.sif
   script_cmd: Rscript /dummy_step.R
   env:
     INPUT_ENV_VARS: "DUMMY_CONTAINER_MAIN_INPUT_FILE_PATHS,DUMMY_CONTAINER_SECONDARY_INPUT_FILE_PATHS"
diff --git a/src/easylink/pipeline_graph.py b/src/easylink/pipeline_graph.py
index c6d86b52..8f73bd29 100644
--- a/src/easylink/pipeline_graph.py
+++ b/src/easylink/pipeline_graph.py
@@ -318,6 +318,9 @@ def validate_implementation_topology(
         is topologically consistent with the list of steps intended to be implemented.
         """
         subgraph = ImplementationGraph(self).subgraph(nodes)
+        # NOTE: The subgraph is not necessarily able to be topologically sorted
+        # in a reproducible way. We instead rely on node name sorting for
+        # error messages.
 
         # Relabel nodes by schema step
         mapping = {
@@ -326,7 +329,7 @@ def validate_implementation_topology(
         }
         if not set(mapping.values()) == set(metadata_steps):
             raise ValueError(
-                f"Pipeline configuration nodes {list(mapping.values())} do not match metadata steps {metadata_steps}."
+                f"Pipeline configuration nodes {sorted(mapping.values())} do not match metadata steps {metadata_steps}."
             )
         subgraph = nx.relabel_nodes(subgraph, mapping)
         # Check for topological inconsistency, i.e. if there is a path from a later node to an earlier node.
diff --git a/src/easylink/pipeline_schema_constants/__init__.py b/src/easylink/pipeline_schema_constants/__init__.py
index 4a6e4a80..3b309fb4 100644
--- a/src/easylink/pipeline_schema_constants/__init__.py
+++ b/src/easylink/pipeline_schema_constants/__init__.py
@@ -7,4 +7,5 @@
 TESTING_SCHEMA_PARAMS = {
     "integration": tests.SINGLE_STEP_SCHEMA_PARAMS,
     "combined_bad_topology": tests.BAD_COMBINED_TOPOLOGY_SCHEMA_PARAMS,
+    "combined_bad_implementation_names": tests.BAD_COMBINED_TOPOLOGY_SCHEMA_PARAMS,
 }
diff --git a/tests/unit/conftest.py b/tests/unit/conftest.py
index 9f7ba054..915fd8e2 100644
--- a/tests/unit/conftest.py
+++ b/tests/unit/conftest.py
@@ -658,6 +658,37 @@
             },
         },
     },
+    "combined_bad_implementation_names": {
+        "steps": {
+            "step_1": {
+                "implementation": {
+                    "name": "step_1_python_pandas",
+                },
+            },
+            "step_2": {
+                "implementation": {
+                    "name": "step_2_python_pandas",
+                },
+                "combined_implementation_key": "step_3_4",  # incorrect key
+            },
+            "step_3": {
+                "implementation": {
+                    "name": "step_3_python_pandas",
+                },
+            },
+            "choice_section": {
+                "type": "simple",
+                "step_4": {
+                    "combined_implementation_key": "step_3_4",
+                },
+            },
+        },
+        "combined_implementations": {
+            "step_3_4": {
+                "name": "step_3_and_step_4_combined_python_pandas",
+            },
+        },
+    },
 }
 
 
diff --git a/tests/unit/test_pipeline_graph.py b/tests/unit/test_pipeline_graph.py
index e6825f8b..91536d89 100644
--- a/tests/unit/test_pipeline_graph.py
+++ b/tests/unit/test_pipeline_graph.py
@@ -602,6 +602,11 @@ def test_merge_combined_implementations_parallel(default_config_params, test_dir
             "Pipeline configuration nodes ['step_1a', 'step_1b'] are not topologically consistent with the intended implementations for ['step_1a', 'step_1b']:\nThere is a path from successor step_1b to predecessor step_1a.",
             True,
         ),
+        (
+            "combined_bad_implementation_names",
+            "Pipeline configuration nodes ['step_2', 'step_4'] do not match metadata steps ['step_3', 'step_4'].",
+            False,
+        ),
     ],
 )
 def test_bad_configuration_raises(