Merge pull request #1067 from douglasjacobsen/fix-workflow-variables

Fix order of default variable definition

Author: linsword13

lib/ramble/ramble/application.py

@@ -227,10 +227,12 @@ def clone(self):
         new_clone = type(self)(self._file_path)
         self.generated_experiments.append(new_clone)
 
-        if self._env_variable_sets:
-            new_clone.set_env_variable_sets(self._env_variable_sets.copy())
         if self.variables:
             new_clone.set_variables(self.variables.copy(), self.experiment_set)
+        if self.variants:
+            new_clone.set_variants(self.variants)
+        if self._env_variable_sets:
+            new_clone.set_env_variable_sets(self._env_variable_sets.copy())
         if self.internals:
             new_clone.set_internals(self.internals.copy())
         if self._formatted_executables:
@@ -240,8 +242,6 @@ def clone(self):
         new_clone.set_template(False)
         new_clone.repeats.set_repeats(False, 0)
         new_clone.set_chained_experiments(None)
-        if self.variants:
-            new_clone.set_variants(self.variants)
 
         return new_clone
 
@@ -321,6 +321,11 @@ def _set_package_manager(self):
                         }
                     )
 
+            # Define any missing package manager variables
+            for var, val in self.package_manager.selected_variables().items():
+                if var not in self.variables:
+                    self.define_variable(var, val.default)
+
     def _set_workflow_manager(self):
         workflow_name = conversions.canonical_none(
             self.object_variants.value(namespace.workflow_manager)
@@ -341,6 +346,12 @@ def _set_workflow_manager(self):
                 "\tramble list --type workflow_managers"
             )
 
+        if self.workflow_manager is not None:
+            # Define any missing workflow manager variables
+            for var, val in self.workflow_manager.selected_variables().items():
+                if var not in self.variables:
+                    self.define_variable(var, val.default)
+
     def set_success_list(self, success_criteria):
         self.success_list = ramble.success_criteria.ScopedCriteriaList()
 
@@ -386,6 +397,26 @@ def set_env_variable_sets(self, env_variable_sets):
 
         self._env_variable_sets = env_variable_sets.copy()
 
