From 647648ed9712e66812ff5b8f6d03cbff51f03424 Mon Sep 17 00:00:00 2001 From: "Bruno P. Kinoshita" Date: Wed, 28 Jun 2023 16:50:34 +0200 Subject: [PATCH] Fix wrong number of configuration templates processed due to list order in resource_listdir --- autosubmit/autosubmit.py | 38 +++++++++++--------- test/unit/test_expid.py | 77 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 98 insertions(+), 17 deletions(-) diff --git a/autosubmit/autosubmit.py b/autosubmit/autosubmit.py index 66553dcea..d626ff4ca 100644 --- a/autosubmit/autosubmit.py +++ b/autosubmit/autosubmit.py @@ -1051,21 +1051,21 @@ class Autosubmit: :return: None """ - def add_comments_to_yaml(yaml_data, parameters, keys=None): + def _add_comments_to_yaml(yaml_data, parameters, keys=None): """A recursive generator that visits every leaf node and yields the flatten parameter.""" if keys is None: keys = [] if isinstance(yaml_data, dict): for key, value in yaml_data.items(): if isinstance(value, dict): - add_comments_to_yaml(value, parameters, [*keys, key]) + _add_comments_to_yaml(value, parameters, [*keys, key]) else: parameter_key = '.'.join([*keys, key]).upper() if parameter_key in parameters: comment = parameters[parameter_key] yaml_data.yaml_set_comment_before_after_key(key, before=comment, indent=yaml_data.lc.col) - def recurse_into_parameters(parameters: Dict[str, Union[Dict, List, str]], keys=None) -> Tuple[str, str]: + def _recurse_into_parameters(parameters: Dict[str, Union[Dict, List, str]], keys=None) -> Tuple[str, str]: """Recurse into the ``PARAMETERS`` dictionary, and emits a dictionary. The key in the dictionary is the flattened parameter key/ID, and the value @@ -1080,15 +1080,13 @@ class Autosubmit: if isinstance(parameters, dict): for key, value in parameters.items(): if isinstance(value, dict): - yield from recurse_into_parameters(value, [*keys, key]) + yield from _recurse_into_parameters(value, [*keys, key]) else: key = key.upper() - # Here's the reason why ``recurse_into_yaml`` and ``recurse_into_parameters`` - # are not one single ``recurse_into_dict`` function. The parameters have some - # keys that contain ``${PARENT}.key`` as that is how they are displayed in - # the Sphinx docs. So we need to detect it and handle it. p.s. We also know - # the max-length of the parameters dict is 2! See the ``autosubmit.helpers.parameters`` - # module for more. + # The parameters have some keys that contain ``${PARENT}.key`` as that is + # how they are displayed in the Sphinx docs. So we need to detect it and + # handle it. p.s. We also know the max-length of the parameters dict is 2! + # See the ``autosubmit.helpers.parameters`` module for more. if not key.startswith(f'{keys[0]}.'): yield '.'.join([*keys, key]).upper(), value else: @@ -1097,23 +1095,29 @@ class Autosubmit: template_files = resource_listdir('autosubmitconfigparser.config', 'files') if parameters is None: parameters = PARAMETERS - parameter_comments = dict(recurse_into_parameters(parameters)) + parameter_comments = dict(_recurse_into_parameters(parameters)) for as_conf_file in template_files: origin = resource_filename('autosubmitconfigparser.config', str(Path('files', as_conf_file))) target = None if dummy: + # Create a ``dummy.yml`` file. if as_conf_file.endswith('dummy.yml'): file_name = f'{as_conf_file.split("-")[0]}_{exp_id}.yml' target = Path(BasicConfig.LOCAL_ROOT_DIR, exp_id, 'conf', file_name) elif minimal_configuration: - if (not local and as_conf_file.endswith('git-minimal.yml')) or as_conf_file.endswith("local-minimal.yml"): + # Create a ``minimal.yml`` file. + # + # Here we have two minimal configuration files that we can copy, the local or the git files. + # The function knows whether it is a local through the ``local`` argument, and that defines + # which files we will copy (``local-minimal.yml`` if ``local``, ``git-minimal.yml`` otherwise.) + if (local and as_conf_file.endswith("local-minimal.yml")) or (not local and as_conf_file.endswith('git-minimal.yml')): target = Path(BasicConfig.LOCAL_ROOT_DIR, exp_id, 'conf/minimal.yml') - else: - if not as_conf_file.endswith('dummy.yml') and not as_conf_file.endswith('minimal.yml'): - file_name = f'{Path(as_conf_file).stem}_{exp_id}.yml' - target = Path(BasicConfig.LOCAL_ROOT_DIR, exp_id, 'conf', file_name) + elif not as_conf_file.endswith('dummy.yml') and not as_conf_file.endswith('minimal.yml'): + # Create any other file that is not ``dummy.yml`` nor ``minimal.yml``. + file_name = f'{Path(as_conf_file).stem}_{exp_id}.yml' + target = Path(BasicConfig.LOCAL_ROOT_DIR, exp_id, 'conf', file_name) # Here we annotate the copied configuration with comments from the Python source code. # This means the YAML configuration files contain the exact same comments from our @@ -1126,7 +1130,7 @@ class Autosubmit: with open(origin, 'r') as input, open(target, 'w+') as output: yaml = YAML(typ='rt') yaml_data = yaml.load(input) - add_comments_to_yaml(yaml_data, parameter_comments) + _add_comments_to_yaml(yaml_data, parameter_comments) yaml.dump(yaml_data, output) @staticmethod diff --git a/test/unit/test_expid.py b/test/unit/test_expid.py index 5828cf04c..466e3879d 100644 --- a/test/unit/test_expid.py +++ b/test/unit/test_expid.py @@ -6,6 +6,7 @@ from autosubmit.experiment.experiment_common import new_experiment from textwrap import dedent from pathlib import Path from autosubmitconfigparser.config.basicconfig import BasicConfig +from itertools import permutations, product class TestExpid(TestCase): @@ -104,3 +105,79 @@ CONFIG: # Reset the local root dir. BasicConfig.LOCAL_ROOT_DIR = original_local_root_dir + + @patch('autosubmit.autosubmit.YAML.dump') + @patch('autosubmit.autosubmit.resource_listdir') + @patch('autosubmit.autosubmit.resource_filename') + def test_autosubmit_generate_config_resource_listdir_order( + self, + resource_filename_mock, + resource_listdir_mock, + yaml_mock + ) -> None: + """ + In https://earth.bsc.es/gitlab/es/autosubmit/-/issues/1063 we had a bug + where we relied on the order of returned entries of ``pkg_resources.resource_listdir`` + (which is actually undefined per https://importlib-resources.readthedocs.io/en/latest/migration.html). + + We use the arrays below to test that defining a git minimal, we process only + the expected files. We permute and then product the arrays to get as many test + cases as possible. + + For every test case, we know that for dummy and minimal we get just one configuration + template file used. But for other cases we get as many files as we have that are not + ``*minimal.yml`` nor ``*dummy.yml``. In our test cases here, when not dummy and not minimal + we must get 2 files, since we have ``include_me_please.yml`` and ``me_too.yml``. + + :param resource_filename_mock: mocked resource_listdir + :param resource_listdir_mock: mocked resource_filename + :param yaml_mock: mocked YAML dump function + :return: None + """ + + # unique lists of resources, no repetitions + resources = permutations(['dummy.yml', 'local-minimal.yml', 'git-minimal.yml', 'include_me_please.yml', 'me_too.yml']) + dummy = [True, False] + local = [True, False] + minimal_configuration = [True, False] + test_cases = product(resources, dummy, local, minimal_configuration) + keys = ['resources', 'dummy', 'local', 'minimal_configuration'] + + for test_case in test_cases: + test = dict(zip(keys, test_case)) + expid = 'ff99' + original_local_root_dir = BasicConfig.LOCAL_ROOT_DIR + + with tempfile.TemporaryDirectory() as temp_dir: + Path(temp_dir, expid, 'conf').mkdir(parents=True) + BasicConfig.LOCAL_ROOT_DIR = temp_dir + + resources_return = [] + filenames_return = [] + + for file_name in test['resources']: + input_path = Path(temp_dir, file_name) + with open(input_path, 'w+') as source_yaml: + + source_yaml.write('TEST: YES') + source_yaml.flush() + + resources_return.append(input_path.name) # path + filenames_return.append(source_yaml.name) # textiowrapper + + resource_listdir_mock.return_value = resources_return + resource_filename_mock.side_effect = filenames_return + + Autosubmit.generate_as_config( + exp_id=expid, + dummy=test['dummy'], + minimal_configuration=test['minimal_configuration'], + local=test['local']) + + msg = f'Incorrect call count for resources={",".join(resources_return)}, dummy={test["dummy"]}, minimal_configuration={test["minimal_configuration"]}, local={test["local"]}' + expected = 2 if (not test['dummy'] and not test['minimal_configuration']) else 1 + self.assertEqual(yaml_mock.call_count, expected, msg=msg) + yaml_mock.reset_mock() + + # Reset the local root dir. + BasicConfig.LOCAL_ROOT_DIR = original_local_root_dir -- GitLab