From c0689120fec7a47ef7f8877c9416de5dd92efda6 Mon Sep 17 00:00:00 2001 From: Douglas Jacobsen Date: Thu, 3 Oct 2024 15:35:30 -0600 Subject: [PATCH 1/3] Enable variable modifications to layer changes on the same variable This commit allows modifiers that modify the same variable to layer their changes onto the modified variable. Previously, only the last modifier to modify a variable would have it's modification stick. --- lib/ramble/ramble/application.py | 4 ++-- lib/ramble/ramble/modifier.py | 9 ++++++--- 2 files changed, 8 insertions(+), 5 deletions(-) diff --git a/lib/ramble/ramble/application.py b/lib/ramble/ramble/application.py index ccff814bb..af01bcd11 100644 --- a/lib/ramble/ramble/application.py +++ b/lib/ramble/ramble/application.py @@ -1091,7 +1091,7 @@ def _define_commands( for mod in self._modifier_instances: if mod.applies_to_executable(exec_node.key): - exec_vars.update(mod.modded_variables(self)) + exec_vars.update(mod.modded_variables(self, exec_vars)) if isinstance(exec_node.attribute, ramble.util.executable.CommandExecutable): # Process directive defined executables @@ -1427,7 +1427,7 @@ def _make_experiments(self, workspace, app_inst=None): exec_vars = {} for mod in self._modifier_instances: - exec_vars.update(mod.modded_variables(self)) + exec_vars.update(mod.modded_variables(self, exec_vars)) for template_name, template_conf in workspace.all_templates(): expand_path = os.path.join(experiment_run_dir, template_name) diff --git a/lib/ramble/ramble/modifier.py b/lib/ramble/ramble/modifier.py index 97e72fd1d..7e8d91299 100644 --- a/lib/ramble/ramble/modifier.py +++ b/lib/ramble/ramble/modifier.py @@ -244,7 +244,7 @@ def format_doc(self, **kwargs): results.write((" " * indent) + line + "\n") return results.getvalue() - def modded_variables(self, app): + def modded_variables(self, app, extra_vars={}): mods = {} if self._usage_mode not in self.variable_modifications: @@ -255,10 +255,13 @@ def modded_variables(self, app): # var_str = app.expander.expansion_str(var) # prev_val = app.expander.expand_var(var_str) prev_val = app.variables[var] + if var in extra_vars: + prev_val = extra_vars[var] + if var_mod["method"] == "append": - mods[var] = f'{prev_val} {var_mod["modification"]}' + mods[var] = f'{prev_val}{var_mod["modification"]}' else: # method == prepend - mods[var] = f'{var_mod["modification"]} {prev_val}' + mods[var] = f'{var_mod["modification"]}{prev_val}' else: # method == set mods[var] = var_mod["modification"] From 520f36d480828803042528effbb8ee33d6d1f38b Mon Sep 17 00:00:00 2001 From: Douglas Jacobsen Date: Fri, 4 Oct 2024 08:49:01 -0600 Subject: [PATCH 2/3] Add a test for layered variable modifications --- lib/ramble/ramble/modifier.py | 7 ++- .../mock_layered_modifications.py | 58 +++++++++++++++++++ .../modifiers/test-mod-2/modifier.py | 31 ++++++++++ .../modifiers/test-mod/modifier.py | 7 +++ 4 files changed, 100 insertions(+), 3 deletions(-) create mode 100644 lib/ramble/ramble/test/modifier_functionality/mock_layered_modifications.py create mode 100644 var/ramble/repos/builtin.mock/modifiers/test-mod-2/modifier.py diff --git a/lib/ramble/ramble/modifier.py b/lib/ramble/ramble/modifier.py index 7e8d91299..1a5464e8d 100644 --- a/lib/ramble/ramble/modifier.py +++ b/lib/ramble/ramble/modifier.py @@ -252,11 +252,12 @@ def modded_variables(self, app, extra_vars={}): for var, var_mod in self.variable_modifications[self._usage_mode].items(): if var_mod["method"] in ["append", "prepend"]: - # var_str = app.expander.expansion_str(var) - # prev_val = app.expander.expand_var(var_str) - prev_val = app.variables[var] if var in extra_vars: prev_val = extra_vars[var] + elif var in app.variables: + prev_val = app.variables[var] + else: + prev_val = "" if var_mod["method"] == "append": mods[var] = f'{prev_val}{var_mod["modification"]}' diff --git a/lib/ramble/ramble/test/modifier_functionality/mock_layered_modifications.py b/lib/ramble/ramble/test/modifier_functionality/mock_layered_modifications.py new file mode 100644 index 000000000..44d3d039f --- /dev/null +++ b/lib/ramble/ramble/test/modifier_functionality/mock_layered_modifications.py @@ -0,0 +1,58 @@ +# Copyright 2022-2024 The Ramble Authors +# +# Licensed under the Apache License, Version 2.0 or the MIT license +# , at your +# option. This file may not be copied, modified, or distributed +# except according to those terms. + +import os + +from ramble.test.dry_run_helpers import dry_run_config, SCOPES +import ramble.test.modifier_functionality.modifier_helpers as modifier_helpers + +import ramble.workspace +from ramble.main import RambleCommand + +workspace = RambleCommand("workspace") + + +def test_layered_variable_modifications( + mutable_mock_workspace_path, mutable_applications, mock_modifiers +): + workspace_name = "test_gromacs_dry_run_mock_spack_mod" + + test_modifiers = [ + (SCOPES.experiment, modifier_helpers.named_modifier("test-mod")), + (SCOPES.experiment, modifier_helpers.named_modifier("test-mod-2")), + ] + + test_template = """ +{test_var_mod} +""" + + with ramble.workspace.create(workspace_name) as ws1: + ws1.write() + + config_path = os.path.join(ws1.config_dir, ramble.workspace.config_file_name) + template_path = os.path.join(ws1.config_dir, "test.tpl") + + with open(template_path, "w+") as f: + f.write(test_template) + + dry_run_config("modifiers", test_modifiers, config_path, "gromacs", "water_bare") + + ws1._re_read() + + workspace("concretize", global_args=["-D", ws1.root]) + workspace("setup", "--dry-run", global_args=["-D", ws1.root]) + + rendered_template = os.path.join( + ws1.experiment_dir, "gromacs", "water_bare", "test_exp", "test" + ) + assert os.path.exists(rendered_template) + + with open(rendered_template) as f: + data = f.read() + assert "test-mod-2-append" in data + assert "test-mod-append" in data diff --git a/var/ramble/repos/builtin.mock/modifiers/test-mod-2/modifier.py b/var/ramble/repos/builtin.mock/modifiers/test-mod-2/modifier.py new file mode 100644 index 000000000..2a7ca93e8 --- /dev/null +++ b/var/ramble/repos/builtin.mock/modifiers/test-mod-2/modifier.py @@ -0,0 +1,31 @@ +# Copyright 2022-2024 The Ramble Authors +# +# Licensed under the Apache License, Version 2.0 or the MIT license +# , at your +# option. This file may not be copied, modified, or distributed +# except according to those terms. + +from ramble.modkit import * # noqa: F403 + + +class TestMod2(BasicModifier): + """Define a test modifier + + This modifier contains a variable modification that can be layered on top of + the modifications defined in test-mod + """ + + name = "test-mod-2" + + tags("test") + + mode("test", description="This is a test mode") + default_mode("test") + + variable_modification( + "test_var_mod", + " test-mod-2-append", + method="append", + modes=["test"], + ) diff --git a/var/ramble/repos/builtin.mock/modifiers/test-mod/modifier.py b/var/ramble/repos/builtin.mock/modifiers/test-mod/modifier.py index ae7e5eb9d..f3348f616 100644 --- a/var/ramble/repos/builtin.mock/modifiers/test-mod/modifier.py +++ b/var/ramble/repos/builtin.mock/modifiers/test-mod/modifier.py @@ -32,6 +32,13 @@ class TestMod(BasicModifier): "exp-scope", description="This is a test mode at the experiment scope" ) + variable_modification( + "test_var_mod", + " test-mod-append", + method="append", + modes=["test"], + ) + variable_modification( "mpi_command", 'echo "prefix_mpi_command" >> {log_file}; ', From 4268a63e20aec87b04243cf07603936c370f8001 Mon Sep 17 00:00:00 2001 From: Douglas Jacobsen Date: Fri, 4 Oct 2024 08:50:56 -0600 Subject: [PATCH 3/3] Add space separator to APS modifier --- var/ramble/repos/builtin/modifiers/intel-aps/modifier.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/var/ramble/repos/builtin/modifiers/intel-aps/modifier.py b/var/ramble/repos/builtin/modifiers/intel-aps/modifier.py index 7753cb47b..57d9edde0 100644 --- a/var/ramble/repos/builtin/modifiers/intel-aps/modifier.py +++ b/var/ramble/repos/builtin/modifiers/intel-aps/modifier.py @@ -36,7 +36,7 @@ class IntelAps(BasicModifier): "aps_flags", "-c mpi -r {aps_log_dir}", method="set", modes=["mpi"] ) variable_modification( - "mpi_command", "aps {aps_flags}", method="append", modes=["mpi"] + "mpi_command", "aps {aps_flags} ", method="append", modes=["mpi"] ) archive_pattern("aps_*_results_dir/*")