From 98ffc331d154cc7717861fde6c908ec510003926 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Mon, 7 Oct 2024 21:56:15 +0200 Subject: [PATCH 01/31] Refactor: Add the `_prepare_yaml` method to `AbstractCode` (#6565) As specified in the docs, the `Data` class implements an `export()` method. For actually exporting `Data` nodes, subclasses should implement a `_prepare_XXX` method, where `XXX` is the desired export file format. When running `verdi data export`, the available data formats for exporting are dynamically determined based on the implemented `_prepare_XXX` methods. The `Code` classes didn't follow this specification (likely because `Code` wasn't historically a subclass of `Data`), but instead a custom implementation was used for `verdi code export` in `cmd_code.py`. With the goal of increasing consistency, this PR moves the code of this custom implementation to the new `_prepare_yaml` method of the `AbstractCode` class (`_prepare_yml` is also added, as the previous default file extension was `.yml`, but it basically just calls `prepare_yaml`). The `export` function in `cmd_code.py` now instead calls the `data_export` function, as is done when exporting other classes derived from `Data`. Thus, exporting a `Code` via the CLI through `verdi code export` remains unchanged. Lastly, for the `PortableCode` class, the `prepare_yaml` method is overridden to temporarily attach the `filepath_files` attribute to the instance, as this field is defined via the `pydantic` model, but not actually saved as an attribute (instead, the contents of the given folder are added to the `NodeRepository` via `put_object_from_tree`). Without temporarily attaching the attribute, this would otherwise lead to an exception in `test_data.py` which tests the exporting of the derived `Data` nodes, as the `filepath_files` field is tried to be accessed from the instance via `getattr`. In addition, the files contained in the `NodeRepository` of the `PortableCode` are also dumped to disk, such that upon exporting the code configuration to a YAML, the `PortableCode` _can_ actually be fully re-created from this file and the additional directory. --- src/aiida/cmdline/commands/cmd_code.py | 38 ++++++++++++------- src/aiida/cmdline/commands/cmd_computer.py | 14 +++---- src/aiida/cmdline/utils/common.py | 14 ++++--- src/aiida/orm/nodes/data/code/abstract.py | 21 ++++++++++ src/aiida/orm/nodes/data/code/portable.py | 35 ++++++++++++++++- tests/cmdline/commands/test_code.py | 11 ++++-- tests/cmdline/commands/test_computer.py | 18 ++++----- ...est_computer_export_setup___no_sort_.yaml} | 0 ...> test_computer_export_setup___sort_.yaml} | 0 tests/cmdline/utils/test_common.py | 31 +++++++-------- tests/orm/data/code/test_portable.py | 18 +++++++++ tests/orm/nodes/data/test_data.py | 37 +++++++++++++++++- 12 files changed, 177 insertions(+), 60 deletions(-) rename tests/cmdline/commands/test_computer/{test_computer_export_setup___no_sort_.yml => test_computer_export_setup___no_sort_.yaml} (100%) rename tests/cmdline/commands/test_computer/{test_computer_export_setup___sort_.yml => test_computer_export_setup___sort_.yaml} (100%) diff --git a/src/aiida/cmdline/commands/cmd_code.py b/src/aiida/cmdline/commands/cmd_code.py index 9740ed8e02..a5e384ed2a 100644 --- a/src/aiida/cmdline/commands/cmd_code.py +++ b/src/aiida/cmdline/commands/cmd_code.py @@ -14,12 +14,13 @@ import click +from aiida.cmdline.commands.cmd_data.cmd_export import data_export from aiida.cmdline.commands.cmd_verdi import verdi from aiida.cmdline.groups.dynamic import DynamicEntryPointCommandGroup from aiida.cmdline.params import arguments, options, types from aiida.cmdline.params.options.commands import code as options_code from aiida.cmdline.utils import echo, echo_tabulate -from aiida.cmdline.utils.common import generate_validate_output_file +from aiida.cmdline.utils.common import validate_output_filename from aiida.cmdline.utils.decorators import with_dbenv from aiida.common import exceptions @@ -241,28 +242,37 @@ def show(code): @options.SORT() @with_dbenv() def export(code, output_file, overwrite, sort): - """Export code to a yaml file.""" + """Export code to a yaml file. If no output file is given, default name is created based on the code label.""" - import yaml + other_args = {'sort': sort} - code_data = {} + fileformat = 'yaml' - for key in code.Model.model_fields.keys(): - value = getattr(code, key).label if key == 'computer' else getattr(code, key) - - # If the attribute is not set, for example ``with_mpi`` do not export it, because the YAML won't be valid for - # use in ``verdi code create`` since ``None`` is not a valid value on the CLI. - if value is not None: - code_data[key] = str(value) + if output_file is None: + output_file = pathlib.Path(f'{code.full_label}.{fileformat}') try: - output_file = generate_validate_output_file( - output_file=output_file, entity_label=code.label, overwrite=overwrite, appendix=f'@{code_data["computer"]}' + # In principle, output file validation is also done in the `data_export` function. However, the + # `validate_output_filename` function is applied here, as well, as it is also used in the `Computer` export, and + # as `Computer` is not derived from `Data`, it cannot be exported by `data_export`, so + # `validate_output_filename` cannot be removed in favor of the validation done in `data_export`. + validate_output_filename( + output_file=output_file, + overwrite=overwrite, ) except (FileExistsError, IsADirectoryError) as exception: raise click.BadParameter(str(exception), param_hint='OUTPUT_FILE') from exception - output_file.write_text(yaml.dump(code_data, sort_keys=sort)) + try: + data_export( + node=code, + output_fname=output_file, + fileformat=fileformat, + other_args=other_args, + overwrite=overwrite, + ) + except Exception as exception: + echo.echo_critical(f'Error in the `data_export` function: {exception}') echo.echo_success(f'Code<{code.pk}> {code.label} exported to file `{output_file}`.') diff --git a/src/aiida/cmdline/commands/cmd_computer.py b/src/aiida/cmdline/commands/cmd_computer.py index 4ac8480258..4cdfc2176f 100644 --- a/src/aiida/cmdline/commands/cmd_computer.py +++ b/src/aiida/cmdline/commands/cmd_computer.py @@ -20,7 +20,7 @@ from aiida.cmdline.params import arguments, options from aiida.cmdline.params.options.commands import computer as options_computer from aiida.cmdline.utils import echo, echo_tabulate -from aiida.cmdline.utils.common import generate_validate_output_file +from aiida.cmdline.utils.common import validate_output_filename from aiida.cmdline.utils.decorators import with_dbenv from aiida.common.exceptions import EntryPointError, ValidationError from aiida.plugins.entry_point import get_entry_point_names @@ -766,10 +766,10 @@ def computer_export_setup(computer, output_file, overwrite, sort): 'append_text': computer.get_append_text(), } + if output_file is None: + output_file = pathlib.Path(f'{computer.label}-setup.yaml') try: - output_file = generate_validate_output_file( - output_file=output_file, entity_label=computer.label, overwrite=overwrite, appendix='-setup' - ) + validate_output_filename(output_file=output_file, overwrite=overwrite) except (FileExistsError, IsADirectoryError) as exception: raise click.BadParameter(str(exception), param_hint='OUTPUT_FILE') from exception @@ -804,10 +804,10 @@ def computer_export_config(computer, output_file, user, overwrite, sort): ' because computer has not been configured yet.' ) else: + if output_file is None: + output_file = pathlib.Path(f'{computer.label}-config.yaml') try: - output_file = generate_validate_output_file( - output_file=output_file, entity_label=computer.label, overwrite=overwrite, appendix='-config' - ) + validate_output_filename(output_file=output_file, overwrite=overwrite) except (FileExistsError, IsADirectoryError) as exception: raise click.BadParameter(str(exception), param_hint='OUTPUT_FILE') from exception diff --git a/src/aiida/cmdline/utils/common.py b/src/aiida/cmdline/utils/common.py index d410b33d91..57aa2e0535 100644 --- a/src/aiida/cmdline/utils/common.py +++ b/src/aiida/cmdline/utils/common.py @@ -486,13 +486,17 @@ def build_entries(ports): echo.echo(style('\nExit codes that invalidate the cache are marked in bold red.\n', italic=True)) -def generate_validate_output_file( - output_file: Path | None, entity_label: str, appendix: str = '', overwrite: bool = False +def validate_output_filename( + output_file: Path | str, + overwrite: bool = False, ): - """Generate default output filename for `Code`/`Computer` export and validate.""" + """Validate output filename.""" if output_file is None: - output_file = Path(f'{entity_label}{appendix}.yml') + raise TypeError('Output filename must be passed for validation.') + + if isinstance(output_file, str): + output_file = Path(output_file) if output_file.is_dir(): raise IsADirectoryError( @@ -501,5 +505,3 @@ def generate_validate_output_file( if output_file.is_file() and not overwrite: raise FileExistsError(f'File `{output_file}` already exists, use `--overwrite` to overwrite.') - - return output_file diff --git a/src/aiida/orm/nodes/data/code/abstract.py b/src/aiida/orm/nodes/data/code/abstract.py index 204312bdd8..b72a51e262 100644 --- a/src/aiida/orm/nodes/data/code/abstract.py +++ b/src/aiida/orm/nodes/data/code/abstract.py @@ -381,3 +381,24 @@ def get_builder(self) -> 'ProcessBuilder': builder.code = self return builder + + def _prepare_yaml(self, *args, **kwargs): + """Export code to a YAML file.""" + import yaml + + code_data = {} + sort = kwargs.get('sort', False) + + for key in self.Model.model_fields.keys(): + value = getattr(self, key).label if key == 'computer' else getattr(self, key) + + # If the attribute is not set, for example ``with_mpi`` do not export it + # so that there are no null-values in the resulting YAML file + if value is not None: + code_data[key] = str(value) + + return yaml.dump(code_data, sort_keys=sort, encoding='utf-8'), {} + + def _prepare_yml(self, *args, **kwargs): + """Also allow for export as .yml""" + return self._prepare_yaml(*args, **kwargs) diff --git a/src/aiida/orm/nodes/data/code/portable.py b/src/aiida/orm/nodes/data/code/portable.py index a09ccb90a4..9db96c24f8 100644 --- a/src/aiida/orm/nodes/data/code/portable.py +++ b/src/aiida/orm/nodes/data/code/portable.py @@ -19,6 +19,8 @@ from __future__ import annotations +import contextlib +import logging import pathlib import typing as t @@ -34,6 +36,7 @@ from .legacy import Code __all__ = ('PortableCode',) +_LOGGER = logging.getLogger(__name__) class PortableCode(Code): @@ -71,7 +74,7 @@ def validate_filepath_files(cls, value: str) -> pathlib.Path: raise ValueError(f'The filepath `{value}` is not a directory.') return filepath - def __init__(self, filepath_executable: str, filepath_files: pathlib.Path, **kwargs): + def __init__(self, filepath_executable: str, filepath_files: pathlib.Path | str, **kwargs): """Construct a new instance. .. note:: If the files necessary for this code are not all located in a single directory or the directory @@ -177,3 +180,33 @@ def filepath_executable(self, value: str) -> None: raise ValueError('The `filepath_executable` should not be absolute.') self.base.attributes.set(self._KEY_ATTRIBUTE_FILEPATH_EXECUTABLE, value) + + def _prepare_yaml(self, *args, **kwargs): + """Export code to a YAML file.""" + try: + target = pathlib.Path().cwd() / f'{self.label}' + setattr(self, 'filepath_files', str(target)) + result = super()._prepare_yaml(*args, **kwargs)[0] + + extra_files = {} + node_repository = self.base.repository + + # Logic taken from `copy_tree` method of the `Repository` class and adapted to return + # the relative file paths and their utf-8 encoded content as `extra_files` dictionary + path = '.' + for root, dirnames, filenames in node_repository.walk(): + for filename in filenames: + rel_output_file_path = root.relative_to(path) / filename + full_output_file_path = target / rel_output_file_path + full_output_file_path.parent.mkdir(exist_ok=True, parents=True) + + extra_files[str(full_output_file_path)] = node_repository.get_object_content( + str(rel_output_file_path), mode='rb' + ) + _LOGGER.warning(f'Repository files for PortableCode <{self.pk}> dumped to folder `{target}`.') + + finally: + with contextlib.suppress(AttributeError): + delattr(self, 'filepath_files') + + return result, extra_files diff --git a/tests/cmdline/commands/test_code.py b/tests/cmdline/commands/test_code.py index b7d1c5cf5f..85f1534ece 100644 --- a/tests/cmdline/commands/test_code.py +++ b/tests/cmdline/commands/test_code.py @@ -345,7 +345,7 @@ def test_code_export_default_filename(run_cli_command, aiida_code_installed): options = [str(code.pk)] run_cli_command(cmd_code.export, options) - assert pathlib.Path('code@localhost.yml').is_file() + assert pathlib.Path('code@localhost.yaml').is_file() @pytest.mark.parametrize('non_interactive_editor', ('vim -cwq',), indirect=True) @@ -461,10 +461,13 @@ def test_code_setup_local_duplicate_full_label_interactive(run_cli_command, non_ @pytest.mark.usefixtures('aiida_profile_clean') -def test_code_setup_local_duplicate_full_label_non_interactive(run_cli_command): +def test_code_setup_local_duplicate_full_label_non_interactive(run_cli_command, tmp_path): """Test ``verdi code setup`` for a local code in non-interactive mode specifying an existing full label.""" label = 'some-label' - code = PortableCode(filepath_executable='bash', filepath_files=pathlib.Path('/bin/bash')) + tmp_bin_dir = tmp_path / 'bin' + tmp_bin_dir.mkdir() + (tmp_bin_dir / 'bash').touch() + code = PortableCode(filepath_executable='bash', filepath_files=tmp_bin_dir) code.label = label code.base.repository.put_object_from_filelike(io.BytesIO(b''), 'bash') code.store() @@ -477,7 +480,7 @@ def test_code_setup_local_duplicate_full_label_non_interactive(run_cli_command): '-P', 'core.arithmetic.add', '--store-in-db', - '--code-folder=/bin', + f'--code-folder={tmp_bin_dir}', '--code-rel-path=bash', '--label', label, diff --git a/tests/cmdline/commands/test_computer.py b/tests/cmdline/commands/test_computer.py index 6ae94c1634..422a155214 100644 --- a/tests/cmdline/commands/test_computer.py +++ b/tests/cmdline/commands/test_computer.py @@ -525,7 +525,7 @@ def test_computer_export_setup(self, tmp_path, file_regression, sort_option): comp = self.comp_builder.new() comp.store() - exported_setup_filename = tmp_path / 'computer-setup.yml' + exported_setup_filename = tmp_path / 'computer-setup.yaml' # Successfull write behavior result = self.cli_runner(computer_export_setup, [comp.label, exported_setup_filename, sort_option]) @@ -534,9 +534,9 @@ def test_computer_export_setup(self, tmp_path, file_regression, sort_option): # file regresssion check content = exported_setup_filename.read_text() - file_regression.check(content, extension='.yml') + file_regression.check(content, extension='.yaml') - # verifying correctness by comparing internal and loaded yml object + # verifying correctness by comparing internal and loaded yaml object configure_setup_data = yaml.safe_load(exported_setup_filename.read_text()) assert configure_setup_data == self.comp_builder.get_computer_spec( comp @@ -550,7 +550,7 @@ def test_computer_export_setup_overwrite(self, tmp_path): comp = self.comp_builder.new() comp.store() - exported_setup_filename = tmp_path / 'computer-setup.yml' + exported_setup_filename = tmp_path / 'computer-setup.yaml' # Check that export fails if the file already exists exported_setup_filename.touch() result = self.cli_runner(computer_export_setup, [comp.label, exported_setup_filename], raises=True) @@ -581,7 +581,7 @@ def test_computer_export_setup_default_filename(self): comp = self.comp_builder.new() comp.store() - exported_setup_filename = f'{comp_label}-setup.yml' + exported_setup_filename = f'{comp_label}-setup.yaml' self.cli_runner(computer_export_setup, [comp.label]) assert pathlib.Path(exported_setup_filename).is_file() @@ -593,7 +593,7 @@ def test_computer_export_config(self, tmp_path): comp = self.comp_builder.new() comp.store() - exported_config_filename = tmp_path / 'computer-configure.yml' + exported_config_filename = tmp_path / 'computer-configure.yaml' # We have not configured the computer yet so it should exit with an critical error result = self.cli_runner(computer_export_config, [comp.label, exported_config_filename], raises=True) @@ -613,7 +613,7 @@ def test_computer_export_config(self, tmp_path): content = exported_config_filename.read_text() assert content.startswith('safe_interval: 0.0') - # verifying correctness by comparing internal and loaded yml object + # verifying correctness by comparing internal and loaded yaml object configure_config_data = yaml.safe_load(exported_config_filename.read_text()) assert ( configure_config_data == comp.get_configuration() @@ -641,7 +641,7 @@ def test_computer_export_config_overwrite(self, tmp_path): comp.store() comp.configure(safe_interval=0.0) - exported_config_filename = tmp_path / 'computer-configure.yml' + exported_config_filename = tmp_path / 'computer-configure.yaml' # Create directory with the same name and check that command fails exported_config_filename.mkdir() @@ -673,7 +673,7 @@ def test_computer_export_config_default_filename(self): comp.store() comp.configure(safe_interval=0.0) - exported_config_filename = f'{comp_label}-config.yml' + exported_config_filename = f'{comp_label}-config.yaml' self.cli_runner(computer_export_config, [comp.label]) assert pathlib.Path(exported_config_filename).is_file() diff --git a/tests/cmdline/commands/test_computer/test_computer_export_setup___no_sort_.yml b/tests/cmdline/commands/test_computer/test_computer_export_setup___no_sort_.yaml similarity index 100% rename from tests/cmdline/commands/test_computer/test_computer_export_setup___no_sort_.yml rename to tests/cmdline/commands/test_computer/test_computer_export_setup___no_sort_.yaml diff --git a/tests/cmdline/commands/test_computer/test_computer_export_setup___sort_.yml b/tests/cmdline/commands/test_computer/test_computer_export_setup___sort_.yaml similarity index 100% rename from tests/cmdline/commands/test_computer/test_computer_export_setup___sort_.yml rename to tests/cmdline/commands/test_computer/test_computer_export_setup___sort_.yaml diff --git a/tests/cmdline/utils/test_common.py b/tests/cmdline/utils/test_common.py index 69a01090df..7b012dcc0f 100644 --- a/tests/cmdline/utils/test_common.py +++ b/tests/cmdline/utils/test_common.py @@ -12,7 +12,7 @@ import pytest from aiida.cmdline.utils import common -from aiida.cmdline.utils.common import generate_validate_output_file +from aiida.cmdline.utils.common import validate_output_filename from aiida.common import LinkType from aiida.engine import Process, calcfunction from aiida.orm import CalcFunctionNode, CalculationNode, WorkflowNode @@ -95,35 +95,30 @@ def test_with_docstring(): @pytest.mark.usefixtures('chdir_tmp_path') -def test_generate_validate_output(): +def test_validate_output_filename(): test_entity_label = 'test_code' test_appendix = '@test_computer' + fileformat = 'yaml' - expected_output_file = Path(f'{test_entity_label}{test_appendix}.yml') + expected_output_file = Path(f'{test_entity_label}{test_appendix}.{fileformat}') - # Test default label creation - obtained_output_file = generate_validate_output_file( - output_file=None, entity_label=test_entity_label, appendix=test_appendix - ) - assert expected_output_file == obtained_output_file, 'Filenames differ' + # Test failure if no actual file to be validated is passed + with pytest.raises(TypeError, match='.*passed for validation.'): + validate_output_filename(output_file=None) # Test failure if file exists, but overwrite False expected_output_file.touch() with pytest.raises(FileExistsError, match='.*use `--overwrite` to overwrite.'): - generate_validate_output_file( - output_file=None, entity_label=test_entity_label, appendix=test_appendix, overwrite=False - ) + validate_output_filename(output_file=expected_output_file, overwrite=False) - # Test that overwrite does the job - obtained_output_file = generate_validate_output_file( - output_file=None, entity_label=test_entity_label, appendix=test_appendix, overwrite=True - ) - assert expected_output_file == obtained_output_file, 'Overwrite unsuccessful' + # Test that overwrite does the job -> No exception raised + validate_output_filename(output_file=expected_output_file, overwrite=True) expected_output_file.unlink() # Test failure if directory exists expected_output_file.mkdir() with pytest.raises(IsADirectoryError, match='A directory with the name.*'): - generate_validate_output_file( - output_file=None, entity_label=test_entity_label, appendix=test_appendix, overwrite=False + validate_output_filename( + output_file=expected_output_file, + overwrite=False, ) diff --git a/tests/orm/data/code/test_portable.py b/tests/orm/data/code/test_portable.py index 839c1ed2c6..a0b3414264 100644 --- a/tests/orm/data/code/test_portable.py +++ b/tests/orm/data/code/test_portable.py @@ -96,3 +96,21 @@ def test_get_execname(tmp_path): code = PortableCode(label='some-label', filepath_executable='bash', filepath_files=tmp_path) with pytest.warns(AiidaDeprecationWarning): assert code.get_execname() == 'bash' + + +def test_portablecode_extra_files(tmp_path, chdir_tmp_path): + """Test that the node repository contents of an orm.PortableCode are dumped upon YAML export.""" + filepath_files = tmp_path / 'tmp' + filepath_files.mkdir() + # (filepath_files / 'bash').touch() + (filepath_files / 'bash').write_text('bash') + (filepath_files / 'subdir').mkdir() + (filepath_files / 'subdir/test').write_text('test') + code = PortableCode(label='some-label', filepath_executable='bash', filepath_files=filepath_files) + code.store() + extra_files = code._prepare_yaml()[1] + repo_dump_path = tmp_path / code.label + extra_files_keys = sorted([str(repo_dump_path / _) for _ in ('subdir/test', 'bash')]) + assert sorted(extra_files.keys()) == extra_files_keys + assert extra_files[str(repo_dump_path / 'bash')] == 'bash'.encode('utf-8') + assert extra_files[str(repo_dump_path / 'subdir/test')] == 'test'.encode('utf-8') diff --git a/tests/orm/nodes/data/test_data.py b/tests/orm/nodes/data/test_data.py index 5ad29640c8..44101df4b1 100644 --- a/tests/orm/nodes/data/test_data.py +++ b/tests/orm/nodes/data/test_data.py @@ -18,7 +18,7 @@ @pytest.fixture -def generate_class_instance(): +def generate_class_instance(tmp_path, aiida_localhost): """Generate a dummy `Data` instance for the given sub class.""" def _generate_class_instance(data_class): @@ -109,6 +109,41 @@ def _generate_class_instance(data_class): ) return instance + if data_class is orm.AbstractCode: + instance = data_class(label='test_abstract_code', remote_computer_exec=(aiida_localhost, '/bin/cat')) + return instance + + if data_class is orm.Code: + instance = data_class(label='test_code', remote_computer_exec=(aiida_localhost, '/bin/cat')) + return instance + + if data_class is orm.InstalledCode: + instance = data_class( + label='test_installed_code', + computer=aiida_localhost, + filepath_executable='/bin/cat', + ) + return instance + + if data_class is orm.PortableCode: + (tmp_path / 'bash').touch() + filepath_executable = 'bash' + instance = data_class( + label='test_portable_code', + filepath_executable=filepath_executable, + filepath_files=tmp_path, + ) + return instance + + if data_class is orm.ContainerizedCode: + return orm.ContainerizedCode( + label='test_containerized_code', + computer=aiida_localhost, + image_name='image', + filepath_executable='bash', + engine_command='docker {image_name}', + ) + raise RuntimeError( 'no instance generator implemented for class `{}`. If you have added a `_prepare_*` method ' 'for this data class, add a generator of a dummy instance here'.format(data_class) From 332a4a915771afedcb144463b012558e4669e529 Mon Sep 17 00:00:00 2001 From: Jusong Yu Date: Thu, 17 Oct 2024 14:57:46 +0200 Subject: [PATCH 02/31] Fix failed docker CI using more reasoning grep regex to parse python version (#6581) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The grep command failed that extracts the python version from mamba list failed because an indentation was added. See the python string in the output of `mamba list`: ``` List of packages in environment: "/opt/conda" Name Version Build Channel ──────────────────────────────────────────────────── python 3.10.13 hd12c33a_1_cpython conda-forge ``` --- .docker/aiida-core-base/Dockerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.docker/aiida-core-base/Dockerfile b/.docker/aiida-core-base/Dockerfile index 87339724bc..40b18f9fa3 100644 --- a/.docker/aiida-core-base/Dockerfile +++ b/.docker/aiida-core-base/Dockerfile @@ -137,7 +137,7 @@ RUN set -x && \ mamba && \ rm micromamba && \ # Pin major.minor version of python - mamba list python | grep '^python ' | tr -s ' ' | cut -d ' ' -f 1,2 >> "${CONDA_DIR}/conda-meta/pinned" && \ + mamba list python | grep -oP 'python\s+\K[\d.]+' | tr -s ' ' | cut -d ' ' -f 1,2 >> "${CONDA_DIR}/conda-meta/pinned" && \ mamba clean --all -f -y && \ fix-permissions "${CONDA_DIR}" && \ fix-permissions "/home/${SYSTEM_USER}" From e1467edca902867e53605e0e60b67f8767bf8d3e Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Thu, 17 Oct 2024 18:09:36 +0200 Subject: [PATCH 03/31] DevOps: Fix json query in reading the docker names to filter out fields not starting with aiida (#6573) With the version update of bake-action the `BAKE_METADATA` a field with warning information (buildx.build.warnings) was added that does not contain the key `image.name` so the json query failed. With this commit another json query was added that filters out every field name that does not start with "aiida", so the field with warning information is filtered out. Co-authored-by: Jusong Yu --- .../workflows/extract-docker-image-names.sh | 35 +++++++++---------- 1 file changed, 17 insertions(+), 18 deletions(-) diff --git a/.github/workflows/extract-docker-image-names.sh b/.github/workflows/extract-docker-image-names.sh index 8609f7c385..e395432ddb 100755 --- a/.github/workflows/extract-docker-image-names.sh +++ b/.github/workflows/extract-docker-image-names.sh @@ -9,38 +9,35 @@ set -euo pipefail # The input to this script is a JSON string passed via BAKE_METADATA env variable # Here's example input (trimmed to relevant bits): # BAKE_METADATA: { -# "base": { +# "aiida-core-base": { +# # ... # "containerimage.descriptor": { # "mediaType": "application/vnd.docker.distribution.manifest.v2+json", # "digest": "sha256:8e57a52b924b67567314b8ed3c968859cad99ea13521e60bbef40457e16f391d", # "size": 6170, # }, # "containerimage.digest": "sha256:8e57a52b924b67567314b8ed3c968859cad99ea13521e60bbef40457e16f391d", -# "image.name": "ghcr.io/aiidalab/base" -# }, -# "aiida-core-base": { # "image.name": "ghcr.io/aiidateam/aiida-core-base" -# "containerimage.digest": "sha256:6753a809b5b2675bf4c22408e07c1df155907a465b33c369ef93ebcb1c4fec26", -# "...": "" -# } -# "aiida-core-with-services": { -# "image.name": "ghcr.io/aiidateam/aiida-core-with-services" -# "containerimage.digest": "sha256:85ee91f61be1ea601591c785db038e5899d68d5fb89e07d66d9efbe8f352ee48", -# "...": "" -# } +# }, # "aiida-core-dev": { -# "image.name": "ghcr.io/aiidateam/aiida-core-with-services" # "containerimage.digest": "sha256:4d9be090da287fcdf2d4658bb82f78bad791ccd15dac9af594fb8306abe47e97", +# "...": ... +# "image.name": "ghcr.io/aiidateam/aiida-core-dev" +# }, +# "aiida-core-with-services": { # "...": "" -# } +# "containerimage.digest": "sha256:85ee91f61be1ea601591c785db038e5899d68d5fb89e07d66d9efbe8f352ee48", +# "image.name": "ghcr.io/aiidateam/aiida-core-with-services" +# }, +# "some-other-key": ... # } # # Example output (real output is on one line): # # images={ -# "AIIDA_CORE_BASE_IMAGE": "ghcr.io/aiidateam/aiida-core-base@sha256:8e57a52b924b67567314b8ed3c968859cad99ea13521e60bbef40457e16f391d", -# "AIIDA_CORE_WITH_SERVICES_IMAGE": "ghcr.io/aiidateam/aiida-core-with-services@sha256:6753a809b5b2675bf4c22408e07c1df155907a465b33c369ef93ebcb1c4fec26", -# "AIIDA_CORE_DEV_IMAGE": "ghcr.io/aiidateam/aiida-core-dev@sha256:85ee91f61be1ea601591c785db038e5899d68d5fb89e07d66d9efbe8f352ee48", +# "AIIDA_CORE_BASE_IMAGE": "ghcr.io/aiidateam/aiida-core-base@sha256:4c402a8bfd635650ad691674f8f29e7ddec5fa656fb425452067950415ee447f", +# "AIIDA_CORE_DEV_IMAGE": "ghcr.io/aiidateam/aiida-core-dev@sha256:f94c06e47f801e751f9829010b31532039b210aad2649d43205e16c08371b2ed", +# "AIIDA_CORE_WITH_SERVICES_IMAGE": "ghcr.io/aiidateam/aiida-core-with-services@sha256:bd8272f2a331af7eac3e83c44cc16d23b2e5f601a20ab4a865402659b758515e" # } # # This json output is later turned to environment variables using fromJson() GHA builtin @@ -52,5 +49,7 @@ if [[ -z ${BAKE_METADATA-} ]];then exit 1 fi -images=$(echo "${BAKE_METADATA}" | jq -c '. as $base |[to_entries[] |{"key": (.key|ascii_upcase|sub("-"; "_"; "g") + "_IMAGE"), "value": [(.value."image.name"|split(",")[0]),.value."containerimage.digest"]|join("@")}] |from_entries') +images=$(echo "${BAKE_METADATA}" | +jq -c 'to_entries | map(select(.key | startswith("aiida"))) | from_entries' | # filters out every key that does not start with aiida +jq -c '. as $base |[to_entries[] |{"key": (.key|ascii_upcase|sub("-"; "_"; "g") + "_IMAGE"), "value": [(.value."image.name"|split(",")[0]),.value."containerimage.digest"]|join("@")}] |from_entries') echo "images=$images" From a7d8dd04ea63bb362e9414a8bd55554f30424bf1 Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Tue, 22 Oct 2024 12:12:57 +0200 Subject: [PATCH 04/31] Update the issue template with the Discourse group (#6588) The AiiDA GitHub discussions platform has been disabled. We want people now to ask questions and start discussions in the Discourse group. Furthermore, as the mailing list is also deprecated, the link to the mailing list has been also removed as it just further links to our Discourse group. --- .github/ISSUE_TEMPLATE/config.yml | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/.github/ISSUE_TEMPLATE/config.yml b/.github/ISSUE_TEMPLATE/config.yml index fc1b046ef3..b8d6ebc19f 100644 --- a/.github/ISSUE_TEMPLATE/config.yml +++ b/.github/ISSUE_TEMPLATE/config.yml @@ -1,7 +1,4 @@ contact_links: -- name: AiiDA Discussions - url: https://github.com/aiidateam/aiida-core/discussions - about: For aiida-core questions and discussion -- name: AiiDA Users Forum - url: http://www.aiida.net/mailing-list/ +- name: AiiDA Discourse group + url: https://aiida.discourse.group about: For general questions and discussion From f57591655a8f91d1b37bd0f46e8c6dc6a9566c2c Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Wed, 23 Oct 2024 11:09:43 +0200 Subject: [PATCH 05/31] Fix broken troubleshooting link in bug report issue template (#6589) We moved the troubleshooting from the intro to the installation section. --- .github/ISSUE_TEMPLATE/bug_report.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/ISSUE_TEMPLATE/bug_report.md b/.github/ISSUE_TEMPLATE/bug_report.md index 0812d2c3ec..f8bb3bcd73 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.md +++ b/.github/ISSUE_TEMPLATE/bug_report.md @@ -9,7 +9,7 @@ assignees: '' -- [ ] [AiiDA Troubleshooting Documentation](https://aiida.readthedocs.io/projects/aiida-core/en/latest/intro/troubleshooting.html) +- [ ] [AiiDA Troubleshooting Documentation](https://aiida.readthedocs.io/projects/aiida-core/en/stable/installation/troubleshooting.html) - [ ] [AiiDA Discourse Forum](https://aiida.discourse.group/) ### Describe the bug From 9fe7672f36c706c7eca87a4807012a1c8a5c0259 Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Wed, 5 Jun 2024 10:43:43 +0200 Subject: [PATCH 06/31] Tests: Remove test for `make_aware` using fixed date The test compares the timezones between a fixed date and of the current date. This causes problem with time zones that use daylight saving time (DST). Fixing the test makes would add too much complexity for what the test is actually covering. The `make_aware` function is still thoroughly tested by the remaining tests. --- tests/common/test_timezone.py | 13 ------------- 1 file changed, 13 deletions(-) diff --git a/tests/common/test_timezone.py b/tests/common/test_timezone.py index be08c7fef3..8115007de7 100644 --- a/tests/common/test_timezone.py +++ b/tests/common/test_timezone.py @@ -40,19 +40,6 @@ def test_now(): assert from_tz >= ref - dt -def test_make_aware(): - """Test the :func:`aiida.common.timezone.make_aware` function. - - This should make a naive datetime object aware using the timezone of the operating system. - """ - system_tzinfo = datetime.now(timezone.utc).astimezone() # This is how to get the timezone of the OS. - naive = datetime(1970, 1, 1) - aware = make_aware(naive) - assert is_aware(aware) - assert aware.tzinfo.tzname(aware) == system_tzinfo.tzname() - assert aware.tzinfo.utcoffset(aware) == system_tzinfo.utcoffset() - - def test_make_aware_already_aware(): """Test the :func:`aiida.common.timezone.make_aware` function for an already aware datetime. From e2699118ea880c3619f0985defec7c5ad09b926e Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Wed, 5 Jun 2024 10:50:21 +0200 Subject: [PATCH 07/31] Devops: Add tox environment for pytest with presto mark --- pyproject.toml | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/pyproject.toml b/pyproject.toml index 52b2536deb..3768fcb05c 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -431,6 +431,13 @@ deps = py311: -rrequirements/requirements-py-3.11.txt py312: -rrequirements/requirements-py-3.12.txt +[testenv:py{39,310,311,312}-presto] +passenv = + PYTHONASYNCIODEBUG +setenv = + AIIDA_WARN_v3 = +commands = pytest -m 'presto' {posargs} + [testenv:py{39,310,311,312}] passenv = PYTHONASYNCIODEBUG From 309352f8bf29e52057bd31153858eb8a3560e704 Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Tue, 6 Aug 2024 08:35:45 +0200 Subject: [PATCH 08/31] Devops: Change tempfile to pytest's tmp_path pytest's tmp_path and tempfile returns a different directory on macOS Sonoma than tempfile therefore we change the tests tmp_path as it is also used in in the tests to create the config file. --- tests/conftest.py | 14 ++++++++++++++ tests/tools/pytest_fixtures/test_configuration.py | 10 ++++------ 2 files changed, 18 insertions(+), 6 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 3a9cd56336..d4ca5ee1db 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -860,3 +860,17 @@ def factory(dirpath: pathlib.Path, read_bytes=True) -> dict: return serialized return factory + + +@pytest.fixture(scope='session') +def bash_path() -> Path: + run_process = subprocess.run(['which', 'bash'], capture_output=True, check=True) + path = run_process.stdout.decode('utf-8').strip() + return Path(path) + + +@pytest.fixture(scope='session') +def cat_path() -> Path: + run_process = subprocess.run(['which', 'cat'], capture_output=True, check=True) + path = run_process.stdout.decode('utf-8').strip() + return Path(path) diff --git a/tests/tools/pytest_fixtures/test_configuration.py b/tests/tools/pytest_fixtures/test_configuration.py index 574d0d4f6a..954e50fa66 100644 --- a/tests/tools/pytest_fixtures/test_configuration.py +++ b/tests/tools/pytest_fixtures/test_configuration.py @@ -1,24 +1,22 @@ """Test the pytest fixtures.""" -import tempfile - -def test_aiida_config(): +def test_aiida_config(tmp_path_factory): """Test that ``aiida_config`` fixture is loaded by default and creates a config instance in temp directory.""" from aiida.manage.configuration import get_config from aiida.manage.configuration.config import Config config = get_config() assert isinstance(config, Config) - assert config.dirpath.startswith(tempfile.gettempdir()) + assert config.dirpath.startswith(str(tmp_path_factory.getbasetemp())) -def test_aiida_config_tmp(aiida_config_tmp): +def test_aiida_config_tmp(aiida_config_tmp, tmp_path_factory): """Test that ``aiida_config_tmp`` returns a config instance in temp directory.""" from aiida.manage.configuration.config import Config assert isinstance(aiida_config_tmp, Config) - assert aiida_config_tmp.dirpath.startswith(tempfile.gettempdir()) + assert aiida_config_tmp.dirpath.startswith(str(tmp_path_factory.getbasetemp())) def test_aiida_profile(): From d3e9333f517ce833b8ce288652007c10e0b99f9f Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Tue, 6 Aug 2024 08:38:05 +0200 Subject: [PATCH 09/31] Devops: Determine command directory dynamically with `which` Before the path for `bash` and `cat` path was hardcoded. This has been changed to run a subprocess running `which bash` and `which cat` to determine the path. This is required to for example run the tests on macOS. --- tests/cmdline/commands/test_code.py | 4 +-- tests/conftest.py | 1 + tests/orm/data/code/test_installed.py | 36 +++++++++++++-------------- tests/orm/data/code/test_portable.py | 4 +-- 4 files changed, 23 insertions(+), 22 deletions(-) diff --git a/tests/cmdline/commands/test_code.py b/tests/cmdline/commands/test_code.py index 85f1534ece..d2f09c0f24 100644 --- a/tests/cmdline/commands/test_code.py +++ b/tests/cmdline/commands/test_code.py @@ -533,7 +533,7 @@ def test_code_test(run_cli_command): @pytest.fixture -def command_options(request, aiida_localhost, tmp_path): +def command_options(request, aiida_localhost, tmp_path, bash_path): """Return tuple of list of options and entry point.""" options = [request.param, '-n', '--label', str(uuid.uuid4())] @@ -553,7 +553,7 @@ def command_options(request, aiida_localhost, tmp_path): '--computer', str(aiida_localhost.pk), '--filepath-executable', - '/usr/bin/bash', + str(bash_path.absolute()), '--engine-command', engine_command, '--image-name', diff --git a/tests/conftest.py b/tests/conftest.py index d4ca5ee1db..1dcb644b21 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -18,6 +18,7 @@ import dataclasses import os import pathlib +import subprocess import types import typing as t import warnings diff --git a/tests/orm/data/code/test_installed.py b/tests/orm/data/code/test_installed.py index 5e1fda4273..928d718080 100644 --- a/tests/orm/data/code/test_installed.py +++ b/tests/orm/data/code/test_installed.py @@ -17,29 +17,29 @@ from aiida.orm.nodes.data.code.installed import InstalledCode -def test_constructor_raises(aiida_localhost): +def test_constructor_raises(aiida_localhost, bash_path): """Test the constructor when it is supposed to raise.""" with pytest.raises(TypeError, match=r'missing .* required positional arguments'): InstalledCode() with pytest.raises(TypeError, match=r'Got object of type .*'): - InstalledCode(computer=aiida_localhost, filepath_executable=pathlib.Path('/usr/bin/bash')) + InstalledCode(computer=aiida_localhost, filepath_executable=bash_path) with pytest.raises(TypeError, match=r'Got object of type .*'): InstalledCode(computer='computer', filepath_executable='/usr/bin/bash') -def test_constructor(aiida_localhost): +def test_constructor(aiida_localhost, bash_path): """Test the constructor.""" - filepath_executable = '/usr/bin/bash' + filepath_executable = str(bash_path.absolute()) code = InstalledCode(computer=aiida_localhost, filepath_executable=filepath_executable) assert code.computer.pk == aiida_localhost.pk assert code.filepath_executable == pathlib.PurePath(filepath_executable) -def test_validate(aiida_localhost): +def test_validate(aiida_localhost, bash_path): """Test the validator is called before storing.""" - filepath_executable = '/usr/bin/bash' + filepath_executable = str(bash_path.absolute()) code = InstalledCode(computer=aiida_localhost, filepath_executable=filepath_executable) code.computer = aiida_localhost @@ -53,18 +53,18 @@ def test_validate(aiida_localhost): assert code.is_stored -def test_can_run_on_computer(aiida_localhost): +def test_can_run_on_computer(aiida_localhost, bash_path): """Test the :meth:`aiida.orm.nodes.data.code.installed.InstalledCode.can_run_on_computer` method.""" - code = InstalledCode(computer=aiida_localhost, filepath_executable='/usr/bin/bash') + code = InstalledCode(computer=aiida_localhost, filepath_executable=str(bash_path.absolute())) computer = Computer() assert code.can_run_on_computer(aiida_localhost) assert not code.can_run_on_computer(computer) -def test_filepath_executable(aiida_localhost): +def test_filepath_executable(aiida_localhost, bash_path, cat_path): """Test the :meth:`aiida.orm.nodes.data.code.installed.InstalledCode.filepath_executable` property.""" - filepath_executable = '/usr/bin/bash' + filepath_executable = str(bash_path.absolute()) code = InstalledCode(computer=aiida_localhost, filepath_executable=filepath_executable) assert code.filepath_executable == pathlib.PurePath(filepath_executable) @@ -74,7 +74,7 @@ def test_filepath_executable(aiida_localhost): assert code.filepath_executable == pathlib.PurePath(filepath_executable) # Change through the property - filepath_executable = '/usr/bin/cat' + filepath_executable = str(cat_path.absolute()) code.filepath_executable = filepath_executable assert code.filepath_executable == pathlib.PurePath(filepath_executable) @@ -101,7 +101,7 @@ def computer(request, aiida_computer_local, aiida_computer_ssh): @pytest.mark.usefixtures('aiida_profile_clean') @pytest.mark.parametrize('computer', ('core.local', 'core.ssh'), indirect=True) -def test_validate_filepath_executable(ssh_key, computer): +def test_validate_filepath_executable(ssh_key, computer, bash_path): """Test the :meth:`aiida.orm.nodes.data.code.installed.InstalledCode.validate_filepath_executable` method.""" filepath_executable = '/usr/bin/not-existing' code = InstalledCode(computer=computer, filepath_executable=filepath_executable) @@ -117,19 +117,19 @@ def test_validate_filepath_executable(ssh_key, computer): with pytest.raises(ValidationError, match=r'The provided remote absolute path .* does not exist on the computer\.'): code.validate_filepath_executable() - code.filepath_executable = '/usr/bin/bash' + code.filepath_executable = str(bash_path.absolute()) code.validate_filepath_executable() -def test_full_label(aiida_localhost): +def test_full_label(aiida_localhost, bash_path): """Test the :meth:`aiida.orm.nodes.data.code.installed.InstalledCode.full_label` property.""" label = 'some-label' - code = InstalledCode(label=label, computer=aiida_localhost, filepath_executable='/usr/bin/bash') + code = InstalledCode(label=label, computer=aiida_localhost, filepath_executable=str(bash_path.absolute())) assert code.full_label == f'{label}@{aiida_localhost.label}' -def test_get_execname(aiida_localhost): +def test_get_execname(aiida_localhost, bash_path): """Test the deprecated :meth:`aiida.orm.nodes.data.code.installed.InstalledCode.get_execname` method.""" - code = InstalledCode(label='some-label', computer=aiida_localhost, filepath_executable='/usr/bin/bash') + code = InstalledCode(label='some-label', computer=aiida_localhost, filepath_executable=str(bash_path.absolute())) with pytest.warns(AiidaDeprecationWarning): - assert code.get_execname() == '/usr/bin/bash' + assert code.get_execname() == str(bash_path.absolute()) diff --git a/tests/orm/data/code/test_portable.py b/tests/orm/data/code/test_portable.py index a0b3414264..4bffa04515 100644 --- a/tests/orm/data/code/test_portable.py +++ b/tests/orm/data/code/test_portable.py @@ -17,13 +17,13 @@ from aiida.orm.nodes.data.code.portable import PortableCode -def test_constructor_raises(tmp_path): +def test_constructor_raises(tmp_path, bash_path): """Test the constructor when it is supposed to raise.""" with pytest.raises(TypeError, match=r'missing .* required positional argument'): PortableCode() with pytest.raises(TypeError, match=r'Got object of type .*'): - PortableCode(filepath_executable=pathlib.Path('/usr/bin/bash'), filepath_files=tmp_path) + PortableCode(filepath_executable=bash_path, filepath_files=tmp_path) with pytest.raises(TypeError, match=r'Got object of type .*'): PortableCode(filepath_executable='bash', filepath_files='string') From fddffca67b4f7e3b76b19df7db8e1511c449d2d9 Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Fri, 25 Oct 2024 23:21:53 +0200 Subject: [PATCH 10/31] `DirectScheduler`: Ensure killing child processes (#6572) The current implementation only issues a kill command for the parent process, but this can leave child processes orphaned. The child processes are now retrieved and added explicitly to the kill command. --- src/aiida/schedulers/plugins/direct.py | 25 ++++++++++++++--- tests/schedulers/test_direct.py | 38 ++++++++++++++++++++++++++ 2 files changed, 59 insertions(+), 4 deletions(-) diff --git a/src/aiida/schedulers/plugins/direct.py b/src/aiida/schedulers/plugins/direct.py index 78421acb73..694ff93863 100644 --- a/src/aiida/schedulers/plugins/direct.py +++ b/src/aiida/schedulers/plugins/direct.py @@ -8,6 +8,8 @@ ########################################################################### """Plugin for direct execution.""" +from typing import Union + import aiida.schedulers from aiida.common.escaping import escape_for_bash from aiida.schedulers import SchedulerError @@ -354,13 +356,28 @@ def _parse_submit_output(self, retval, stdout, stderr): return stdout.strip() - def _get_kill_command(self, jobid): - """Return the command to kill the job with specified jobid.""" - submit_command = f'kill {jobid}' + def _get_kill_command(self, jobid: Union[int, str]) -> str: + """Return the command to kill the process with specified id and all its descendants. + + :param jobid: The job id is in the case of the + :py:class:`~aiida.schedulers.plugins.direct.DirectScheduler` the process id. + + :return: A string containing the kill command. + """ + from psutil import Process + + # get a list of the process id of all descendants + process = Process(int(jobid)) + children = process.children(recursive=True) + jobids = [str(jobid)] + jobids.extend([str(child.pid) for child in children]) + jobids_str = ' '.join(jobids) + + kill_command = f'kill {jobids_str}' self.logger.info(f'killing job {jobid}') - return submit_command + return kill_command def _parse_kill_output(self, retval, stdout, stderr): """Parse the output of the kill command. diff --git a/tests/schedulers/test_direct.py b/tests/schedulers/test_direct.py index 8879bdc88e..175f5ede48 100644 --- a/tests/schedulers/test_direct.py +++ b/tests/schedulers/test_direct.py @@ -70,3 +70,41 @@ def test_submit_script_with_num_cores_per_mpiproc(scheduler, template): ) result = scheduler.get_submit_script(template) assert f'export OMP_NUM_THREADS={num_cores_per_mpiproc}' in result + + +@pytest.mark.timeout(timeout=10) +def test_kill_job(scheduler, tmpdir): + """Test if kill_job kill all descendant children from the process. + For that we spawn a new process that runs a sleep command, then we + kill it and check if the sleep process is still alive. + + current process forked process run script.sh + python─────────────python───────────────────bash──────sleep + we kill this process we check if still running + """ + import multiprocessing + import time + + from aiida.transports.plugins.local import LocalTransport + from psutil import Process + + def run_sleep_100(): + import subprocess + + script = tmpdir / 'sleep.sh' + script.write('sleep 100') + # this is blocking for the process entering + subprocess.run(['bash', script.strpath], check=False) + + forked_process = multiprocessing.Process(target=run_sleep_100) + forked_process.start() + while len(forked_process_children := Process(forked_process.pid).children(recursive=True)) != 2: + time.sleep(0.1) + bash_process = forked_process_children[0] + sleep_process = forked_process_children[1] + with LocalTransport() as transport: + scheduler.set_transport(transport) + scheduler.kill_job(forked_process.pid) + while bash_process.is_running() or sleep_process.is_running(): + time.sleep(0.1) + forked_process.join() From 867353c415c61d94a2427d5225dd5224a1b95fb9 Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Fri, 25 Oct 2024 23:59:20 +0200 Subject: [PATCH 11/31] Engine: Fix state change broadcast before process node is updated (#6580) Processes start to broadcast their event before they update their process status in the database. This can cause issues if the next process directly tries to access the last process state retrieving it from the database while it has not been updated in the database. --- src/aiida/engine/processes/process.py | 12 ++++++----- tests/engine/test_process.py | 30 +++++++++++++++++++++++++++ 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/src/aiida/engine/processes/process.py b/src/aiida/engine/processes/process.py index c5fc50bcc3..bdd1dd279d 100644 --- a/src/aiida/engine/processes/process.py +++ b/src/aiida/engine/processes/process.py @@ -419,8 +419,6 @@ def on_entered(self, from_state: Optional[plumpy.process_states.State]) -> None: from aiida.engine.utils import set_process_state_change_timestamp - super().on_entered(from_state) - if self._state.LABEL is ProcessState.EXCEPTED: # The process is already excepted so simply update the process state on the node and let the process # complete the state transition to the terminal state. If another exception is raised during this exception @@ -428,9 +426,7 @@ def on_entered(self, from_state: Optional[plumpy.process_states.State]) -> None: self.node.set_process_state(self._state.LABEL) return - # For reasons unknown, it is important to update the outputs first, before doing anything else, otherwise there - # is the risk that certain outputs do not get attached before the process reaches a terminal state. Nevertheless - # we need to guarantee that the process state gets updated even if the ``update_outputs`` call excepts, for + # We need to guarantee that the process state gets updated even if the ``update_outputs`` call excepts, for # example if the process implementation attaches an invalid output through ``Process.out``, and so we call the # ``ProcessNode.set_process_state`` in the finally-clause. This way the state gets properly set on the node even # if the process is transitioning to the terminal excepted state. @@ -444,6 +440,12 @@ def on_entered(self, from_state: Optional[plumpy.process_states.State]) -> None: self._save_checkpoint() set_process_state_change_timestamp(self.node) + # The updating of outputs and state has to be performed before the super is called because the super will + # broadcast state changes and parent processes may start running again before the state change is completed. It + # is possible that they will read the old process state and outputs that they check may not yet have been + # attached. + super().on_entered(from_state) + @override def on_terminated(self) -> None: """Called when a Process enters a terminal state.""" diff --git a/tests/engine/test_process.py b/tests/engine/test_process.py index 3035b432b7..9b1f230041 100644 --- a/tests/engine/test_process.py +++ b/tests/engine/test_process.py @@ -175,6 +175,36 @@ def test_work_calc_finish(self): run(process) assert process.node.is_finished_ok + def test_on_finish_node_updated_before_broadcast(self, monkeypatch): + """Tests if the process state and output has been updated in the database before a broadcast is invoked. + + In plumpy.Process.on_entered the state update is broadcasted. When a process is finished this results in the + next process being run. If the next process will access the process that just finished, it can result in not + being able to retrieve the outputs or correct process state because this information has yet not been updated + them in the database. + """ + import copy + + # By monkeypatching the parent class we can check the state when the + # parents class method is invoked and check if the state has be + # correctly updated. + original_on_entered = copy.deepcopy(plumpy.Process.on_entered) + + def on_entered(self, from_state): + if self._state.LABEL.value == 'finished': + assert ( + self.node.is_finished_ok + ), 'Node state should have been updated before plumpy.Process.on_entered is invoked.' + assert ( + self.node.outputs.result.value == 2 + ), 'Outputs should have been attached before plumpy.Process.on_entered is invoked.' + original_on_entered(self, from_state) + + monkeypatch.setattr(plumpy.Process, 'on_entered', on_entered) + # Ensure that process has run correctly otherwise the asserts in the + # monkeypatched member function have been skipped + assert run_get_node(test_processes.AddProcess, a=1, b=1).node.is_finished_ok, 'Process should not fail.' + @staticmethod def test_save_instance_state(): """Test save instance's state.""" From 70572380bf05998f27ea54df3653ec357e44fc69 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Mon, 28 Oct 2024 13:35:27 +0100 Subject: [PATCH 12/31] CLI: Dump only `sealed` process nodes (#6591) Previously, no checks were applied on the state of a process when dumping its data to disk via `verdi process dump`. Now, by default, only `sealed` processes are being dumped. This can be disabled with the new `--dump-unsealed` flag. In addition, the option of `--incremental` dumping is added, in which case an incomplete existing dumping output directory can be gradually filled up with data, e.g., while the process is running in conjunction with the `--dump-unsealed` flag. Before, only the `--overwrite` flag was available to handle the case of an already existing directory, however, it directly cleans the entire top-level directory. Lastly, the `prepare_dump_path` method (previously `validate_make_dump_path`) was refactored out of the `ProcessDumper` and put as a normal function into `utils.py` in the `tools/dumping` directory. --- docs/source/howto/data.rst | 31 +++--- src/aiida/cmdline/commands/cmd_process.py | 16 +++ src/aiida/cmdline/params/options/main.py | 13 ++- src/aiida/tools/dumping/processes.py | 59 +++-------- src/aiida/tools/dumping/utils.py | 75 ++++++++++++++ tests/cmdline/commands/test_process.py | 6 +- tests/tools/dumping/test_processes.py | 114 +++++++++++++++------- 7 files changed, 219 insertions(+), 95 deletions(-) create mode 100644 src/aiida/tools/dumping/utils.py diff --git a/docs/source/howto/data.rst b/docs/source/howto/data.rst index 33c9c33aba..19b8391db0 100644 --- a/docs/source/howto/data.rst +++ b/docs/source/howto/data.rst @@ -129,19 +129,24 @@ top-level process. Further, numbered subdirectories are created for each step of ``aiida.out`` of the ``ArithmeticAddCalculation`` are placed in ``inputs`` and ``outputs``. In addition, these also contain the submission script ``_aiidasubmit.sh``, as well as the scheduler stdout and stderr, ``_scheduler-stdout.txt`` and ``_scheduler-stderr.txt``, respectively. Lastly, the source code of the ``multiply`` ``calcfunction`` presenting the -first step of the workflow is contained in the ``source_file``. - -Upon having a closer look at the directory, we also find the hidden ``.aiida_node_metadata.yaml`` files, which are -created for every ``ProcessNode`` and contain additional information about the ``Node``, the ``User``, and the -``Computer``, as well as the ``.aiida`` subdirectory with machine-readable AiiDA-internal data in JSON format. - -Since child processes are explored recursively, arbitrarily complex, nested workflows can be dumped. As already seen -above, the ``-p`` flag allows to specify a custom dumping path. If none is provided, it is automatically generated from -the ``process_label`` (or ``process_type``) and the ``pk``. In addition, the command provides the ``-o`` flag to -overwrite existing directories, the ``-f`` flag to dump all files for each ``CalculationNode`` of the workflow in a flat -directory structure, and the ``--include-inputs/--exclude-inputs`` (``--include-outputs/--exclude-outputs``) flags to -also dump additional node inputs (outputs) of each ``CalculationNode`` of the workflow into ``node_inputs`` -(``node_outputs``) subdirectories. For a full list of available options, call :code:`verdi process dump --help`. +first step of the workflow is contained in the ``source_file``. Since child processes are explored recursively, +arbitrarily complex, nested workflows can be dumped. Upon having a closer look at the directory, we also find the hidden +``.aiida_node_metadata.yaml`` files, which are created for every ``ProcessNode`` and contain additional information +about the ``Node``, the ``User``, and the ``Computer``, as well as the ``.aiida`` subdirectory with machine-readable +AiiDA-internal data in JSON format. + +As already seen above, the ``-p`` flag allows to specify a custom dumping path. If none is provided, it is automatically +generated from the ``process_label`` (or ``process_type``) and the ``pk``. In addition, the command provides the +``-o/--overwrite`` flag to fully overwrite an existing dumping directory, as well as the ``--incremental`` flag, with +which files are gradually added to an existing directory (this is the default behavior). By default, only sealed process +nodes can be dumped, however, the behavior can be changed with the ``--dump-unsealed`` flag, which can be useful in +conjunction with ``--incremental`` to gradually obtain data while a process is running. Furthermore, the ``-f/--flat`` +flag can be used to dump all files for each ``CalculationNode`` of the workflow in a flat directory structure, and the +``--include-inputs/--exclude-inputs`` (``--include-outputs/--exclude-outputs``) flags are used to also dump additional +node inputs (outputs) of each ``CalculationNode`` of the workflow into ``node_inputs`` (``node_outputs``) +subdirectories. + +For a full list of available options, call :code:`verdi process dump --help`. .. _how-to:data:import:provenance: diff --git a/src/aiida/cmdline/commands/cmd_process.py b/src/aiida/cmdline/commands/cmd_process.py index c9c492ae14..e203bdddfc 100644 --- a/src/aiida/cmdline/commands/cmd_process.py +++ b/src/aiida/cmdline/commands/cmd_process.py @@ -581,8 +581,17 @@ def process_repair(manager, broker, dry_run): '--flat', is_flag=True, default=False, + show_default=True, help='Dump files in a flat directory for every step of the workflow.', ) +@click.option( + '--dump-unsealed', + is_flag=True, + default=False, + show_default=True, + help='Also allow the dumping of unsealed process nodes.', +) +@options.INCREMENTAL() def process_dump( process, path, @@ -592,6 +601,8 @@ def process_dump( include_attributes, include_extras, flat, + dump_unsealed, + incremental, ) -> None: """Dump process input and output files to disk. @@ -609,6 +620,7 @@ def process_dump( node data for further inspection. """ + from aiida.tools.archive.exceptions import ExportValidationError from aiida.tools.dumping.processes import ProcessDumper process_dumper = ProcessDumper( @@ -618,6 +630,8 @@ def process_dump( include_extras=include_extras, overwrite=overwrite, flat=flat, + dump_unsealed=dump_unsealed, + incremental=incremental, ) try: @@ -626,6 +640,8 @@ def process_dump( echo.echo_critical( 'Dumping directory exists and overwrite is False. Set overwrite to True, or delete directory manually.' ) + except ExportValidationError as e: + echo.echo_critical(f'{e!s}') except Exception as e: echo.echo_critical(f'Unexpected error while dumping {process.__class__.__name__} <{process.pk}>:\n ({e!s}).') diff --git a/src/aiida/cmdline/params/options/main.py b/src/aiida/cmdline/params/options/main.py index 381199d199..3bf05e4191 100644 --- a/src/aiida/cmdline/params/options/main.py +++ b/src/aiida/cmdline/params/options/main.py @@ -68,6 +68,7 @@ 'GROUP_CLEAR', 'HOSTNAME', 'IDENTIFIER', + 'INCREMENTAL', 'INPUT_FORMAT', 'INPUT_PLUGIN', 'LABEL', @@ -765,12 +766,12 @@ def set_log_level(ctx, _param, value): ) OVERWRITE = OverridableOption( - '--overwrite', '-o', + '--overwrite', is_flag=True, default=False, show_default=True, - help='Overwrite file/directory if writing to disk.', + help='Overwrite file/directory when writing to disk.', ) SORT = OverridableOption( @@ -781,3 +782,11 @@ def set_log_level(ctx, _param, value): help='Sort the keys of the output YAML.', show_default=True, ) + +INCREMENTAL = OverridableOption( + '--incremental/--no-incremental', + is_flag=True, + default=True, + show_default=True, + help="Incremental dumping of data to disk. Doesn't require using overwrite to clean previous directories.", +) diff --git a/src/aiida/tools/dumping/processes.py b/src/aiida/tools/dumping/processes.py index 3d970c421c..794b1fcab2 100644 --- a/src/aiida/tools/dumping/processes.py +++ b/src/aiida/tools/dumping/processes.py @@ -30,6 +30,8 @@ WorkFunctionNode, ) from aiida.orm.utils import LinkTriple +from aiida.tools.archive.exceptions import ExportValidationError +from aiida.tools.dumping.utils import prepare_dump_path LOGGER = logging.getLogger(__name__) @@ -43,6 +45,8 @@ def __init__( include_extras: bool = True, overwrite: bool = False, flat: bool = False, + dump_unsealed: bool = False, + incremental: bool = True, ) -> None: self.include_inputs = include_inputs self.include_outputs = include_outputs @@ -50,6 +54,8 @@ def __init__( self.include_extras = include_extras self.overwrite = overwrite self.flat = flat + self.dump_unsealed = dump_unsealed + self.incremental = incremental @staticmethod def _generate_default_dump_path(process_node: ProcessNode) -> Path: @@ -178,12 +184,18 @@ def dump( :param output_path: The output path where the directory tree will be created. :param io_dump_paths: Subdirectories created for each `CalculationNode`. Default: ['inputs', 'outputs', 'node_inputs', 'node_outputs'] + :raises: ExportValidationError if the node is not sealed and dump_unsealed is False. """ + if not process_node.is_sealed and not self.dump_unsealed: + raise ExportValidationError( + f'Process `{process_node.pk} must be sealed before it can be dumped, or `dump_unsealed` set to True.' + ) + if output_path is None: output_path = self._generate_default_dump_path(process_node=process_node) - self._validate_make_dump_path(validate_path=output_path) + prepare_dump_path(path_to_validate=output_path, overwrite=self.overwrite, incremental=self.incremental) if isinstance(process_node, CalculationNode): self._dump_calculation( @@ -213,7 +225,7 @@ def _dump_workflow( :param io_dump_paths: Custom subdirectories for `CalculationNode` s, defaults to None """ - self._validate_make_dump_path(validate_path=output_path) + prepare_dump_path(path_to_validate=output_path, overwrite=self.overwrite, incremental=self.incremental) self._dump_node_yaml(process_node=workflow_node, output_path=output_path) called_links = workflow_node.base.links.get_outgoing(link_type=(LinkType.CALL_CALC, LinkType.CALL_WORK)).all() @@ -254,7 +266,7 @@ def _dump_calculation( Default: ['inputs', 'outputs', 'node_inputs', 'node_outputs'] """ - self._validate_make_dump_path(validate_path=output_path) + prepare_dump_path(path_to_validate=output_path, overwrite=self.overwrite, incremental=self.incremental) self._dump_node_yaml(process_node=calculation_node, output_path=output_path) io_dump_mapping = self._generate_calculation_io_mapping(io_dump_paths=io_dump_paths) @@ -303,47 +315,6 @@ def _dump_calculation_io(self, parent_path: Path, link_triples: LinkManager | Li link_triple.node.base.repository.copy_tree(linked_node_path.resolve()) - def _validate_make_dump_path(self, validate_path: Path, safeguard_file: str = '.aiida_node_metadata.yaml') -> Path: - """Create default dumping directory for a given process node and return it as absolute path. - - :param validate_path: Path to validate for dumping. - :param safeguard_file: Dumping-specific file to avoid deleting wrong directory. - Default: `.aiida_node_metadata.yaml` - :return: The absolute created dump path. - """ - import shutil - - if validate_path.is_dir(): - # Existing, empty directory -> OK - if not any(validate_path.iterdir()): - pass - - # Existing, non-empty directory and overwrite False -> FileExistsError - elif not self.overwrite: - raise FileExistsError(f'Path `{validate_path}` already exists and overwrite set to False.') - - # Existing, non-empty directory and overwrite True - # Check for safeguard file ('.aiida_node_metadata.yaml') for safety - # If present -> Remove directory - elif (validate_path / safeguard_file).is_file(): - LOGGER.info(f'Overwrite set to true, will overwrite directory `{validate_path}`.') - shutil.rmtree(validate_path) - - # Existing and non-empty directory and overwrite True - # Check for safeguard file ('.aiida_node_metadata.yaml') for safety - # If absent -> Don't remove directory as to not accidentally remove a wrong one - else: - raise Exception( - f"Path `{validate_path}` already exists and doesn't contain safeguard file {safeguard_file}." - f' Not removing for safety reasons.' - ) - - # Not included in if-else as to avoid having to repeat the `mkdir` call. - # `exist_ok=True` as checks implemented above - validate_path.mkdir(exist_ok=True, parents=True) - - return validate_path.resolve() - def _generate_calculation_io_mapping(self, io_dump_paths: List[str | Path] | None = None) -> SimpleNamespace: """Helper function to generate mapping for entities dumped for each `CalculationNode`. diff --git a/src/aiida/tools/dumping/utils.py b/src/aiida/tools/dumping/utils.py new file mode 100644 index 0000000000..a631ac25e5 --- /dev/null +++ b/src/aiida/tools/dumping/utils.py @@ -0,0 +1,75 @@ +########################################################################### +# Copyright (c), The AiiDA team. All rights reserved. # +# This file is part of the AiiDA code. # +# # +# The code is hosted on GitHub at https://github.com/aiidateam/aiida-core # +# For further information on the license, see the LICENSE.txt file # +# For further information please visit http://www.aiida.net # +########################################################################### +"""Utility functions for dumping features.""" + +from __future__ import annotations + +import logging +import shutil +from pathlib import Path + +__all__ = ['prepare_dump_path'] + +logger = logging.getLogger(__name__) + + +def prepare_dump_path( + path_to_validate: Path, + overwrite: bool = False, + incremental: bool = True, + safeguard_file: str = '.aiida_node_metadata.yaml', +) -> None: + """Create default dumping directory for a given process node and return it as absolute path. + + :param validate_path: Path to validate for dumping. + :param safeguard_file: Dumping-specific file that indicates that the directory indeed originated from a `verdi ... + dump` command to avoid accidentally deleting wrong directory. + Default: `.aiida_node_metadata.yaml` + :return: The absolute created dump path. + :raises ValueError: If both `overwrite` and `incremental` are set to True. + :raises FileExistsError: If a file or non-empty directory exists at the given path and none of `overwrite` or + `incremental` are enabled. + :raises FileNotFoundError: If no `safeguard_file` is found.""" + + if overwrite and incremental: + raise ValueError('Both overwrite and incremental set to True. Only specify one.') + + if path_to_validate.is_file(): + raise FileExistsError(f'A file at the given path `{path_to_validate}` already exists.') + + # Handle existing directory + if path_to_validate.is_dir(): + is_empty = not any(path_to_validate.iterdir()) + + # Case 1: Non-empty directory and overwrite is False + if not is_empty and not overwrite: + if incremental: + logger.info('Incremental dumping selected. Will keep directory.') + else: + raise FileExistsError( + f'Path `{path_to_validate}` already exists, and neither overwrite nor incremental is enabled.' + ) + + # Case 2: Non-empty directory, overwrite is True + if not is_empty and overwrite: + safeguard_exists = (path_to_validate / safeguard_file).is_file() + + if safeguard_exists: + logger.info(f'Overwriting directory `{path_to_validate}`.') + shutil.rmtree(path_to_validate) + + else: + raise FileNotFoundError( + f'Path `{path_to_validate}` exists without safeguard file ' + f'`{safeguard_file}`. Not removing because path might be a directory not created by AiiDA.' + ) + + # Create directory if it doesn't exist or was removed + path_to_validate.mkdir(exist_ok=True, parents=True) + (path_to_validate / safeguard_file).touch() diff --git a/tests/cmdline/commands/test_process.py b/tests/cmdline/commands/test_process.py index fae9957f80..b70991aa4d 100644 --- a/tests/cmdline/commands/test_process.py +++ b/tests/cmdline/commands/test_process.py @@ -365,13 +365,13 @@ def test_process_dump(self, run_cli_command, tmp_path, generate_workchain_multip assert result.exception is None, result.output assert 'Success:' in result.output - # Trying to run the dumping again in the same path but without overwrite=True should raise exception - options = [str(node.pk), '-p', str(test_path)] + # Trying to run the dumping again in the same path but with overwrite=False should raise exception + options = [str(node.pk), '-p', str(test_path), '--no-incremental'] result = run_cli_command(cmd_process.process_dump, options, raises=True) assert result.exit_code is ExitCode.CRITICAL # Works fine when using overwrite=True - options = [str(node.pk), '-p', str(test_path), '-o'] + options = [str(node.pk), '-p', str(test_path), '-o', '--no-incremental'] result = run_cli_command(cmd_process.process_dump, options) assert result.exception is None, result.output assert 'Success:' in result.output diff --git a/tests/tools/dumping/test_processes.py b/tests/tools/dumping/test_processes.py index 82e704f4e2..be05d50441 100644 --- a/tests/tools/dumping/test_processes.py +++ b/tests/tools/dumping/test_processes.py @@ -11,6 +11,7 @@ from __future__ import annotations import io +import shutil from pathlib import Path import pytest @@ -114,11 +115,19 @@ def _generate_workchain_node_io(cj_nodes, store_all: bool = True): # Only test top-level actions, like path and README creation # Other things tested via `_dump_workflow` and `_dump_calculation` def test_dump(generate_calculation_node_io, generate_workchain_node_io, tmp_path): + from aiida.tools.archive.exceptions import ExportValidationError + dump_parent_path = tmp_path / 'wc-dump-test-io' process_dumper = ProcessDumper() # Don't attach outputs, as it would require storing the calculation_node and then it cannot be used in the workchain cj_nodes = [generate_calculation_node_io(attach_outputs=False), generate_calculation_node_io(attach_outputs=False)] wc_node = generate_workchain_node_io(cj_nodes=cj_nodes) + + # Raises if ProcessNode not sealed + with pytest.raises(ExportValidationError): + return_path = process_dumper.dump(process_node=wc_node, output_path=dump_parent_path) + + wc_node.seal() return_path = process_dumper.dump(process_node=wc_node, output_path=dump_parent_path) assert dump_parent_path.is_dir() @@ -266,14 +275,31 @@ def test_dump_calculation_flat(tmp_path, generate_calculation_node_io): # Here, in principle, test only non-default arguments, as defaults tested above -# @pytest.mark.parametrize('overwrite', (True, False)) -def test_dump_calculation_overwrite(tmp_path, generate_calculation_node_io): +def test_dump_calculation_overwr_incr(tmp_path, generate_calculation_node_io): + """Tests the ProcessDumper for the overwrite and incremental option.""" dump_parent_path = tmp_path / 'cj-dump-test-overwrite' - process_dumper = ProcessDumper(overwrite=False) + process_dumper = ProcessDumper(overwrite=False, incremental=False) calculation_node = generate_calculation_node_io() - process_dumper._dump_calculation(calculation_node=calculation_node, output_path=dump_parent_path) + calculation_node.seal() + # Create safeguard file to mock existing dump directory + dump_parent_path.mkdir() + # we create safeguard file so the dumping works + (dump_parent_path / '.aiida_node_metadata.yaml').touch() with pytest.raises(FileExistsError): process_dumper._dump_calculation(calculation_node=calculation_node, output_path=dump_parent_path) + # With overwrite option true no error is raised and the dumping can run through. + process_dumper = ProcessDumper(overwrite=True, incremental=False) + process_dumper._dump_calculation(calculation_node=calculation_node, output_path=dump_parent_path) + assert (dump_parent_path / inputs_relpath / filename).is_file() + + shutil.rmtree(dump_parent_path) + + # Incremental also does work + dump_parent_path.mkdir() + (dump_parent_path / '.aiida_node_metadata.yaml').touch() + process_dumper = ProcessDumper(overwrite=False, incremental=True) + process_dumper._dump_calculation(calculation_node=calculation_node, output_path=dump_parent_path) + assert (dump_parent_path / inputs_relpath / filename).is_file() # With both inputs and outputs being dumped is the standard test case above, so only test without inputs here @@ -303,43 +329,65 @@ def test_dump_calculation_add(tmp_path, generate_calculation_node_add): # Tests for helper methods @pytest.mark.usefixtures('chdir_tmp_path') -def test_validate_make_dump_path(tmp_path): +def test_prepare_dump_path(tmp_path): + from aiida.tools.dumping.utils import prepare_dump_path + + test_dir = tmp_path / Path('test-dir') + test_file = test_dir / filename safeguard_file = node_metadata_file + safeguard_file_path = test_dir / safeguard_file - # Path must be provided - process_dumper = ProcessDumper(overwrite=False) - with pytest.raises(TypeError): - process_dumper._validate_make_dump_path() + # Cannot set both overwrite and incremental to True + with pytest.raises(ValueError): + prepare_dump_path(path_to_validate=test_dir, overwrite=True, incremental=True) + + # Check that fails if file with same name as output dir + test_dir.touch() + with pytest.raises(FileExistsError): + prepare_dump_path(path_to_validate=test_dir) + test_dir.unlink() # Check if path created if non-existent - test_dir = tmp_path / Path('test-dir') - test_dir.mkdir() - output_path = process_dumper._validate_make_dump_path(validate_path=test_dir) - assert output_path == test_dir + prepare_dump_path(path_to_validate=test_dir) + assert test_dir.exists() + assert safeguard_file_path.is_file() - # Empty path is fine -> No error and full path returned - output_path = process_dumper._validate_make_dump_path(validate_path=test_dir) - assert output_path == test_dir + # Directory exists, but empty -> is fine + safeguard_file_path.unlink() + prepare_dump_path(path_to_validate=test_dir) + assert test_dir.exists() + assert safeguard_file_path.is_file() # Fails if directory not empty, safeguard file existent, and overwrite set to False - (test_dir / filename).touch() - (test_dir / safeguard_file).touch() + test_file.touch() + safeguard_file_path.touch() with pytest.raises(FileExistsError): - output_path = process_dumper._validate_make_dump_path(validate_path=test_dir) - assert (test_dir / filename).is_file() - - # Works if directory not empty, but overwrite=True and safeguard_file (e.g. `.aiida_node_metadata.yaml`) contained - process_dumper = ProcessDumper(overwrite=True) - output_path = process_dumper._validate_make_dump_path(validate_path=test_dir, safeguard_file=safeguard_file) - assert output_path == test_dir - assert not (test_dir / safeguard_file).is_file() - - # Fails if directory not empty and overwrite set to True, but safeguard_file not found (for safety reasons) - # Could define new Exception for this... - (test_dir / filename).touch() - with pytest.raises(Exception): - output_path = process_dumper._validate_make_dump_path(validate_path=test_dir) - assert (test_dir / filename).is_file() + prepare_dump_path(path_to_validate=test_dir, overwrite=False, incremental=False) + + # Fails if directory not empty, overwrite set to True, but safeguard_file not found (for safety reasons) + safeguard_file_path.unlink() + test_file.touch() + with pytest.raises(FileNotFoundError): + prepare_dump_path(path_to_validate=test_dir, overwrite=True, incremental=False) + + # Works if directory not empty, overwrite set to True and safeguard_file contained + # -> After function call, test_file is deleted, and safeguard_file again created + safeguard_file_path.touch() + prepare_dump_path( + path_to_validate=test_dir, + safeguard_file=safeguard_file, + overwrite=True, + incremental=False, + ) + assert not test_file.is_file() + assert safeguard_file_path.is_file() + + # Works if directory not empty, but incremental=True and safeguard_file (e.g. `.aiida_node_metadata.yaml`) contained + # -> After function call, test file and safeguard_file still there + test_file.touch() + prepare_dump_path(path_to_validate=test_dir, safeguard_file=safeguard_file, incremental=True) + assert safeguard_file_path.is_file() + assert test_file.is_file() def test_generate_default_dump_path( From 0fa958285bb07ece05b944cbd694aed663b5193a Mon Sep 17 00:00:00 2001 From: ConradJohnston <40352432+ConradJohnston@users.noreply.github.com> Date: Tue, 5 Nov 2024 00:57:45 -0800 Subject: [PATCH 13/31] Scheduler: Allow a memory specification of zero for the SLURM plugin (#6605) In the SLURM memory specification, a value of zero is treated as a special case indicating that no memory limit should be used. This is documented here: https://slurm.schedmd.com/sbatch.html under the '--mem' section. However, currently the SLURM plugin demands a positive integer, but this logic puts an unexpected limitation on the SLURM plugin. This commit changes this logic to allow a value of 0 to be accepted. --- src/aiida/schedulers/plugins/slurm.py | 6 ++-- tests/schedulers/test_slurm.py | 52 +++++++++++++++++++++++++++ 2 files changed, 55 insertions(+), 3 deletions(-) diff --git a/src/aiida/schedulers/plugins/slurm.py b/src/aiida/schedulers/plugins/slurm.py index 0ef2568c22..ef62d125fd 100644 --- a/src/aiida/schedulers/plugins/slurm.py +++ b/src/aiida/schedulers/plugins/slurm.py @@ -366,14 +366,14 @@ def _get_submit_script_header(self, job_tmpl): lines.append(f'#SBATCH --time={days:d}-{hours:02d}:{minutes:02d}:{seconds:02d}') # It is the memory per node, not per cpu! - if job_tmpl.max_memory_kb: + if job_tmpl.max_memory_kb is not None: try: physical_memory_kb = int(job_tmpl.max_memory_kb) - if physical_memory_kb <= 0: + if physical_memory_kb < 0: # 0 is allowed and means no limit (https://slurm.schedmd.com/sbatch.html) raise ValueError except ValueError: raise ValueError( - f'max_memory_kb must be a positive integer (in kB)! It is instead `{job_tmpl.max_memory_kb}`' + f'max_memory_kb must be a non-negative integer (in kB)! It is instead `{job_tmpl.max_memory_kb}`' ) # --mem: Specify the real memory required per node in MegaBytes. # --mem and --mem-per-cpu are mutually exclusive. diff --git a/tests/schedulers/test_slurm.py b/tests/schedulers/test_slurm.py index 55a9af459d..1ba991a5d3 100644 --- a/tests/schedulers/test_slurm.py +++ b/tests/schedulers/test_slurm.py @@ -362,6 +362,58 @@ def test_submit_script_with_num_cores_per_machine_and_mpiproc2(self): num_machines=1, num_mpiprocs_per_machine=1, num_cores_per_machine=24, num_cores_per_mpiproc=23 ) + def test_submit_script_with_mem(self): + """Test to verify if script can be created with memory specification. + + It should pass this check: + if physical_memory_kb < 0: # 0 is allowed and means no limit (https://slurm.schedmd.com/sbatch.html) + raise ValueError + and correctly set the memory value in the script with the --mem option. + """ + from aiida.common.datastructures import CodeRunMode + from aiida.schedulers.datastructures import JobTemplate, JobTemplateCodeInfo + + scheduler = SlurmScheduler() + job_tmpl = JobTemplate() + + job_tmpl.uuid = str(uuid.uuid4()) + job_tmpl.max_wallclock_seconds = 24 * 3600 + tmpl_code_info = JobTemplateCodeInfo() + tmpl_code_info.cmdline_params = ['mpirun', '-np', '23', 'pw.x', '-npool', '1'] + tmpl_code_info.stdin_name = 'aiida.in' + job_tmpl.codes_info = [tmpl_code_info] + job_tmpl.codes_run_mode = CodeRunMode.SERIAL + job_tmpl.job_resource = scheduler.create_job_resource(num_machines=1, num_mpiprocs_per_machine=1) + # Check for a regular (positive) value + job_tmpl.max_memory_kb = 316 * 1024 + submit_script_text = scheduler.get_submit_script(job_tmpl) + assert '#SBATCH --mem=316' in submit_script_text + # Check for the special zero value + job_tmpl.max_memory_kb = 0 + submit_script_text = scheduler.get_submit_script(job_tmpl) + assert '#SBATCH --mem=0' in submit_script_text + + def test_submit_script_with_negative_mem_value(self): + """Test to verify if script can be created with an invalid memory value. + + It should fail in check: + if physical_memory_kb < 0: # 0 is allowed and means no limit (https://slurm.schedmd.com/sbatch.html) + raise ValueError + """ + import re + + from aiida.schedulers.datastructures import JobTemplate + + scheduler = SlurmScheduler() + job_tmpl = JobTemplate() + + with pytest.raises( + ValueError, match=re.escape('max_memory_kb must be a non-negative integer (in kB)! It is instead `-9`') + ): + job_tmpl.job_resource = scheduler.create_job_resource(num_machines=1, num_mpiprocs_per_machine=1) + job_tmpl.max_memory_kb = -9 + scheduler.get_submit_script(job_tmpl) + def test_submit_script_rerunnable(self): """Test the creation of a submission script with the `rerunnable` option.""" from aiida.common.datastructures import CodeRunMode From 8350df0cb0c48588899d732feb26ce7e43903173 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Tue, 5 Nov 2024 11:39:46 +0100 Subject: [PATCH 14/31] CLI: Check user execute in `verdi code test` for `InstalledCode` (#6597) An additional test is added for user execute permissions in the `validate_filepath_executable` method of `InstalledCode` called by `verdi code test`. This can help debugging (or even avoiding) the issue of `touch`ing a dummy code, registering it in AiiDA, and then not being able to use it due to the script not being executable (as default Linux file permissions are `rw-rw-r--`). Some of us have actually encountered this while developing, and the resulting issue was difficult to debug. With this change, one should be able to find it quicker using `verdi code test`. --- src/aiida/orm/nodes/data/code/installed.py | 14 ++++++++++++++ tests/orm/data/code/test_installed.py | 14 +++++++++++++- 2 files changed, 27 insertions(+), 1 deletion(-) diff --git a/src/aiida/orm/nodes/data/code/installed.py b/src/aiida/orm/nodes/data/code/installed.py index 5f9fb39749..7546c2acb8 100644 --- a/src/aiida/orm/nodes/data/code/installed.py +++ b/src/aiida/orm/nodes/data/code/installed.py @@ -137,6 +137,13 @@ def validate_filepath_executable(self): with override_log_level(): # Temporarily suppress noisy logging with self.computer.get_transport() as transport: file_exists = transport.isfile(str(self.filepath_executable)) + if file_exists: + mode = transport.get_mode(str(self.filepath_executable)) + # `format(mode, 'b')` with default permissions + # gives 110110100, representing rw-rw-r-- + # Check on index 2 if user has execute + user_has_execute = format(mode, 'b')[2] == '1' + except Exception as exception: raise exceptions.ValidationError( 'Could not connect to the configured computer to determine whether the specified executable exists.' @@ -147,6 +154,13 @@ def validate_filepath_executable(self): f'The provided remote absolute path `{self.filepath_executable}` does not exist on the computer.' ) + if not user_has_execute: + execute_msg = ( + f'The file at the remote absolute path `{self.filepath_executable}` exists, ' + 'but might not actually be executable. Check the permissions.' + ) + raise exceptions.ValidationError(execute_msg) + def can_run_on_computer(self, computer: Computer) -> bool: """Return whether the code can run on a given computer. diff --git a/tests/orm/data/code/test_installed.py b/tests/orm/data/code/test_installed.py index 928d718080..15c297f43e 100644 --- a/tests/orm/data/code/test_installed.py +++ b/tests/orm/data/code/test_installed.py @@ -101,10 +101,15 @@ def computer(request, aiida_computer_local, aiida_computer_ssh): @pytest.mark.usefixtures('aiida_profile_clean') @pytest.mark.parametrize('computer', ('core.local', 'core.ssh'), indirect=True) -def test_validate_filepath_executable(ssh_key, computer, bash_path): +def test_validate_filepath_executable(ssh_key, computer, bash_path, tmp_path): """Test the :meth:`aiida.orm.nodes.data.code.installed.InstalledCode.validate_filepath_executable` method.""" + filepath_executable = '/usr/bin/not-existing' + dummy_executable = tmp_path / 'dummy.sh' + # Default Linux permissions are 664, so file is not executable + dummy_executable.touch() code = InstalledCode(computer=computer, filepath_executable=filepath_executable) + dummy_code = InstalledCode(computer=computer, filepath_executable=str(dummy_executable)) with pytest.raises(ValidationError, match=r'Could not connect to the configured computer.*'): code.validate_filepath_executable() @@ -117,6 +122,13 @@ def test_validate_filepath_executable(ssh_key, computer, bash_path): with pytest.raises(ValidationError, match=r'The provided remote absolute path .* does not exist on the computer\.'): code.validate_filepath_executable() + with pytest.raises( + ValidationError, + match=r'The file at the remote absolute path .* exists, but might not actually be executable\.', + ): + # Didn't put this in the if, using transport.put, as we anyway only connect to localhost via SSH in this test + dummy_code.validate_filepath_executable() + code.filepath_executable = str(bash_path.absolute()) code.validate_filepath_executable() From b7c82867b5cca08f1ee77bda31bf11d77e96b548 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Mon, 4 Nov 2024 17:20:51 +0100 Subject: [PATCH 15/31] Add `verdi devel launch-multiply-add` --- docs/source/reference/command_line.rst | 1 + src/aiida/cmdline/commands/cmd_devel.py | 51 +++++++++++++++++++++++++ tests/cmdline/commands/test_devel.py | 37 +++++++++++++++++- 3 files changed, 88 insertions(+), 1 deletion(-) diff --git a/docs/source/reference/command_line.rst b/docs/source/reference/command_line.rst index c42c331ac9..1e8370e5e8 100644 --- a/docs/source/reference/command_line.rst +++ b/docs/source/reference/command_line.rst @@ -197,6 +197,7 @@ Below is a list with all available subcommands. check-load-time Check for common indicators that slowdown `verdi`. check-undesired-imports Check that verdi does not import python modules it shouldn't. launch-add Launch an ``ArithmeticAddCalculation``. + launch-multiply-add Launch a ``MultipylAddWorkChain``. rabbitmq Commands to interact with RabbitMQ. run-sql Run a raw SQL command on the profile database (only... validate-plugins Validate all plugins by checking they can be loaded. diff --git a/src/aiida/cmdline/commands/cmd_devel.py b/src/aiida/cmdline/commands/cmd_devel.py index d42f0f1451..f8c89d14c1 100644 --- a/src/aiida/cmdline/commands/cmd_devel.py +++ b/src/aiida/cmdline/commands/cmd_devel.py @@ -180,6 +180,57 @@ def devel_launch_arithmetic_add(code, daemon, sleep): echo.echo_warning(f'ArithmeticAddCalculation<{node.pk}> did not finish successfully.') +@verdi_devel.command('launch-multiply-add') +@options.CODE(type=types.CodeParamType(entry_point='core.arithmetic.add')) +@click.option('-d', '--daemon', is_flag=True, help='Submit to the daemon instead of running blockingly.') +def devel_launch_multiply_add(code, daemon): + """Launch a ``MultipylAddWorkChain``. + + Unless specified with the option ``--code``, a suitable ``Code`` is automatically setup. By default the command + configures ``bash`` on the ``localhost``. If the localhost is not yet configured as a ``Computer``, that is also + done automatically. + """ + from shutil import which + + from aiida.engine import run_get_node, submit + from aiida.orm import InstalledCode, Int, load_code + from aiida.plugins.factories import WorkflowFactory + + default_calc_job_plugin = 'core.arithmetic.add' + + if not code: + try: + code = load_code('bash@localhost') + except exceptions.NotExistent: + localhost = prepare_localhost() + code = InstalledCode( + label='bash', + computer=localhost, + filepath_executable=which('bash'), + default_calc_job_plugin=default_calc_job_plugin, + ).store() + else: + assert code.default_calc_job_plugin == default_calc_job_plugin + + multiply_add = WorkflowFactory('core.arithmetic.multiply_add') + inputs = { + 'x': Int(1), + 'y': Int(1), + 'z': Int(1), + 'code': code, + } + + if daemon: + node = submit(multiply_add, inputs=inputs) + echo.echo_success(f'Submitted workflow `{node}`') + else: + _, node = run_get_node(multiply_add, inputs) + if node.is_finished_ok: + echo.echo_success(f'MultiplyAddWorkChain<{node.pk}> finished successfully.') + else: + echo.echo_warning(f'MultiplyAddWorkChain<{node.pk}> did not finish successfully.') + + def prepare_localhost(): """Prepare and return the localhost as ``Computer``. diff --git a/tests/cmdline/commands/test_devel.py b/tests/cmdline/commands/test_devel.py index f164813625..4cb7f73808 100644 --- a/tests/cmdline/commands/test_devel.py +++ b/tests/cmdline/commands/test_devel.py @@ -12,7 +12,7 @@ import pytest from aiida.cmdline.commands import cmd_devel -from aiida.orm import Node, ProcessNode, QueryBuilder +from aiida.orm import Node, ProcessNode, QueryBuilder, WorkChainNode @pytest.mark.requires_psql @@ -56,3 +56,38 @@ def test_launch_add_code(run_cli_command, aiida_code_installed): node = QueryBuilder().append(ProcessNode, tag='node').order_by({'node': {'ctime': 'desc'}}).first(flat=True) assert node.is_finished_ok + + +def test_launch_multiply_add(run_cli_command): + """Test ``verdi devel launch-multiply-add``. + + Start with a clean profile such that the functionality that automatically sets up the localhost computer and code is + tested properly. + """ + result = run_cli_command(cmd_devel.devel_launch_multiply_add) + assert re.search(r'Warning: No `localhost` computer exists yet: creating and configuring', result.stdout) + assert re.search(r'Success: MultiplyAddWorkChain<.*> finished successfully', result.stdout) + + node = QueryBuilder().append(WorkChainNode, tag='node').order_by({'node': {'ctime': 'desc'}}).first(flat=True) + assert node.is_finished_ok + + +@pytest.mark.usefixtures('started_daemon_client') +def test_launch_multiply_add_daemon(run_cli_command, submit_and_await): + """Test ``verdi devel launch-multiply-add`` with the ``--daemon`` flag.""" + result = run_cli_command(cmd_devel.devel_launch_multiply_add, ['--daemon']) + assert re.search(r'Submitted workflow', result.stdout) + + node = QueryBuilder().append(ProcessNode, tag='node').order_by({'node': {'ctime': 'desc'}}).first(flat=True) + submit_and_await(node) + assert node.is_finished_ok + + +def test_launch_add_multiply_code(run_cli_command, aiida_code_installed): + """Test ``verdi devel launch-multiply-add`` passing an explicit ``Code``.""" + code = aiida_code_installed(default_calc_job_plugin='core.arithmetic.add', filepath_executable='/bin/bash') + result = run_cli_command(cmd_devel.devel_launch_multiply_add, ['-X', str(code.pk)]) + assert not re.search(r'Warning: No `localhost` computer exists yet: creating and configuring', result.stdout) + + node = QueryBuilder().append(ProcessNode, tag='node').order_by({'node': {'ctime': 'desc'}}).first(flat=True) + assert node.is_finished_ok From 2ed19dcd824e1b9a5003dae93613a56ecca7c3a7 Mon Sep 17 00:00:00 2001 From: Julian Geiger Date: Tue, 5 Nov 2024 11:09:28 +0100 Subject: [PATCH 16/31] Add `aiida_profile_clean` to `test_devel.py` Without it, the `localhost` computer would have already been created in the tests for `launch-add`, so the first test for `launch-multiply-add` would fail. --- tests/cmdline/commands/test_devel.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/tests/cmdline/commands/test_devel.py b/tests/cmdline/commands/test_devel.py index 4cb7f73808..856073f524 100644 --- a/tests/cmdline/commands/test_devel.py +++ b/tests/cmdline/commands/test_devel.py @@ -23,6 +23,7 @@ def test_run_sql(run_cli_command): assert str(Node.collection.count()) in result.output, result.output +@pytest.mark.usefixtures('aiida_profile_clean') def test_launch_add(run_cli_command): """Test ``verdi devel launch-add``. @@ -58,6 +59,7 @@ def test_launch_add_code(run_cli_command, aiida_code_installed): assert node.is_finished_ok +@pytest.mark.usefixtures('aiida_profile_clean') def test_launch_multiply_add(run_cli_command): """Test ``verdi devel launch-multiply-add``. From 6f5c35ed16beaf585e2c3045cab5ed4fb0ef3bc1 Mon Sep 17 00:00:00 2001 From: Ali Khosravi Date: Tue, 5 Nov 2024 16:53:02 +0100 Subject: [PATCH 17/31] `Transport` & `Engine`: factor out `getcwd()` & `chdir()` for compatibility with upcoming async transport (#6594) The practice of setting the working directory for a whole instance of the Transport class is abandoned. This is done, by *always* passing absolute paths to `Transport`, instead of relative paths. However, both `getcwd()` and `chdir()` remain in the code base, for backward compatibility. From now on, any change in `aiida-core`, should respect the new practice. --- src/aiida/calculations/monitors/base.py | 7 +- src/aiida/cmdline/commands/cmd_computer.py | 6 +- src/aiida/engine/daemon/execmanager.py | 135 ++++++++++-------- .../engine/processes/calcjobs/calcjob.py | 1 - src/aiida/engine/processes/calcjobs/tasks.py | 1 - src/aiida/orm/nodes/data/remote/base.py | 49 +++---- src/aiida/orm/utils/remote.py | 5 +- src/aiida/schedulers/plugins/bash.py | 7 +- src/aiida/schedulers/scheduler.py | 2 +- src/aiida/transports/plugins/local.py | 43 +++--- src/aiida/transports/plugins/ssh.py | 53 ++++--- src/aiida/transports/transport.py | 71 +++++++-- tests/conftest.py | 30 ++++ tests/engine/daemon/test_execmanager.py | 5 +- tests/transports/test_all_plugins.py | 3 +- 15 files changed, 257 insertions(+), 161 deletions(-) diff --git a/src/aiida/calculations/monitors/base.py b/src/aiida/calculations/monitors/base.py index e165647ea4..459f4eba9d 100644 --- a/src/aiida/calculations/monitors/base.py +++ b/src/aiida/calculations/monitors/base.py @@ -3,6 +3,7 @@ from __future__ import annotations import tempfile +from pathlib import Path from aiida.orm import CalcJobNode from aiida.transports import Transport @@ -19,7 +20,11 @@ def always_kill(node: CalcJobNode, transport: Transport) -> str | None: :returns: A string if the job should be killed, `None` otherwise. """ with tempfile.NamedTemporaryFile('w+') as handle: - transport.getfile('_aiidasubmit.sh', handle.name) + cwd = node.get_remote_workdir() + if cwd is None: + raise ValueError('The remote work directory cannot be None') + + transport.getfile(str(Path(cwd).joinpath('_aiidasubmit.sh')), handle.name) handle.seek(0) output = handle.read() diff --git a/src/aiida/cmdline/commands/cmd_computer.py b/src/aiida/cmdline/commands/cmd_computer.py index 4cdfc2176f..42feb36d25 100644 --- a/src/aiida/cmdline/commands/cmd_computer.py +++ b/src/aiida/cmdline/commands/cmd_computer.py @@ -134,11 +134,7 @@ def _computer_create_temp_file(transport, scheduler, authinfo, computer): file_content = f"Test from 'verdi computer test' on {datetime.datetime.now().isoformat()}" workdir = authinfo.get_workdir().format(username=transport.whoami()) - try: - transport.chdir(workdir) - except OSError: - transport.makedirs(workdir) - transport.chdir(workdir) + transport.makedirs(workdir, ignore_existing=True) with tempfile.NamedTemporaryFile(mode='w+') as tempf: fname = os.path.split(tempf.name)[1] diff --git a/src/aiida/engine/daemon/execmanager.py b/src/aiida/engine/daemon/execmanager.py index 045347404c..73c30cab61 100644 --- a/src/aiida/engine/daemon/execmanager.py +++ b/src/aiida/engine/daemon/execmanager.py @@ -15,10 +15,10 @@ from __future__ import annotations import os -import pathlib import shutil from collections.abc import Mapping from logging import LoggerAdapter +from pathlib import Path from tempfile import NamedTemporaryFile, TemporaryDirectory from typing import TYPE_CHECKING, Any, List, Optional, Tuple, Union from typing import Mapping as MappingType @@ -103,7 +103,7 @@ def upload_calculation( # If we are performing a dry-run, the working directory should actually be a local folder that should already exist if dry_run: - workdir = transport.getcwd() + workdir = Path(folder.abspath) else: remote_user = transport.whoami() remote_working_directory = computer.get_workdir().format(username=remote_user) @@ -114,16 +114,13 @@ def upload_calculation( ) # If it already exists, no exception is raised - try: - transport.chdir(remote_working_directory) - except OSError: + if not transport.path_exists(remote_working_directory): logger.debug( - f'[submission of calculation {node.pk}] Unable to ' - f'chdir in {remote_working_directory}, trying to create it' + f'[submission of calculation {node.pk}] Path ' + f'{remote_working_directory} does not exist, trying to create it' ) try: transport.makedirs(remote_working_directory) - transport.chdir(remote_working_directory) except EnvironmentError as exc: raise exceptions.ConfigurationError( f'[submission of calculation {node.pk}] ' @@ -135,20 +132,18 @@ def upload_calculation( # in the calculation properties using _set_remote_dir # and I do not have to know the logic, but I just need to # read the absolute path from the calculation properties. - transport.mkdir(calc_info.uuid[:2], ignore_existing=True) - transport.chdir(calc_info.uuid[:2]) - transport.mkdir(calc_info.uuid[2:4], ignore_existing=True) - transport.chdir(calc_info.uuid[2:4]) + workdir = Path(remote_working_directory).joinpath(calc_info.uuid[:2], calc_info.uuid[2:4]) + transport.makedirs(str(workdir), ignore_existing=True) try: # The final directory may already exist, most likely because this function was already executed once, but - # failed and as a result was rescheduled by the eninge. In this case it would be fine to delete the folder + # failed and as a result was rescheduled by the engine. In this case it would be fine to delete the folder # and create it from scratch, except that we cannot be sure that this the actual case. Therefore, to err on # the safe side, we move the folder to the lost+found directory before recreating the folder from scratch - transport.mkdir(calc_info.uuid[4:]) + transport.mkdir(str(workdir.joinpath(calc_info.uuid[4:]))) except OSError: # Move the existing directory to lost+found, log a warning and create a clean directory anyway - path_existing = os.path.join(transport.getcwd(), calc_info.uuid[4:]) + path_existing = os.path.join(str(workdir), calc_info.uuid[4:]) path_lost_found = os.path.join(remote_working_directory, REMOTE_WORK_DIRECTORY_LOST_FOUND) path_target = os.path.join(path_lost_found, calc_info.uuid) logger.warning( @@ -161,13 +156,11 @@ def upload_calculation( transport.rmtree(path_existing) # Now we can create a clean folder for this calculation - transport.mkdir(calc_info.uuid[4:]) + transport.mkdir(str(workdir.joinpath(calc_info.uuid[4:]))) finally: - transport.chdir(calc_info.uuid[4:]) + workdir = workdir.joinpath(calc_info.uuid[4:]) - # I store the workdir of the calculation for later file retrieval - workdir = transport.getcwd() - node.set_remote_workdir(workdir) + node.set_remote_workdir(str(workdir)) # I first create the code files, so that the code can put # default files to be overwritten by the plugin itself. @@ -178,22 +171,25 @@ def upload_calculation( # Note: this will possibly overwrite files for root, dirnames, filenames in code.base.repository.walk(): # mkdir of root - transport.makedirs(str(root), ignore_existing=True) + transport.makedirs(str(workdir.joinpath(root)), ignore_existing=True) # remotely mkdir first for dirname in dirnames: - transport.makedirs(str(root / dirname), ignore_existing=True) + transport.makedirs(str(workdir.joinpath(root, dirname)), ignore_existing=True) # Note, once #2579 is implemented, use the `node.open` method instead of the named temporary file in # combination with the new `Transport.put_object_from_filelike` # Since the content of the node could potentially be binary, we read the raw bytes and pass them on for filename in filenames: with NamedTemporaryFile(mode='wb+') as handle: - content = code.base.repository.get_object_content((pathlib.Path(root) / filename), mode='rb') + content = code.base.repository.get_object_content(Path(root) / filename, mode='rb') handle.write(content) handle.flush() - transport.put(handle.name, str(root / filename)) - transport.chmod(str(code.filepath_executable), 0o755) # rwxr-xr-x + transport.put(handle.name, str(workdir.joinpath(root, filename))) + if code.filepath_executable.is_absolute(): + transport.chmod(str(code.filepath_executable), 0o755) # rwxr-xr-x + else: + transport.chmod(str(workdir.joinpath(code.filepath_executable)), 0o755) # rwxr-xr-x # local_copy_list is a list of tuples, each with (uuid, dest_path, rel_path) # NOTE: validation of these lists are done inside calculation.presubmit() @@ -210,20 +206,22 @@ def upload_calculation( for file_copy_operation in file_copy_operation_order: if file_copy_operation is FileCopyOperation.LOCAL: - _copy_local_files(logger, node, transport, inputs, local_copy_list) + _copy_local_files(logger, node, transport, inputs, local_copy_list, workdir=workdir) elif file_copy_operation is FileCopyOperation.REMOTE: if not dry_run: - _copy_remote_files(logger, node, computer, transport, remote_copy_list, remote_symlink_list) + _copy_remote_files( + logger, node, computer, transport, remote_copy_list, remote_symlink_list, workdir=workdir + ) elif file_copy_operation is FileCopyOperation.SANDBOX: if not dry_run: - _copy_sandbox_files(logger, node, transport, folder) + _copy_sandbox_files(logger, node, transport, folder, workdir=workdir) else: raise RuntimeError(f'file copy operation {file_copy_operation} is not yet implemented.') # In a dry_run, the working directory is the raw input folder, which will already contain these resources if dry_run: if remote_copy_list: - filepath = os.path.join(workdir, '_aiida_remote_copy_list.txt') + filepath = os.path.join(str(workdir), '_aiida_remote_copy_list.txt') with open(filepath, 'w', encoding='utf-8') as handle: # type: ignore[assignment] for _, remote_abs_path, dest_rel_path in remote_copy_list: handle.write( @@ -232,7 +230,7 @@ def upload_calculation( ) if remote_symlink_list: - filepath = os.path.join(workdir, '_aiida_remote_symlink_list.txt') + filepath = os.path.join(str(workdir), '_aiida_remote_symlink_list.txt') with open(filepath, 'w', encoding='utf-8') as handle: # type: ignore[assignment] for _, remote_abs_path, dest_rel_path in remote_symlink_list: handle.write( @@ -276,12 +274,12 @@ def upload_calculation( node.base.repository._update_repository_metadata() if not dry_run: - return RemoteData(computer=computer, remote_path=workdir) + return RemoteData(computer=computer, remote_path=str(workdir)) return None -def _copy_remote_files(logger, node, computer, transport, remote_copy_list, remote_symlink_list): +def _copy_remote_files(logger, node, computer, transport, remote_copy_list, remote_symlink_list, workdir: Path): """Perform the copy instructions of the ``remote_copy_list`` and ``remote_symlink_list``.""" for remote_computer_uuid, remote_abs_path, dest_rel_path in remote_copy_list: if remote_computer_uuid == computer.uuid: @@ -290,7 +288,7 @@ def _copy_remote_files(logger, node, computer, transport, remote_copy_list, remo f'remotely, directly on the machine {computer.label}' ) try: - transport.copy(remote_abs_path, dest_rel_path) + transport.copy(remote_abs_path, str(workdir.joinpath(dest_rel_path))) except FileNotFoundError: logger.warning( f'[submission of calculation {node.pk}] Unable to copy remote ' @@ -314,10 +312,10 @@ def _copy_remote_files(logger, node, computer, transport, remote_copy_list, remo f'[submission of calculation {node.pk}] copying {dest_rel_path} remotely, ' f'directly on the machine {computer.label}' ) - remote_dirname = pathlib.Path(dest_rel_path).parent + remote_dirname = Path(dest_rel_path).parent try: - transport.makedirs(remote_dirname, ignore_existing=True) - transport.symlink(remote_abs_path, dest_rel_path) + transport.makedirs(str(workdir.joinpath(remote_dirname)), ignore_existing=True) + transport.symlink(remote_abs_path, str(workdir.joinpath(dest_rel_path))) except OSError: logger.warning( f'[submission of calculation {node.pk}] Unable to create remote symlink ' @@ -330,9 +328,8 @@ def _copy_remote_files(logger, node, computer, transport, remote_copy_list, remo ) -def _copy_local_files(logger, node, transport, inputs, local_copy_list): +def _copy_local_files(logger, node, transport, inputs, local_copy_list, workdir: Path): """Perform the copy instructions of the ``local_copy_list``.""" - for uuid, filename, target in local_copy_list: logger.debug(f'[submission of calculation {node.uuid}] copying local file/folder to {target}') @@ -348,7 +345,7 @@ def _copy_local_files(logger, node, transport, inputs, local_copy_list): # The transport class can only copy files directly from the file system, so the files in the source node's repo # have to first be copied to a temporary directory on disk. with TemporaryDirectory() as tmpdir: - dirpath = pathlib.Path(tmpdir) + dirpath = Path(tmpdir) # If no explicit source filename is defined, we assume the top-level directory filename_source = filename or '.' @@ -359,14 +356,14 @@ def _copy_local_files(logger, node, transport, inputs, local_copy_list): # The logic below takes care of an edge case where the source is a file but the target is a directory. In # this case, the v2.5.1 implementation would raise an `IsADirectoryError` exception, because it would try # to open the directory in the sandbox folder as a file when writing the contents. - if file_type_source == FileType.FILE and target and transport.isdir(target): + if file_type_source == FileType.FILE and target and transport.isdir(str(workdir.joinpath(target))): raise IsADirectoryError # In case the source filename is specified and it is a directory that already exists in the remote, we # want to avoid nested directories in the target path to replicate the behavior of v2.5.1. This is done by # setting the target filename to '.', which means the contents of the node will be copied in the top level # of the temporary directory, whose contents are then copied into the target directory. - if filename and transport.isdir(filename): + if filename and transport.isdir(str(workdir.joinpath(filename))): filename_target = '.' filepath_target = (dirpath / filename_target).resolve().absolute() @@ -375,21 +372,25 @@ def _copy_local_files(logger, node, transport, inputs, local_copy_list): if file_type_source == FileType.DIRECTORY: # If the source object is a directory, we copy its entire contents data_node.base.repository.copy_tree(filepath_target, filename_source) - transport.put(f'{dirpath}/*', target or '.', overwrite=True) + transport.put( + f'{dirpath}/*', + str(workdir.joinpath(target)) if target else str(workdir.joinpath('.')), + overwrite=True, + ) else: # Otherwise, simply copy the file with filepath_target.open('wb') as handle: with data_node.base.repository.open(filename_source, 'rb') as source: shutil.copyfileobj(source, handle) - transport.makedirs(str(pathlib.Path(target).parent), ignore_existing=True) - transport.put(str(filepath_target), target) + transport.makedirs(str(workdir.joinpath(Path(target).parent)), ignore_existing=True) + transport.put(str(filepath_target), str(workdir.joinpath(target))) -def _copy_sandbox_files(logger, node, transport, folder): +def _copy_sandbox_files(logger, node, transport, folder, workdir: Path): """Copy the contents of the sandbox folder to the working directory.""" for filename in folder.get_content_list(): logger.debug(f'[submission of calculation {node.pk}] copying file/folder {filename}...') - transport.put(folder.get_abs_path(filename), filename) + transport.put(folder.get_abs_path(filename), str(workdir.joinpath(filename))) def submit_calculation(calculation: CalcJobNode, transport: Transport) -> str | ExitCode: @@ -454,14 +455,14 @@ def stash_calculation(calculation: CalcJobNode, transport: Transport) -> None: EXEC_LOGGER.debug(f'stashing files for calculation<{calculation.pk}>: {source_list}', extra=logger_extra) uuid = calculation.uuid - source_basepath = pathlib.Path(calculation.get_remote_workdir()) - target_basepath = pathlib.Path(stash_options['target_base']) / uuid[:2] / uuid[2:4] / uuid[4:] + source_basepath = Path(calculation.get_remote_workdir()) + target_basepath = Path(stash_options['target_base']) / uuid[:2] / uuid[2:4] / uuid[4:] for source_filename in source_list: if transport.has_magic(source_filename): copy_instructions = [] for globbed_filename in transport.glob(str(source_basepath / source_filename)): - target_filepath = target_basepath / pathlib.Path(globbed_filename).relative_to(source_basepath) + target_filepath = target_basepath / Path(globbed_filename).relative_to(source_basepath) copy_instructions.append((globbed_filename, target_filepath)) else: copy_instructions = [(source_basepath / source_filename, target_basepath / source_filename)] @@ -523,8 +524,6 @@ def retrieve_calculation( retrieved_files = FolderData() with transport: - transport.chdir(workdir) - # First, retrieve the files of folderdata retrieve_list = calculation.get_retrieve_list() retrieve_temporary_list = calculation.get_retrieve_temporary_list() @@ -601,10 +600,10 @@ def retrieve_files_from_list( * a string * a list - If it is a string, it represents the remote absolute filepath of the file. + If it is a string, it represents the remote absolute or relative filepath of the file. If the item is a list, the elements will correspond to the following: - * remotepath + * remotepath (relative path) * localpath * depth @@ -616,18 +615,21 @@ def retrieve_files_from_list( :param folder: an absolute path to a folder that contains the files to copy. :param retrieve_list: the list of files to retrieve. """ + workdir = Path(calculation.get_remote_workdir()) for item in retrieve_list: if isinstance(item, (list, tuple)): tmp_rname, tmp_lname, depth = item # if there are more than one file I do something differently if transport.has_magic(tmp_rname): - remote_names = transport.glob(tmp_rname) + remote_names = transport.glob(str(workdir.joinpath(tmp_rname))) local_names = [] for rem in remote_names: + # get the relative path so to make local_names relative + rel_rem = os.path.relpath(rem, str(workdir)) if depth is None: - local_names.append(os.path.join(tmp_lname, rem)) + local_names.append(os.path.join(tmp_lname, rel_rem)) else: - to_append = rem.split(os.path.sep)[-depth:] if depth > 0 else [] + to_append = rel_rem.split(os.path.sep)[-depth:] if depth > 0 else [] local_names.append(os.path.sep.join([tmp_lname] + to_append)) else: remote_names = [tmp_rname] @@ -638,13 +640,22 @@ def retrieve_files_from_list( new_folder = os.path.join(folder, os.path.split(this_local_file)[0]) if not os.path.exists(new_folder): os.makedirs(new_folder) - elif transport.has_magic(item): # it is a string - remote_names = transport.glob(item) - local_names = [os.path.split(rem)[1] for rem in remote_names] else: - remote_names = [item] - local_names = [os.path.split(item)[1]] + abs_item = item if item.startswith('/') else str(workdir.joinpath(item)) + + if transport.has_magic(abs_item): + remote_names = transport.glob(abs_item) + local_names = [os.path.split(rem)[1] for rem in remote_names] + else: + remote_names = [abs_item] + local_names = [os.path.split(abs_item)[1]] for rem, loc in zip(remote_names, local_names): transport.logger.debug(f"[retrieval of calc {calculation.pk}] Trying to retrieve remote item '{rem}'") - transport.get(rem, os.path.join(folder, loc), ignore_nonexisting=True) + + if rem.startswith('/'): + to_get = rem + else: + to_get = str(workdir.joinpath(rem)) + + transport.get(to_get, os.path.join(folder, loc), ignore_nonexisting=True) diff --git a/src/aiida/engine/processes/calcjobs/calcjob.py b/src/aiida/engine/processes/calcjobs/calcjob.py index 70a72ba307..d5acfca5fc 100644 --- a/src/aiida/engine/processes/calcjobs/calcjob.py +++ b/src/aiida/engine/processes/calcjobs/calcjob.py @@ -643,7 +643,6 @@ def _perform_dry_run(self): with LocalTransport() as transport: with SubmitTestFolder() as folder: calc_info = self.presubmit(folder) - transport.chdir(folder.abspath) upload_calculation(self.node, transport, calc_info, folder, inputs=self.inputs, dry_run=True) self.node.dry_run_info = { # type: ignore[attr-defined] 'folder': folder.abspath, diff --git a/src/aiida/engine/processes/calcjobs/tasks.py b/src/aiida/engine/processes/calcjobs/tasks.py index 8b8231634f..1059d277ba 100644 --- a/src/aiida/engine/processes/calcjobs/tasks.py +++ b/src/aiida/engine/processes/calcjobs/tasks.py @@ -253,7 +253,6 @@ async def task_monitor_job( async def do_monitor(): with transport_queue.request_transport(authinfo) as request: transport = await cancellable.with_interrupt(request) - transport.chdir(node.get_remote_workdir()) return monitors.process(node, transport) try: diff --git a/src/aiida/orm/nodes/data/remote/base.py b/src/aiida/orm/nodes/data/remote/base.py index 9147a58d10..1fc691d113 100644 --- a/src/aiida/orm/nodes/data/remote/base.py +++ b/src/aiida/orm/nodes/data/remote/base.py @@ -58,13 +58,10 @@ def is_empty(self): transport = authinfo.get_transport() with transport: - try: - transport.chdir(self.get_remote_path()) - except OSError: - # If the transport OSError the directory no longer exists and was deleted + if not transport.isdir(self.get_remote_path()): return True - return not transport.listdir() + return not transport.listdir(self.get_remote_path()) def getfile(self, relpath, destpath): """Connects to the remote folder and retrieves the content of a file. @@ -96,22 +93,15 @@ def listdir(self, relpath='.'): authinfo = self.get_authinfo() with authinfo.get_transport() as transport: - try: - full_path = os.path.join(self.get_remote_path(), relpath) - transport.chdir(full_path) - except OSError as exception: - if exception.errno in (2, 20): # directory not existing or not a directory - exc = OSError( - f'The required remote folder {full_path} on {self.computer.label} does not exist, is not a ' - 'directory or has been deleted.' - ) - exc.errno = exception.errno - raise exc from exception - else: - raise + full_path = os.path.join(self.get_remote_path(), relpath) + if not transport.isdir(full_path): + raise OSError( + f'The required remote folder {full_path} on {self.computer.label} does not exist, is not a ' + 'directory or has been deleted.' + ) try: - return transport.listdir() + return transport.listdir(full_path) except OSError as exception: if exception.errno in (2, 20): # directory not existing or not a directory exc = OSError( @@ -132,22 +122,15 @@ def listdir_withattributes(self, path='.'): authinfo = self.get_authinfo() with authinfo.get_transport() as transport: - try: - full_path = os.path.join(self.get_remote_path(), path) - transport.chdir(full_path) - except OSError as exception: - if exception.errno in (2, 20): # directory not existing or not a directory - exc = OSError( - f'The required remote folder {full_path} on {self.computer.label} does not exist, is not a ' - 'directory or has been deleted.' - ) - exc.errno = exception.errno - raise exc from exception - else: - raise + full_path = os.path.join(self.get_remote_path(), path) + if not transport.isdir(full_path): + raise OSError( + f'The required remote folder {full_path} on {self.computer.label} does not exist, is not a ' + 'directory or has been deleted.' + ) try: - return transport.listdir_withattributes() + return transport.listdir_withattributes(full_path) except OSError as exception: if exception.errno in (2, 20): # directory not existing or not a directory exc = OSError( diff --git a/src/aiida/orm/utils/remote.py b/src/aiida/orm/utils/remote.py index 2518791fb8..f55cedc35a 100644 --- a/src/aiida/orm/utils/remote.py +++ b/src/aiida/orm/utils/remote.py @@ -39,11 +39,8 @@ def clean_remote(transport: Transport, path: str) -> None: if not transport.is_open: raise ValueError('the transport should already be open') - basedir, relative_path = os.path.split(path) - try: - transport.chdir(basedir) - transport.rmtree(relative_path) + transport.rmtree(path) except OSError: pass diff --git a/src/aiida/schedulers/plugins/bash.py b/src/aiida/schedulers/plugins/bash.py index 0511a4cb99..f2e1da6db6 100644 --- a/src/aiida/schedulers/plugins/bash.py +++ b/src/aiida/schedulers/plugins/bash.py @@ -26,11 +26,12 @@ class BashCliScheduler(Scheduler, metaclass=abc.ABCMeta): def submit_job(self, working_directory: str, filename: str) -> str | ExitCode: """Submit a job. - :param working_directory: The absolute filepath to the working directory where the job is to be exectued. + :param working_directory: The absolute filepath to the working directory where the job is to be executed. :param filename: The filename of the submission script relative to the working directory. """ - self.transport.chdir(working_directory) - result = self.transport.exec_command_wait(self._get_submit_command(escape_for_bash(filename))) + result = self.transport.exec_command_wait( + self._get_submit_command(escape_for_bash(filename)), workdir=working_directory + ) return self._parse_submit_output(*result) def get_jobs( diff --git a/src/aiida/schedulers/scheduler.py b/src/aiida/schedulers/scheduler.py index 5168762f80..3cd4136984 100644 --- a/src/aiida/schedulers/scheduler.py +++ b/src/aiida/schedulers/scheduler.py @@ -129,7 +129,7 @@ def create_job_resource(cls, **kwargs): def submit_job(self, working_directory: str, filename: str) -> str | ExitCode: """Submit a job. - :param working_directory: The absolute filepath to the working directory where the job is to be exectued. + :param working_directory: The absolute filepath to the working directory where the job is to be executed. :param filename: The filename of the submission script relative to the working directory. :returns: """ diff --git a/src/aiida/transports/plugins/local.py b/src/aiida/transports/plugins/local.py index b8263620d3..755476a066 100644 --- a/src/aiida/transports/plugins/local.py +++ b/src/aiida/transports/plugins/local.py @@ -8,14 +8,6 @@ ########################################################################### """Local transport""" -### -### GP: a note on the local transport: -### I believe that we must not use os.chdir to keep track of the folder -### in which we are, since this may have very nasty side effects in other -### parts of code, and make things not thread-safe. -### we should instead keep track internally of the 'current working directory' -### in the exact same way as paramiko does already. - import contextlib import errno import glob @@ -101,7 +93,11 @@ def curdir(self): raise TransportInternalError('Error, local method called for LocalTransport without opening the channel first') def chdir(self, path): - """Changes directory to path, emulated internally. + """ + PLEASE DON'T USE `chdir()` IN NEW DEVELOPMENTS, INSTEAD DIRECTLY PASS ABSOLUTE PATHS TO INTERFACE. + `chdir()` is DEPRECATED and will be removed in the next major version. + + Changes directory to path, emulated internally. :param path: path to cd into :raise OSError: if the directory does not have read attributes. """ @@ -123,7 +119,11 @@ def normalize(self, path='.'): return os.path.realpath(os.path.join(self.curdir, path)) def getcwd(self): - """Returns the current working directory, emulated by the transport""" + """ + PLEASE DON'T USE `getcwd()` IN NEW DEVELOPMENTS, INSTEAD DIRECTLY PASS ABSOLUTE PATHS TO INTERFACE. + `getcwd()` is DEPRECATED and will be removed in the next major version. + + Returns the current working directory, emulated by the transport""" return self.curdir @staticmethod @@ -695,11 +695,9 @@ def isfile(self, path): return os.path.isfile(os.path.join(self.curdir, path)) @contextlib.contextmanager - def _exec_command_internal(self, command, **kwargs): + def _exec_command_internal(self, command, workdir=None, **kwargs): """Executes the specified command in bash login shell. - Before the command is executed, changes directory to the current - working directory as returned by self.getcwd(). For executing commands and waiting for them to finish, use exec_command_wait. @@ -710,6 +708,10 @@ def _exec_command_internal(self, command, **kwargs): :param command: the command to execute. The command is assumed to be already escaped using :py:func:`aiida.common.escaping.escape_for_bash`. + :param workdir: (optional, default=None) if set, the command will be executed + in the specified working directory. + if None, the command will be executed in the current working directory, + from DEPRECATED `self.getcwd()`. :return: a tuple with (stdin, stdout, stderr, proc), where stdin, stdout and stderr behave as file-like objects, @@ -724,26 +726,35 @@ def _exec_command_internal(self, command, **kwargs): command = bash_commmand + escape_for_bash(command) + if workdir: + cwd = workdir + else: + cwd = self.getcwd() + with subprocess.Popen( command, shell=True, stdin=subprocess.PIPE, stdout=subprocess.PIPE, stderr=subprocess.PIPE, - cwd=self.getcwd(), + cwd=cwd, start_new_session=True, ) as process: yield process - def exec_command_wait_bytes(self, command, stdin=None, **kwargs): + def exec_command_wait_bytes(self, command, stdin=None, workdir=None, **kwargs): """Executes the specified command and waits for it to finish. :param command: the command to execute + :param workdir: (optional, default=None) if set, the command will be executed + in the specified working directory. + if None, the command will be executed in the current working directory, + from DEPRECATED `self.getcwd()`. :return: a tuple with (return_value, stdout, stderr) where stdout and stderr are both bytes and the return_value is an int. """ - with self._exec_command_internal(command) as process: + with self._exec_command_internal(command, workdir) as process: if stdin is not None: # Implicitly assume that the desired encoding is 'utf-8' if I receive a string. # Also, if I get a StringIO, I just read it all in memory and put it into a BytesIO. diff --git a/src/aiida/transports/plugins/ssh.py b/src/aiida/transports/plugins/ssh.py index c62f17a67b..d6159fe46f 100644 --- a/src/aiida/transports/plugins/ssh.py +++ b/src/aiida/transports/plugins/ssh.py @@ -581,7 +581,11 @@ def __str__(self): return f"{'OPEN' if self._is_open else 'CLOSED'} [{conn_info}]" def chdir(self, path): - """Change directory of the SFTP session. Emulated internally by paramiko. + """ + PLEASE DON'T USE `chdir()` IN NEW DEVELOPMENTS, INSTEAD DIRECTLY PASS ABSOLUTE PATHS TO INTERFACE. + `chdir()` is DEPRECATED and will be removed in the next major version. + + Change directory of the SFTP session. Emulated internally by paramiko. Differently from paramiko, if you pass None to chdir, nothing happens and the cwd is unchanged. @@ -646,7 +650,11 @@ def lstat(self, path): return self.sftp.lstat(path) def getcwd(self): - """Return the current working directory for this SFTP session, as + """ + PLEASE DON'T USE `getcwd()` IN NEW DEVELOPMENTS, INSTEAD DIRECTLY PASS ABSOLUTE PATHS TO INTERFACE. + `getcwd()` is DEPRECATED and will be removed in the next major version. + + Return the current working directory for this SFTP session, as emulated by paramiko. If no directory has been set with chdir, this method will return None. But in __enter__ this is set explicitly, so this should never happen within this class. @@ -1218,17 +1226,18 @@ def listdir(self, path='.', pattern=None): :param pattern: returns the list of files matching pattern. Unix only. (Use to emulate ``ls *`` for example) """ - if not pattern: - return self.sftp.listdir(path) if path.startswith('/'): - base_dir = path + abs_dir = path else: - base_dir = os.path.join(self.getcwd(), path) + abs_dir = os.path.join(self.getcwd(), path) - filtered_list = self.glob(os.path.join(base_dir, pattern)) - if not base_dir.endswith('/'): - base_dir += '/' - return [re.sub(base_dir, '', i) for i in filtered_list] + if not pattern: + return self.sftp.listdir(abs_dir) + + filtered_list = self.glob(os.path.join(abs_dir, pattern)) + if not abs_dir.endswith('/'): + abs_dir += '/' + return [re.sub(abs_dir, '', i) for i in filtered_list] def remove(self, path): """Remove a single file at 'path'""" @@ -1276,11 +1285,9 @@ def isfile(self, path): return False raise # Typically if I don't have permissions (errno=13) - def _exec_command_internal(self, command, combine_stderr=False, bufsize=-1): + def _exec_command_internal(self, command, combine_stderr=False, bufsize=-1, workdir=None): """Executes the specified command in bash login shell. - Before the command is executed, changes directory to the current - working directory as returned by self.getcwd(). For executing commands and waiting for them to finish, use exec_command_wait. @@ -1291,6 +1298,10 @@ def _exec_command_internal(self, command, combine_stderr=False, bufsize=-1): stderr on the same buffer (i.e., stdout). Note: If combine_stderr is True, stderr will always be empty. :param bufsize: same meaning of the one used by paramiko. + :param workdir: (optional, default=None) if set, the command will be executed + in the specified working directory. + if None, the command will be executed in the current working directory, + from DEPRECATED `self.getcwd()`, if that has a value. :return: a tuple with (stdin, stdout, stderr, channel), where stdin, stdout and stderr behave as file-like objects, @@ -1300,8 +1311,10 @@ def _exec_command_internal(self, command, combine_stderr=False, bufsize=-1): channel = self.sshclient.get_transport().open_session() channel.set_combine_stderr(combine_stderr) - if self.getcwd() is not None: - escaped_folder = escape_for_bash(self.getcwd()) + if workdir is not None: + command_to_execute = f'cd {workdir} && ( {command} )' + elif (cwd := self.getcwd()) is not None: + escaped_folder = escape_for_bash(cwd) command_to_execute = f'cd {escaped_folder} && ( {command} )' else: command_to_execute = command @@ -1320,7 +1333,9 @@ def _exec_command_internal(self, command, combine_stderr=False, bufsize=-1): return stdin, stdout, stderr, channel - def exec_command_wait_bytes(self, command, stdin=None, combine_stderr=False, bufsize=-1, timeout=0.01): + def exec_command_wait_bytes( + self, command, stdin=None, combine_stderr=False, bufsize=-1, timeout=0.01, workdir=None + ): """Executes the specified command and waits for it to finish. :param command: the command to execute @@ -1330,6 +1345,8 @@ def exec_command_wait_bytes(self, command, stdin=None, combine_stderr=False, buf self._exec_command_internal() :param bufsize: same meaning of paramiko. :param timeout: ssh channel timeout for stdout, stderr. + :param workdir: (optional, default=None) if set, the command will be executed + in the specified working directory. :return: a tuple with (return_value, stdout, stderr) where stdout and stderr are both bytes and the return_value is an int. @@ -1337,7 +1354,9 @@ def exec_command_wait_bytes(self, command, stdin=None, combine_stderr=False, buf import socket import time - ssh_stdin, stdout, stderr, channel = self._exec_command_internal(command, combine_stderr, bufsize=bufsize) + ssh_stdin, stdout, stderr, channel = self._exec_command_internal( + command, combine_stderr, bufsize=bufsize, workdir=workdir + ) if stdin is not None: if isinstance(stdin, str): diff --git a/src/aiida/transports/transport.py b/src/aiida/transports/transport.py index b144a02485..a6d755973e 100644 --- a/src/aiida/transports/transport.py +++ b/src/aiida/transports/transport.py @@ -14,9 +14,11 @@ import re import sys from collections import OrderedDict +from pathlib import Path from aiida.common.exceptions import InternalError from aiida.common.lang import classproperty +from aiida.common.warnings import warn_deprecation __all__ = ('Transport',) @@ -245,13 +247,21 @@ def get_safe_open_interval(self): @abc.abstractmethod def chdir(self, path): - """Change directory to 'path' + """ + DEPRECATED: This method is deprecated and should be removed in the next major version. + PLEASE DON'T USE IT IN THE INTERFACE!! + Change directory to 'path'. :param str path: path to change working directory into. :raises: OSError, if the requested path does not exist :rtype: str """ + warn_deprecation( + '`chdir()` is deprecated and will be removed in the next major version.', + version=3, + ) + @abc.abstractmethod def chmod(self, path, mode): """Change permissions of a path. @@ -363,35 +373,38 @@ def copy_from_remote_to_remote(self, transportdestination, remotesource, remoted transportdestination.put(os.path.join(sandbox.abspath, filename), remotedestination, **kwargs_put) @abc.abstractmethod - def _exec_command_internal(self, command, **kwargs): + def _exec_command_internal(self, command, workdir=None, **kwargs): """Execute the command on the shell, similarly to os.system. - Enforce the execution to be run from the cwd (as given by - self.getcwd), if this is not None. + Enforce the execution to be run from `workdir`. If possible, use the higher-level exec_command_wait function. :param str command: execute the command given as a string + :param workdir: (optional, default=None) if set, the command will be executed + in the specified working directory. :return: stdin, stdout, stderr and the session, when this exists \ (can be None). """ @abc.abstractmethod - def exec_command_wait_bytes(self, command, stdin=None, **kwargs): + def exec_command_wait_bytes(self, command, stdin=None, workdir=None, **kwargs): """Execute the command on the shell, waits for it to finish, and return the retcode, the stdout and the stderr as bytes. - Enforce the execution to be run from the pwd (as given by self.getcwd), if this is not None. + Enforce the execution to be run from workdir, if this is not None. The command implementation can have some additional plugin-specific kwargs. :param str command: execute the command given as a string :param stdin: (optional,default=None) can be a string or a file-like object. + :param workdir: (optional, default=None) if set, the command will be executed + in the specified working directory. :return: a tuple: the retcode (int), stdout (bytes) and stderr (bytes). """ - def exec_command_wait(self, command, stdin=None, encoding='utf-8', **kwargs): + def exec_command_wait(self, command, stdin=None, encoding='utf-8', workdir=None, **kwargs): """Executes the specified command and waits for it to finish. :note: this function also decodes the bytes received into a string with the specified encoding, @@ -406,11 +419,15 @@ def exec_command_wait(self, command, stdin=None, encoding='utf-8', **kwargs): :param command: the command to execute :param stdin: (optional,default=None) can be a string or a file-like object. :param encoding: the encoding to use to decode the byte stream received from the remote command execution. + :param workdir: (optional, default=None) if set, the command will be executed + in the specified working directory. :return: a tuple with (return_value, stdout, stderr) where stdout and stderr are both strings, decoded with the specified encoding. """ - retval, stdout_bytes, stderr_bytes = self.exec_command_wait_bytes(command=command, stdin=stdin, **kwargs) + retval, stdout_bytes, stderr_bytes = self.exec_command_wait_bytes( + command=command, stdin=stdin, workdir=workdir, **kwargs + ) # Return the decoded strings return (retval, stdout_bytes.decode(encoding), stderr_bytes.decode(encoding)) @@ -443,11 +460,19 @@ def gettree(self, remotepath, localpath, *args, **kwargs): @abc.abstractmethod def getcwd(self): - """Get working directory + """ + DEPRECATED: This method is deprecated and should be removed in the next major version. + PLEASE DON'T USE IT IN THE INTERFACE!! + Get working directory :return: a string identifying the current working directory """ + warn_deprecation( + '`getcwd()` is deprecated and will be removed in the next major version.', + version=3, + ) + @abc.abstractmethod def get_attribute(self, path): """Return an object FixedFieldsAttributeDict for file in a given path, @@ -514,6 +539,8 @@ def listdir_withattributes(self, path='.', pattern=None): entries '.' and '..' even if they are present in the directory. :param str path: path to list (default to '.') + if using a relative path, it is relative to the current working directory, + taken from DEPRECATED `self.getcwd()`. :param str pattern: if used, listdir returns a list of files matching filters in Unix style. Unix only. :return: a list of dictionaries, one per entry. @@ -531,9 +558,17 @@ def listdir_withattributes(self, path='.', pattern=None): transport.get_attribute(); isdir is a boolean indicating if the object is a directory or not. """ retlist = [] - full_path = self.getcwd() - for file_name in self.listdir(): - filepath = os.path.join(full_path, file_name) + if path.startswith('/'): + cwd = Path(path).resolve().as_posix() + else: + warn_deprecation( + 'Using relative paths in `listdir_withattributes` is no longer supported ' + 'and will be removed in the next major version.', + version=3, + ) + cwd = self.getcwd() + for file_name in self.listdir(cwd): + filepath = os.path.join(cwd, file_name) attributes = self.get_attribute(filepath) retlist.append({'name': file_name, 'attributes': attributes, 'isdir': self.isdir(filepath)}) return retlist @@ -689,7 +724,18 @@ def glob(self, pathname): """Return a list of paths matching a pathname pattern. The pattern may contain simple shell-style wildcards a la fnmatch. + + :param str pathname: the pathname pattern to match. + It should only be absolute path. + DEPRECATED: using relative path is deprecated. + :return: a list of paths matching the pattern. """ + if not pathname.startswith('/'): + warn_deprecation( + 'Using relative paths across transport in `glob` is deprecated ' + 'and will be removed in the next major version.', + version=3, + ) return list(self.iglob(pathname)) def iglob(self, pathname): @@ -757,6 +803,7 @@ def glob0(self, dirname, basename): return [] def has_magic(self, string): + """Return True if the given string contains any special shell characters.""" return self._MAGIC_CHECK.search(string) is not None def _gotocomputer_string(self, remotedir): diff --git a/tests/conftest.py b/tests/conftest.py index 1dcb644b21..2166cc06c0 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -202,6 +202,36 @@ def _generate_work_chain(entry_point, inputs=None): return _generate_work_chain +@pytest.fixture +def generate_calcjob_node(): + """Generate an instance of a `CalcJobNode`.""" + from aiida.engine import ProcessState + + def _generate_calcjob_node( + process_state: ProcessState = ProcessState.FINISHED, + exit_status: int | None = None, + entry_point: str | None = None, + workdir: pathlib.Path | None = None, + ): + """Generate an instance of a `CalcJobNode`.. + + :param process_state: state to set + :param exit_status: optional exit status, will be set to `0` if `process_state` is `ProcessState.FINISHED` + :return: a `CalcJobNode` instance. + """ + from aiida.orm import CalcJobNode + + if process_state is ProcessState.FINISHED and exit_status is None: + exit_status = 0 + + calcjob_node = CalcJobNode(process_type=entry_point) + calcjob_node.set_remote_workdir(workdir) + + return calcjob_node + + return _generate_calcjob_node + + @pytest.fixture def generate_calculation_node(): """Generate an instance of a `CalculationNode`.""" diff --git a/tests/engine/daemon/test_execmanager.py b/tests/engine/daemon/test_execmanager.py index 5b34f099a7..79692b689b 100644 --- a/tests/engine/daemon/test_execmanager.py +++ b/tests/engine/daemon/test_execmanager.py @@ -111,7 +111,7 @@ def test_hierarchy_utility(file_hierarchy, tmp_path, create_file_hierarchy, seri ) def test_retrieve_files_from_list( tmp_path_factory, - generate_calculation_node, + generate_calcjob_node, file_hierarchy, retrieve_list, expected_hierarchy, @@ -125,8 +125,7 @@ def test_retrieve_files_from_list( create_file_hierarchy(file_hierarchy, source) with LocalTransport() as transport: - node = generate_calculation_node() - transport.chdir(source) + node = generate_calcjob_node(workdir=source) execmanager.retrieve_files_from_list(node, transport, target, retrieve_list) assert serialize_file_hierarchy(target, read_bytes=False) == expected_hierarchy diff --git a/tests/transports/test_all_plugins.py b/tests/transports/test_all_plugins.py index c536b196a2..0a25add0a7 100644 --- a/tests/transports/test_all_plugins.py +++ b/tests/transports/test_all_plugins.py @@ -1305,8 +1305,7 @@ def test_asynchronous_execution(custom_transport): tmpf.write(b'#!/bin/bash\nsleep 10\n') tmpf.flush() - transport.chdir('/tmp') - transport.putfile(tmpf.name, script_fname) + transport.putfile(tmpf.name, os.path.join('/tmp', script_fname)) timestamp_before = time.time() job_id_string = scheduler.submit_job('/tmp', script_fname) From dd866ce816e986285f2c5794f431b6e3c68a369b Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Wed, 6 Nov 2024 20:57:06 +0100 Subject: [PATCH 18/31] Devops: Bump `peter-evans/create-pull-request` (#6576) Bumps the gha-dependencies group with 1 update: [peter-evans/create-pull-request](https://github.com/peter-evans/create-pull-request). Updates `peter-evans/create-pull-request` from 6 to 7 - [Release notes](https://github.com/peter-evans/create-pull-request/releases) - [Commits](https://github.com/peter-evans/create-pull-request/compare/v6...v7) --- updated-dependencies: - dependency-name: peter-evans/create-pull-request dependency-type: direct:production update-type: version-update:semver-major dependency-group: gha-dependencies ... Signed-off-by: dependabot[bot] Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> --- .github/workflows/test-install.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/test-install.yml b/.github/workflows/test-install.yml index 637b48a445..2f919a8c99 100644 --- a/.github/workflows/test-install.yml +++ b/.github/workflows/test-install.yml @@ -306,7 +306,7 @@ jobs: if: steps.check_reqs.outcome == 'Failure' # only run if requirements/ are inconsistent id: create_update_requirements_pr continue-on-error: true - uses: peter-evans/create-pull-request@v6 + uses: peter-evans/create-pull-request@v7 with: branch: update-requirements commit-message: Automated update of requirements/ files. From 779cc29d8a47eddabdf9b274d7fa711220ee1aa9 Mon Sep 17 00:00:00 2001 From: Karl Liu Date: Tue, 12 Nov 2024 10:24:39 +0100 Subject: [PATCH 19/31] implement has_key filter for SQLite backend --- src/aiida/storage/sqlite_zip/orm.py | 8 ++++++-- tests/cmdline/commands/test_calcjob.py | 4 ---- tests/orm/test_querybuilder.py | 1 - tests/storage/sqlite/test_orm.py | 20 +++++++++++++------- tests/test_nodes.py | 1 - 5 files changed, 19 insertions(+), 15 deletions(-) diff --git a/src/aiida/storage/sqlite_zip/orm.py b/src/aiida/storage/sqlite_zip/orm.py index ad0412f006..0f51c12534 100644 --- a/src/aiida/storage/sqlite_zip/orm.py +++ b/src/aiida/storage/sqlite_zip/orm.py @@ -17,7 +17,7 @@ from functools import singledispatch from typing import Any, List, Optional, Tuple, Union -from sqlalchemy import JSON, case, func +from sqlalchemy import JSON, case, func, select from sqlalchemy.orm.util import AliasedClass from sqlalchemy.sql import ColumnElement @@ -289,7 +289,11 @@ def _cast_json_type(comparator: JSON.Comparator, value: Any) -> Tuple[ColumnElem raise NotImplementedError('The operator `contains` is not implemented for SQLite-based storage plugins.') if operator == 'has_key': - raise NotImplementedError('The operator `has_key` is not implemented for SQLite-based storage plugins.') + return ( + select(database_entity) + .where(func.json_each(database_entity).table_valued('key', joins_implicitly=True).c.key == value) + .exists() + ) if operator == 'in': type_filter, casted_entity = _cast_json_type(database_entity, value[0]) diff --git a/tests/cmdline/commands/test_calcjob.py b/tests/cmdline/commands/test_calcjob.py index a9ca84a991..c3f45f195e 100644 --- a/tests/cmdline/commands/test_calcjob.py +++ b/tests/cmdline/commands/test_calcjob.py @@ -241,9 +241,6 @@ def test_calcjob_outputcat(self): retrieved.base.repository._repository.put_object_from_filelike(io.BytesIO(b'5\n'), 'aiida.out') retrieved.base.repository._update_repository_metadata() - # This currently fails with sqlite backend since the filtering relies on the `has_key` filter which is not - # implemented in SQLite, see https://github.com/aiidateam/aiida-core/pull/6497 - @pytest.mark.requires_psql def test_calcjob_cleanworkdir_basic(self): """Test verdi calcjob cleanworkdir""" # Specifying no filtering options and no explicit calcjobs should exit with non-zero status @@ -269,7 +266,6 @@ def test_calcjob_cleanworkdir_basic(self): result = self.cli_runner.invoke(command.calcjob_cleanworkdir, options) assert result.exception is not None, result.output - @pytest.mark.requires_psql def test_calcjob_cleanworkdir_advanced(self): # Check applying both p and o filters for flag_p in ['-p', '--past-days']: diff --git a/tests/orm/test_querybuilder.py b/tests/orm/test_querybuilder.py index 862474bc76..e39f20a7b9 100644 --- a/tests/orm/test_querybuilder.py +++ b/tests/orm/test_querybuilder.py @@ -1537,7 +1537,6 @@ def test_iterall_with_store_group(self): for pk, pk_clone in zip(pks, [e[1] for e in sorted(pks_clone)]): assert orm.load_node(pk) == orm.load_node(pk_clone) - @pytest.mark.requires_psql @pytest.mark.usefixtures('aiida_profile_clean') def test_iterall_persistence(self, manager): """Test that mutations made during ``QueryBuilder.iterall`` context are automatically committed and persisted. diff --git a/tests/storage/sqlite/test_orm.py b/tests/storage/sqlite/test_orm.py index 21c75f1302..0d859d6bac 100644 --- a/tests/storage/sqlite/test_orm.py +++ b/tests/storage/sqlite/test_orm.py @@ -24,7 +24,7 @@ ({'attributes.float': {'of_type': 'number'}}, 1), ({'attributes.true': {'of_type': 'boolean'}}, 1), ({'attributes.false': {'of_type': 'boolean'}}, 1), - ({'attributes.null': {'of_type': 'null'}}, 2), + ({'attributes.null': {'of_type': 'null'}}, 3), ({'attributes.list': {'of_type': 'array'}}, 1), ({'attributes.dict': {'of_type': 'object'}}, 1), # equality match @@ -35,7 +35,7 @@ ({'attributes.false': {'==': False}}, 1), ({'attributes.list': {'==': [1, 2]}}, 1), ({'attributes.list2': {'==': ['a', 'b']}}, 1), - ({'attributes.dict': {'==': {'key1': 1, 'key2': None}}}, 1), + ({'attributes.dict': {'==': {'key-1': 1, 'key-none': None}}}, 1), # equality non-match ({'attributes.text': {'==': 'lmn'}}, 0), ({'attributes.integer': {'==': 2}}, 0), @@ -89,9 +89,11 @@ ({'attributes.integer': {'in': [5, 6, 7]}}, 0), ({'attributes.integer': {'in': [1, 2, 3]}}, 1), # object operators - # Reenable when ``has_key`` operator is implemented, see https://github.com/aiidateam/aiida-core/issues/6498 - # ({'attributes.dict': {'has_key': 'k'}}, 0), - # ({'attributes.dict': {'has_key': 'key1'}}, 1), + ({'attributes.dict': {'has_key': 'non-exist'}}, 0), + ({'attributes.dict': {'!has_key': 'non-exist'}}, 3), + ({'attributes.dict': {'has_key': 'key-1'}}, 1), + ({'attributes.dict': {'has_key': 'key-none'}}, 1), + ({'attributes.dict': {'!has_key': 'key-none'}}, 2), ), ids=json.dumps, ) @@ -111,13 +113,17 @@ def test_qb_json_filters(filters, matches): 'list': [1, 2], 'list2': ['a', 'b'], 'dict': { - 'key1': 1, - 'key2': None, + 'key-1': 1, + 'key-none': None, }, }, backend=backend, ).store() Dict({'text2': 'abcxXYZ'}, backend=backend).store() + + # a false dict, added to test `has_key`'s behavior when key is not of json type + Dict({'dict': 0xFA15ED1C7}, backend=backend).store() + qbuilder = QueryBuilder(backend=backend) qbuilder.append(Dict, filters=filters) assert qbuilder.count() == matches diff --git a/tests/test_nodes.py b/tests/test_nodes.py index bd971d37db..6f64ab6d2d 100644 --- a/tests/test_nodes.py +++ b/tests/test_nodes.py @@ -162,7 +162,6 @@ def init_profile(self, aiida_localhost): """Initialize the profile.""" self.computer = aiida_localhost - @pytest.mark.requires_psql def test_with_subclasses(self): from aiida.plugins import DataFactory From 1c5f10968f40934b81eef72ed72dc814497707a8 Mon Sep 17 00:00:00 2001 From: Jusong Yu Date: Fri, 22 Nov 2024 13:38:08 +0100 Subject: [PATCH 20/31] Changes required make it possible to run with pytest-xdist (#6631) `pytest-xdist` although not run through CI, but can run from local if the changes here are adapted. The changes needed since xdist has a know limitation that the order and amount of test must be consistent. --- tests/orm/test_fields.py | 35 ++++++++++++++++++----------------- 1 file changed, 18 insertions(+), 17 deletions(-) diff --git a/tests/orm/test_fields.py b/tests/orm/test_fields.py index 46d69d20a7..df74f58b07 100644 --- a/tests/orm/test_fields.py +++ b/tests/orm/test_fields.py @@ -11,6 +11,7 @@ import pytest from aiida import orm from aiida.orm.fields import add_field +from aiida.plugins import load_entry_point from importlib_metadata import entry_points EPS = entry_points() @@ -27,24 +28,24 @@ def test_all_entity_fields(entity_cls, data_regression): ) -@pytest.mark.parametrize( - 'group,name', - ( - (group, name) - for group in ( - 'aiida.node', - 'aiida.data', - ) - for name in EPS.select(group=group).names - ), -) -def test_all_node_fields(group, name, data_regression): +@pytest.fixture +def node_and_data_entry_points() -> list[tuple[str, str]]: + """Return a list of available entry points.""" + _eps: list[tuple[str, str]] = [] + eps = entry_points() + for group in ['aiida.node', 'aiida.data']: + _eps.extend((group, ep.name) for ep in eps.select(group=group)) + return _eps + + +def test_all_node_fields(node_and_data_entry_points: list[tuple[str, str]], data_regression): """Test that all the node fields are correctly registered.""" - node_cls = next(iter(tuple(EPS.select(group=group, name=name)))).load() - data_regression.check( - {key: repr(value) for key, value in node_cls.fields._dict.items()}, - basename=f'fields_{group}.{name}.{node_cls.__name__}', - ) + for group, name in node_and_data_entry_points: + node_cls = load_entry_point(group, name) + data_regression.check( + {key: repr(value) for key, value in node_cls.fields._dict.items()}, + basename=f'fields_{group}.{name}.{node_cls.__name__}', + ) def test_add_field(): From a863d1e887822334df4db9120421cd38ded04d3e Mon Sep 17 00:00:00 2001 From: Daniel Hollas Date: Mon, 25 Nov 2024 10:22:42 +0000 Subject: [PATCH 21/31] Tests: add --db-backend pytest option (#6625) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Instead of choosing the database backend (SQLite or Postgres) implicitly via markers, we add an explicit `--db-backend ` option to pytest. ``` --db-backend=DB_BACKEND Database backend to be used for tests ('sqlite', 'psql') ``` - The default backend was changed from `psql` to `sqlite` to make local testing easier. - The tests running in CI are still using Postgres in most places. - The only change in CI should be switching the rabbitmq tests and release tests to use sqlite. Error if the user provides an invalid option ``` ❯ pytest --db-backend=invalid ERROR: Invalid --db-backend option 'invalid' Must be one of: ('sqlite', 'psql') ``` To clarify, the existing presto marker works as before. --- .github/workflows/benchmark.yml | 4 +- .github/workflows/ci-code.yml | 4 +- .github/workflows/nightly.yml | 18 +----- .github/workflows/release.yml | 20 +----- .github/workflows/test-install.yml | 2 +- .github/workflows/tests_nightly.sh | 2 +- tests/conftest.py | 52 +++++++++++++-- tests/storage/psql_dos/conftest.py | 13 ++-- .../django_branch/test_migrate_to_head.py | 9 +++ tests/storage/psql_dos/test_backend.py | 8 +++ tests/test_markers.py | 64 +++++++++++++++++++ 11 files changed, 141 insertions(+), 55 deletions(-) create mode 100644 tests/test_markers.py diff --git a/.github/workflows/benchmark.yml b/.github/workflows/benchmark.yml index a2a42687b1..5bb4b61c55 100644 --- a/.github/workflows/benchmark.yml +++ b/.github/workflows/benchmark.yml @@ -56,7 +56,7 @@ jobs: python-version: '3.10' - name: Run benchmarks - run: pytest --benchmark-only --benchmark-json benchmark.json + run: pytest --db-backend psql --benchmark-only --benchmark-json benchmark.json tests/ - name: Store benchmark result uses: aiidateam/github-action-benchmark@v3 @@ -73,4 +73,4 @@ jobs: alert-threshold: 200% comment-on-alert: true fail-on-alert: false - alert-comment-cc-users: '@chrisjsewell,@giovannipizzi' + alert-comment-cc-users: '@giovannipizzi,@agoscinski,@GeigerJ2,@khsrali,@unkcpz' diff --git a/.github/workflows/ci-code.yml b/.github/workflows/ci-code.yml index ff95047bb4..dfa3cc787c 100644 --- a/.github/workflows/ci-code.yml +++ b/.github/workflows/ci-code.yml @@ -104,7 +104,7 @@ jobs: AIIDA_WARN_v3: 1 # Python 3.12 has a performance regression when running with code coverage # so run code coverage only for python 3.9. - run: pytest -v tests -m 'not nightly' ${{ matrix.python-version == '3.9' && '--cov aiida' || '' }} + run: pytest -v --db-backend psql -m 'not nightly' tests/ ${{ matrix.python-version == '3.9' && '--cov aiida' || '' }} - name: Upload coverage report if: matrix.python-version == 3.9 && github.repository == 'aiidateam/aiida-core' @@ -139,7 +139,7 @@ jobs: - name: Run test suite env: AIIDA_WARN_v3: 0 - run: pytest -m 'presto' + run: pytest -m 'presto' tests/ verdi: diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index c7ea8cb787..9217e534f8 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -104,19 +104,6 @@ jobs: rabbitmq-version: ['3.11', '3.12', '3.13'] services: - postgres: - image: postgres:16 - env: - POSTGRES_DB: test_aiida - POSTGRES_PASSWORD: '' - POSTGRES_HOST_AUTH_METHOD: trust - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 5432:5432 rabbitmq: image: rabbitmq:${{ matrix.rabbitmq-version }}-management ports: @@ -132,9 +119,6 @@ jobs: with: python-version: '3.11' - - name: Install system dependencies - run: sudo apt update && sudo apt install postgresql - - name: Setup SSH on localhost run: .github/workflows/setup_ssh.sh @@ -145,7 +129,7 @@ jobs: id: tests env: AIIDA_WARN_v3: 0 - run: pytest -sv -m 'requires_rmq' + run: pytest -sv --db-backend sqlite -m 'requires_rmq' tests/ - name: Slack notification # Always run this step (otherwise it would be skipped if any of the previous steps fail) but only if the diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 80f7e35326..524a625afc 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -54,19 +54,6 @@ jobs: timeout-minutes: 30 services: - postgres: - image: postgres:10 - env: - POSTGRES_DB: test_aiida - POSTGRES_PASSWORD: '' - POSTGRES_HOST_AUTH_METHOD: trust - options: >- - --health-cmd pg_isready - --health-interval 10s - --health-timeout 5s - --health-retries 5 - ports: - - 5432:5432 rabbitmq: image: rabbitmq:3.8.14-management ports: @@ -76,16 +63,11 @@ jobs: steps: - uses: actions/checkout@v4 - - name: Install system dependencies - run: | - sudo apt update - sudo apt install postgresql graphviz - - name: Install aiida-core uses: ./.github/actions/install-aiida-core - name: Run sub-set of test suite - run: pytest -sv -k 'requires_rmq' + run: pytest -sv -m requires_rmq --db-backend=sqlite tests/ publish: diff --git a/.github/workflows/test-install.yml b/.github/workflows/test-install.yml index 2f919a8c99..75371449bb 100644 --- a/.github/workflows/test-install.yml +++ b/.github/workflows/test-install.yml @@ -229,7 +229,7 @@ jobs: env: AIIDA_TEST_PROFILE: test_aiida AIIDA_WARN_v3: 1 - run: pytest --verbose tests -m 'not nightly' + run: pytest -v --db-backend psql tests -m 'not nightly' tests/ - name: Freeze test environment run: pip freeze | sed '1d' | tee requirements-py-${{ matrix.python-version }}.txt diff --git a/.github/workflows/tests_nightly.sh b/.github/workflows/tests_nightly.sh index 10f26f7f15..dbb1b92a8a 100755 --- a/.github/workflows/tests_nightly.sh +++ b/.github/workflows/tests_nightly.sh @@ -13,4 +13,4 @@ verdi -p test_aiida run ${SYSTEM_TESTS}/test_containerized_code.py bash ${SYSTEM_TESTS}/test_polish_workchains.sh verdi daemon stop -AIIDA_TEST_PROFILE=test_aiida pytest -v tests -m 'nightly' +AIIDA_TEST_PROFILE=test_aiida pytest -v --db-backend psql -m nightly tests/ diff --git a/tests/conftest.py b/tests/conftest.py index 2166cc06c0..fb6780d966 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -22,6 +22,7 @@ import types import typing as t import warnings +from enum import Enum from pathlib import Path import click @@ -37,7 +38,14 @@ pytest_plugins = ['aiida.tools.pytest_fixtures', 'sphinx.testing.fixtures'] -def pytest_collection_modifyitems(items): +class TestDbBackend(Enum): + """Options for the '--db-backend' CLI argument when running pytest.""" + + SQLITE = 'sqlite' + PSQL = 'psql' + + +def pytest_collection_modifyitems(items, config): """Automatically generate markers for certain tests. Most notably, we add the 'presto' marker for all tests that @@ -47,6 +55,14 @@ def pytest_collection_modifyitems(items): filepath_django = Path(__file__).parent / 'storage' / 'psql_dos' / 'migrations' / 'django_branch' filepath_sqla = Path(__file__).parent / 'storage' / 'psql_dos' / 'migrations' / 'sqlalchemy_branch' + # If the user requested the SQLite backend, automatically skip incompatible tests + if config.option.db_backend is TestDbBackend.SQLITE: + if config.option.markexpr != '': + # Don't overwrite markers that the user already provided via '-m ' cmdline argument + config.option.markexpr += ' and (not requires_psql)' + else: + config.option.markexpr = 'not requires_psql' + for item in items: filepath_item = Path(item.fspath) @@ -68,6 +84,30 @@ def pytest_collection_modifyitems(items): item.add_marker('presto') +def db_backend_type(string): + """Conversion function for the custom '--db-backend' pytest CLI option + + :param string: String provided by the user via CLI + :returns: DbBackend enum corresponding to user input string + """ + try: + return TestDbBackend(string) + except ValueError: + msg = f"Invalid --db-backend option '{string}'\nMust be one of: {tuple(db.value for db in TestDbBackend)}" + raise pytest.UsageError(msg) + + +def pytest_addoption(parser): + parser.addoption( + '--db-backend', + action='store', + default=TestDbBackend.SQLITE, + required=False, + help=f'Database backend to be used for tests {tuple(db.value for db in TestDbBackend)}', + type=db_backend_type, + ) + + @pytest.fixture(scope='session') def aiida_profile(pytestconfig, aiida_config, aiida_profile_factory, config_psql_dos, config_sqlite_dos): """Create and load a profile with ``core.psql_dos`` as a storage backend and RabbitMQ as the broker. @@ -77,18 +117,22 @@ def aiida_profile(pytestconfig, aiida_config, aiida_profile_factory, config_psql be run against the main storage backend, which is ``core.sqlite_dos``. """ marker_opts = pytestconfig.getoption('-m') + db_backend = pytestconfig.getoption('--db-backend') - # By default we use RabbitMQ broker and psql_dos storage + # We use RabbitMQ broker by default unless 'presto' marker is specified broker = 'core.rabbitmq' if 'not requires_rmq' in marker_opts or 'presto' in marker_opts: broker = None - if 'not requires_psql' in marker_opts or 'presto' in marker_opts: + if db_backend is TestDbBackend.SQLITE: storage = 'core.sqlite_dos' config = config_sqlite_dos() - else: + elif db_backend is TestDbBackend.PSQL: storage = 'core.psql_dos' config = config_psql_dos() + else: + # This should be unreachable + raise ValueError(f'Invalid DB backend {db_backend}') with aiida_profile_factory( aiida_config, storage_backend=storage, storage_config=config, broker_backend=broker diff --git a/tests/storage/psql_dos/conftest.py b/tests/storage/psql_dos/conftest.py index 16136b8df9..c24db0ac72 100644 --- a/tests/storage/psql_dos/conftest.py +++ b/tests/storage/psql_dos/conftest.py @@ -13,17 +13,12 @@ from aiida.common.exceptions import MissingConfigurationError from aiida.manage.configuration import get_config +STORAGE_BACKEND_ENTRY_POINT = None try: if test_profile := os.environ.get('AIIDA_TEST_PROFILE'): STORAGE_BACKEND_ENTRY_POINT = get_config().get_profile(test_profile).storage_backend - # TODO: The else branch is wrong - else: - STORAGE_BACKEND_ENTRY_POINT = 'core.psql_dos' -except MissingConfigurationError: - # TODO: This is actually not true anymore! - # Case when ``pytest`` is invoked without existing config, in which case it will rely on the automatic test profile - # creation which currently always uses ``core.psql_dos`` for the storage backend - STORAGE_BACKEND_ENTRY_POINT = 'core.psql_dos' +except MissingConfigurationError as e: + raise ValueError(f"Could not parse configuration of AiiDA test profile '{test_profile}'") from e -if STORAGE_BACKEND_ENTRY_POINT != 'core.psql_dos': +if STORAGE_BACKEND_ENTRY_POINT is not None and STORAGE_BACKEND_ENTRY_POINT != 'core.psql_dos': collect_ignore_glob = ['*'] diff --git a/tests/storage/psql_dos/migrations/django_branch/test_migrate_to_head.py b/tests/storage/psql_dos/migrations/django_branch/test_migrate_to_head.py index 69c7e643d9..11f9fb8c3a 100644 --- a/tests/storage/psql_dos/migrations/django_branch/test_migrate_to_head.py +++ b/tests/storage/psql_dos/migrations/django_branch/test_migrate_to_head.py @@ -11,6 +11,15 @@ from aiida.storage.psql_dos.migrator import PsqlDosMigrator +def test_all_tests_marked_as_nightly(request): + """Test that all tests in this folder are tagged with 'nightly' marker""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 2 + assert 'nightly' in own_markers + assert 'requires_psql' in own_markers + + def test_migrate(perform_migrations: PsqlDosMigrator): """Test that the migrator can migrate from the base of the django branch, to the main head.""" perform_migrations.migrate_up('django@django_0001') # the base of the django branch diff --git a/tests/storage/psql_dos/test_backend.py b/tests/storage/psql_dos/test_backend.py index 6fe35ac747..bbc77b1a14 100644 --- a/tests/storage/psql_dos/test_backend.py +++ b/tests/storage/psql_dos/test_backend.py @@ -13,6 +13,14 @@ from aiida.orm import User +def test_all_tests_marked_with_requires_psql(request): + """Test that all tests in this folder are marked with 'requires_psql'""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'requires_psql' + + @pytest.mark.usefixtures('aiida_profile_clean') def test_default_user(): assert isinstance(get_manager().get_profile_storage().default_user, User) diff --git a/tests/test_markers.py b/tests/test_markers.py new file mode 100644 index 0000000000..d9d6e63871 --- /dev/null +++ b/tests/test_markers.py @@ -0,0 +1,64 @@ +"""Tests markers that have custom in conftest.py""" + +import pytest + + +def test_presto_auto_mark(request): + """Test that the presto marker is added automatically""" + own_markers = [marker.name for marker in request.node.own_markers] + assert len(own_markers) == 1 + assert own_markers[0] == 'presto' + + +@pytest.mark.sphinx +def test_presto_mark_and_another_mark(request): + """Test that presto marker is added even if there is an existing marker (except requires_rmq|psql)""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 2 + assert 'presto' in own_markers + assert 'sphinx' in own_markers + + +@pytest.mark.requires_rmq +def test_no_presto_mark_if_rmq(request): + """Test that presto marker is NOT added if the test is mark with "requires_rmq""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'requires_rmq' + + +@pytest.mark.requires_psql +def test_no_presto_mark_if_psql(request): + """Test that presto marker is NOT added if the test is mark with "requires_psql""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'requires_psql' + + +@pytest.mark.nightly +def test_no_presto_mark_if_nightly(request): + """Test that presto marker is NOT added if the test is mark with "requires_psql""" + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'nightly' + + +@pytest.mark.requires_psql +def test_requires_psql_with_sqlite_impossible(pytestconfig): + db_backend = pytestconfig.getoption('--db-backend') + if db_backend.value == 'sqlite': + pytest.fail('This test should not have been executed with SQLite backend!') + + +def test_daemon_client_fixture_automarked(request, daemon_client): + """Test that any test using ``daemon_client`` fixture is + automatically tagged with 'requires_rmq' mark + """ + own_markers = [marker.name for marker in request.node.own_markers] + + assert len(own_markers) == 1 + assert own_markers[0] == 'requires_rmq' From c93fb4f75554802e46fdcb7cf8caf27318ad04d0 Mon Sep 17 00:00:00 2001 From: Jusong Yu Date: Mon, 25 Nov 2024 16:12:32 +0100 Subject: [PATCH 22/31] Bump mypy version to ~=1.13.0 (#6630) --- pyproject.toml | 2 +- src/aiida/common/lang.py | 2 +- src/aiida/common/pydantic.py | 2 +- src/aiida/engine/processes/calcjobs/calcjob.py | 2 +- src/aiida/engine/processes/workchains/restart.py | 2 +- src/aiida/manage/manager.py | 2 +- src/aiida/orm/nodes/data/array/projection.py | 2 +- src/aiida/orm/nodes/data/list.py | 4 ++-- src/aiida/orm/nodes/data/singlefile.py | 4 +--- src/aiida/plugins/utils.py | 2 +- src/aiida/tools/pytest_fixtures/daemon.py | 2 +- tests/cmdline/groups/test_dynamic.py | 2 +- 12 files changed, 13 insertions(+), 15 deletions(-) diff --git a/pyproject.toml b/pyproject.toml index 3768fcb05c..6b3f630e1a 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -211,7 +211,7 @@ notebook = [ ] pre-commit = [ 'aiida-core[atomic_tools,rest,tests,tui]', - 'mypy~=1.10.0', + 'mypy~=1.13.0', 'packaging~=23.0', 'pre-commit~=3.5', 'sqlalchemy[mypy]~=2.0', diff --git a/src/aiida/common/lang.py b/src/aiida/common/lang.py index ec9fb45ddb..6a8f4d3d5b 100644 --- a/src/aiida/common/lang.py +++ b/src/aiida/common/lang.py @@ -96,4 +96,4 @@ def __init__(self, getter: Callable[[SelfType], ReturnType]) -> None: self.getter = getter def __get__(self, instance: Any, owner: SelfType) -> ReturnType: - return self.getter(owner) + return self.getter(owner) # type: ignore[arg-type] diff --git a/src/aiida/common/pydantic.py b/src/aiida/common/pydantic.py index 633e83428d..2fa3d2bd68 100644 --- a/src/aiida/common/pydantic.py +++ b/src/aiida/common/pydantic.py @@ -41,7 +41,7 @@ class Model(BaseModel): field_info = Field(default, **kwargs) for key, value in (('priority', priority), ('short_name', short_name), ('option_cls', option_cls)): - if value is not None: + if value is not None and field_info is not None: field_info.metadata.append({key: value}) return field_info diff --git a/src/aiida/engine/processes/calcjobs/calcjob.py b/src/aiida/engine/processes/calcjobs/calcjob.py index d5acfca5fc..8ced783a5f 100644 --- a/src/aiida/engine/processes/calcjobs/calcjob.py +++ b/src/aiida/engine/processes/calcjobs/calcjob.py @@ -1062,7 +1062,7 @@ def presubmit(self, folder: Folder) -> CalcInfo: def encoder(obj): if dataclasses.is_dataclass(obj): - return dataclasses.asdict(obj) + return dataclasses.asdict(obj) # type: ignore[arg-type] raise TypeError(f' {obj!r} is not JSON serializable') subfolder = folder.get_subfolder('.aiida', create=True) diff --git a/src/aiida/engine/processes/workchains/restart.py b/src/aiida/engine/processes/workchains/restart.py index ad5cd8a181..34544704f2 100644 --- a/src/aiida/engine/processes/workchains/restart.py +++ b/src/aiida/engine/processes/workchains/restart.py @@ -29,7 +29,7 @@ def validate_handler_overrides( - process_class: 'BaseRestartWorkChain', handler_overrides: Optional[orm.Dict], ctx: 'PortNamespace' + process_class: type['BaseRestartWorkChain'], handler_overrides: Optional[orm.Dict], ctx: 'PortNamespace' ) -> Optional[str]: """Validator for the ``handler_overrides`` input port of the ``BaseRestartWorkChain``. diff --git a/src/aiida/manage/manager.py b/src/aiida/manage/manager.py index 8621b324f4..651190454e 100644 --- a/src/aiida/manage/manager.py +++ b/src/aiida/manage/manager.py @@ -430,7 +430,7 @@ def create_runner(self, with_persistence: bool = True, **kwargs: Any) -> 'Runner if with_persistence and 'persister' not in settings: settings['persister'] = self.get_persister() - return runners.Runner(**settings) + return runners.Runner(**settings) # type: ignore[arg-type] def create_daemon_runner(self, loop: Optional['asyncio.AbstractEventLoop'] = None) -> 'Runner': """Create and return a new daemon runner. diff --git a/src/aiida/orm/nodes/data/array/projection.py b/src/aiida/orm/nodes/data/array/projection.py index e58442d9c5..881ac727c2 100644 --- a/src/aiida/orm/nodes/data/array/projection.py +++ b/src/aiida/orm/nodes/data/array/projection.py @@ -278,7 +278,7 @@ def array_list_checker(array_list, array_name, orb_length): raise exceptions.ValidationError('Tags must set a list of strings') self.base.attributes.set('tags', tags) - def set_orbitals(self, **kwargs): + def set_orbitals(self, **kwargs): # type: ignore[override] """This method is inherited from OrbitalData, but is blocked here. If used will raise a NotImplementedError """ diff --git a/src/aiida/orm/nodes/data/list.py b/src/aiida/orm/nodes/data/list.py index 9fdd6ec866..fc39dd1acd 100644 --- a/src/aiida/orm/nodes/data/list.py +++ b/src/aiida/orm/nodes/data/list.py @@ -81,7 +81,7 @@ def remove(self, value): self.set_list(data) return item - def pop(self, **kwargs): + def pop(self, **kwargs): # type: ignore[override] """Remove and return item at index (default last).""" data = self.get_list() item = data.pop(**kwargs) @@ -89,7 +89,7 @@ def pop(self, **kwargs): self.set_list(data) return item - def index(self, value): + def index(self, value): # type: ignore[override] """Return first index of value..""" return self.get_list().index(value) diff --git a/src/aiida/orm/nodes/data/singlefile.py b/src/aiida/orm/nodes/data/singlefile.py index c0f3797f24..8faefc8cb4 100644 --- a/src/aiida/orm/nodes/data/singlefile.py +++ b/src/aiida/orm/nodes/data/singlefile.py @@ -71,9 +71,7 @@ def open(self, path: FilePath, mode: t.Literal['rb']) -> t.Iterator[t.BinaryIO]: @t.overload @contextlib.contextmanager - def open( # type: ignore[overload-overlap] - self, path: None = None, mode: t.Literal['r'] = ... - ) -> t.Iterator[t.TextIO]: ... + def open(self, path: None = None, mode: t.Literal['r'] = ...) -> t.Iterator[t.TextIO]: ... @t.overload @contextlib.contextmanager diff --git a/src/aiida/plugins/utils.py b/src/aiida/plugins/utils.py index c284b25912..7d1e16a363 100644 --- a/src/aiida/plugins/utils.py +++ b/src/aiida/plugins/utils.py @@ -38,7 +38,7 @@ def __init__(self): def logger(self) -> Logger: return self._logger - def get_version_info(self, plugin: str | type) -> dict[t.Any, dict[t.Any, t.Any]]: + def get_version_info(self, plugin: str | t.Any) -> dict[t.Any, dict[t.Any, t.Any]]: """Get the version information for a given plugin. .. note:: diff --git a/src/aiida/tools/pytest_fixtures/daemon.py b/src/aiida/tools/pytest_fixtures/daemon.py index 74e3620193..2b74e4ce77 100644 --- a/src/aiida/tools/pytest_fixtures/daemon.py +++ b/src/aiida/tools/pytest_fixtures/daemon.py @@ -116,7 +116,7 @@ def test(submit_and_await): from aiida.engine import ProcessState def factory( - submittable: 'Process' | 'ProcessBuilder' | 'ProcessNode', + submittable: type[Process] | ProcessBuilder | ProcessNode | t.Any, state: ProcessState = ProcessState.FINISHED, timeout: int = 20, **kwargs, diff --git a/tests/cmdline/groups/test_dynamic.py b/tests/cmdline/groups/test_dynamic.py index 3c741b8fe2..48c18f0941 100644 --- a/tests/cmdline/groups/test_dynamic.py +++ b/tests/cmdline/groups/test_dynamic.py @@ -17,7 +17,7 @@ class Model(BaseModel): union_type: t.Union[int, float] = Field(title='Union type') without_default: str = Field(title='Without default') with_default: str = Field(title='With default', default='default') - with_default_factory: str = Field(title='With default factory', default_factory=lambda: True) + with_default_factory: str = Field(title='With default factory', default_factory=lambda: True) # type: ignore[assignment] def test_list_options(entry_points): From 54c4a0d06660f522717d87ffae1ca84211d8c79e Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Mon, 25 Nov 2024 17:55:02 +0100 Subject: [PATCH 23/31] Update changelog for release v2.6.3 (#6637) This includes the updates of the changelog for v2.6.3 Furthermore a link of the changelog to pyproject.toml has been added. This makes the url to the changelog appear for releases on pypi. --- CHANGELOG.md | 15 ++++++++++++++- pyproject.toml | 1 + 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index ea3c843d8a..04327d7105 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,18 @@ # Changelog +## v2.6.3 - 2024-11-6 + +### Fixes +- CLI: Fix exception for `verdi plugin list` (#6560) [[c3b10b7]](https://github.com/aiidateam/aiida-core/commit/c3b10b759a9cd062800ef120591d5c7fd0ae4ee7) +- `DirectScheduler`: Ensure killing child processes (#6572) [[fddffca]](https://github.com/aiidateam/aiida-core/commit/fddffca67b4f7e3b76b19df7db8e1511c449d2d9) +- Engine: Fix state change broadcast before process node is updated (#6580) [[867353c]](https://github.com/aiidateam/aiida-core/commit/867353c415c61d94a2427d5225dd5224a1b95fb9) + +### Devops +- Docker: Replace sleep with `s6-notifyoncheck` (#6475) [[9579378b]](https://github.com/aiidateam/aiida-core/commit/9579378ba063237baa5b73380eb8e9f0a28529ee) +- Fix failed docker CI using more reasoning grep regex to parse python version (#6581) [[332a4a91]](https://github.com/aiidateam/aiida-core/commit/332a4a915771afedcb144463b012558e4669e529) +- DevOps: Fix json query in reading the docker names to filter out fields not starting with aiida (#6573) [[e1467edc]](https://github.com/aiidateam/aiida-core/commit/e1467edca902867e53605e0e60b67f8767bf8d3e) + + ## v2.6.2 - 2024-08-07 ### Fixes @@ -31,7 +44,7 @@ ## v2.6.1 - 2024-07-01 -### Fixes: +### Fixes - Fixtures: Make `pgtest` truly an optional dependency [[9fe8fd2e0]](https://github.com/aiidateam/aiida-core/commit/9fe8fd2e0b88e746ee2156eccb71b7adbab6b2c5) diff --git a/pyproject.toml b/pyproject.toml index 6b3f630e1a..7a884cc1f8 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -255,6 +255,7 @@ runaiida = 'aiida.cmdline.commands.cmd_run:run' verdi = 'aiida.cmdline.commands.cmd_verdi:verdi' [project.urls] +Changelog = 'https://github.com/aiidateam/aiida-core/blob/main/CHANGELOG.md' Documentation = 'https://aiida.readthedocs.io' Home = 'http://www.aiida.net/' Source = 'https://github.com/aiidateam/aiida-core' From 41a0fd92bf6233a758b10a785534f6186b567e16 Mon Sep 17 00:00:00 2001 From: Jusong Yu Date: Mon, 25 Nov 2024 18:07:43 +0100 Subject: [PATCH 24/31] CI: Turn off verbose pytest output (#6633) Using verbose option for running `pytest` will give a long output in Github CI action and making it hard to check the error when it happens. Turn off the verbose will reduce the output and still have the error when tests failed. --- .github/workflows/ci-code.yml | 2 +- .github/workflows/nightly.yml | 2 +- .github/workflows/release.yml | 2 +- .github/workflows/test-install.yml | 2 +- .github/workflows/tests_nightly.sh | 2 +- 5 files changed, 5 insertions(+), 5 deletions(-) diff --git a/.github/workflows/ci-code.yml b/.github/workflows/ci-code.yml index dfa3cc787c..a8d0cb9a08 100644 --- a/.github/workflows/ci-code.yml +++ b/.github/workflows/ci-code.yml @@ -104,7 +104,7 @@ jobs: AIIDA_WARN_v3: 1 # Python 3.12 has a performance regression when running with code coverage # so run code coverage only for python 3.9. - run: pytest -v --db-backend psql -m 'not nightly' tests/ ${{ matrix.python-version == '3.9' && '--cov aiida' || '' }} + run: pytest --db-backend psql -m 'not nightly' tests/ ${{ matrix.python-version == '3.9' && '--cov aiida' || '' }} - name: Upload coverage report if: matrix.python-version == 3.9 && github.repository == 'aiidateam/aiida-core' diff --git a/.github/workflows/nightly.yml b/.github/workflows/nightly.yml index 9217e534f8..84ed617125 100644 --- a/.github/workflows/nightly.yml +++ b/.github/workflows/nightly.yml @@ -129,7 +129,7 @@ jobs: id: tests env: AIIDA_WARN_v3: 0 - run: pytest -sv --db-backend sqlite -m 'requires_rmq' tests/ + run: pytest -s --db-backend sqlite -m 'requires_rmq' tests/ - name: Slack notification # Always run this step (otherwise it would be skipped if any of the previous steps fail) but only if the diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 524a625afc..c47595baee 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -67,7 +67,7 @@ jobs: uses: ./.github/actions/install-aiida-core - name: Run sub-set of test suite - run: pytest -sv -m requires_rmq --db-backend=sqlite tests/ + run: pytest -s -m requires_rmq --db-backend=sqlite tests/ publish: diff --git a/.github/workflows/test-install.yml b/.github/workflows/test-install.yml index 75371449bb..d315a50691 100644 --- a/.github/workflows/test-install.yml +++ b/.github/workflows/test-install.yml @@ -229,7 +229,7 @@ jobs: env: AIIDA_TEST_PROFILE: test_aiida AIIDA_WARN_v3: 1 - run: pytest -v --db-backend psql tests -m 'not nightly' tests/ + run: pytest --db-backend psql tests -m 'not nightly' tests/ - name: Freeze test environment run: pip freeze | sed '1d' | tee requirements-py-${{ matrix.python-version }}.txt diff --git a/.github/workflows/tests_nightly.sh b/.github/workflows/tests_nightly.sh index dbb1b92a8a..2712a5124e 100755 --- a/.github/workflows/tests_nightly.sh +++ b/.github/workflows/tests_nightly.sh @@ -13,4 +13,4 @@ verdi -p test_aiida run ${SYSTEM_TESTS}/test_containerized_code.py bash ${SYSTEM_TESTS}/test_polish_workchains.sh verdi daemon stop -AIIDA_TEST_PROFILE=test_aiida pytest -v --db-backend psql -m nightly tests/ +AIIDA_TEST_PROFILE=test_aiida pytest --db-backend psql -m nightly tests/ From c789d0ae827cc928a6b0a1bfa6be83d0a5f32c68 Mon Sep 17 00:00:00 2001 From: Zisen Liu <29354199+rabbull@users.noreply.github.com> Date: Tue, 26 Nov 2024 17:18:37 +0800 Subject: [PATCH 25/31] Tests: Add Tests to `contains` Filter Operator on PostgreSQL Backend (#6617) This PR adds tests to validate the behavior of the `contains` operator when applied to JSON objects and arrays in the PostgreSQL backend. The tests cover a variety of scenarios, including type matching, negations, and edge cases such as empty arrays or objects. This is a test-only PR, and no features or functionality in aiida-core have been modified. --- tests/orm/test_querybuilder.py | 161 +++++++++++++++++++++++++++++++++ 1 file changed, 161 insertions(+) diff --git a/tests/orm/test_querybuilder.py b/tests/orm/test_querybuilder.py index e39f20a7b9..8797fe4e03 100644 --- a/tests/orm/test_querybuilder.py +++ b/tests/orm/test_querybuilder.py @@ -9,6 +9,7 @@ """Tests for the QueryBuilder.""" import copy +import json import uuid import warnings from collections import defaultdict @@ -1702,3 +1703,163 @@ def test_statistics_default_class(self, aiida_localhost): # data are correct res = next(iter(qb.dict()[0].values())) assert res == expected_dict + + +class TestJsonFilters: + @pytest.mark.parametrize( + 'data,filters,is_match', + ( + # contains different types of element + ({'arr': [1, '2', None]}, {'attributes.arr': {'contains': [1]}}, True), + ({'arr': [1, '2', None]}, {'attributes.arr': {'contains': ['2']}}, True), + ({'arr': [1, '2', None]}, {'attributes.arr': {'contains': [None]}}, True), + # contains multiple elements of various types + ({'arr': [1, '2', None]}, {'attributes.arr': {'contains': [1, None]}}, True), + # contains non-exist elements + ({'arr': [1, '2', None]}, {'attributes.arr': {'contains': [114514]}}, False), + # contains empty set + ({'arr': [1, '2', None]}, {'attributes.arr': {'contains': []}}, True), + ({'arr': []}, {'attributes.arr': {'contains': []}}, True), + # nested arrays + ({'arr': [[1, 0], [0, 2]]}, {'attributes.arr': {'contains': [[1, 0]]}}, True), + ({'arr': [[2, 3], [0, 1], []]}, {'attributes.arr': {'contains': [[1, 0]]}}, True), + ({'arr': [[2, 3], [1]]}, {'attributes.arr': {'contains': [[4]]}}, False), + ({'arr': [[1, 0], [0, 2]]}, {'attributes.arr': {'contains': [[3]]}}, False), + ({'arr': [[1, 0], [0, 2]]}, {'attributes.arr': {'contains': [3]}}, False), + ({'arr': [[1, 0], [0, 2]]}, {'attributes.arr': {'contains': [[2]]}}, True), + ({'arr': [[1, 0], [0, 2]]}, {'attributes.arr': {'contains': [2]}}, False), + ({'arr': [[1, 0], [0, 2], 3]}, {'attributes.arr': {'contains': [[3]]}}, False), + ({'arr': [[1, 0], [0, 2], 3]}, {'attributes.arr': {'contains': [3]}}, True), + # negations + ({'arr': [1, '2', None]}, {'attributes.arr': {'!contains': [1]}}, False), + ({'arr': [1, '2', None]}, {'attributes.arr': {'!contains': []}}, False), + ({'arr': [1, '2', None]}, {'attributes.arr': {'!contains': [114514]}}, True), + ({'arr': [1, '2', None]}, {'attributes.arr': {'!contains': [1, 114514]}}, True), + # TODO: these pass, but why? are these behaviors expected? + # non-exist `attr_key`s + ({'foo': []}, {'attributes.arr': {'contains': []}}, False), + ({'foo': []}, {'attributes.arr': {'!contains': []}}, False), + ), + ids=json.dumps, + ) + @pytest.mark.usefixtures('aiida_profile_clean') + @pytest.mark.requires_psql + def test_json_filters_contains_arrays(self, data, filters, is_match): + """Test QueryBuilder filter `contains` for JSON array fields""" + orm.Dict(data).store() + qb = orm.QueryBuilder().append(orm.Dict, filters=filters) + assert qb.count() in {0, 1} + found = qb.count() == 1 + assert found == is_match + + @pytest.mark.parametrize( + 'data,filters,is_match', + ( + # contains different types of values + ( + { + 'dict': { + 'k1': 1, + 'k2': '2', + 'k3': None, + } + }, + {'attributes.dict': {'contains': {'k1': 1}}}, + True, + ), + ( + { + 'dict': { + 'k1': 1, + 'k2': '2', + 'k3': None, + } + }, + {'attributes.dict': {'contains': {'k1': 1, 'k2': '2'}}}, + True, + ), + ( + { + 'dict': { + 'k1': 1, + 'k2': '2', + 'k3': None, + } + }, + {'attributes.dict': {'contains': {'k3': None}}}, + True, + ), + # contains empty set + ( + { + 'dict': { + 'k1': 1, + 'k2': '2', + 'k3': None, + } + }, + {'attributes.dict': {'contains': {}}}, + True, + ), + # doesn't contain non-exist entries + ( + { + 'dict': { + 'k1': 1, + 'k2': '2', + 'k3': None, + } + }, + {'attributes.dict': {'contains': {'k1': 1, 'k': 'v'}}}, + False, + ), + # negations + ( + { + 'dict': { + 'k1': 1, + 'k2': '2', + 'k3': None, + } + }, + {'attributes.dict': {'!contains': {'k1': 1}}}, + False, + ), + ( + { + 'dict': { + 'k1': 1, + 'k2': '2', + 'k3': None, + } + }, + {'attributes.dict': {'!contains': {'k1': 1, 'k': 'v'}}}, + True, + ), + ( + { + 'dict': { + 'k1': 1, + 'k2': '2', + 'k3': None, + } + }, + {'attributes.dict': {'!contains': {}}}, + False, + ), + # TODO: these pass, but why? are these behaviors expected? + # non-exist `attr_key`s + ({'map': {}}, {'attributes.dict': {'contains': {}}}, False), + ({'map': {}}, {'attributes.dict': {'!contains': {}}}, False), + ), + ids=json.dumps, + ) + @pytest.mark.usefixtures('aiida_profile_clean') + @pytest.mark.requires_psql + def test_json_filters_contains_object(self, data, filters, is_match): + """Test QueryBuilder filter `contains` for JSON object fields""" + orm.Dict(data).store() + qb = orm.QueryBuilder().append(orm.Dict, filters=filters) + assert qb.count() in {0, 1} + found = qb.count() == 1 + assert found == is_match From 451375b7cfa08c7a373654f4ac64d6e1204bd865 Mon Sep 17 00:00:00 2001 From: Jusong Yu Date: Tue, 26 Nov 2024 10:27:53 +0100 Subject: [PATCH 26/31] CI: remove ci-style.yml (#6638) The pre-commit hook is turned on for the repo and will run for PRs and automatically fix what it can fix. The ci-style.yml is duplicate for that, so removed. --- .github/workflows/ci-style.yml | 30 ------------------------------ 1 file changed, 30 deletions(-) delete mode 100644 .github/workflows/ci-style.yml diff --git a/.github/workflows/ci-style.yml b/.github/workflows/ci-style.yml deleted file mode 100644 index 1f4b549ad2..0000000000 --- a/.github/workflows/ci-style.yml +++ /dev/null @@ -1,30 +0,0 @@ -name: ci-style - -on: - push: - branches-ignore: [gh-pages] - pull_request: - branches-ignore: [gh-pages] - -env: - FORCE_COLOR: 1 - -jobs: - - pre-commit: - - runs-on: ubuntu-latest - timeout-minutes: 30 - - steps: - - uses: actions/checkout@v4 - - - name: Install python dependencies - uses: ./.github/actions/install-aiida-core - with: - python-version: '3.11' - extras: '[pre-commit]' - from-requirements: 'false' - - - name: Run pre-commit - run: pre-commit run --all-files || ( git status --short ; git diff ; exit 1 ) From 846c11f8c323c60a8e17eb715980e114cc9e6363 Mon Sep 17 00:00:00 2001 From: Jusong Yu Date: Tue, 26 Nov 2024 10:30:20 +0100 Subject: [PATCH 27/31] Pre-commit: exclude mypy for all tests (#6639) Not worth to have strict type check by mypy for tests/* . Keeping a static type checker still would improve readability, but it also increases the development time, especially for tests that goes the expected path of the logic. Lose up on readability for tests can speed up development time. --- .pre-commit-config.yaml | 12 +----------- 1 file changed, 1 insertion(+), 11 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 32305828b4..3bcd486ef1 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -90,6 +90,7 @@ repos: .docker/.*| docs/.*| utils/.*| + tests/.*| src/aiida/calculations/arithmetic/add.py| src/aiida/calculations/diff_tutorial/calculations.py| @@ -192,17 +193,6 @@ repos: src/aiida/transports/plugins/local.py| src/aiida/transports/plugins/ssh.py| src/aiida/workflows/arithmetic/multiply_add.py| - - tests/conftest.py| - tests/repository/conftest.py| - tests/repository/test_repository.py| - tests/sphinxext/sources/workchain/conf.py| - tests/sphinxext/sources/workchain_broken/conf.py| - tests/storage/psql_dos/migrations/conftest.py| - tests/storage/psql_dos/migrations/django_branch/test_0026_0027_traj_data.py| - tests/test_calculation_node.py| - tests/test_nodes.py| - )$ - id: dm-generate-all From ec8a055a533b6422ed95b4ec30faf70efd667761 Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Tue, 26 Nov 2024 14:40:48 +0100 Subject: [PATCH 28/31] Add helper script for creating patch releases from a list of commits (#6602) The script helps with creating a patch release. It takes a list of commits, cherry-picks them, appends the original commit hash to the cherry-picked commit message and outputs a short summary of all commits for the changelog. --- utils/patch-release.sh | 56 ++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 56 insertions(+) create mode 100755 utils/patch-release.sh diff --git a/utils/patch-release.sh b/utils/patch-release.sh new file mode 100755 index 0000000000..a9d67bee79 --- /dev/null +++ b/utils/patch-release.sh @@ -0,0 +1,56 @@ +#!/bin/bash + +# Script: patch-release.sh +# Description: +# Cherry-picks a list of commits, amends each with the original commit hash for tracking, +# and generates a summary from the short github commit messages with links to each commit. +# +# Usage: +# ./patch-release.sh ... +# +# Example: +# ./patch-release.sh abc1234 def5678 + +set -e + +# Check if at least two arguments are provided (repo and at least one commit) +if [ "$#" -lt 1 ]; then + echo "Usage: $0 ..." + echo "Example: $0 abc1234 def5678" + exit 1 +fi + +GITHUB_REPO="aiidateam/aiida-core" + +# Create an array to store commit summaries +declare -a commit_summaries=() + +# Loop through each commit hash +for commit in "$@"; do + # Cherry-pick the commit + if git cherry-pick "$commit"; then + # If cherry-pick succeeds, get the short message and short hash + commit_message=$(git log -1 --pretty=format:"%B" HEAD) + original_short_hash=$(git log -1 --pretty=format:"%h" "$commit") + original_long_hash=$(git rev-parse $original_short_hash) + + # Amend the cherry-picked commit to include the original commit ID for tracking + git commit --amend -m "$commit_message" -m "Cherry-pick: $original_long_hash" + + # Format the output as a Markdown list item and add to the array + short_commit_message=$(git log -1 --pretty=format:"%s" HEAD) + cherry_picked_hash=$(git log -1 --pretty=format:"%h" HEAD) + commit_summaries+=("- $short_commit_message [[${commit}]](https://github.com/$GITHUB_REPO/commit/${original_long_hash})") + else + echo "Failed to cherry-pick commit $commit" + # Abort the cherry-pick in case of conflict + git cherry-pick --abort + exit 1 + fi +done + +# Print the summary +echo -e "\n### Cherry-Picked Commits Summary:\n" +for summary in "${commit_summaries[@]}"; do + echo "$summary" +done From 36eab779793a6dd0184c83f0b0af6a75a62fac82 Mon Sep 17 00:00:00 2001 From: Jusong Yu Date: Tue, 26 Nov 2024 14:48:52 +0100 Subject: [PATCH 29/31] Typing: mypy fix for `orm.List` the `pop` and `index` methods (#6635) The function signature of `pop` and `index` methods of `orm.List` is updated to synchronous with `MutableSequence` which it inherit from. The implementation of `orm.List` is expected to be the same as vanilla python `list` type. The new function signature has the index parameter for `pop` with default `-1` for pop the last element from the list. This change might be a break change because the previous implementation can be used differently. --- src/aiida/orm/nodes/data/list.py | 7 ++++--- src/aiida/tools/pytest_fixtures/daemon.py | 2 +- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/src/aiida/orm/nodes/data/list.py b/src/aiida/orm/nodes/data/list.py index fc39dd1acd..d2c0857b35 100644 --- a/src/aiida/orm/nodes/data/list.py +++ b/src/aiida/orm/nodes/data/list.py @@ -9,6 +9,7 @@ """`Data` sub class to represent a list.""" from collections.abc import MutableSequence +from typing import Any from .base import to_aiida_type from .data import Data @@ -81,15 +82,15 @@ def remove(self, value): self.set_list(data) return item - def pop(self, **kwargs): # type: ignore[override] + def pop(self, index: int = -1) -> Any: """Remove and return item at index (default last).""" data = self.get_list() - item = data.pop(**kwargs) + item = data.pop(index) if not self._using_list_reference(): self.set_list(data) return item - def index(self, value): # type: ignore[override] + def index(self, value: Any, start: int = 0, stop: int = 0) -> int: """Return first index of value..""" return self.get_list().index(value) diff --git a/src/aiida/tools/pytest_fixtures/daemon.py b/src/aiida/tools/pytest_fixtures/daemon.py index 2b74e4ce77..89ef02d841 100644 --- a/src/aiida/tools/pytest_fixtures/daemon.py +++ b/src/aiida/tools/pytest_fixtures/daemon.py @@ -116,7 +116,7 @@ def test(submit_and_await): from aiida.engine import ProcessState def factory( - submittable: type[Process] | ProcessBuilder | ProcessNode | t.Any, + submittable: type[Process] | ProcessBuilder | ProcessNode, state: ProcessState = ProcessState.FINISHED, timeout: int = 20, **kwargs, From f74adb94cc1e8439c8076f563ec112466fdd174b Mon Sep 17 00:00:00 2001 From: Alexander Goscinski Date: Tue, 26 Nov 2024 15:53:38 +0100 Subject: [PATCH 30/31] CLI: Handle `None` process states in build_call_graph (#6590) The process definition allows a None state but the `build_call_graph` utils function intended for creating a printable output of the call graph does crash in this case. A None state indicates an undefined behavior in the code, but the application should not fail because of the formatting of the output. This commit makes `build_call_graph` handle `None` states. Partial fix for #6585. --- src/aiida/cmdline/utils/ascii_vis.py | 2 +- tests/cmdline/utils/test_ascii_vis.py | 20 ++++++++++++++++++++ 2 files changed, 21 insertions(+), 1 deletion(-) create mode 100644 tests/cmdline/utils/test_ascii_vis.py diff --git a/src/aiida/cmdline/utils/ascii_vis.py b/src/aiida/cmdline/utils/ascii_vis.py index f42317e7a8..502abf3bcf 100644 --- a/src/aiida/cmdline/utils/ascii_vis.py +++ b/src/aiida/cmdline/utils/ascii_vis.py @@ -29,7 +29,7 @@ def calc_info(node, call_link_label: bool = False) -> str: raise TypeError(f'Unknown type: {type(node)}') process_label = node.process_label - process_state = node.process_state.value.capitalize() + process_state = 'None' if node.process_state is None else node.process_state.value.capitalize() exit_status = node.exit_status if call_link_label and (caller := node.caller): diff --git a/tests/cmdline/utils/test_ascii_vis.py b/tests/cmdline/utils/test_ascii_vis.py new file mode 100644 index 0000000000..9fb6d26423 --- /dev/null +++ b/tests/cmdline/utils/test_ascii_vis.py @@ -0,0 +1,20 @@ +########################################################################### +# Copyright (c), The AiiDA team. All rights reserved. # +# This file is part of the AiiDA code. # +# # +# The code is hosted on GitHub at https://github.com/aiidateam/aiida-core # +# For further information on the license, see the LICENSE.txt file # +# For further information please visit http://www.aiida.net # +########################################################################### +"""Tests for the :mod:`aiida.cmdline.utils.ascii_vis` module.""" + +from aiida.orm.nodes.process.process import ProcessNode + + +def test_build_call_graph(): + from aiida.cmdline.utils.ascii_vis import build_call_graph + + node = ProcessNode() + + call_graph = build_call_graph(node) + assert call_graph == 'None None' From 9baf3ca96caa5577ec8ed6cef69512f430bb5675 Mon Sep 17 00:00:00 2001 From: Jusong Yu Date: Wed, 27 Nov 2024 19:36:54 +0100 Subject: [PATCH 31/31] Use only one global var for marking config folder tree (#6610) The changes reduce the use of global variables by replacing `DAEMON_DIR`, `DAEMON_LOG_DIR` and `ACCESS_CONTROL_DIR` as derived from helper class `AiiDAConfigPathResolver`. The `AIIDA_CONFIG_FOLDER` is moved as the class attribute `_glb_aiida_config_folder` of `AiiDAConfigDir` which provide `setter/getter` method for access and set the config_folder globally. Meanwhile, the `filepaths` method is depracted from profile and moved to config module. It now called with passing the profile. Since the derived files makes more sense as attaching to the config folder location for every profile. --- .pre-commit-config.yaml | 1 - src/aiida/cmdline/commands/cmd_presto.py | 6 +- src/aiida/cmdline/commands/cmd_profile.py | 4 +- src/aiida/cmdline/commands/cmd_status.py | 5 +- .../cmdline/params/options/commands/setup.py | 5 +- src/aiida/engine/daemon/client.py | 18 +-- src/aiida/manage/configuration/__init__.py | 6 +- src/aiida/manage/configuration/config.py | 30 ++++ src/aiida/manage/configuration/profile.py | 22 +-- src/aiida/manage/configuration/settings.py | 135 ++++++++++-------- src/aiida/manage/profile_access.py | 8 +- src/aiida/manage/tests/pytest_fixtures.py | 6 +- src/aiida/storage/sqlite_dos/backend.py | 4 +- .../tools/pytest_fixtures/configuration.py | 6 +- tests/conftest.py | 6 +- tests/manage/configuration/test_config.py | 27 ++-- tests/manage/test_caching_config.py | 9 +- 17 files changed, 175 insertions(+), 123 deletions(-) diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index 3bcd486ef1..7a88a2ab99 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -125,7 +125,6 @@ repos: src/aiida/engine/processes/ports.py| src/aiida/manage/configuration/__init__.py| src/aiida/manage/configuration/config.py| - src/aiida/manage/configuration/profile.py| src/aiida/manage/external/rmq/launcher.py| src/aiida/manage/tests/main.py| src/aiida/manage/tests/pytest_fixtures.py| diff --git a/src/aiida/cmdline/commands/cmd_presto.py b/src/aiida/cmdline/commands/cmd_presto.py index 09d7070e7c..eeb98fad75 100644 --- a/src/aiida/cmdline/commands/cmd_presto.py +++ b/src/aiida/cmdline/commands/cmd_presto.py @@ -67,7 +67,7 @@ def detect_postgres_config( """ import secrets - from aiida.manage.configuration.settings import AIIDA_CONFIG_FOLDER + from aiida.manage.configuration.settings import AiiDAConfigDir from aiida.manage.external.postgres import Postgres dbinfo = { @@ -92,13 +92,15 @@ def detect_postgres_config( except Exception as exception: raise ConnectionError(f'Unable to automatically create the PostgreSQL user and database: {exception}') + aiida_config_folder = AiiDAConfigDir.get() + return { 'database_hostname': postgres_hostname, 'database_port': postgres_port, 'database_name': database_name, 'database_username': database_username, 'database_password': database_password, - 'repository_uri': f'file://{AIIDA_CONFIG_FOLDER / "repository" / profile_name}', + 'repository_uri': f'file://{aiida_config_folder / "repository" / profile_name}', } diff --git a/src/aiida/cmdline/commands/cmd_profile.py b/src/aiida/cmdline/commands/cmd_profile.py index 3dd21b56bf..7cb0e018ae 100644 --- a/src/aiida/cmdline/commands/cmd_profile.py +++ b/src/aiida/cmdline/commands/cmd_profile.py @@ -169,9 +169,9 @@ def profile_list(): # This can happen for a fresh install and the `verdi setup` has not yet been run. In this case it is still nice # to be able to see the configuration directory, for instance for those who have set `AIIDA_PATH`. This way # they can at least verify that it is correctly set. - from aiida.manage.configuration.settings import AIIDA_CONFIG_FOLDER + from aiida.manage.configuration.settings import AiiDAConfigDir - echo.echo_report(f'configuration folder: {AIIDA_CONFIG_FOLDER}') + echo.echo_report(f'configuration folder: {AiiDAConfigDir.get()}') echo.echo_critical(str(exception)) else: echo.echo_report(f'configuration folder: {config.dirpath}') diff --git a/src/aiida/cmdline/commands/cmd_status.py b/src/aiida/cmdline/commands/cmd_status.py index f3c32327dc..85ef292fa7 100644 --- a/src/aiida/cmdline/commands/cmd_status.py +++ b/src/aiida/cmdline/commands/cmd_status.py @@ -61,13 +61,14 @@ def verdi_status(print_traceback, no_rmq): from aiida.common.docs import URL_NO_BROKER from aiida.common.exceptions import ConfigurationError from aiida.engine.daemon.client import DaemonException, DaemonNotRunningException - from aiida.manage.configuration.settings import AIIDA_CONFIG_FOLDER + from aiida.manage.configuration.settings import AiiDAConfigDir from aiida.manage.manager import get_manager exit_code = ExitCode.SUCCESS + configure_directory = AiiDAConfigDir.get() print_status(ServiceStatus.UP, 'version', f'AiiDA v{__version__}') - print_status(ServiceStatus.UP, 'config', AIIDA_CONFIG_FOLDER) + print_status(ServiceStatus.UP, 'config', configure_directory) manager = get_manager() diff --git a/src/aiida/cmdline/params/options/commands/setup.py b/src/aiida/cmdline/params/options/commands/setup.py index 930aa97018..49cfc1e121 100644 --- a/src/aiida/cmdline/params/options/commands/setup.py +++ b/src/aiida/cmdline/params/options/commands/setup.py @@ -66,11 +66,12 @@ def get_repository_uri_default(ctx): """ import os - from aiida.manage.configuration.settings import AIIDA_CONFIG_FOLDER + from aiida.manage.configuration.settings import AiiDAConfigDir validate_profile_parameter(ctx) + configure_directory = AiiDAConfigDir.get() - return os.path.join(AIIDA_CONFIG_FOLDER, 'repository', ctx.params['profile'].name) + return os.path.join(configure_directory, 'repository', ctx.params['profile'].name) def get_quicksetup_repository_uri(ctx, param, value): diff --git a/src/aiida/engine/daemon/client.py b/src/aiida/engine/daemon/client.py index ef250802f7..4e47e7ed60 100644 --- a/src/aiida/engine/daemon/client.py +++ b/src/aiida/engine/daemon/client.py @@ -94,10 +94,10 @@ def __init__(self, profile: Profile): from aiida.common.docs import URL_NO_BROKER type_check(profile, Profile) - config = get_config() + self._config = get_config() self._profile = profile self._socket_directory: str | None = None - self._daemon_timeout: int = config.get_option('daemon.timeout', scope=profile.name) + self._daemon_timeout: int = self._config.get_option('daemon.timeout', scope=profile.name) if self._profile.process_control_backend is None: raise ConfigurationError( @@ -156,31 +156,31 @@ def virtualenv(self) -> str | None: @property def circus_log_file(self) -> str: - return self.profile.filepaths['circus']['log'] + return self._config.filepaths(self.profile)['circus']['log'] @property def circus_pid_file(self) -> str: - return self.profile.filepaths['circus']['pid'] + return self._config.filepaths(self.profile)['circus']['pid'] @property def circus_port_file(self) -> str: - return self.profile.filepaths['circus']['port'] + return self._config.filepaths(self.profile)['circus']['port'] @property def circus_socket_file(self) -> str: - return self.profile.filepaths['circus']['socket']['file'] + return self._config.filepaths(self.profile)['circus']['socket']['file'] @property def circus_socket_endpoints(self) -> dict[str, str]: - return self.profile.filepaths['circus']['socket'] + return self._config.filepaths(self.profile)['circus']['socket'] @property def daemon_log_file(self) -> str: - return self.profile.filepaths['daemon']['log'] + return self._config.filepaths(self.profile)['daemon']['log'] @property def daemon_pid_file(self) -> str: - return self.profile.filepaths['daemon']['pid'] + return self._config.filepaths(self.profile)['daemon']['pid'] def get_circus_port(self) -> int: """Retrieve the port for the circus controller, which should be written to the circus port file. diff --git a/src/aiida/manage/configuration/__init__.py b/src/aiida/manage/configuration/__init__.py index 7227281507..097fcfc012 100644 --- a/src/aiida/manage/configuration/__init__.py +++ b/src/aiida/manage/configuration/__init__.py @@ -65,10 +65,10 @@ def get_config_path(): - """Returns path to .aiida configuration directory.""" - from .settings import AIIDA_CONFIG_FOLDER, DEFAULT_CONFIG_FILE_NAME + """Returns path to aiida configuration file.""" + from .settings import DEFAULT_CONFIG_FILE_NAME, AiiDAConfigDir - return os.path.join(AIIDA_CONFIG_FOLDER, DEFAULT_CONFIG_FILE_NAME) + return os.path.join(AiiDAConfigDir.get(), DEFAULT_CONFIG_FILE_NAME) def load_config(create=False) -> 'Config': diff --git a/src/aiida/manage/configuration/config.py b/src/aiida/manage/configuration/config.py index 4b1f032271..fff83f4330 100644 --- a/src/aiida/manage/configuration/config.py +++ b/src/aiida/manage/configuration/config.py @@ -21,6 +21,7 @@ import json import os import uuid +from pathlib import Path from typing import Any, Dict, List, Optional, Tuple from pydantic import ( @@ -780,3 +781,32 @@ def _atomic_write(self, filepath=None): handle.flush() os.rename(handle.name, self.filepath) + + def filepaths(self, profile: Profile): + """Return the filepaths used by a profile. + + :return: a dictionary of filepaths + """ + from aiida.manage.configuration.settings import AiiDAConfigPathResolver + + _config_path_resolver: AiiDAConfigPathResolver = AiiDAConfigPathResolver(Path(self.dirpath)) + daemon_dir = _config_path_resolver.daemon_dir + daemon_log_dir = _config_path_resolver.daemon_log_dir + + return { + 'circus': { + 'log': str(daemon_log_dir / f'circus-{profile.name}.log'), + 'pid': str(daemon_dir / f'circus-{profile.name}.pid'), + 'port': str(daemon_dir / f'circus-{profile.name}.port'), + 'socket': { + 'file': str(daemon_dir / f'circus-{profile.name}.sockets'), + 'controller': 'circus.c.sock', + 'pubsub': 'circus.p.sock', + 'stats': 'circus.s.sock', + }, + }, + 'daemon': { + 'log': str(daemon_log_dir / f'aiida-{profile.name}.log'), + 'pid': str(daemon_dir / f'aiida-{profile.name}.pid'), + }, + } diff --git a/src/aiida/manage/configuration/profile.py b/src/aiida/manage/configuration/profile.py index acaca2e892..93a3c5c911 100644 --- a/src/aiida/manage/configuration/profile.py +++ b/src/aiida/manage/configuration/profile.py @@ -56,7 +56,7 @@ def __init__(self, name: str, config: Mapping[str, Any], validate=True): ) self._name = name - self._attributes: Dict[str, Any] = deepcopy(config) + self._attributes: Dict[str, Any] = deepcopy(config) # type: ignore[arg-type] # Create a default UUID if not specified if self._attributes.get(self.KEY_UUID, None) is None: @@ -235,22 +235,28 @@ def filepaths(self): :return: a dictionary of filepaths """ - from .settings import DAEMON_DIR, DAEMON_LOG_DIR + from aiida.common.warnings import warn_deprecation + from aiida.manage.configuration.settings import AiiDAConfigPathResolver + + warn_deprecation('This method has been deprecated, use `filepaths` method from `Config` obj instead', version=3) + + daemon_dir = AiiDAConfigPathResolver().daemon_dir + daemon_log_dir = AiiDAConfigPathResolver().daemon_log_dir return { 'circus': { - 'log': str(DAEMON_LOG_DIR / f'circus-{self.name}.log'), - 'pid': str(DAEMON_DIR / f'circus-{self.name}.pid'), - 'port': str(DAEMON_DIR / f'circus-{self.name}.port'), + 'log': str(daemon_log_dir / f'circus-{self.name}.log'), + 'pid': str(daemon_dir / f'circus-{self.name}.pid'), + 'port': str(daemon_dir / f'circus-{self.name}.port'), 'socket': { - 'file': str(DAEMON_DIR / f'circus-{self.name}.sockets'), + 'file': str(daemon_dir / f'circus-{self.name}.sockets'), 'controller': 'circus.c.sock', 'pubsub': 'circus.p.sock', 'stats': 'circus.s.sock', }, }, 'daemon': { - 'log': str(DAEMON_LOG_DIR / f'aiida-{self.name}.log'), - 'pid': str(DAEMON_DIR / f'aiida-{self.name}.pid'), + 'log': str(daemon_log_dir / f'aiida-{self.name}.log'), + 'pid': str(daemon_dir / f'aiida-{self.name}.pid'), }, } diff --git a/src/aiida/manage/configuration/settings.py b/src/aiida/manage/configuration/settings.py index 168abb8879..f47a2dd66e 100644 --- a/src/aiida/manage/configuration/settings.py +++ b/src/aiida/manage/configuration/settings.py @@ -13,6 +13,7 @@ import os import pathlib import warnings +from typing import final DEFAULT_UMASK = 0o0077 DEFAULT_AIIDA_PATH_VARIABLE = 'AIIDA_PATH' @@ -25,38 +26,86 @@ DEFAULT_DAEMON_LOG_DIR_NAME = 'log' DEFAULT_ACCESS_CONTROL_DIR_NAME = 'access' -# Assign defaults which may be overriden in set_configuration_directory() below -AIIDA_CONFIG_FOLDER: pathlib.Path = pathlib.Path(DEFAULT_AIIDA_PATH).expanduser() / DEFAULT_CONFIG_DIR_NAME -DAEMON_DIR: pathlib.Path = AIIDA_CONFIG_FOLDER / DEFAULT_DAEMON_DIR_NAME -DAEMON_LOG_DIR: pathlib.Path = DAEMON_DIR / DEFAULT_DAEMON_LOG_DIR_NAME -ACCESS_CONTROL_DIR: pathlib.Path = AIIDA_CONFIG_FOLDER / DEFAULT_ACCESS_CONTROL_DIR_NAME +__all__ = ('AiiDAConfigPathResolver', 'AiiDAConfigDir') -def create_instance_directories() -> None: +@final +class AiiDAConfigDir: + """Singleton for setting and getting the path to configuration directory.""" + + _glb_aiida_config_folder: pathlib.Path = pathlib.Path(DEFAULT_AIIDA_PATH).expanduser() / DEFAULT_CONFIG_DIR_NAME + + @classmethod + def get(cls): + """Return the path of the configuration directory.""" + return cls._glb_aiida_config_folder + + @classmethod + def set(cls, aiida_config_folder: pathlib.Path | None = None) -> None: + """Set the configuration directory, related global variables and create instance directories. + + The location of the configuration directory is defined by ``aiida_config_folder`` or if not defined, + the path that is returned by ``_get_configuration_directory_from_envvar``. + Or if the environment_variable not set, use the default. + If the directory does not exist yet, it is created, together with all its subdirectories. + """ + _default_dirpath_config = pathlib.Path(DEFAULT_AIIDA_PATH).expanduser() / DEFAULT_CONFIG_DIR_NAME + + aiida_config_folder = aiida_config_folder or _get_configuration_directory_from_envvar() + cls._glb_aiida_config_folder = aiida_config_folder or _default_dirpath_config + + _create_instance_directories(cls._glb_aiida_config_folder) + + +@final +class AiiDAConfigPathResolver: + """For resolving configuration directory, daemon dir, daemon log dir and access control dir. + The locations are all trivially derived from the config directory. + """ + + def __init__(self, config_folder: pathlib.Path | None = None) -> None: + self._aiida_path = config_folder or AiiDAConfigDir.get() + + @property + def aiida_path(self) -> pathlib.Path: + return self._aiida_path + + @property + def daemon_dir(self) -> pathlib.Path: + return self._aiida_path / DEFAULT_DAEMON_DIR_NAME + + @property + def daemon_log_dir(self) -> pathlib.Path: + return self._aiida_path / DEFAULT_DAEMON_DIR_NAME / DEFAULT_DAEMON_LOG_DIR_NAME + + @property + def access_control_dir(self) -> pathlib.Path: + return self._aiida_path / DEFAULT_ACCESS_CONTROL_DIR_NAME + + +def _create_instance_directories(aiida_config_folder: pathlib.Path | None) -> None: """Create the base directories required for a new AiiDA instance. - This will create the base AiiDA directory defined by the AIIDA_CONFIG_FOLDER variable, unless it already exists. - Subsequently, it will create the daemon directory within it and the daemon log directory. + This will create the base AiiDA directory in ``aiida_config_folder``. + If it not provided, the directory returned from ``AiiDAConfigDir.get()`` will be the default config folder, + unless it already exists. Subsequently, it will create the daemon directory within it and the daemon log directory. """ from aiida.common import ConfigurationError - directory_base = AIIDA_CONFIG_FOLDER.expanduser() - directory_daemon = directory_base / DAEMON_DIR - directory_daemon_log = directory_base / DAEMON_LOG_DIR - directory_access = directory_base / ACCESS_CONTROL_DIR + path_resolver = AiiDAConfigPathResolver(aiida_config_folder) list_of_paths = [ - directory_base, - directory_daemon, - directory_daemon_log, - directory_access, + path_resolver.aiida_path, + path_resolver.daemon_dir, + path_resolver.daemon_log_dir, + path_resolver.access_control_dir, ] umask = os.umask(DEFAULT_UMASK) try: for path in list_of_paths: - if path is directory_base and not path.exists(): + if path is path_resolver.aiida_path and not path.exists(): warnings.warn(f'Creating AiiDA configuration folder `{path}`.') try: @@ -64,31 +113,10 @@ def create_instance_directories() -> None: except OSError as exc: raise ConfigurationError(f'could not create the `{path}` configuration directory: {exc}') from exc finally: - os.umask(umask) + _ = os.umask(umask) -def get_configuration_directory(): - """Return the path of the configuration directory. - - The location of the configuration directory is defined following these heuristics in order: - - * If the ``AIIDA_PATH`` variable is set, all the paths will be checked to see if they contain a - configuration folder. The first one to be encountered will be set as ``AIIDA_CONFIG_FOLDER``. If none of them - contain one, the last path defined in the environment variable considered is used. - * If an existing directory is still not found, the ``DEFAULT_AIIDA_PATH`` is used. - - :returns: The path of the configuration directory. - """ - dirpath_config = get_configuration_directory_from_envvar() - - # If no existing configuration directory is found, fall back to the default - if dirpath_config is None: - dirpath_config = pathlib.Path(DEFAULT_AIIDA_PATH).expanduser() / DEFAULT_CONFIG_DIR_NAME - - return dirpath_config - - -def get_configuration_directory_from_envvar() -> pathlib.Path | None: +def _get_configuration_directory_from_envvar() -> pathlib.Path | None: """Return the path of a config directory from the ``AIIDA_PATH`` environment variable. The environment variable should be a colon separated string of filepaths that either point directly to a config @@ -99,10 +127,13 @@ def get_configuration_directory_from_envvar() -> pathlib.Path | None: """ environment_variable = os.environ.get(DEFAULT_AIIDA_PATH_VARIABLE) + default_dirpath_config = pathlib.Path(DEFAULT_AIIDA_PATH).expanduser() / DEFAULT_CONFIG_DIR_NAME + if environment_variable is None: return None # Loop over all the paths in the ``AIIDA_PATH`` variable to see if any of them contain a configuration folder + dirpath_config = None for base_dir_path in [path for path in environment_variable.split(':') if path]: dirpath_config = pathlib.Path(base_dir_path).expanduser() @@ -115,28 +146,8 @@ def get_configuration_directory_from_envvar() -> pathlib.Path | None: if dirpath_config.is_dir(): break - return dirpath_config - - -def set_configuration_directory(aiida_config_folder: pathlib.Path | None = None) -> None: - """Set the configuration directory, related global variables and create instance directories. - - The location of the configuration directory is defined by ``aiida_config_folder`` or if not defined, the path that - is returned by ``get_configuration_directory``. If the directory does not exist yet, it is created, together with - all its subdirectories. - """ - global AIIDA_CONFIG_FOLDER # noqa: PLW0603 - global DAEMON_DIR # noqa: PLW0603 - global DAEMON_LOG_DIR # noqa: PLW0603 - global ACCESS_CONTROL_DIR # noqa: PLW0603 - - AIIDA_CONFIG_FOLDER = aiida_config_folder or get_configuration_directory() - DAEMON_DIR = AIIDA_CONFIG_FOLDER / DEFAULT_DAEMON_DIR_NAME - DAEMON_LOG_DIR = DAEMON_DIR / DEFAULT_DAEMON_LOG_DIR_NAME - ACCESS_CONTROL_DIR = AIIDA_CONFIG_FOLDER / DEFAULT_ACCESS_CONTROL_DIR_NAME - - create_instance_directories() + return dirpath_config or default_dirpath_config # Initialize the configuration directory settings -set_configuration_directory() +AiiDAConfigDir.set() diff --git a/src/aiida/manage/profile_access.py b/src/aiida/manage/profile_access.py index 5b04481e66..c65364af03 100644 --- a/src/aiida/manage/profile_access.py +++ b/src/aiida/manage/profile_access.py @@ -18,8 +18,10 @@ from aiida.common.exceptions import LockedProfileError, LockingProfileError from aiida.common.lang import type_check from aiida.manage.configuration import Profile +from aiida.manage.configuration.settings import AiiDAConfigPathResolver +@typing.final class ProfileAccessManager: """Class to manage access to a profile. @@ -45,12 +47,10 @@ def __init__(self, profile: Profile): :param profile: the profile whose access to manage. """ - from aiida.manage.configuration.settings import ACCESS_CONTROL_DIR - - type_check(profile, Profile) + _ = type_check(profile, Profile) self.profile = profile self.process = psutil.Process(os.getpid()) - self._dirpath_records = ACCESS_CONTROL_DIR / profile.name + self._dirpath_records = AiiDAConfigPathResolver().access_control_dir / profile.name self._dirpath_records.mkdir(exist_ok=True) def request_access(self) -> None: diff --git a/src/aiida/manage/tests/pytest_fixtures.py b/src/aiida/manage/tests/pytest_fixtures.py index 92856aff66..6c5d04d3bc 100644 --- a/src/aiida/manage/tests/pytest_fixtures.py +++ b/src/aiida/manage/tests/pytest_fixtures.py @@ -162,6 +162,7 @@ def aiida_instance( """ from aiida.manage import configuration from aiida.manage.configuration import settings + from aiida.manage.configuration.settings import AiiDAConfigDir if aiida_test_profile: yield configuration.get_config() @@ -178,8 +179,7 @@ def aiida_instance( dirpath_config = tmp_path_factory.mktemp('config') os.environ[settings.DEFAULT_AIIDA_PATH_VARIABLE] = str(dirpath_config) - settings.AIIDA_CONFIG_FOLDER = dirpath_config - settings.set_configuration_directory() + AiiDAConfigDir.set(dirpath_config) configuration.CONFIG = configuration.load_config(create=True) try: @@ -191,7 +191,7 @@ def aiida_instance( else: os.environ[settings.DEFAULT_AIIDA_PATH_VARIABLE] = current_path_variable - settings.AIIDA_CONFIG_FOLDER = current_config_path + AiiDAConfigDir.set(current_config_path) configuration.CONFIG = current_config if current_profile: aiida_manager.load_profile(current_profile.name, allow_switch=True) diff --git a/src/aiida/storage/sqlite_dos/backend.py b/src/aiida/storage/sqlite_dos/backend.py index 7be70f4a1c..2443fee259 100644 --- a/src/aiida/storage/sqlite_dos/backend.py +++ b/src/aiida/storage/sqlite_dos/backend.py @@ -26,7 +26,7 @@ from aiida.common import exceptions from aiida.common.log import AIIDA_LOGGER from aiida.manage.configuration.profile import Profile -from aiida.manage.configuration.settings import AIIDA_CONFIG_FOLDER +from aiida.manage.configuration.settings import AiiDAConfigDir from aiida.orm.implementation import BackendEntity from aiida.storage.log import MIGRATE_LOGGER from aiida.storage.psql_dos.models.settings import DbSetting @@ -203,7 +203,7 @@ class Model(BaseModel, defer_build=True): filepath: str = Field( title='Directory of the backend', description='Filepath of the directory in which to store data for this backend.', - default_factory=lambda: str(AIIDA_CONFIG_FOLDER / 'repository' / f'sqlite_dos_{uuid4().hex}'), + default_factory=lambda: str(AiiDAConfigDir.get() / 'repository' / f'sqlite_dos_{uuid4().hex}'), ) @field_validator('filepath') diff --git a/src/aiida/tools/pytest_fixtures/configuration.py b/src/aiida/tools/pytest_fixtures/configuration.py index ca38db3fb0..ed06072b71 100644 --- a/src/aiida/tools/pytest_fixtures/configuration.py +++ b/src/aiida/tools/pytest_fixtures/configuration.py @@ -10,6 +10,8 @@ import pytest +from aiida.manage.configuration.settings import AiiDAConfigDir + if t.TYPE_CHECKING: from aiida.manage.configuration.config import Config @@ -53,7 +55,7 @@ def factory(dirpath: pathlib.Path): dirpath_config = dirpath / settings.DEFAULT_CONFIG_DIR_NAME os.environ[settings.DEFAULT_AIIDA_PATH_VARIABLE] = str(dirpath_config) - settings.set_configuration_directory(dirpath_config) + AiiDAConfigDir.set(dirpath_config) config = get_config(create=True) try: @@ -61,7 +63,7 @@ def factory(dirpath: pathlib.Path): finally: if current_config: reset_config() - settings.set_configuration_directory(pathlib.Path(current_config.dirpath)) + AiiDAConfigDir.set(pathlib.Path(current_config.dirpath)) get_config() if current_path_variable is None: diff --git a/tests/conftest.py b/tests/conftest.py index fb6780d966..b50cf90843 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -373,7 +373,7 @@ def empty_config(tmp_path) -> Config: """ from aiida.common.utils import Capturing from aiida.manage import configuration, get_manager - from aiida.manage.configuration import settings + from aiida.manage.configuration.settings import AiiDAConfigDir manager = get_manager() @@ -387,7 +387,7 @@ def empty_config(tmp_path) -> Config: # Set the configuration directory to a temporary directory. This will create the necessary folders for an empty # AiiDA configuration and set relevant global variables in :mod:`aiida.manage.configuration.settings`. - settings.set_configuration_directory(tmp_path) + AiiDAConfigDir.set(tmp_path) # The constructor of `Config` called by `load_config` will print warning messages about migrating it with Capturing(): @@ -405,7 +405,7 @@ def empty_config(tmp_path) -> Config: # like the :class:`aiida.engine.daemon.client.DaemonClient` will not function properly after a test that uses # this fixture because the paths of the daemon files would still point to the path of the temporary config # folder created by this fixture. - settings.set_configuration_directory(pathlib.Path(current_config_path)) + AiiDAConfigDir.set(pathlib.Path(current_config_path)) # Reload the original profile manager.load_profile(current_profile_name) diff --git a/tests/manage/configuration/test_config.py b/tests/manage/configuration/test_config.py index b1decb09a7..7fedb4bf2a 100644 --- a/tests/manage/configuration/test_config.py +++ b/tests/manage/configuration/test_config.py @@ -19,6 +19,7 @@ from aiida.manage.configuration.config import Config from aiida.manage.configuration.migrations import CURRENT_CONFIG_VERSION, OLDEST_COMPATIBLE_CONFIG_VERSION from aiida.manage.configuration.options import get_option +from aiida.manage.configuration.settings import AiiDAConfigDir from aiida.storage.sqlite_temp import SqliteTempBackend @@ -42,7 +43,7 @@ def cache_aiida_path_variable(): # Make sure to reset the global variables set by the following call that are dependent on the environment variable # ``DEFAULT_AIIDA_PATH_VARIABLE``. It may have been changed by a test using this fixture. - settings.set_configuration_directory() + AiiDAConfigDir.set() @pytest.mark.filterwarnings('ignore:Creating AiiDA configuration folder') @@ -65,11 +66,11 @@ def test_environment_variable_not_set(chdir_tmp_path, monkeypatch): del os.environ[settings.DEFAULT_AIIDA_PATH_VARIABLE] except KeyError: pass - settings.set_configuration_directory() + AiiDAConfigDir.set() config_folder = chdir_tmp_path / settings.DEFAULT_CONFIG_DIR_NAME assert os.path.isdir(config_folder) - assert settings.AIIDA_CONFIG_FOLDER == pathlib.Path(config_folder) + assert AiiDAConfigDir.get() == pathlib.Path(config_folder) @pytest.mark.filterwarnings('ignore:Creating AiiDA configuration folder') @@ -78,12 +79,12 @@ def test_environment_variable_set_single_path_without_config_folder(tmp_path): """If `AIIDA_PATH` is set but does not contain a configuration folder, it should be created.""" # Set the environment variable and call configuration initialization os.environ[settings.DEFAULT_AIIDA_PATH_VARIABLE] = str(tmp_path) - settings.set_configuration_directory() + AiiDAConfigDir.set() # This should have created the configuration directory in the path config_folder = tmp_path / settings.DEFAULT_CONFIG_DIR_NAME assert config_folder.is_dir() - assert settings.AIIDA_CONFIG_FOLDER == config_folder + assert AiiDAConfigDir.get() == config_folder @pytest.mark.filterwarnings('ignore:Creating AiiDA configuration folder') @@ -94,12 +95,12 @@ def test_environment_variable_set_single_path_with_config_folder(tmp_path): # Set the environment variable and call configuration initialization os.environ[settings.DEFAULT_AIIDA_PATH_VARIABLE] = str(tmp_path) - settings.set_configuration_directory() + AiiDAConfigDir.set() # This should have created the configuration directory in the path config_folder = tmp_path / settings.DEFAULT_CONFIG_DIR_NAME assert config_folder.is_dir() - assert settings.AIIDA_CONFIG_FOLDER == config_folder + assert AiiDAConfigDir.get() == config_folder @pytest.mark.filterwarnings('ignore:Creating AiiDA configuration folder') @@ -114,12 +115,12 @@ def test_environment_variable_path_including_config_folder(tmp_path): """ # Set the environment variable with a path that include base folder name and call config initialization os.environ[settings.DEFAULT_AIIDA_PATH_VARIABLE] = str(tmp_path / settings.DEFAULT_CONFIG_DIR_NAME) - settings.set_configuration_directory() + AiiDAConfigDir.set() # This should have created the configuration directory in the pathpath config_folder = tmp_path / settings.DEFAULT_CONFIG_DIR_NAME assert config_folder.is_dir() - assert settings.AIIDA_CONFIG_FOLDER == config_folder + assert AiiDAConfigDir.get() == config_folder @pytest.mark.filterwarnings('ignore:Creating AiiDA configuration folder') @@ -137,12 +138,12 @@ def test_environment_variable_set_multiple_path(tmp_path): # Set the environment variable to contain three paths and call configuration initialization env_variable = f'{directory_a}:{directory_b}:{directory_c}' os.environ[settings.DEFAULT_AIIDA_PATH_VARIABLE] = env_variable - settings.set_configuration_directory() + AiiDAConfigDir.set() # This should have created the configuration directory in the last path config_folder = directory_c / settings.DEFAULT_CONFIG_DIR_NAME assert os.path.isdir(config_folder) - assert settings.AIIDA_CONFIG_FOLDER == config_folder + assert AiiDAConfigDir.get() == config_folder def compare_config_in_memory_and_on_disk(config, filepath): @@ -152,9 +153,7 @@ def compare_config_in_memory_and_on_disk(config, filepath): :param filepath: absolute filepath to a configuration file :raises AssertionError: if content of `config` is not equal to that of file on disk """ - from aiida.manage.configuration.settings import DEFAULT_CONFIG_INDENT_SIZE - - in_memory = json.dumps(config.dictionary, indent=DEFAULT_CONFIG_INDENT_SIZE) + in_memory = json.dumps(config.dictionary, indent=settings.DEFAULT_CONFIG_INDENT_SIZE) # Read the content stored on disk with open(filepath, 'r', encoding='utf8') as handle: diff --git a/tests/manage/test_caching_config.py b/tests/manage/test_caching_config.py index 50a2e4ac48..932a32873c 100644 --- a/tests/manage/test_caching_config.py +++ b/tests/manage/test_caching_config.py @@ -45,11 +45,12 @@ def test_merge_deprecated_yaml(tmp_path): """ from aiida.common.warnings import AiidaDeprecationWarning from aiida.manage import configuration, get_manager - from aiida.manage.configuration import get_config_option, load_profile, settings + from aiida.manage.configuration import get_config_option, load_profile + from aiida.manage.configuration.settings import AiiDAConfigDir # Store the current configuration instance and config directory path current_config = configuration.CONFIG - current_config_path = current_config.dirpath + current_config_path = pathlib.Path(current_config.dirpath) current_profile_name = configuration.get_profile().name try: @@ -57,7 +58,7 @@ def test_merge_deprecated_yaml(tmp_path): configuration.CONFIG = None # Create a temporary folder, set it as the current config directory path - settings.AIIDA_CONFIG_FOLDER = str(tmp_path) + AiiDAConfigDir.set(pathlib.Path(tmp_path)) config_dictionary = json.loads( pathlib.Path(__file__) .parent.joinpath('configuration/migrations/test_samples/reference/6.json') @@ -86,7 +87,7 @@ def test_merge_deprecated_yaml(tmp_path): # Reset the config folder path and the config instance. Note this will always be executed after the yield no # matter what happened in the test that used this fixture. get_manager().unload_profile() - settings.AIIDA_CONFIG_FOLDER = current_config_path + AiiDAConfigDir.set(current_config_path) configuration.CONFIG = current_config load_profile(current_profile_name)