From cbb35d0c4277e5cd594198570c3f9fd38570490d Mon Sep 17 00:00:00 2001 From: Joe Zuntz Date: Mon, 29 Jan 2024 13:20:15 +0000 Subject: [PATCH 1/4] add a test for issue #115 --- cosmosis/test/campaign.yml | 10 ++++++++++ cosmosis/test/example.ini | 1 + cosmosis/test/test_campaign.py | 7 +++++++ 3 files changed, 18 insertions(+) diff --git a/cosmosis/test/campaign.yml b/cosmosis/test/campaign.yml index 5adb7519..4643cb00 100644 --- a/cosmosis/test/campaign.yml +++ b/cosmosis/test/campaign.yml @@ -55,3 +55,13 @@ runs: submission: submit: cat walltime: 04:00:00 + + - name: env-test-1 + base: cosmosis/test/example.ini + env: + TEST : xxx + + - name: env-test-2 + parent: env-test-1 + env: + TEST : yyy diff --git a/cosmosis/test/example.ini b/cosmosis/test/example.ini index 25e8cce0..c86c8b40 100644 --- a/cosmosis/test/example.ini +++ b/cosmosis/test/example.ini @@ -11,3 +11,4 @@ priors = cosmosis/test/example-priors.ini [test1] file = cosmosis/test/example_module.py +env_test_var=${TEST} diff --git a/cosmosis/test/test_campaign.py b/cosmosis/test/test_campaign.py index e4f6756e..bcb8c5ab 100644 --- a/cosmosis/test/test_campaign.py +++ b/cosmosis/test/test_campaign.py @@ -136,3 +136,10 @@ def test_campaign_functions2(): submit_run("cosmosis/test/campaign.yml", runs["v3"]) submit_run("cosmosis/test/campaign.yml", runs["v4"]) + + +def test_campaign_env(): + with run_from_source_dir(): + runs = parse_yaml_run_file("cosmosis/test/campaign.yml") + assert runs["env-test-1"]["params"].get("test1", "env_test_var") == "xxx" + assert runs["env-test-2"]["params"].get("test1", "env_test_var") == "yyy" From 5877fe1dfa00dbd20bfd014dbf5035f03da55f21 Mon Sep 17 00:00:00 2001 From: Joe Zuntz Date: Mon, 29 Jan 2024 13:53:07 +0000 Subject: [PATCH 2/4] Fix issue #115 by moving the env var expansion to the end --- cosmosis/campaign.py | 175 ++++++++++++++++++++------------- cosmosis/test/test_campaign.py | 4 +- 2 files changed, 111 insertions(+), 68 deletions(-) diff --git a/cosmosis/campaign.py b/cosmosis/campaign.py index 96c87f53..572ed8d6 100644 --- a/cosmosis/campaign.py +++ b/cosmosis/campaign.py @@ -383,74 +383,78 @@ def build_run(name, run_info, runs, components, output_dir, submission_info, out # We set the environment both now, when reading the ini files, # so that any variable expansion is done, and also later when running - with temporary_environment(env_vars): - if "base" in run_info: - params = Inifile(run_info["base"], print_include_messages=False) - elif "parent" in run_info: - try: - parent = runs[run_info["parent"]] - except KeyError: - warnings.warn(f"Run {name} specifies parent {run_info['parent']} but there is no run with that name yet") - return None - params = Inifile(parent["params"], print_include_messages=False) - else: - warnings.warn(f"Run {name} specifies neither 'parent' nor 'base' so is invalid") + if "base" in run_info: + params = Inifile(run_info["base"], print_include_messages=False) + elif "parent" in run_info: + try: + parent = runs[run_info["parent"]] + except KeyError: + warnings.warn(f"Run {name} specifies parent {run_info['parent']} but there is no run with that name yet") return None + params = Inifile(parent["params"], print_include_messages=False) + else: + warnings.warn(f"Run {name} specifies neither 'parent' nor 'base' so is invalid") + return None - # Build values file, which is mandatory - if "parent" in run_info: - values = Inifile(parent["values"], print_include_messages=False) - else: - values_file = params.get('pipeline', 'values') - values = Inifile(values_file, print_include_messages=False) - - # Build optional priors file - if "parent" in run_info: - priors = Inifile(parent["priors"], print_include_messages=False) - elif "priors" in params.options("pipeline"): - priors_file = params.get('pipeline', 'priors') - priors = Inifile(priors_file, print_include_messages=False) - else: - priors = Inifile(None) - - - - # Make a list of all the modifications to be applied - # to the different bits of this pipeline - - param_updates = [] - value_updates = [] - prior_updates = [] - pipeline_updates = [] - - # First from any generic components specified - for component in run_info.get("components", []): - component_info = components[component] - param_updates.extend(component_info.get("params", [])) - value_updates.extend(component_info.get("values", [])) - prior_updates.extend(component_info.get("priors", [])) - pipeline_updates.extend(component_info.get("pipeline", [])) - - # And then for anything specific to this pipeline - param_updates.extend(run_info.get("params", [])) - value_updates.extend(run_info.get("values", [])) - prior_updates.extend(run_info.get("priors", [])) - pipeline_updates.extend(run_info.get("pipeline", [])) - - # Now apply all the steps - apply_updates(params, param_updates, is_params=True) - apply_updates(values, value_updates) - apply_updates(priors, prior_updates) - apply_pipeline_updates(params, pipeline_updates) - - output_name = run_info.get("output_name", output_name) - set_output_dir(params, name, output_dir, output_name) - - # Finally, set the submission information - submission_info = submission_info.copy() - submission_info["output_dir"] = output_dir - submission_info.update(run_info.get("submission", {})) - run["submission"] = submission_info + # Build values file, which is mandatory + if "parent" in run_info: + values = Inifile(parent["values"], print_include_messages=False) + else: + values_file = params.get('pipeline', 'values') + values = Inifile(values_file, print_include_messages=False) + + # Build optional priors file + if "parent" in run_info: + priors = Inifile(parent["priors"], print_include_messages=False) + elif "priors" in params.options("pipeline"): + priors_file = params.get('pipeline', 'priors') + priors = Inifile(priors_file, print_include_messages=False) + else: + priors = Inifile(None) + + # We want to delay expanding environment variables so that child runs + # have a chance to override them + params.no_expand_vars = True + values.no_expand_vars = True + priors.no_expand_vars = True + + + # Make a list of all the modifications to be applied + # to the different bits of this pipeline + + param_updates = [] + value_updates = [] + prior_updates = [] + pipeline_updates = [] + + # First from any generic components specified + for component in run_info.get("components", []): + component_info = components[component] + param_updates.extend(component_info.get("params", [])) + value_updates.extend(component_info.get("values", [])) + prior_updates.extend(component_info.get("priors", [])) + pipeline_updates.extend(component_info.get("pipeline", [])) + + # And then for anything specific to this pipeline + param_updates.extend(run_info.get("params", [])) + value_updates.extend(run_info.get("values", [])) + prior_updates.extend(run_info.get("priors", [])) + pipeline_updates.extend(run_info.get("pipeline", [])) + + # Now apply all the steps + apply_updates(params, param_updates, is_params=True) + apply_updates(values, value_updates) + apply_updates(priors, prior_updates) + apply_pipeline_updates(params, pipeline_updates) + + output_name = run_info.get("output_name", output_name) + set_output_dir(params, name, output_dir, output_name) + + # Finally, set the submission information + submission_info = submission_info.copy() + submission_info["output_dir"] = output_dir + submission_info.update(run_info.get("submission", {})) + run["submission"] = submission_info run["params"] = params run["values"] = values @@ -459,6 +463,41 @@ def build_run(name, run_info, runs, components, output_dir, submission_info, out return run +def expand_environment_variables(runs): + """ + For each run, expand any environment variables in the all three parameter files. + We expand environment variables in the options, section names, and values. + + Parameters + ---------- + runs : dict + A dictionary of runs, keyed by name + """ + for run in runs.values(): + # This sets environment varibles for the duration of the with block + with temporary_environment(run["env"]): + + # We apply this to all the different ini files + for ini in [run["params"], run["values"], run["priors"]]: + for section in ini.sections(): + # I've never seen anyone set a section name with an environment + # variable but I guess it could happen + new_section = os.path.expandvars(section) + + # If the section has changed we have to set the new name. + # We could delete the old one but there shouldn't be a need to. + # I guess it might neaten the metadata output. + if new_section != section and not ini.has_section(new_section): + ini.add_section(new_section) + + # Now we can expand all the actual options + for option in ini.options(section): + new_option = os.path.expandvars(option) + value = ini.get(section, option) + new_value = os.path.expandvars(value) + if (new_value != value) or (new_option != option) or (new_section != section): + ini.set(new_section, new_option, new_value) + def parse_yaml_run_file(run_config): """ Parse a yaml file (or process a previously parsed file) @@ -555,6 +594,10 @@ def parse_yaml_run_file(run_config): name = run_dict["name"] runs[name] = build_run(name, run_dict, runs, components, output_dir, submission_info, output_name) + # Only now do we expand environment variables in the runs. This gives the child runs + # a chance to override the environment variables of their parents. + expand_environment_variables(runs) + return runs diff --git a/cosmosis/test/test_campaign.py b/cosmosis/test/test_campaign.py index bcb8c5ab..1e660770 100644 --- a/cosmosis/test/test_campaign.py +++ b/cosmosis/test/test_campaign.py @@ -71,7 +71,7 @@ def test_campaign_functions(): with run_from_source_dir(): runs = parse_yaml_run_file("cosmosis/test/campaign.yml") - assert len(runs) == 4 + assert len(runs) == 6 assert "v1" in runs assert runs["v2"]["values"].get("parameters", "p1") == "-2.0 0.0 2.0" assert runs["v2"]["priors"].get("parameters", "p2") == "gaussian 0.0 1.0" @@ -113,7 +113,7 @@ def test_campaign_functions2(): print(name) - assert len(runs) == 4 + assert len(runs) == 6 assert "v1" in runs assert runs["v2"]["values"].get("parameters", "p1") == "-2.0 0.0 2.0" assert runs["v2"]["priors"].get("parameters", "p2") == "gaussian 0.0 1.0" From 860ae913b2e6bc71b9319318c5fabe8598b013b1 Mon Sep 17 00:00:00 2001 From: Joe Zuntz Date: Mon, 29 Jan 2024 14:57:27 +0000 Subject: [PATCH 3/4] Put no_expand_vars on the ini file --- cosmosis/campaign.py | 25 +++++++++---------------- cosmosis/runtime/config.py | 10 ++++++---- cosmosis/test/test_campaign.py | 2 ++ 3 files changed, 17 insertions(+), 20 deletions(-) diff --git a/cosmosis/campaign.py b/cosmosis/campaign.py index 572ed8d6..b3fbd159 100644 --- a/cosmosis/campaign.py +++ b/cosmosis/campaign.py @@ -381,43 +381,36 @@ def build_run(name, run_info, runs, components, output_dir, submission_info, out run["env"] = env_vars - # We set the environment both now, when reading the ini files, - # so that any variable expansion is done, and also later when running + # We want to delay expanding environment variables so that child runs + # have a chance to override them. So we set no_expand_vars=True on all of these if "base" in run_info: - params = Inifile(run_info["base"], print_include_messages=False) + params = Inifile(run_info["base"], print_include_messages=False, no_expand_vars=True) elif "parent" in run_info: try: parent = runs[run_info["parent"]] except KeyError: warnings.warn(f"Run {name} specifies parent {run_info['parent']} but there is no run with that name yet") return None - params = Inifile(parent["params"], print_include_messages=False) + params = Inifile(parent["params"], print_include_messages=False, no_expand_vars=True) else: warnings.warn(f"Run {name} specifies neither 'parent' nor 'base' so is invalid") return None # Build values file, which is mandatory if "parent" in run_info: - values = Inifile(parent["values"], print_include_messages=False) + values = Inifile(parent["values"], print_include_messages=False, no_expand_vars=True) else: values_file = params.get('pipeline', 'values') - values = Inifile(values_file, print_include_messages=False) + values = Inifile(values_file, print_include_messages=False, no_expand_vars=True) # Build optional priors file if "parent" in run_info: - priors = Inifile(parent["priors"], print_include_messages=False) + priors = Inifile(parent["priors"], print_include_messages=False, no_expand_vars=True) elif "priors" in params.options("pipeline"): priors_file = params.get('pipeline', 'priors') - priors = Inifile(priors_file, print_include_messages=False) + priors = Inifile(priors_file, print_include_messages=False, no_expand_vars=True) else: - priors = Inifile(None) - - # We want to delay expanding environment variables so that child runs - # have a chance to override them - params.no_expand_vars = True - values.no_expand_vars = True - priors.no_expand_vars = True - + priors = Inifile(None, no_expand_vars=True) # Make a list of all the modifications to be applied # to the different bits of this pipeline diff --git a/cosmosis/runtime/config.py b/cosmosis/runtime/config.py index 8c5888b7..13855fe1 100644 --- a/cosmosis/runtime/config.py +++ b/cosmosis/runtime/config.py @@ -31,7 +31,8 @@ class IncludingConfigParser(configparser.ConfigParser): comments, but still delineate sections). """ - def __init__(self, defaults=None, print_include_messages=True): + def __init__(self, defaults=None, print_include_messages=True, no_expand_vars=False): + self.no_expand_vars = no_expand_vars configparser.ConfigParser.__init__(self, defaults=defaults, dict_type=collections.OrderedDict, @@ -52,7 +53,7 @@ def _read(self, fp, fpname): s = io.StringIO() for line in fp: # check for include directives - if not getattr(self, 'no_expand_vars', False): + if not self.no_expand_vars: line = os.path.expandvars(line) if line.lower().startswith('%include'): @@ -95,7 +96,7 @@ class Inifile(IncludingConfigParser): """ - def __init__(self, filename, defaults=None, override=None, print_include_messages=True): + def __init__(self, filename, defaults=None, override=None, print_include_messages=True, no_expand_vars=False): u"""Read in a configuration from `filename`. The `defaults` will be applied if a parameter is not specified in @@ -110,7 +111,8 @@ def __init__(self, filename, defaults=None, override=None, print_include_message IncludingConfigParser.__init__(self, defaults=defaults, - print_include_messages=print_include_messages) + print_include_messages=print_include_messages, + no_expand_vars=no_expand_vars) # if we are pased a dict, convert it to an inifile if isinstance(filename, dict): diff --git a/cosmosis/test/test_campaign.py b/cosmosis/test/test_campaign.py index 1e660770..66ce0f37 100644 --- a/cosmosis/test/test_campaign.py +++ b/cosmosis/test/test_campaign.py @@ -1,5 +1,6 @@ from ..campaign import * from ..runtime import Inifile +import os import tempfile import yaml import contextlib @@ -139,6 +140,7 @@ def test_campaign_functions2(): def test_campaign_env(): + os.environ["TEST"] = "aaa" with run_from_source_dir(): runs = parse_yaml_run_file("cosmosis/test/campaign.yml") assert runs["env-test-1"]["params"].get("test1", "env_test_var") == "xxx" From 74c5f9a2d97f4e6b45b1f11583f1cae2788f9431 Mon Sep 17 00:00:00 2001 From: Joe Zuntz Date: Mon, 29 Jan 2024 15:58:48 +0000 Subject: [PATCH 4/4] stop trying to support env vars in sections and keys. Keys are stored in lower case so this is not reliable --- cosmosis/campaign.py | 18 ++++-------------- 1 file changed, 4 insertions(+), 14 deletions(-) diff --git a/cosmosis/campaign.py b/cosmosis/campaign.py index b3fbd159..cad3843b 100644 --- a/cosmosis/campaign.py +++ b/cosmosis/campaign.py @@ -459,7 +459,8 @@ def build_run(name, run_info, runs, components, output_dir, submission_info, out def expand_environment_variables(runs): """ For each run, expand any environment variables in the all three parameter files. - We expand environment variables in the options, section names, and values. + + Environment variables are only supported in values, not in keys or sections. Parameters ---------- @@ -473,23 +474,12 @@ def expand_environment_variables(runs): # We apply this to all the different ini files for ini in [run["params"], run["values"], run["priors"]]: for section in ini.sections(): - # I've never seen anyone set a section name with an environment - # variable but I guess it could happen - new_section = os.path.expandvars(section) - - # If the section has changed we have to set the new name. - # We could delete the old one but there shouldn't be a need to. - # I guess it might neaten the metadata output. - if new_section != section and not ini.has_section(new_section): - ini.add_section(new_section) - # Now we can expand all the actual options for option in ini.options(section): - new_option = os.path.expandvars(option) value = ini.get(section, option) new_value = os.path.expandvars(value) - if (new_value != value) or (new_option != option) or (new_section != section): - ini.set(new_section, new_option, new_value) + if new_value != value: + ini.set(section, option, new_value) def parse_yaml_run_file(run_config): """