From 05f34f0490babc03f02be0fa2df7c0f2dece5a71 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Tue, 11 Oct 2022 20:20:39 +1100 Subject: [PATCH 01/17] Add option to always keep temporary files --- openff/bespokefit/executor/executor.py | 2 +- .../bespokefit/executor/services/_settings.py | 3 ++ .../bespokefit/executor/services/gateway.py | 2 +- .../executor/services/optimizer/worker.py | 2 +- .../optimizers/forcebalance/factories.py | 2 +- openff/bespokefit/optimizers/model.py | 2 +- .../optimizers/forcebalance/test_factories.py | 3 +- .../forcebalance/test_forcebalance.py | 3 +- .../tests/workflows/test_bespoke.py | 3 +- openff/bespokefit/utilities/tempcd.py | 53 +++++++++++++++++++ 10 files changed, 67 insertions(+), 8 deletions(-) create mode 100644 openff/bespokefit/utilities/tempcd.py diff --git a/openff/bespokefit/executor/executor.py b/openff/bespokefit/executor/executor.py index 248590bf..1978dabf 100644 --- a/openff/bespokefit/executor/executor.py +++ b/openff/bespokefit/executor/executor.py @@ -14,7 +14,6 @@ import requests import rich from openff.toolkit.typing.engines.smirnoff import ForceField -from openff.utilities import temporary_cd from pydantic import Field from rich.padding import Padding from typing_extensions import Literal @@ -34,6 +33,7 @@ from openff.bespokefit.schema.fitting import BespokeOptimizationSchema from openff.bespokefit.schema.results import BespokeOptimizationResults from openff.bespokefit.utilities.pydantic import BaseModel +from openff.bespokefit.utilities.tempcd import temporary_cd _T = TypeVar("_T") diff --git a/openff/bespokefit/executor/services/_settings.py b/openff/bespokefit/executor/services/_settings.py index a70106cb..51406920 100644 --- a/openff/bespokefit/executor/services/_settings.py +++ b/openff/bespokefit/executor/services/_settings.py @@ -63,6 +63,9 @@ class Settings(BaseSettings): BEFLOW_OPTIMIZER_WORKER_MAX_MEM: Union[float, Literal["auto"]] = "auto" BEFLOW_OPTIMIZER_KEEP_FILES: bool = False + BEFLOW_KEEP_TMP_FILES = False + """Keep all temporary files.""" + @property def fragmenter_settings(self) -> WorkerSettings: return WorkerSettings( diff --git a/openff/bespokefit/executor/services/gateway.py b/openff/bespokefit/executor/services/gateway.py index 64410269..453bff45 100644 --- a/openff/bespokefit/executor/services/gateway.py +++ b/openff/bespokefit/executor/services/gateway.py @@ -7,10 +7,10 @@ import requests import uvicorn from fastapi import APIRouter, FastAPI -from openff.utilities import temporary_cd from starlette.middleware.cors import CORSMiddleware from openff.bespokefit.executor.services import current_settings +from openff.bespokefit.utilities.tempcd import temporary_cd def __load_router(path: str) -> APIRouter: diff --git a/openff/bespokefit/executor/services/optimizer/worker.py b/openff/bespokefit/executor/services/optimizer/worker.py index dc492336..d7c2f187 100644 --- a/openff/bespokefit/executor/services/optimizer/worker.py +++ b/openff/bespokefit/executor/services/optimizer/worker.py @@ -2,7 +2,6 @@ import shutil from typing import Union -from openff.utilities import temporary_cd from pydantic import parse_raw_as from qcelemental.util import serialize @@ -19,6 +18,7 @@ BespokeOptimizationResults, OptimizationStageResults, ) +from openff.bespokefit.utilities.tempcd import temporary_cd celery_app = configure_celery_app("optimizer", connect_to_default_redis(validate=False)) diff --git a/openff/bespokefit/optimizers/forcebalance/factories.py b/openff/bespokefit/optimizers/forcebalance/factories.py index 41126b58..813f9e1b 100644 --- a/openff/bespokefit/optimizers/forcebalance/factories.py +++ b/openff/bespokefit/optimizers/forcebalance/factories.py @@ -16,7 +16,6 @@ from openff.toolkit.topology import Molecule as OFFMolecule from openff.toolkit.typing.engines.smirnoff import ForceField from openff.units import unit -from openff.utilities import temporary_cd from qcelemental.models import AtomicResult from qcelemental.models.procedures import OptimizationResult, TorsionDriveResult from qcportal.models import TorsionDriveRecord @@ -41,6 +40,7 @@ TorsionProfileTargetSchema, VibrationTargetSchema, ) +from openff.bespokefit.utilities.tempcd import temporary_cd if TYPE_CHECKING: from qcelemental.models import Molecule as QCMolecule diff --git a/openff/bespokefit/optimizers/model.py b/openff/bespokefit/optimizers/model.py index 0ddf2866..56e7afe2 100644 --- a/openff/bespokefit/optimizers/model.py +++ b/openff/bespokefit/optimizers/model.py @@ -10,13 +10,13 @@ from typing import Dict, Optional, Type from openff.toolkit.typing.engines.smirnoff import ForceField -from openff.utilities import temporary_cd from openff.bespokefit.exceptions import OptimizerError, TargetRegisterError from openff.bespokefit.schema.fitting import OptimizationStageSchema from openff.bespokefit.schema.optimizers import OptimizerSchema from openff.bespokefit.schema.results import OptimizationStageResults from openff.bespokefit.schema.targets import BaseTargetSchema +from openff.bespokefit.utilities.tempcd import temporary_cd TargetSchemaType = Type[BaseTargetSchema] diff --git a/openff/bespokefit/tests/optimizers/forcebalance/test_factories.py b/openff/bespokefit/tests/optimizers/forcebalance/test_factories.py index c1299e25..b047c8bc 100644 --- a/openff/bespokefit/tests/optimizers/forcebalance/test_factories.py +++ b/openff/bespokefit/tests/optimizers/forcebalance/test_factories.py @@ -7,7 +7,7 @@ import pytest from openff.toolkit.topology import Molecule from openff.toolkit.typing.engines.smirnoff import ForceField -from openff.utilities import skip_if_missing, temporary_cd +from openff.utilities import skip_if_missing from qcelemental.models.procedures import TorsionDriveResult from openff.bespokefit.optimizers.forcebalance.factories import ( @@ -26,6 +26,7 @@ TorsionProfileTargetSchema, VibrationTargetSchema, ) +from openff.bespokefit.utilities.tempcd import temporary_cd def read_qdata(qdata_file: str) -> Tuple[List[np.array], List[float], List[np.array]]: diff --git a/openff/bespokefit/tests/optimizers/forcebalance/test_forcebalance.py b/openff/bespokefit/tests/optimizers/forcebalance/test_forcebalance.py index 6a765e5f..9b249860 100644 --- a/openff/bespokefit/tests/optimizers/forcebalance/test_forcebalance.py +++ b/openff/bespokefit/tests/optimizers/forcebalance/test_forcebalance.py @@ -9,12 +9,13 @@ import numpy as np import pytest from openff.toolkit.typing.engines.smirnoff import ForceField -from openff.utilities import get_data_file_path, temporary_cd +from openff.utilities import get_data_file_path from openff.bespokefit.optimizers import ForceBalanceOptimizer from openff.bespokefit.schema.fitting import BaseOptimizationSchema from openff.bespokefit.schema.optimizers import ForceBalanceSchema from openff.bespokefit.schema.results import OptimizationStageResults +from openff.bespokefit.utilities.tempcd import temporary_cd @pytest.fixture() diff --git a/openff/bespokefit/tests/workflows/test_bespoke.py b/openff/bespokefit/tests/workflows/test_bespoke.py index 73fc344f..ba0c9736 100644 --- a/openff/bespokefit/tests/workflows/test_bespoke.py +++ b/openff/bespokefit/tests/workflows/test_bespoke.py @@ -8,7 +8,7 @@ from openff.qcsubmit.common_structures import QCSpec from openff.toolkit.topology import Molecule from openff.toolkit.typing.engines.smirnoff import ForceField -from openff.utilities import get_data_file_path, temporary_cd +from openff.utilities import get_data_file_path from pydantic import ValidationError from openff.bespokefit.exceptions import ( @@ -25,6 +25,7 @@ from openff.bespokefit.schema.targets import AbInitioTargetSchema from openff.bespokefit.schema.tasks import Torsion1DTaskSpec from openff.bespokefit.tests import does_not_raise +from openff.bespokefit.utilities.tempcd import temporary_cd from openff.bespokefit.workflows.bespoke import ( _DEFAULT_ROTATABLE_SMIRKS, BespokeWorkflowFactory, diff --git a/openff/bespokefit/utilities/tempcd.py b/openff/bespokefit/utilities/tempcd.py new file mode 100644 index 00000000..2c252bf9 --- /dev/null +++ b/openff/bespokefit/utilities/tempcd.py @@ -0,0 +1,53 @@ +"""""" +import os +import shutil +import tempfile +from contextlib import contextmanager +from typing import Optional + +from openff.bespokefit.executor.services import current_settings + + +@contextmanager +def temporary_cd(path: Optional[str] = None, cleanup: Optional[bool] = None): + """Temporarily move the current working directory to the path specified. + + Parameters + ---------- + path + The path to CD into. If ``None`` or not specified, a temporary directory + will be created. + cleanup + Whether the directory and its contents should be kept after the context + manager exits. If ``True``, the temporary directory and its contents + will be destroyed when the context manager exits; if ``False``, it will + be kept. If ``None`` or not specified, the directory is destroyed if + ``Path`` is not specified and the BEFLOW_KEEP_TMP_FILES setting is + ``False``. + Returns + ------- + """ + if path is not None and len(path) == 0: + yield + return + + if path is None: + path = tempfile.mkdtemp() + cleanup = ( + (not current_settings().BEFLOW_KEEP_TMP_FILES) + if cleanup is None + else cleanup + ) + else: + cleanup = False if cleanup is None else cleanup + + old_directory = os.getcwd() + + try: + os.chdir(path) + yield + + finally: + os.chdir(old_directory) + if cleanup: + shutil.rmtree(path) From 21a8f0a8f492f07f47260d7d267cebbacd792078 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Mon, 30 Jan 2023 17:01:02 +1100 Subject: [PATCH 02/17] Simplify tempcd --- openff/bespokefit/utilities/tempcd.py | 24 +++++++++--------------- 1 file changed, 9 insertions(+), 15 deletions(-) diff --git a/openff/bespokefit/utilities/tempcd.py b/openff/bespokefit/utilities/tempcd.py index 2c252bf9..ff12a77e 100644 --- a/openff/bespokefit/utilities/tempcd.py +++ b/openff/bespokefit/utilities/tempcd.py @@ -9,7 +9,7 @@ @contextmanager -def temporary_cd(path: Optional[str] = None, cleanup: Optional[bool] = None): +def temporary_cd(path: Optional[str] = None): """Temporarily move the current working directory to the path specified. Parameters @@ -17,29 +17,21 @@ def temporary_cd(path: Optional[str] = None, cleanup: Optional[bool] = None): path The path to CD into. If ``None`` or not specified, a temporary directory will be created. - cleanup - Whether the directory and its contents should be kept after the context - manager exits. If ``True``, the temporary directory and its contents - will be destroyed when the context manager exits; if ``False``, it will - be kept. If ``None`` or not specified, the directory is destroyed if - ``Path`` is not specified and the BEFLOW_KEEP_TMP_FILES setting is - ``False``. Returns ------- """ + # If the target path is "", we just want the current working directory. if path is not None and len(path) == 0: yield return + # If a path is not given, create a temporary directory if path is None: - path = tempfile.mkdtemp() - cleanup = ( - (not current_settings().BEFLOW_KEEP_TMP_FILES) - if cleanup is None - else cleanup - ) + path = tempfile.mkdtemp(dir=".") + print(f"created temporary directory {path}") + cleanup = not current_settings().BEFLOW_KEEP_TMP_FILES else: - cleanup = False if cleanup is None else cleanup + cleanup = False old_directory = os.getcwd() @@ -49,5 +41,7 @@ def temporary_cd(path: Optional[str] = None, cleanup: Optional[bool] = None): finally: os.chdir(old_directory) + # If we created a temporary directory, clean it up if cleanup: + print(f"cleaning up temporary directory {path}") shutil.rmtree(path) From ee627800d8334c3a4da8220f3bd03850f5f8c77d Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 18:07:10 +1100 Subject: [PATCH 03/17] Specify type of BEFLOW_KEEP_TMP_FILES --- openff/bespokefit/executor/services/_settings.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openff/bespokefit/executor/services/_settings.py b/openff/bespokefit/executor/services/_settings.py index 51406920..4c21d89c 100644 --- a/openff/bespokefit/executor/services/_settings.py +++ b/openff/bespokefit/executor/services/_settings.py @@ -63,7 +63,7 @@ class Settings(BaseSettings): BEFLOW_OPTIMIZER_WORKER_MAX_MEM: Union[float, Literal["auto"]] = "auto" BEFLOW_OPTIMIZER_KEEP_FILES: bool = False - BEFLOW_KEEP_TMP_FILES = False + BEFLOW_KEEP_TMP_FILES: bool = False """Keep all temporary files.""" @property From 2506e63cb04a501e301f275dfdf100087d392d38 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 18:07:35 +1100 Subject: [PATCH 04/17] Add tests for temporary_cd --- .../bespokefit/tests/utilities/test_tempcd.py | 132 ++++++++++++++++++ 1 file changed, 132 insertions(+) create mode 100644 openff/bespokefit/tests/utilities/test_tempcd.py diff --git a/openff/bespokefit/tests/utilities/test_tempcd.py b/openff/bespokefit/tests/utilities/test_tempcd.py new file mode 100644 index 00000000..9e5e3237 --- /dev/null +++ b/openff/bespokefit/tests/utilities/test_tempcd.py @@ -0,0 +1,132 @@ +from pathlib import Path + +import pytest + +from openff.bespokefit.utilities.tempcd import temporary_cd + + +@pytest.mark.parametrize( + "path", + [ + "tempcd_test", + Path("tempcd_test"), + None, + ".", + "", + ], +) +class TestTemporaryCD: + @pytest.fixture(autouse=True) + def run_test_in_empty_temporary_directory(self, monkeypatch, tmp_path): + """ + Tests in this class should run in their own empty temporary directories + + This fixture creates a temporary directory for the test, moves into it, + and deletes it when the test is finished. + """ + monkeypatch.chdir(tmp_path) + + def test_tempcd_changes_dir(self, path): + """ + Check temporary_cd changes directory to the target directory + """ + # Arrange + starting_path = Path(".").resolve() + + # Act + with temporary_cd(path): + temp_path = Path(".").resolve() + + # Assert + if path is None: + assert temp_path.parent == starting_path + else: + assert temp_path == (starting_path / path).resolve() + + def test_tempcd_changes_back(self, path): + """ + Check temporary_cd returns to original directory when context manager exits + """ + # Arrange + starting_path = Path(".").resolve() + + # Act + with temporary_cd(path): + pass + + # Assert + assert Path(".").resolve() == starting_path + + def test_tempcd_cleans_up_temporary_directory(self, path, monkeypatch): + """ + Check temporary_cd cleans up temporary directories it creates when + BEFLOW_KEEP_TMP_FILES is not set + + This test is skipped when it is parametrized to operate on the working + directory because the working directory must exist and therefore cannot + be a temporary directory. This check is hard-coded to check for the + path being ``"."`` or ``""``, rather than simply checking if the path + exists, so that conflicts between runs will be detected (though such + conflicts should be prevented by the + ``run_test_in_empty_temporary_directory`` fixture) + """ + if path in [".", ""]: + pytest.skip("'Temporary' directory exists") + + # Arrange + monkeypatch.delenv("BEFLOW_KEEP_TMP_FILES", raising=False) + + # Act + with temporary_cd(path): + Path("touch").write_text("Ensure cleanup of directories with files") + temp_path = Path(".").resolve() + + # Assert + assert not temp_path.exists() + + def test_tempcd_keeps_temporary_directory(self, path, monkeypatch): + """ + Check temporary_cd keeps temporary directories it creates when + BEFLOW_KEEP_TMP_FILES is set + """ + # Arrange + monkeypatch.setenv("BEFLOW_KEEP_TMP_FILES", 1) + + # Act + with temporary_cd(path): + temp_path = Path(".").resolve() + + # Assert + assert temp_path.exists() + + @pytest.mark.parametrize("keep_tmp_files", [True, False]) + def test_tempcd_keeps_existing_directory(self, path, monkeypatch, keep_tmp_files): + """ + Check temporary_cd keeps existing directories + + This test is skipped when the path is ``None`` because it guarantees + that the path does not exist. The target directory will be created in + the Arrange section, unless it is the working directory (in which case + it already exists). This test is hard-coded to check for the path being + ``"."`` or ``""``, rather than simply checking if the path exists, so + that conflicts between runs will be detected (though such conflicts + should be prevented by the ``run_test_in_empty_temporary_directory`` + fixture) + """ + if path is None: + pytest.skip("Temporary directory is guaranteed not to exist") + + # Arrange + if path not in [".", ""]: + Path(path).mkdir() + if keep_tmp_files: + monkeypatch.setenv("BEFLOW_KEEP_TMP_FILES", 1) + else: + monkeypatch.delenv("BEFLOW_KEEP_TMP_FILES", raising=False) + + # Act + with temporary_cd(path): + pass + + # Assert + assert Path(path).is_dir() From 309dc71c862254269196a87c0b1a16535ca3f7c3 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 18:07:56 +1100 Subject: [PATCH 05/17] Update temporary_cd to pass tests --- openff/bespokefit/utilities/tempcd.py | 40 ++++++++++++++++++--------- 1 file changed, 27 insertions(+), 13 deletions(-) diff --git a/openff/bespokefit/utilities/tempcd.py b/openff/bespokefit/utilities/tempcd.py index ff12a77e..3110a1dc 100644 --- a/openff/bespokefit/utilities/tempcd.py +++ b/openff/bespokefit/utilities/tempcd.py @@ -2,34 +2,48 @@ import os import shutil import tempfile +from pathlib import Path from contextlib import contextmanager -from typing import Optional +from typing import Optional, Union from openff.bespokefit.executor.services import current_settings @contextmanager -def temporary_cd(path: Optional[str] = None): - """Temporarily move the current working directory to the path specified. +def temporary_cd(path: Optional[Union[str, Path]] = None, parents=False): + """ + Context manager to move the current working directory to the path specified. + + If no path is given or the path does not exist, a temporary directory will + be created. This temporary directory and its contents will be deleted when + the context manager exits. Parameters ---------- path The path to CD into. If ``None`` or not specified, a temporary directory - will be created. - Returns - ------- + will be created. If specified but the path does not exist, a temporary + directory with that name will be created. + parents + If parents is ``True``, any missing parents of this path are created as + needed; they are created with the default permissions without taking + mode into account. If ``False``, missing parents will result in a + :py:exc:`FileNotFoundError` error. """ - # If the target path is "", we just want the current working directory. - if path is not None and len(path) == 0: - yield - return + path: Optional[Path] = None if path is None else Path(path) + + # Decide whether to clean up based on bespokefit settings + cleanup = not current_settings().BEFLOW_KEEP_TMP_FILES # If a path is not given, create a temporary directory if path is None: - path = tempfile.mkdtemp(dir=".") + path = Path(tempfile.mkdtemp(dir=".")) + print(f"created temporary directory {path}") + # If a path is given but does not already exist, create it + elif not path.exists(): + path.mkdir(parents=parents) print(f"created temporary directory {path}") - cleanup = not current_settings().BEFLOW_KEEP_TMP_FILES + # If we didn't create the path, do NOT clean it up else: cleanup = False @@ -41,7 +55,7 @@ def temporary_cd(path: Optional[str] = None): finally: os.chdir(old_directory) - # If we created a temporary directory, clean it up + # If we created the directory, clean it up if cleanup: print(f"cleaning up temporary directory {path}") shutil.rmtree(path) From 3b719ba1a7f124da3aa39ecef429df43b457191b Mon Sep 17 00:00:00 2001 From: "pre-commit-ci[bot]" <66853113+pre-commit-ci[bot]@users.noreply.github.com> Date: Wed, 22 Mar 2023 07:08:09 +0000 Subject: [PATCH 06/17] [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci --- openff/bespokefit/utilities/tempcd.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/openff/bespokefit/utilities/tempcd.py b/openff/bespokefit/utilities/tempcd.py index 3110a1dc..61c7e371 100644 --- a/openff/bespokefit/utilities/tempcd.py +++ b/openff/bespokefit/utilities/tempcd.py @@ -2,8 +2,8 @@ import os import shutil import tempfile -from pathlib import Path from contextlib import contextmanager +from pathlib import Path from typing import Optional, Union from openff.bespokefit.executor.services import current_settings From 7f31ba3e88644b44daebe8974f9418b70d913a35 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 18:11:56 +1100 Subject: [PATCH 07/17] Fix test warnings --- openff/bespokefit/tests/utilities/test_tempcd.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/openff/bespokefit/tests/utilities/test_tempcd.py b/openff/bespokefit/tests/utilities/test_tempcd.py index 9e5e3237..c68eda83 100644 --- a/openff/bespokefit/tests/utilities/test_tempcd.py +++ b/openff/bespokefit/tests/utilities/test_tempcd.py @@ -90,7 +90,7 @@ def test_tempcd_keeps_temporary_directory(self, path, monkeypatch): BEFLOW_KEEP_TMP_FILES is set """ # Arrange - monkeypatch.setenv("BEFLOW_KEEP_TMP_FILES", 1) + monkeypatch.setenv("BEFLOW_KEEP_TMP_FILES", "1") # Act with temporary_cd(path): @@ -120,7 +120,7 @@ def test_tempcd_keeps_existing_directory(self, path, monkeypatch, keep_tmp_files if path not in [".", ""]: Path(path).mkdir() if keep_tmp_files: - monkeypatch.setenv("BEFLOW_KEEP_TMP_FILES", 1) + monkeypatch.setenv("BEFLOW_KEEP_TMP_FILES", "1") else: monkeypatch.delenv("BEFLOW_KEEP_TMP_FILES", raising=False) From f59cf9be1243a379f1434f4df2931f0ca9c7510c Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 18:15:20 +1100 Subject: [PATCH 08/17] Add comment --- openff/bespokefit/utilities/tempcd.py | 1 + 1 file changed, 1 insertion(+) diff --git a/openff/bespokefit/utilities/tempcd.py b/openff/bespokefit/utilities/tempcd.py index 61c7e371..2470527c 100644 --- a/openff/bespokefit/utilities/tempcd.py +++ b/openff/bespokefit/utilities/tempcd.py @@ -30,6 +30,7 @@ def temporary_cd(path: Optional[Union[str, Path]] = None, parents=False): mode into account. If ``False``, missing parents will result in a :py:exc:`FileNotFoundError` error. """ + # Normalize path to a pathlib Path path: Optional[Path] = None if path is None else Path(path) # Decide whether to clean up based on bespokefit settings From 746d1ec9f53477943fe77a4d6fd01d1e40b3f5d6 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 18:22:39 +1100 Subject: [PATCH 09/17] Support multi-part paths without parents argument --- openff/bespokefit/tests/utilities/test_tempcd.py | 3 ++- openff/bespokefit/utilities/tempcd.py | 9 ++------- 2 files changed, 4 insertions(+), 8 deletions(-) diff --git a/openff/bespokefit/tests/utilities/test_tempcd.py b/openff/bespokefit/tests/utilities/test_tempcd.py index c68eda83..ccad52b0 100644 --- a/openff/bespokefit/tests/utilities/test_tempcd.py +++ b/openff/bespokefit/tests/utilities/test_tempcd.py @@ -9,6 +9,7 @@ "path", [ "tempcd_test", + "tempcd/test", Path("tempcd_test"), None, ".", @@ -118,7 +119,7 @@ def test_tempcd_keeps_existing_directory(self, path, monkeypatch, keep_tmp_files # Arrange if path not in [".", ""]: - Path(path).mkdir() + Path(path).mkdir(parents=True) if keep_tmp_files: monkeypatch.setenv("BEFLOW_KEEP_TMP_FILES", "1") else: diff --git a/openff/bespokefit/utilities/tempcd.py b/openff/bespokefit/utilities/tempcd.py index 2470527c..6993e522 100644 --- a/openff/bespokefit/utilities/tempcd.py +++ b/openff/bespokefit/utilities/tempcd.py @@ -10,7 +10,7 @@ @contextmanager -def temporary_cd(path: Optional[Union[str, Path]] = None, parents=False): +def temporary_cd(path: Optional[Union[str, Path]] = None): """ Context manager to move the current working directory to the path specified. @@ -24,11 +24,6 @@ def temporary_cd(path: Optional[Union[str, Path]] = None, parents=False): The path to CD into. If ``None`` or not specified, a temporary directory will be created. If specified but the path does not exist, a temporary directory with that name will be created. - parents - If parents is ``True``, any missing parents of this path are created as - needed; they are created with the default permissions without taking - mode into account. If ``False``, missing parents will result in a - :py:exc:`FileNotFoundError` error. """ # Normalize path to a pathlib Path path: Optional[Path] = None if path is None else Path(path) @@ -42,7 +37,7 @@ def temporary_cd(path: Optional[Union[str, Path]] = None, parents=False): print(f"created temporary directory {path}") # If a path is given but does not already exist, create it elif not path.exists(): - path.mkdir(parents=parents) + path.mkdir(parents=True) print(f"created temporary directory {path}") # If we didn't create the path, do NOT clean it up else: From 18662d42c859ba4ed2c5f8222a51b9457b2a7322 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 18:36:24 +1100 Subject: [PATCH 10/17] Remove print statements from temporary_cd --- openff/bespokefit/utilities/tempcd.py | 2 -- 1 file changed, 2 deletions(-) diff --git a/openff/bespokefit/utilities/tempcd.py b/openff/bespokefit/utilities/tempcd.py index 6993e522..a2814f4c 100644 --- a/openff/bespokefit/utilities/tempcd.py +++ b/openff/bespokefit/utilities/tempcd.py @@ -34,11 +34,9 @@ def temporary_cd(path: Optional[Union[str, Path]] = None): # If a path is not given, create a temporary directory if path is None: path = Path(tempfile.mkdtemp(dir=".")) - print(f"created temporary directory {path}") # If a path is given but does not already exist, create it elif not path.exists(): path.mkdir(parents=True) - print(f"created temporary directory {path}") # If we didn't create the path, do NOT clean it up else: cleanup = False From 8d8a773025c2b10da01d55e99da9a9c184f49cba Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 18:36:43 +1100 Subject: [PATCH 11/17] Changes to optimizer worker to support KEEP_TMP_FILES --- openff/bespokefit/executor/services/optimizer/worker.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/openff/bespokefit/executor/services/optimizer/worker.py b/openff/bespokefit/executor/services/optimizer/worker.py index d7c2f187..9f0f4be3 100644 --- a/openff/bespokefit/executor/services/optimizer/worker.py +++ b/openff/bespokefit/executor/services/optimizer/worker.py @@ -61,7 +61,8 @@ def optimize(self, optimization_input_json: str) -> str: result = optimizer.optimize( schema=stage, initial_force_field=input_force_field, - keep_files=settings.BEFLOW_OPTIMIZER_KEEP_FILES, + keep_files=settings.BEFLOW_OPTIMIZER_KEEP_FILES + or settings.BEFLOW_KEEP_TMP_FILES, root_directory=f"stage_{i}", ) @@ -85,7 +86,7 @@ def optimize(self, optimization_input_json: str) -> str: if settings.BEFLOW_OPTIMIZER_KEEP_FILES: os.makedirs(os.path.join(home, input_schema.id), exist_ok=True) - shutil.move(os.getcwd(), os.path.join(home, input_schema.id)) + shutil.copytree(os.getcwd(), os.path.join(home, input_schema.id)) result = BespokeOptimizationResults(input_schema=input_schema, stages=stage_results) # cache the final parameters From ff8dc2e62f9d995d1c23911810c4d8c65def3a3d Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 19:26:40 +1100 Subject: [PATCH 12/17] Try to keep QCEngine's scratch files --- .../bespokefit/executor/services/qcgenerator/worker.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/openff/bespokefit/executor/services/qcgenerator/worker.py b/openff/bespokefit/executor/services/qcgenerator/worker.py index 0cb7befe..e42cc0a3 100644 --- a/openff/bespokefit/executor/services/qcgenerator/worker.py +++ b/openff/bespokefit/executor/services/qcgenerator/worker.py @@ -1,4 +1,5 @@ import logging +from pathlib import Path from typing import Any, Dict, List import psutil @@ -48,7 +49,13 @@ def _task_config() -> Dict[str, Any]: ) ) - return dict(ncores=n_cores, nnodes=1, memory=round(max_memory, 3)) + return dict( + ncores=n_cores, + nnodes=1, + memory=round(max_memory, 3), + scratch_directory=str(Path().resolve()), + scratch_messy=True, + ) def _select_atom(atoms: List[Atom]) -> int: From c7018130d9790c2a4d117a5266eaaa21bd5c1090 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 19:27:08 +1100 Subject: [PATCH 13/17] Deprecate BEFLOW_OPTIMIZER_KEEP_FILES --- openff/bespokefit/executor/services/_settings.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/openff/bespokefit/executor/services/_settings.py b/openff/bespokefit/executor/services/_settings.py index 4c21d89c..5c762d2d 100644 --- a/openff/bespokefit/executor/services/_settings.py +++ b/openff/bespokefit/executor/services/_settings.py @@ -62,6 +62,12 @@ class Settings(BaseSettings): BEFLOW_OPTIMIZER_WORKER_N_CORES: Union[int, Literal["auto"]] = "auto" BEFLOW_OPTIMIZER_WORKER_MAX_MEM: Union[float, Literal["auto"]] = "auto" BEFLOW_OPTIMIZER_KEEP_FILES: bool = False + """ + .. deprecated:: 0.2.1 + use BEFLOW_KEEP_TMP_FILES instead + + Keep the optimizer's temporary files. + """ BEFLOW_KEEP_TMP_FILES: bool = False """Keep all temporary files.""" From dcbc58a19d8ef14126dc51db446af6af519ec945 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Wed, 22 Mar 2023 20:40:25 +1100 Subject: [PATCH 14/17] Revert "Try to keep QCEngine's scratch files" This reverts commit ff8dc2e62f9d995d1c23911810c4d8c65def3a3d. --- .../bespokefit/executor/services/qcgenerator/worker.py | 9 +-------- 1 file changed, 1 insertion(+), 8 deletions(-) diff --git a/openff/bespokefit/executor/services/qcgenerator/worker.py b/openff/bespokefit/executor/services/qcgenerator/worker.py index e42cc0a3..0cb7befe 100644 --- a/openff/bespokefit/executor/services/qcgenerator/worker.py +++ b/openff/bespokefit/executor/services/qcgenerator/worker.py @@ -1,5 +1,4 @@ import logging -from pathlib import Path from typing import Any, Dict, List import psutil @@ -49,13 +48,7 @@ def _task_config() -> Dict[str, Any]: ) ) - return dict( - ncores=n_cores, - nnodes=1, - memory=round(max_memory, 3), - scratch_directory=str(Path().resolve()), - scratch_messy=True, - ) + return dict(ncores=n_cores, nnodes=1, memory=round(max_memory, 3)) def _select_atom(atoms: List[Atom]) -> int: From 4c17076cdc9c077f63e38c67b9612f4c6dc10f32 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Thu, 23 Mar 2023 15:41:08 +1100 Subject: [PATCH 15/17] Revert tests to use openff.utilities' temporary_cd --- .../bespokefit/tests/optimizers/forcebalance/test_factories.py | 3 +-- .../tests/optimizers/forcebalance/test_forcebalance.py | 3 +-- openff/bespokefit/tests/workflows/test_bespoke.py | 3 +-- 3 files changed, 3 insertions(+), 6 deletions(-) diff --git a/openff/bespokefit/tests/optimizers/forcebalance/test_factories.py b/openff/bespokefit/tests/optimizers/forcebalance/test_factories.py index b047c8bc..c1299e25 100644 --- a/openff/bespokefit/tests/optimizers/forcebalance/test_factories.py +++ b/openff/bespokefit/tests/optimizers/forcebalance/test_factories.py @@ -7,7 +7,7 @@ import pytest from openff.toolkit.topology import Molecule from openff.toolkit.typing.engines.smirnoff import ForceField -from openff.utilities import skip_if_missing +from openff.utilities import skip_if_missing, temporary_cd from qcelemental.models.procedures import TorsionDriveResult from openff.bespokefit.optimizers.forcebalance.factories import ( @@ -26,7 +26,6 @@ TorsionProfileTargetSchema, VibrationTargetSchema, ) -from openff.bespokefit.utilities.tempcd import temporary_cd def read_qdata(qdata_file: str) -> Tuple[List[np.array], List[float], List[np.array]]: diff --git a/openff/bespokefit/tests/optimizers/forcebalance/test_forcebalance.py b/openff/bespokefit/tests/optimizers/forcebalance/test_forcebalance.py index 9b249860..6a765e5f 100644 --- a/openff/bespokefit/tests/optimizers/forcebalance/test_forcebalance.py +++ b/openff/bespokefit/tests/optimizers/forcebalance/test_forcebalance.py @@ -9,13 +9,12 @@ import numpy as np import pytest from openff.toolkit.typing.engines.smirnoff import ForceField -from openff.utilities import get_data_file_path +from openff.utilities import get_data_file_path, temporary_cd from openff.bespokefit.optimizers import ForceBalanceOptimizer from openff.bespokefit.schema.fitting import BaseOptimizationSchema from openff.bespokefit.schema.optimizers import ForceBalanceSchema from openff.bespokefit.schema.results import OptimizationStageResults -from openff.bespokefit.utilities.tempcd import temporary_cd @pytest.fixture() diff --git a/openff/bespokefit/tests/workflows/test_bespoke.py b/openff/bespokefit/tests/workflows/test_bespoke.py index ba0c9736..73fc344f 100644 --- a/openff/bespokefit/tests/workflows/test_bespoke.py +++ b/openff/bespokefit/tests/workflows/test_bespoke.py @@ -8,7 +8,7 @@ from openff.qcsubmit.common_structures import QCSpec from openff.toolkit.topology import Molecule from openff.toolkit.typing.engines.smirnoff import ForceField -from openff.utilities import get_data_file_path +from openff.utilities import get_data_file_path, temporary_cd from pydantic import ValidationError from openff.bespokefit.exceptions import ( @@ -25,7 +25,6 @@ from openff.bespokefit.schema.targets import AbInitioTargetSchema from openff.bespokefit.schema.tasks import Torsion1DTaskSpec from openff.bespokefit.tests import does_not_raise -from openff.bespokefit.utilities.tempcd import temporary_cd from openff.bespokefit.workflows.bespoke import ( _DEFAULT_ROTATABLE_SMIRKS, BespokeWorkflowFactory, From 5aad257e57803a49676ef2db7da90b2288e86cde Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Thu, 30 Mar 2023 17:19:47 +1100 Subject: [PATCH 16/17] Consolidate various methods of deleting temporary files --- openff/bespokefit/executor/executor.py | 5 ++++- .../executor/services/optimizer/worker.py | 12 +--------- openff/bespokefit/optimizers/model.py | 22 +++++-------------- openff/bespokefit/utilities/tempcd.py | 5 ++++- 4 files changed, 14 insertions(+), 30 deletions(-) diff --git a/openff/bespokefit/executor/executor.py b/openff/bespokefit/executor/executor.py index 1978dabf..103c0a65 100644 --- a/openff/bespokefit/executor/executor.py +++ b/openff/bespokefit/executor/executor.py @@ -214,7 +214,10 @@ def __init__( self._optimizer_worker_config = optimizer_worker_config self._directory = directory - self._remove_directory = directory is None + settings = current_settings() + self._remove_directory = directory is None and not ( + settings.BEFLOW_OPTIMIZER_KEEP_FILES or settings.BEFLOW_KEEP_TMP_FILES + ) self._launch_redis_if_unavailable = launch_redis_if_unavailable diff --git a/openff/bespokefit/executor/services/optimizer/worker.py b/openff/bespokefit/executor/services/optimizer/worker.py index 9f0f4be3..ab52d179 100644 --- a/openff/bespokefit/executor/services/optimizer/worker.py +++ b/openff/bespokefit/executor/services/optimizer/worker.py @@ -1,5 +1,3 @@ -import os -import shutil from typing import Union from pydantic import parse_raw_as @@ -41,9 +39,7 @@ def optimize(self, optimization_input_json: str) -> str: stage_results = [] - home = os.getcwd() - - with temporary_cd(): + with temporary_cd(input_schema.id): for i, stage in enumerate(input_schema.stages): optimizer = get_optimizer(stage.optimizer.type) # If there are no parameters to optimise as they have all been cached mock @@ -61,8 +57,6 @@ def optimize(self, optimization_input_json: str) -> str: result = optimizer.optimize( schema=stage, initial_force_field=input_force_field, - keep_files=settings.BEFLOW_OPTIMIZER_KEEP_FILES - or settings.BEFLOW_KEEP_TMP_FILES, root_directory=f"stage_{i}", ) @@ -84,10 +78,6 @@ def optimize(self, optimization_input_json: str) -> str: ).to_string(discard_cosmetic_attributes=True) ) - if settings.BEFLOW_OPTIMIZER_KEEP_FILES: - os.makedirs(os.path.join(home, input_schema.id), exist_ok=True) - shutil.copytree(os.getcwd(), os.path.join(home, input_schema.id)) - result = BespokeOptimizationResults(input_schema=input_schema, stages=stage_results) # cache the final parameters if ( diff --git a/openff/bespokefit/optimizers/model.py b/openff/bespokefit/optimizers/model.py index 56e7afe2..a4cfd5e4 100644 --- a/openff/bespokefit/optimizers/model.py +++ b/openff/bespokefit/optimizers/model.py @@ -5,7 +5,6 @@ import abc import copy import os -import shutil from collections import defaultdict from typing import Dict, Optional, Type @@ -208,7 +207,6 @@ def optimize( cls, schema: OptimizationStageSchema, initial_force_field: ForceField, - keep_files: bool = False, root_directory: Optional[str] = None, ) -> OptimizationStageResults: """ @@ -218,21 +216,11 @@ def optimize( It should loop over the targets and assert they are registered and then dispatch compute and optimization. """ + if root_directory is not None: + os.makedirs(root_directory, exist_ok=True) - try: - if root_directory is not None: - os.makedirs(root_directory, exist_ok=True) - - with temporary_cd(root_directory): - cls.prepare(schema, initial_force_field, ".") - results = cls._optimize(schema, initial_force_field) - - finally: - if ( - root_directory is not None - and not keep_files - and os.path.isdir(root_directory) - ): - shutil.rmtree(root_directory, ignore_errors=True) + with temporary_cd(root_directory): + cls.prepare(schema, initial_force_field, ".") + results = cls._optimize(schema, initial_force_field) return results diff --git a/openff/bespokefit/utilities/tempcd.py b/openff/bespokefit/utilities/tempcd.py index a2814f4c..e4df13e0 100644 --- a/openff/bespokefit/utilities/tempcd.py +++ b/openff/bespokefit/utilities/tempcd.py @@ -29,7 +29,10 @@ def temporary_cd(path: Optional[Union[str, Path]] = None): path: Optional[Path] = None if path is None else Path(path) # Decide whether to clean up based on bespokefit settings - cleanup = not current_settings().BEFLOW_KEEP_TMP_FILES + settings = current_settings() + cleanup = not ( + settings.BEFLOW_OPTIMIZER_KEEP_FILES or settings.BEFLOW_KEEP_TMP_FILES + ) # If a path is not given, create a temporary directory if path is None: From 9642a26264a70653b2edddebd88c87f3504405b0 Mon Sep 17 00:00:00 2001 From: Josh Mitchell Date: Thu, 30 Mar 2023 17:25:02 +1100 Subject: [PATCH 17/17] Ensure deprecated alias of setting is unset in tests --- openff/bespokefit/tests/utilities/test_tempcd.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/openff/bespokefit/tests/utilities/test_tempcd.py b/openff/bespokefit/tests/utilities/test_tempcd.py index ccad52b0..3ffcd5bb 100644 --- a/openff/bespokefit/tests/utilities/test_tempcd.py +++ b/openff/bespokefit/tests/utilities/test_tempcd.py @@ -76,6 +76,7 @@ def test_tempcd_cleans_up_temporary_directory(self, path, monkeypatch): # Arrange monkeypatch.delenv("BEFLOW_KEEP_TMP_FILES", raising=False) + monkeypatch.delenv("BEFLOW_OPTIMIZER_KEEP_TMP_FILES", raising=False) # Act with temporary_cd(path): @@ -92,6 +93,7 @@ def test_tempcd_keeps_temporary_directory(self, path, monkeypatch): """ # Arrange monkeypatch.setenv("BEFLOW_KEEP_TMP_FILES", "1") + monkeypatch.delenv("BEFLOW_OPTIMIZER_KEEP_TMP_FILES", raising=False) # Act with temporary_cd(path): @@ -124,6 +126,7 @@ def test_tempcd_keeps_existing_directory(self, path, monkeypatch, keep_tmp_files monkeypatch.setenv("BEFLOW_KEEP_TMP_FILES", "1") else: monkeypatch.delenv("BEFLOW_KEEP_TMP_FILES", raising=False) + monkeypatch.delenv("BEFLOW_OPTIMIZER_KEEP_TMP_FILES", raising=False) # Act with temporary_cd(path):