+        # Extract workload environment variable sets
+        if self.expander.workload_name in self.workloads:
+            workload = self.workloads[self.expander.workload_name]
+
+            new_env_vars = {}
+            for env_var in workload.environment_variables.values():
+                action = "set"
+                value = env_var.value
+
+                add = True
+                for env_var_set in self._env_variable_sets:
+                    if action in env_var_set:
+                        if env_var.name in env_var_set[action].keys():
+                            add = False
+
+                if add:
+                    new_env_vars[env_var.name] = value
+
+            self._env_variable_sets.append({"set": new_env_vars})
+
     def set_variables(self, variables, experiment_set):
         """Set internal reference to variables
 
@@ -405,6 +436,11 @@ def set_variables(self, variables, experiment_set):
                         if not var.expandable:
                             self.no_expand_vars.add(var.name)
 
+        # Define missing workload variables
+        for var, val in self.selected_variables().items():
+            if var not in self.variables:
+                self.define_variable(var, val.default)
+
         self.expander.set_no_expand_vars(self.no_expand_vars)
 
     def set_internals(self, internals):
@@ -928,6 +964,11 @@ def build_modifier_instances(self):
                 self.expander.add_no_expand_var(var)
                 mod_inst.expander.add_no_expand_var(var)
 
+            # Define any missing modifier variables
+            for var, val in mod_inst.selected_variables().items():
+                if var not in self.variables:
+                    self.define_variable(var, val.default)
+
             # Set standard variants for all modifiers
             obj_variants = getattr(mod_inst, "object_variants", None)
             if obj_variants is not None:
@@ -1083,36 +1124,6 @@ def selected_variables(self):
 
         return wl_vars
 
-    def _set_default_experiment_variables(self):
-        """Set default experiment variables (for add_expand_vars),
-        if they haven't been set already"""
-        # Set default experiment variables, if they haven't been set already
-
-        # Define variables from var_sets
-        for _, obj in self._objects():
-            for var, val in obj.selected_variables().items():
-                if var not in self.variables:
-                    self.define_variable(var, val.default)
-
-        if self.expander.workload_name in self.workloads:
-            workload = self.workloads[self.expander.workload_name]
-
-            new_env_vars = {}
-            for env_var in workload.environment_variables.values():
-                action = "set"
-                value = env_var.value
-
-                add = True
-                for env_var_set in self._env_variable_sets:
-                    if action in env_var_set:
-                        if env_var.name in env_var_set[action].keys():
-                            add = False
-
-                if add:
-                    new_env_vars[env_var.name] = value
-
-            self._env_variable_sets.append({"set": new_env_vars})
-
     def _define_commands(self, exec_graph, success_list=None):
         """Populate the internal list of commands based on executables
 
@@ -1328,7 +1339,6 @@ def add_expand_vars(self, workspace):
         if not self._vars_are_expanded:
             self._validate_experiment()
             self._executable_graph = self._get_executable_graph(self.expander.workload_name)
-            self._set_default_experiment_variables()
             self._set_input_path()
 
             self._derive_variables_for_template_path(workspace)

lib/ramble/ramble/test/application.py

@@ -382,15 +382,15 @@ def test_set_input_path_multi_input(mutable_mock_apps_repo):
     assert executable_application_instance.variables["test-input3"] == input3_path
 
 
-def test_set_default_experiment_variables(mutable_mock_apps_repo):
-    """_set_default_experiment_variables"""
+def test_set_variables(mutable_mock_apps_repo):
+    """Test that set_variables defines workload variables"""
 
     executable_application_instance = mutable_mock_apps_repo.get("basic")
 
     expansion_vars = basic_exp_dict()
+    del expansion_vars["n_ranks"]
 
     # Set up the instance to pass the initial part of the function
-    executable_application_instance.expander = ramble.expander.Expander(expansion_vars, None)
 
     test_wl = ramble.workload.Workload("test_wl", executables=["foo"], inputs=["input"])
     test_wl2 = ramble.workload.Workload("test_wl2", executables=["bar"], inputs=["input"])
@@ -400,9 +400,8 @@ def test_set_default_experiment_variables(mutable_mock_apps_repo):
     executable_application_instance.internals = {}
 
     executable_application_instance.inputs[_FS] = {"input": {"target_dir": "."}}
-    executable_application_instance.variables = {}
 
-    executable_application_instance._set_default_experiment_variables()
+    executable_application_instance.set_variables(expansion_vars, None)
 
     assert executable_application_instance.variables["n_ranks"] == "1"
 
@@ -414,9 +413,6 @@ def test_define_commands(mutable_mock_apps_repo):
 
     expansion_vars = basic_exp_dict()
 
-    # Set up the instance to pass the initial part of the function
-    executable_application_instance.expander = ramble.expander.Expander(expansion_vars, None)
-
     test_wl = ramble.workload.Workload("test_wl", executables=["foo"], inputs=["input"])
     test_wl2 = ramble.workload.Workload("test_wl2", executables=["bar"], inputs=["input"])
     test_wl2.add_variable(ramble.workload.WorkloadVariable("n_ranks", default="1"))
@@ -425,14 +421,13 @@ def test_define_commands(mutable_mock_apps_repo):
     executable_application_instance.internals = {}
 
     executable_application_instance.inputs[_FS] = {"input": {"target_dir": "."}}
-    executable_application_instance.variables = {}
+    executable_application_instance.set_variables(expansion_vars, None)
 
     exec_graph = executable_application_instance._get_executable_graph("test_wl2")
 
     executable_application_instance.set_formatted_executables(
         {"command": {"join_separator": "\n"}}
     )
-    executable_application_instance._set_default_experiment_variables()
 
     executable_application_instance.chain_prepend = []
     executable_application_instance._define_commands(exec_graph)
@@ -484,9 +479,6 @@ def test_derive_variables_for_template_path(mutable_mock_apps_repo, workspace_na
 
     expansion_vars = basic_exp_dict()
 
-    # Set up the instance to pass the initial part of the function
-    executable_application_instance.expander = ramble.expander.Expander(expansion_vars, None)
-
     test_wl = ramble.workload.Workload("test_wl", executables=["foo"], inputs=["input"])
     test_wl2 = ramble.workload.Workload("test_wl2", executables=["bar"], inputs=["input"])
     test_wl2.add_variable(ramble.workload.WorkloadVariable("n_ranks", default="1"))
@@ -495,12 +487,10 @@ def test_derive_variables_for_template_path(mutable_mock_apps_repo, workspace_na
     executable_application_instance.internals = {}
 
     executable_application_instance.inputs[_FS] = {"input": {"target_dir": "."}}
-    executable_application_instance.variables = {}
+    executable_application_instance.set_variables(expansion_vars, None)
 
     exec_graph = executable_application_instance._get_executable_graph("test_wl2")
 
-    executable_application_instance._set_default_experiment_variables()
-
     executable_application_instance.chain_prepend = []
     executable_application_instance._define_commands(exec_graph)
     executable_application_instance._define_formatted_executables()

var/ramble/repos/builtin/package_managers/spack-lightweight/test/spack_runner.py

@@ -327,6 +327,8 @@ def test_new_compiler_installs(tmpdir, capsys, request):
     import os
 
     with tmpdir.as_cwd():
+        # compilers.yaml is written to support older spack versions.
+        # The compiler information was changed to exist in the packages.yaml file around v1.0.0
         compilers_config = """
 compilers::
 - compiler:
@@ -352,6 +354,11 @@ def test_new_compiler_installs(tmpdir, capsys, request):
     externals:
     - spec: [email protected] languages=c,fortran
       prefix: {os.getcwd()}
+      extra_attributes:
+        compilers:
+          c: /tmpdir_path/gcc
+          cxx: /tmpdir_path/g++
+          fortran: /tmpdir_path/gfortran
     buildable: false
 """
 

var/ramble/repos/builtin/workflow_managers/google-batch/workflow_manager.py

@@ -71,7 +71,7 @@ def __init__(self, file_path):
 
     workflow_manager_variable(
         name="mpi_command",
-        default="srun {srun_args}",
+        default="mpirun -n {n_ranks}",
         description="mpirun prefix, mostly served as an overridable default",
     )