bugfix/output-path-substitution (#430)

* fix bug with output_path and variable substitution * add tests for cli substitutions
LLNL · Jul 11, 2023 · 12ff3d7 · 12ff3d7
1 parent e3e1a30
commit 12ff3d7
Show file tree

Hide file tree

Showing 4 changed files with 51 additions and 2 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -9,10 +9,12 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 - A bug where the .orig, .partial, and .expanded file names were using the study name rather than the original file name
 - A bug where the openfoam_wf_singularity example was not being found
 - Some build warnings in the docs (unknown targets, duplicate targets, title underlines too short, etc.)
+- A bug where when the output path contained a variable that was overridden, the overridden variable was not changed in the output_path
 
 ### Added
 - Tests for ensuring `$(MERLIN_SPEC_ORIGINAL_TEMPLATE)`, `$(MERLIN_SPEC_ARCHIVED_COPY)`, and `$(MERLIN_SPEC_EXECUTED_RUN)` are stored correctly
 - A pdf download format for the docs
+- Tests for cli substitutions
 
 ### Changed
 - The ProvenanceYAMLFileHasRegex condition for integration tests now saves the study name and spec file name as attributes instead of just the study name

diff --git a/merlin/study/study.py b/merlin/study/study.py
@@ -306,15 +306,26 @@ def output_path(self):
 
         output_path = str(self.original_spec.output_path)
 
-        if (self.override_vars is not None) and ("OUTPUT_PATH" in self.override_vars):
-            output_path = str(self.override_vars["OUTPUT_PATH"])
+        # If there are override vars we need to check that the output path doesn't need changed
+        if self.override_vars is not None:
+            # Case where output path is directly modified
+            if "OUTPUT_PATH" in self.override_vars:
+                output_path = str(self.override_vars["OUTPUT_PATH"])
+            else:
+                for var_name, var_val in self.override_vars.items():
+                    token = f"$({var_name})"
+                    # Case where output path contains a variable that was overridden
+                    if token in output_path:
+                        output_path = output_path.replace(token, str(var_val))
 
         output_path = expand_line(output_path, self.user_vars, env_vars=True)
         output_path = os.path.abspath(output_path)
         if not os.path.isdir(output_path):
             os.makedirs(output_path)
             LOG.info(f"Made dir(s) to output path '{output_path}'.")
 
+        LOG.info(f"OUTPUT_PATH: {os.path.basename(output_path)}")
+
         return output_path
 
     @cached_property

diff --git a/tests/integration/test_definitions.py b/tests/integration/test_definitions.py
@@ -127,6 +127,7 @@ def define_tests():  # pylint: disable=R0914,R0915
     flux_native = f"{test_specs}/flux_par_native_test.yaml"
     lsf = f"{examples}/lsf/lsf_par.yaml"
     mul_workers_demo = f"{dev_examples}/multiple_workers.yaml"
+    cli_substitution_wf = f"{test_specs}/cli_substitution_test.yaml"
 
     # Other shortcuts
     black = "black --check --target-version py36"
@@ -323,6 +324,26 @@ def define_tests():  # pylint: disable=R0914,R0915
             "run type": "local",
         },
     }
+    cli_substitution_tests = {
+        "no substitutions": {
+            "cmds": f"merlin run {cli_substitution_wf} --local",
+            "conditions": [HasReturnCode(), HasRegex(r"OUTPUT_PATH: output_path_no_substitution")],
+            "run type": "local",
+            "cleanup": "rm -r output_path_no_substitution",
+        },
+        "output_path substitution": {
+            "cmds": f"merlin run {cli_substitution_wf} --local --vars OUTPUT_PATH=output_path_substitution",
+            "conditions": [HasReturnCode(), HasRegex(r"OUTPUT_PATH: output_path_substitution")],
+            "run type": "local",
+            "cleanup": "rm -r output_path_substitution",
+        },
+        "output_path w/ variable substitution": {
+            "cmds": f"merlin run {cli_substitution_wf} --local --vars SUB=variable_sub",
+            "conditions": [HasReturnCode(), HasRegex(r"OUTPUT_PATH: output_path_variable_sub")],
+            "run type": "local",
+            "cleanup": "rm -r output_path_variable_sub",
+        },
+    }
     example_tests = {
         "example failure": {"cmds": "merlin example failure", "conditions": HasRegex("not found"), "run type": "local"},
         "example simple_chain": {
@@ -748,6 +769,7 @@ def define_tests():  # pylint: disable=R0914,R0915
         examples_check,
         run_workers_echo_tests,
         wf_format_tests,
+        cli_substitution_tests,
         example_tests,
         restart_step_tests,
         restart_wf_tests,

diff --git a/tests/integration/test_specs/cli_substitution_test.yaml b/tests/integration/test_specs/cli_substitution_test.yaml
@@ -0,0 +1,14 @@
+description:
+    name: cli_substitution_test
+    description: a spec that helps test cli substitutions
+
+env:
+    variables:
+        SUB: no_substitution
+        OUTPUT_PATH: output_path_$(SUB)
+
+study:
+    - name: step1
+      description: step 1
+      run:
+         cmd: echo "test"