From 5dcbb1e42a5ebfa9cce1af65dc73b747cd16f90d Mon Sep 17 00:00:00 2001 From: Matthew Date: Fri, 3 Nov 2023 15:42:30 +0000 Subject: [PATCH 01/27] Added support for resarting from pre-existing checkpoint files. Adds a restart flag that checks the output_directory for pre-existing sire systems, if they are present they are loaded and used as a starting point for the runner. In this case runtime represents the total time for the system. Also adds a supress_overwrite_warning flag that supresses the need for the user to press enter before files are overwritten in the case of restart=False. --- src/somd2/config/_config.py | 31 +++++++++++++++++ src/somd2/runner/_dynamics.py | 50 +++++++++++++++++++++------ src/somd2/runner/_runner.py | 64 +++++++++++++++++++++++++++-------- 3 files changed, 121 insertions(+), 24 deletions(-) diff --git a/src/somd2/config/_config.py b/src/somd2/config/_config.py index 52063e3..ac00489 100644 --- a/src/somd2/config/_config.py +++ b/src/somd2/config/_config.py @@ -96,9 +96,11 @@ def __init__( max_gpus=None, run_parallel=True, output_directory="output", + restart=True, write_config=True, log_level="info", log_file=None, + supress_overwrite_warning=False, ): """ Constructor. @@ -195,6 +197,9 @@ def __init__( output_directory: str Path to a directory to store output files. + restart: bool + Whether to restart from a previous simulation - files found in {output-directory}. + write_config: bool Whether to write the configuration options to a YAML file in the output directory. @@ -232,10 +237,12 @@ def __init__( self.max_threads = max_threads self.max_gpus = max_gpus self.run_parallel = run_parallel + self.restart = restart self.output_directory = output_directory self.write_config = write_config self.log_level = log_level self.log_file = log_file + self.supress_overwrite_warning = supress_overwrite_warning def __str__(self): """Return a string representation of this object.""" @@ -867,6 +874,16 @@ def run_parallel(self, run_parallel): raise ValueError("'run_parallel' must be of type 'bool'") self._run_parallel = run_parallel + @property + def restart(self): + return self._restart + + @restart.setter + def restart(self, restart): + if not isinstance(restart, bool): + raise ValueError("'restart' must be of type 'bool'") + self._restart = restart + @property def output_directory(self): return self._output_directory @@ -885,6 +902,10 @@ def output_directory(self, output_directory): raise ValueError( f"Output directory {output_directory} does not exist and cannot be created" ) + elif not self.restart: + _logger.warning( + f"Output directory {output_directory} already exists files may be overwritten" + ) self._output_directory = output_directory @property @@ -922,6 +943,16 @@ def log_file(self, log_file): raise TypeError("'log_file' must be of type 'str'") self._log_file = log_file + @property + def supress_overwrite_warning(self): + return self._supress_overwrite_warning + + @supress_overwrite_warning.setter + def supress_overwrite_warning(self, supress_overwrite_warning): + if not isinstance(supress_overwrite_warning, bool): + raise ValueError("'supress_overwrite_warning' must be of type 'bool'") + self._supress_overwrite_warning = supress_overwrite_warning + @classmethod def _create_parser(cls): """ diff --git a/src/somd2/runner/_dynamics.py b/src/somd2/runner/_dynamics.py index e899927..28b80d1 100644 --- a/src/somd2/runner/_dynamics.py +++ b/src/somd2/runner/_dynamics.py @@ -88,11 +88,45 @@ def __init__( raise TypeError("config must be a Config object") self._config = config + # If resarting, subtract the time already run from the total runtime + if self._config.restart: + self._config.runtime = str(self._config.runtime - self._system.time()) self._lambda_val = lambda_val self._lambda_array = lambda_array self._increment = increment self._device = device self._has_space = has_space + self._filenames = self.create_filenames( + self._lambda_array, + self._lambda_val, + self._config.output_directory, + self._config.restart, + ) + + @staticmethod + def create_filenames(lambda_array, lambda_value, output_directory, restart=False): + # Create incremental file - used for writing trajectory files + def increment_filename(base_filename, suffix): + file_number = 0 + file_path = _Path(output_directory) + while True: + filename = f"{base_filename}_{file_number}.{suffix}" + full_path = file_path / filename + if not full_path.exists(): + return filename + file_number += 1 + + if lambda_value not in lambda_array: + raise ValueError("lambda_value not in lambda_array") + filenames = {} + index = lambda_array.index(lambda_value) + filenames["checkpoint"] = f"checkpoint_{index}.s3" + filenames["energy_traj"] = f"energy_traj_{index}.parquet" + if restart: + filenames["trajectory"] = increment_filename(f"traj_{index}", "dcd") + else: + filenames["trajectory"] = f"traj_{index}_0.dcd" + return filenames def _setup_dynamics(self, equilibration=False): """ @@ -206,8 +240,6 @@ def _run(self, lambda_minimisation=None): from sire import u as _u from sire import stream as _stream - _logger.debug("LOG TEST") - def generate_lam_vals(lambda_base, increment): """Generate lambda values for a given lambda_base and increment""" if lambda_base + increment > 1.0 and lambda_base - increment < 0.0: @@ -251,9 +283,8 @@ def generate_lam_vals(lambda_base, increment): energy_per_block = ( self._config.checkpoint_frequency / self._config.energy_frequency ) - sire_checkpoint_name = ( - _Path(self._config.output_directory) - / f"checkpoint_{self._lambda_array.index(self._lambda_val)}.s3" + sire_checkpoint_name = str( + _Path(self._config.output_directory) / self._filenames["checkpoint"] ) # Run num_blocks dynamics and then run a final block if rem > 0 for _ in range(int(num_blocks)): @@ -284,7 +315,7 @@ def generate_lam_vals(lambda_base, increment): "temperature": str(self._config.temperature.value()), }, filepath=self._config.output_directory, - filename=f"energy_traj_{self._lambda_array.index(self._lambda_val)}.parquet", + filename=self._filenames["energy_traj"], ) else: _parquet_append( @@ -322,15 +353,14 @@ def generate_lam_vals(lambda_base, increment): self._system = self._dyn.commit() if self._config.save_trajectories: - traj_filename = ( - self._config.output_directory - / f"traj_{self._lambda_array.index(self._lambda_val)}.dcd" + traj_filename = str( + self._config.output_directory / self._filenames["trajectory"] ) from sire import save as _save _save(self._system.trajectory(), traj_filename, format=["DCD"]) # dump final system to checkpoint file - _stream.save(self._system, str(sire_checkpoint_name)) + _stream.save(self._system, sire_checkpoint_name) df = self._system.energy_trajectory(to_alchemlyb=True) return df diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index 3c91b61..6fc6f27 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -138,6 +138,8 @@ def __init__(self, system, config): enqueue=True, ) + self._check_directory() + def __str__(self): """Return a string representation of the object.""" return f"Runner(system={self._system}, config={self._config})" @@ -179,19 +181,35 @@ def _check_space(self): def _check_directory(self): """ - Check if the output directory has files already present. - Paired with the 'overwrite' option. + Find the name of the file from which simulations will be started. + Paired with the 'restart' option. """ - import os as _os - import glob as _glob + from pathlib import Path as __Path + from ._dynamics import Dynamics - if self._config.overwrite: - return True - else: - if _glob.glob(_os.path.join(self._config.output_directory, "*.parquet")): - return False - else: - return True + fnames = {} + deleted = [] + for lambda_value in self._lambda_values: + files = Dynamics.create_filenames( + self._lambda_values, + lambda_value, + self._config.output_directory, + self._config.restart, + ) + fnames[lambda_value] = files + if not self._config.restart: + for file in files.values(): + fullpath = self._config.output_directory / file + if __Path.exists(fullpath): + deleted.append(fullpath) + if len(deleted) > 0: + _logger.warning( + f"The following files already exist and will be overwritten: {deleted}" + ) + input("Press Enter to erase and continue...") + for file in deleted: + file.unlink() + self._fnames = fnames def get_options(self): """ @@ -502,8 +520,24 @@ def _run(sim): f"raised: {e}. This may be due to a lack of minimisation." ) - system = self._system.clone() - + if self._config.restart: + system = _stream.load( + str( + self._config.output_directory + / self._fnames[lambda_value]["checkpoint"] + ) + ).clone() + else: + system = self._system.clone() + if self._config.restart: + acc_time = system.time() + if acc_time > self._config.runtime - self._config.timestep: + _logger.success(f"Lambda = {lambda_value} already complete. Skipping.") + return True + else: + _logger.debug( + f"Restarting lambda = {lambda_value} at time {acc_time}, time remaining = {self._config.runtime - acc_time}" + ) # GPU platform. if self._is_gpu: if self._config.run_parallel: @@ -543,6 +577,8 @@ def _run(sim): raise self._sim._cleanup() + # Write final dataframe for the system to the energy trajectory file. + # Note that sire s3 checkpoint files contain energy trajectory data, so this works even for restarts. _ = _dataframe_to_parquet( df, metadata={ @@ -554,7 +590,7 @@ def _run(sim): "temperature": str(self._config.temperature.value()), }, filepath=self._config.output_directory, - filename=f"energy_traj_{self._lambda_values.index(lambda_value)}.parquet", + filename=self._fnames[lambda_value]["energy_traj"], ) del system _logger.success("Lambda = {} complete".format(lambda_value)) From 35ccd32fe2232adcd2a01bdf0beae6fe79e6e17e Mon Sep 17 00:00:00 2001 From: Matthew Date: Fri, 3 Nov 2023 16:19:28 +0000 Subject: [PATCH 02/27] Added test for the restart feature. --- tests/runner/test_restart.py | 78 ++++++++++++++++++++++++++++++++++++ 1 file changed, 78 insertions(+) create mode 100644 tests/runner/test_restart.py diff --git a/tests/runner/test_restart.py b/tests/runner/test_restart.py new file mode 100644 index 0000000..7a5af59 --- /dev/null +++ b/tests/runner/test_restart.py @@ -0,0 +1,78 @@ +import tempfile +from somd2.runner import Runner +from somd2.config import Config +from somd2.io import * +from pathlib import Path +import sire as sr + + +def test_restart(): + """Validate that a simulation can be run from a checkpoint file, + with all trajcetories preserved. + """ + with tempfile.TemporaryDirectory() as tmpdir: + # Load the demo stream file. + mols = sr.load(sr.expand(sr.tutorial_url, "merged_molecule.s3")) + + config = { + "runtime": "12fs", + "restart": False, + "output_directory": tmpdir, + "energy_frequency": "4fs", + "checkpoint_frequency": "4fs", + "frame_frequency": "4fs", + "platform": "CPU", + "max_threads": 1, + "num_lambda": 2, + } + + # Instantiate a runner using the config defined above. + runner = Runner(mols, Config(**config)) + + # Run the simulation. + runner.run() + + # Load the energy trajectory. + energy_traj_1, meta_1 = parquet_to_dataframe( + Path(tmpdir) / "energy_traj_0.parquet" + ) + + num_entries = len(energy_traj_1.index) + + del runner + + config_new = { + "runtime": "24fs", + "restart": True, + "output_directory": tmpdir, + "energy_frequency": "4fs", + "checkpoint_frequency": "4fs", + "frame_frequency": "4fs", + "platform": "CPU", + "max_threads": 1, + "num_lambda": 2, + "supress_overwrite_warning": True, + } + + runner2 = Runner(mols, Config(**config_new)) + + # Run the simulation. + runner2.run() + + # Load the energy trajectory. + energy_traj_2, meta_2 = parquet_to_dataframe( + Path(tmpdir) / "energy_traj_0.parquet" + ) + + # Check that first half of energy trajectory is the same + assert energy_traj_1.equals(energy_traj_2.iloc[:num_entries]) + # Check that second energy trajectory is twice as long as the first + assert len(energy_traj_2.index) == 2 * num_entries + # Check that a second trajectory was written and that the first still exists + assert Path.exists(Path(tmpdir) / "traj_0.dcd") + assert Path.exists(Path(tmpdir) / "traj_0_1.dcd") + + +if __name__ == "__main__": + test_restart() + print("test_restart.py: All tests passed.") From 16f7d86220ea4bbe5593f24beccbf2f9f5a5265b Mon Sep 17 00:00:00 2001 From: Matthew Date: Fri, 3 Nov 2023 16:32:14 +0000 Subject: [PATCH 03/27] Added config filename to filename generator --- src/somd2/runner/_dynamics.py | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/src/somd2/runner/_dynamics.py b/src/somd2/runner/_dynamics.py index 28b80d1..e886d90 100644 --- a/src/somd2/runner/_dynamics.py +++ b/src/somd2/runner/_dynamics.py @@ -110,7 +110,11 @@ def increment_filename(base_filename, suffix): file_number = 0 file_path = _Path(output_directory) while True: - filename = f"{base_filename}_{file_number}.{suffix}" + filename = ( + f"{base_filename}_{file_number}.{suffix}" + if file_number > 0 + else f"{base_filename}.{suffix}" + ) full_path = file_path / filename if not full_path.exists(): return filename @@ -124,8 +128,10 @@ def increment_filename(base_filename, suffix): filenames["energy_traj"] = f"energy_traj_{index}.parquet" if restart: filenames["trajectory"] = increment_filename(f"traj_{index}", "dcd") + filenames["config"] = increment_filename("config", "yaml") else: - filenames["trajectory"] = f"traj_{index}_0.dcd" + filenames["trajectory"] = f"traj_{index}.dcd" + filenames["config"] = "config.yaml" return filenames def _setup_dynamics(self, equilibration=False): From c8e9f5206c9392ea477b4350e21f3df42f4d00b3 Mon Sep 17 00:00:00 2001 From: Matthew Date: Fri, 3 Nov 2023 16:42:32 +0000 Subject: [PATCH 04/27] Fixed config writing --- src/somd2/runner/_runner.py | 38 ++++++++++++++++++++++++++++++------- 1 file changed, 31 insertions(+), 7 deletions(-) diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index 6fc6f27..927e239 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -119,9 +119,16 @@ def __init__(self, system, config): if self._config.h_mass_factor > 1: self._repartition_h_mass() + # Check the output directories and create names of output files. + self._check_directory() + # Save config whenever 'configure' is called to keep it up to date if self._config.write_config: - _dict_to_yaml(self._config.as_dict(), self._config.output_directory) + _dict_to_yaml( + self._config.as_dict(), + self._config.output_directory, + self._fnames[self._lambda_values[0]]["config"], + ) # Flag whether this is a GPU simulation. self._is_gpu = self._config.platform in ["cuda", "opencl", "hip"] @@ -138,8 +145,6 @@ def __init__(self, system, config): enqueue=True, ) - self._check_directory() - def __str__(self): """Return a string representation of the object.""" return f"Runner(system={self._system}, config={self._config})" @@ -203,14 +208,33 @@ def _check_directory(self): if __Path.exists(fullpath): deleted.append(fullpath) if len(deleted) > 0: - _logger.warning( - f"The following files already exist and will be overwritten: {deleted}" - ) - input("Press Enter to erase and continue...") + if not self._config.supress_overwrite_warning: + _logger.warning( + f"The following files already exist and will be overwritten: {deleted}" + ) + input("Press Enter to erase and continue...") for file in deleted: file.unlink() self._fnames = fnames + def _verify_restart_config(self): + """ + Verify that the config file matches the config file used to create the + checkpoint file. + """ + import yaml as _yaml + + with open( + self._config.output_directory + / self._fnames[self._lambda_values[0]]["config"] + ) as file: + config = _yaml.safe_load(file) + if config != self._config.as_dict(): + raise ValueError( + "The configuration file does not match the configuration used to create the " + "checkpoint file." + ) + def get_options(self): """ Returns a dictionary of simulation options. From 9e66360f8073389486c17e1bf79556b2a468147b Mon Sep 17 00:00:00 2001 From: Matthew Date: Mon, 6 Nov 2023 10:24:19 +0000 Subject: [PATCH 05/27] Remove duplicates from list of files to be deleted when --no-restart is used. [ci skip] --- src/somd2/runner/_runner.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index 16173b6..b45758b 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -216,10 +216,11 @@ def _check_directory(self): if len(deleted) > 0: if not self._config.supress_overwrite_warning: _logger.warning( - f"The following files already exist and will be overwritten: {deleted}" + f"The following files already exist and will be overwritten: {list(set((deleted)))}" ) input("Press Enter to erase and continue...") - for file in deleted: + # Loop over files to be deleted, ignoring duplicates + for file in list(set(deleted)): file.unlink() self._fnames = fnames From 7965730a91695d2fcd144a8a678a7c8d77ec3f6b Mon Sep 17 00:00:00 2001 From: Matthew Date: Mon, 6 Nov 2023 11:30:56 +0000 Subject: [PATCH 06/27] Simulations will now start from scratch if restart==True but no checkpoint file is found --- src/somd2/runner/_runner.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index b45758b..2b65b43 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -555,12 +555,17 @@ def _run(sim): ) if self._config.restart: - system = _stream.load( - str( - self._config.output_directory - / self._fnames[lambda_value]["checkpoint"] + try: + system = _stream.load( + str( + self._config.output_directory + / self._fnames[lambda_value]["checkpoint"] + ) + ).clone() + except: + _logger.warning( + f"Unable to load checkpoint file for λ={lambda_value}, starting from scratch." ) - ).clone() else: system = self._system.clone() if self._config.restart: From b85be1b94433c74e3b121d5e67b3db5eef49348b Mon Sep 17 00:00:00 2001 From: Matthew Date: Mon, 6 Nov 2023 15:11:51 +0000 Subject: [PATCH 07/27] Slight change to setting of charge scaled morph Fixes a bug in which the charge scale factor is referenced before being set during the setup of a charge scaled morph, this simply sets it to a temporary value which is then overwritten when charge scale factor is set --- src/somd2/config/_config.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/src/somd2/config/_config.py b/src/somd2/config/_config.py index ac00489..91505c5 100644 --- a/src/somd2/config/_config.py +++ b/src/somd2/config/_config.py @@ -492,9 +492,7 @@ def lambda_schedule(self, lambda_schedule): if lambda_schedule == "standard_morph": self._lambda_schedule = _LambdaSchedule.standard_morph() elif lambda_schedule == "charge_scaled_morph": - self._lambda_schedule = _LambdaSchedule.charge_scaled_morph( - self._charge_scale_factor - ) + self._lambda_schedule = _LambdaSchedule.charge_scaled_morph(0.2) else: self._lambda_schedule = lambda_schedule else: From 485ae5a15686cab1bac01b534a3be71bb757d2e8 Mon Sep 17 00:00:00 2001 From: Matthew Date: Mon, 6 Nov 2023 15:30:38 +0000 Subject: [PATCH 08/27] Added thorough testing for restarting, verifying that all options that cannot be changed between runs throw the relevant error. Options that cannot be changed are the following: 'runtime', 'restart', 'temperature', 'minimise', 'max_threads', 'equilibration_time', 'equilibration_timestep', 'energy_frequency', 'save_trajectory', 'frame_frequency', 'save_velocities', 'checkpoint_frequency', 'platform', 'max_threads', 'max_gpus', 'run_parallel', 'restart', 'save_trajectories', 'write_config', 'log_level', 'log_file', 'supress_overwrite_warning' --- src/somd2/runner/_runner.py | 93 ++++++++++++++++++++++++------- tests/runner/test_restart.py | 103 +++++++++++++++++++++++++++++++++++ 2 files changed, 176 insertions(+), 20 deletions(-) diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index 2b65b43..58e4f23 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -119,20 +119,13 @@ def __init__(self, system, config): if self._config.h_mass_factor > 1: self._repartition_h_mass() - # Check the output directories and create names of output files. - self._check_directory() - - # Save config whenever 'configure' is called to keep it up to date - if self._config.write_config: - _dict_to_yaml( - self._config.as_dict(), - self._config.output_directory, - self._fnames[self._lambda_values[0]]["config"], - ) - # Flag whether this is a GPU simulation. self._is_gpu = self._config.platform in ["cuda", "opencl", "hip"] + # Need to verify before doing any directory checks + if self._config.restart: + self._verify_restart_config() + # Setup proper logging level import sys @@ -145,6 +138,17 @@ def __init__(self, system, config): enqueue=True, ) + # Check the output directories and create names of output files. + self._check_directory() + + # Save config whenever 'configure' is called to keep it up to date + if self._config.write_config: + _dict_to_yaml( + self._config.as_dict(), + self._config.output_directory, + self._fnames[self._lambda_values[0]]["config"], + ) + def __str__(self): """Return a string representation of the object.""" return f"Runner(system={self._system}, config={self._config})" @@ -231,16 +235,65 @@ def _verify_restart_config(self): """ import yaml as _yaml - with open( - self._config.output_directory - / self._fnames[self._lambda_values[0]]["config"] - ) as file: - config = _yaml.safe_load(file) - if config != self._config.as_dict(): - raise ValueError( - "The configuration file does not match the configuration used to create the " - "checkpoint file." + def get_last_config(output_directory): + """ + Returns the last config file in the output directory. + """ + import os as _os + + config_files = [ + file + for file in _os.listdir(output_directory) + if file.endswith(".yaml") and file.startswith("config") + ] + config_files.sort() + return config_files[-1] + + try: + last_config = get_last_config(self._config.output_directory) + except IndexError: + raise IndexError( + f"No config files found in {self._config.output_directory}" ) + with open(self._config.output_directory / last_config) as file: + _logger.debug(f"Opening config file {last_config}") + config = _yaml.safe_load(file) + # Define the subset of settings that are allowed to change after restart + allowed_diffs = [ + "runtime", + "restart", + "temperature", + "minimise", + "max_threads", + "equilibration_time", + "equilibration_timestep", + "energy_frequency", + "save_trajectory", + "frame_frequency", + "save_velocities", + "checkpoint_frequency", + "platform", + "max_threads", + "max_gpus", + "run_parallel", + "restart", + "save_trajectories", + "write_config", + "log_level", + "log_file", + "supress_overwrite_warning", + ] + for key in config.keys(): + if key not in allowed_diffs: + _logger.debug(f"Checking {key}") + _logger.debug( + f"""old value: {config[key]} + new value: {self._config.as_dict()[key]}""" + ) + if config[key] != self._config.as_dict()[key]: + raise ValueError( + f"{key} has changed since the last run. This is not allowed when using the restart option." + ) def get_options(self): """ diff --git a/tests/runner/test_restart.py b/tests/runner/test_restart.py index 7a5af59..0065246 100644 --- a/tests/runner/test_restart.py +++ b/tests/runner/test_restart.py @@ -4,6 +4,7 @@ from somd2.io import * from pathlib import Path import sire as sr +import pytest def test_restart(): @@ -52,6 +53,7 @@ def test_restart(): "max_threads": 1, "num_lambda": 2, "supress_overwrite_warning": True, + "log_level": "DEBUG", } runner2 = Runner(mols, Config(**config_new)) @@ -72,6 +74,107 @@ def test_restart(): assert Path.exists(Path(tmpdir) / "traj_0.dcd") assert Path.exists(Path(tmpdir) / "traj_0_1.dcd") + config_difftimestep = config_new.copy() + config_difftimestep["runtime"] = "36fs" + config_difftimestep["timestep"] = "2fs" + + with pytest.raises(ValueError): + runner_timestep = Runner(mols, Config(**config_difftimestep)) + + config_diffscalefactor = config_new.copy() + config_diffscalefactor["runtime"] = "36fs" + config_diffscalefactor["charge_scale_factor"] = 0.5 + + with pytest.raises(ValueError): + runner_scalefactor = Runner(mols, Config(**config_diffscalefactor)) + + config_diffconstraint = config_new.copy() + config_diffconstraint["runtime"] = "36fs" + config_diffconstraint["constraint"] = "bonds" + + with pytest.raises(ValueError): + runner_constraints = Runner(mols, Config(**config_diffconstraint)) + + config_diffcoulombpower = config_new.copy() + config_diffcoulombpower["runtime"] = "36fs" + config_diffcoulombpower["coulomb_power"] = 0.5 + + with pytest.raises(ValueError): + runner_coulombpower = Runner(mols, Config(**config_diffcoulombpower)) + + config_diffcutofftype = config_new.copy() + config_diffcutofftype["runtime"] = "36fs" + config_diffcutofftype["cutoff_type"] = "rf" + + with pytest.raises(ValueError): + runner_cutofftype = Runner(mols, Config(**config_diffcutofftype)) + + config_diffhmassfactor = config_new.copy() + config_diffhmassfactor["runtime"] = "36fs" + config_diffhmassfactor["h_mass_factor"] = 2.0 + + with pytest.raises(ValueError): + runner_hmassfactor = Runner(mols, Config(**config_diffhmassfactor)) + + config_diffintegrator = config_new.copy() + config_diffintegrator["runtime"] = "36fs" + config_diffintegrator["integrator"] = "verlet" + + with pytest.raises(ValueError): + runner_integrator = Runner(mols, Config(**config_diffintegrator)) + + config_difflambdaschedule = config_new.copy() + config_difflambdaschedule["runtime"] = "36fs" + config_difflambdaschedule["charge_scale_factor"] = 0.5 + config_difflambdaschedule["lambda_schedule"] = "charge_scaled_morph" + + with pytest.raises(ValueError): + runner_lambdaschedule = Runner(mols, Config(**config_difflambdaschedule)) + + config_diffnumlambda = config_new.copy() + config_diffnumlambda["runtime"] = "36fs" + config_diffnumlambda["num_lambda"] = 3 + + with pytest.raises(ValueError): + runner_numlambda = Runner(mols, Config(**config_diffnumlambda)) + + config_diffoutputdirectory = config_new.copy() + config_diffoutputdirectory["runtime"] = "36fs" + config_diffoutputdirectory["output_directory"] = "test" + + with pytest.raises(IndexError): + runner_outputdirectory = Runner(mols, Config(**config_diffoutputdirectory)) + + config_diffperturbableconstraint = config_new.copy() + config_diffperturbableconstraint["runtime"] = "36fs" + config_diffperturbableconstraint["perturbable_constraint"] = "bonds" + + with pytest.raises(ValueError): + runner_perturbableconstraint = Runner( + mols, Config(**config_diffperturbableconstraint) + ) + + config_diffpressure = config_new.copy() + config_diffpressure["runtime"] = "36fs" + config_diffpressure["pressure"] = "1.5 atm" + + with pytest.raises(ValueError): + runner_pressure = Runner(mols, Config(**config_diffpressure)) + + config_diffshiftdelta = config_new.copy() + config_diffshiftdelta["runtime"] = "36fs" + config_diffshiftdelta["shift_delta"] = "3 Angstrom" + + with pytest.raises(ValueError): + runner_shiftdelta = Runner(mols, Config(**config_diffshiftdelta)) + + config_diffswapendstates = config_new.copy() + config_diffswapendstates["runtime"] = "36fs" + config_diffswapendstates["swap_end_states"] = True + + with pytest.raises(ValueError): + runner_swapendstates = Runner(mols, Config(**config_diffswapendstates)) + if __name__ == "__main__": test_restart() From d4faed669f8c0cc7acb40d1423e30fb701a9467a Mon Sep 17 00:00:00 2001 From: Matthew Date: Mon, 6 Nov 2023 16:16:58 +0000 Subject: [PATCH 09/27] Changed default behaviour to restart=False, warnings regarding overwriting still exist. --- src/somd2/config/_config.py | 2 +- tests/runner/test_config.py | 2 +- tests/runner/test_restart.py | 14 ++++++-------- 3 files changed, 8 insertions(+), 10 deletions(-) diff --git a/src/somd2/config/_config.py b/src/somd2/config/_config.py index 91505c5..6223b00 100644 --- a/src/somd2/config/_config.py +++ b/src/somd2/config/_config.py @@ -96,7 +96,7 @@ def __init__( max_gpus=None, run_parallel=True, output_directory="output", - restart=True, + restart=False, write_config=True, log_level="info", log_file=None, diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index 6d594eb..a0af022 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -13,7 +13,7 @@ def test_dynamics_options(): # Instantiate a runner using the default config. # (All default options, other than platform="cpu".) - runner = Runner(mols, Config(platform="cpu")) + runner = Runner(mols, Config(platform="cpu", output_directory=tmpdir)) # Initalise a fake simulation. runner._initialise_simulation(runner._system.clone(), 0.0) diff --git a/tests/runner/test_restart.py b/tests/runner/test_restart.py index 0065246..e53829c 100644 --- a/tests/runner/test_restart.py +++ b/tests/runner/test_restart.py @@ -140,10 +140,13 @@ def test_restart(): config_diffoutputdirectory = config_new.copy() config_diffoutputdirectory["runtime"] = "36fs" - config_diffoutputdirectory["output_directory"] = "test" + with tempfile.TemporaryDirectory() as tmpdir2: + config_diffoutputdirectory["output_directory"] = tmpdir2 - with pytest.raises(IndexError): - runner_outputdirectory = Runner(mols, Config(**config_diffoutputdirectory)) + with pytest.raises(IndexError): + runner_outputdirectory = Runner( + mols, Config(**config_diffoutputdirectory) + ) config_diffperturbableconstraint = config_new.copy() config_diffperturbableconstraint["runtime"] = "36fs" @@ -174,8 +177,3 @@ def test_restart(): with pytest.raises(ValueError): runner_swapendstates = Runner(mols, Config(**config_diffswapendstates)) - - -if __name__ == "__main__": - test_restart() - print("test_restart.py: All tests passed.") From 257a99c1858b8250f7cc93dc2336c97fc9916ba6 Mon Sep 17 00:00:00 2001 From: Matthew Date: Tue, 7 Nov 2023 12:24:22 +0000 Subject: [PATCH 10/27] Re-factor of config to allow for the logger to be used within config itself at the correct level. Also adds a test for the writing of the logfile --- src/somd2/config/_config.py | 38 ++++++++++++++++++++++++++++--------- tests/runner/test_config.py | 21 ++++++++++++++++++++ 2 files changed, 50 insertions(+), 9 deletions(-) diff --git a/src/somd2/config/_config.py b/src/somd2/config/_config.py index 6223b00..6569cf5 100644 --- a/src/somd2/config/_config.py +++ b/src/somd2/config/_config.py @@ -68,6 +68,8 @@ class Config: def __init__( self, + log_level="info", + log_file=None, runtime="1ns", timestep="4fs", temperature="300K", @@ -98,8 +100,6 @@ def __init__( output_directory="output", restart=False, write_config=True, - log_level="info", - log_file=None, supress_overwrite_warning=False, ): """ @@ -208,8 +208,16 @@ def __init__( log_file: str Name of log file, will be saved in output directory. + + supress_overwrite_warning: bool + Whether to supress the warning when overwriting files in the output directory. """ + # Setup logger before doing anything else + self.log_level = log_level + self.log_file = log_file + self.output_directory = output_directory + self.runtime = runtime self.temperature = temperature self.pressure = pressure @@ -238,10 +246,9 @@ def __init__( self.max_gpus = max_gpus self.run_parallel = run_parallel self.restart = restart - self.output_directory = output_directory + self.write_config = write_config - self.log_level = log_level - self.log_file = log_file + self.supress_overwrite_warning = supress_overwrite_warning def __str__(self): @@ -880,6 +887,10 @@ def restart(self): def restart(self, restart): if not isinstance(restart, bool): raise ValueError("'restart' must be of type 'bool'") + if not restart and self.directory_existed: + _logger.warning( + f"Output directory {self.output_directory} already exists files may be overwritten" + ) self._restart = restart @property @@ -888,6 +899,7 @@ def output_directory(self): @output_directory.setter def output_directory(self, output_directory): + self.cirectory_existed = False if not isinstance(output_directory, _Path): try: output_directory = _Path(output_directory) @@ -900,10 +912,12 @@ def output_directory(self, output_directory): raise ValueError( f"Output directory {output_directory} does not exist and cannot be created" ) - elif not self.restart: - _logger.warning( - f"Output directory {output_directory} already exists files may be overwritten" - ) + else: + self.directory_existed = True + if self.log_file is not None: + # Can now add the log file + _logger.add(output_directory / self.log_file, level=self.log_level.upper()) + _logger.debug(f"Logging to {output_directory / self.log_file}") self._output_directory = output_directory @property @@ -929,6 +943,11 @@ def log_level(self, log_level): raise ValueError( f"Log level not recognised. Valid log levels are: {', '.join(self._choices['log_level'])}" ) + # Do logging setup here for use in the rest of the ocnfig and all other modules. + import sys + + _logger.remove() + _logger.add(sys.stderr, level=log_level.upper(), enqueue=True) self._log_level = log_level @property @@ -939,6 +958,7 @@ def log_file(self): def log_file(self, log_file): if log_file is not None and not isinstance(log_file, str): raise TypeError("'log_file' must be of type 'str'") + # Can't add the logfile to the logger here as we don't know the output directory yet. self._log_file = log_file @property diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index a0af022..8365992 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -68,3 +68,24 @@ def test_dynamics_options(): assert config_inp.integrator.lower().replace( "_", "" ) == d.integrator().__class__.__name__.lower().replace("integrator", "") + + +def test_logfile_creation(): + # Test that the logfile is created by either the initialisation of the runner or of a config + with tempfile.TemporaryDirectory() as tmpdir: + # Load the demo stream file. + mols = sr.load(sr.expand(sr.tutorial_url, "merged_molecule.s3")) + from pathlib import Path + + # Instantiate a runner using the default config. + # (All default options, other than platform="cpu".) + config = Config(output_directory=tmpdir, log_file="test.log") + assert config.log_file is not None + assert Path.exists(config.output_directory / config.log_file) + Path.unlink(config.output_directory / config.log_file) + + # Instantiate a runner using the default config. + # (All default options, other than platform="cpu".) + runner = Runner(mols, Config(output_directory=tmpdir, log_file="test.log")) + assert runner._config.log_file is not None + assert Path.exists(runner._config.output_directory / runner._config.log_file) From 8b4094219e460a10ed9907c9550c88cfdad699f3 Mon Sep 17 00:00:00 2001 From: Matthew Date: Tue, 7 Nov 2023 15:08:16 +0000 Subject: [PATCH 11/27] Config now encoded in sire checkpoint files Added extra parameter to config.as_dict to make it equivalent to sire outputs. Restarts are checked against previous config, if this config doesn't exist the runner checks the checkpoint file for lambda=0 for the previous config. yaml safe load throws a different exception on windows - this was causing the test to fail. Should now be fixed. --- src/somd2/config/_config.py | 14 ++++++++-- src/somd2/runner/_dynamics.py | 5 ++++ src/somd2/runner/_runner.py | 50 ++++++++++++++++++----------------- tests/runner/test_restart.py | 2 +- 4 files changed, 44 insertions(+), 27 deletions(-) diff --git a/src/somd2/config/_config.py b/src/somd2/config/_config.py index 6569cf5..53204e0 100644 --- a/src/somd2/config/_config.py +++ b/src/somd2/config/_config.py @@ -300,8 +300,16 @@ def from_yaml(path): return Config(**d) - def as_dict(self): - """Convert config object to dictionary""" + def as_dict(self, sire_compatible=False): + """Convert config object to dictionary + + Parameters + ---------- + sire_compatible: bool + Whether to convert to a dictionary compatible with Sire, + this simply converts any options with a value of None to a + boolean with the value False. + """ from pathlib import PosixPath as _PosixPath from sire.cas import LambdaSchedule as _LambdaSchedule @@ -315,6 +323,8 @@ def as_dict(self): d[attr_l] = value.to_string() except AttributeError: d[attr_l] = value + if value is None and sire_compatible: + d[attr_l] = False # Handle the lambda schedule separately so that we can use simplified # keyword options. diff --git a/src/somd2/runner/_dynamics.py b/src/somd2/runner/_dynamics.py index 1b09113..a29cb75 100644 --- a/src/somd2/runner/_dynamics.py +++ b/src/somd2/runner/_dynamics.py @@ -332,6 +332,11 @@ def generate_lam_vals(lambda_base, increment): filepath=self._config.output_directory, filename=self._filenames["energy_traj"], ) + # Also want to add the simulation config to the + # system properties once a block has been succsessfully run. + self._system.set_property( + "config", self._config.as_dict(sire_compatible=True) + ) else: _parquet_append( f, diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index 58e4f23..f3dcb54 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -126,18 +126,6 @@ def __init__(self, system, config): if self._config.restart: self._verify_restart_config() - # Setup proper logging level - import sys - - _logger.remove() - _logger.add(sys.stderr, level=self._config.log_level.upper(), enqueue=True) - if self._config.log_file is not None: - _logger.add( - self._config.output_directory / self._config.log_file, - level=self._config.log_level.upper(), - enqueue=True, - ) - # Check the output directories and create names of output files. self._check_directory() @@ -250,14 +238,28 @@ def get_last_config(output_directory): return config_files[-1] try: - last_config = get_last_config(self._config.output_directory) - except IndexError: - raise IndexError( - f"No config files found in {self._config.output_directory}" + last_config_file = get_last_config(self._config.output_directory) + with open(self._config.output_directory / last_config_file) as file: + _logger.debug(f"Opening config file {last_config_file}") + last_config = _yaml.safe_load(file) + except: + _logger.info( + f"""No config files found in {self._config.output_directory}, + attempting to retrieve config from lambda = 0 checkpoint file.""" ) - with open(self._config.output_directory / last_config) as file: - _logger.debug(f"Opening config file {last_config}") - config = _yaml.safe_load(file) + try: + system_temp = _stream.load( + str(self._config.output_directory / "checkpoint_0.s3") + ) + except: + _logger.error( + f"Unable to load checkpoint file from {self._config.output_directory}." + ) + raise + else: + last_config = dict(system_temp.property("config")) + del system_temp + # Define the subset of settings that are allowed to change after restart allowed_diffs = [ "runtime", @@ -283,14 +285,14 @@ def get_last_config(output_directory): "log_file", "supress_overwrite_warning", ] - for key in config.keys(): + for key in last_config.keys(): if key not in allowed_diffs: _logger.debug(f"Checking {key}") _logger.debug( - f"""old value: {config[key]} + f"""old value: {last_config[key]} new value: {self._config.as_dict()[key]}""" ) - if config[key] != self._config.as_dict()[key]: + if last_config[key] != self._config.as_dict()[key]: raise ValueError( f"{key} has changed since the last run. This is not allowed when using the restart option." ) @@ -624,11 +626,11 @@ def _run(sim): if self._config.restart: acc_time = system.time() if acc_time > self._config.runtime - self._config.timestep: - _logger.success(f"Lambda = {lambda_value} already complete. Skipping.") + _logger.success(f"λ = {lambda_value} already complete. Skipping.") return True else: _logger.debug( - f"Restarting lambda = {lambda_value} at time {acc_time}, time remaining = {self._config.runtime - acc_time}" + f"Restarting λ = {lambda_value} at time {acc_time}, time remaining = {self._config.runtime - acc_time}" ) # GPU platform. if self._is_gpu: diff --git a/tests/runner/test_restart.py b/tests/runner/test_restart.py index e53829c..528b2c7 100644 --- a/tests/runner/test_restart.py +++ b/tests/runner/test_restart.py @@ -143,7 +143,7 @@ def test_restart(): with tempfile.TemporaryDirectory() as tmpdir2: config_diffoutputdirectory["output_directory"] = tmpdir2 - with pytest.raises(IndexError): + with pytest.raises(OSError): runner_outputdirectory = Runner( mols, Config(**config_diffoutputdirectory) ) From 595d86bb517e0b3ea277e1faf7d1b349410655c6 Mon Sep 17 00:00:00 2001 From: Matthew Date: Tue, 7 Nov 2023 16:56:05 +0000 Subject: [PATCH 12/27] Removing changed the writing of the output directory from PosixPath to Path in as_dict, removing lambda symbols on windows, changed filenames in test_logfile_creation --- src/somd2/config/_config.py | 4 ++-- src/somd2/runner/_dynamics.py | 17 +++++++++------ src/somd2/runner/_runner.py | 40 ++++++++++++++++++++++------------- tests/runner/test_config.py | 3 +-- 4 files changed, 39 insertions(+), 25 deletions(-) diff --git a/src/somd2/config/_config.py b/src/somd2/config/_config.py index 53204e0..a597263 100644 --- a/src/somd2/config/_config.py +++ b/src/somd2/config/_config.py @@ -310,13 +310,13 @@ def as_dict(self, sire_compatible=False): this simply converts any options with a value of None to a boolean with the value False. """ - from pathlib import PosixPath as _PosixPath + from pathlib import Path as _Path from sire.cas import LambdaSchedule as _LambdaSchedule d = {} for attr, value in self.__dict__.items(): attr_l = attr[1:] - if isinstance(value, _PosixPath): + if isinstance(value, _Path): d[attr_l] = str(value) else: try: diff --git a/src/somd2/runner/_dynamics.py b/src/somd2/runner/_dynamics.py index a29cb75..213d43e 100644 --- a/src/somd2/runner/_dynamics.py +++ b/src/somd2/runner/_dynamics.py @@ -20,7 +20,7 @@ ##################################################################### __all__ = ["Dynamics"] - +import os as _os from pathlib import Path as _Path from ..config import Config as _Config @@ -29,6 +29,11 @@ from somd2 import _logger +if _os.platform == "win32": + lam_sym = "lambda" +else: + lam_sym = "λ" + class Dynamics: """ @@ -191,7 +196,7 @@ def _minimisation(self, lambda_min=None): lambda_val. """ if lambda_min is None: - _logger.info(f"Minimising at λ = {self._lambda_val}") + _logger.info(f"Minimising at {lam_sym} = {self._lambda_val}") try: m = self._system.minimisation( cutoff_type=self._config.cutoff_type, @@ -206,7 +211,7 @@ def _minimisation(self, lambda_min=None): except: raise else: - _logger.info(f"Minimising at λ = {lambda_min}") + _logger.info(f"Minimising at {lam_sym} = {lambda_min}") try: m = self._system.minimisation( cutoff_type=self._config.cutoff_type, @@ -229,7 +234,7 @@ def _equilibration(self): Currently just runs dynamics without any saving """ - _logger.info(f"Equilibrating at λ = {self._lambda_val}") + _logger.info(f"Equilibrating at {lam_sym} = {self._lambda_val}") self._setup_dynamics(equilibration=True) self._dyn.run( self._config.equilibration_time, @@ -282,7 +287,7 @@ def generate_lam_vals(lambda_base, increment): else: lam_arr = self._lambda_array + self._lambda_grad - _logger.info(f"Running dynamics at λ = {self._lambda_val}") + _logger.info(f"Running dynamics at {lam_sym} = {self._lambda_val}") if self._config.checkpoint_frequency.value() > 0.0: ### Calc number of blocks and remainder (surely there's a better way?)### @@ -343,7 +348,7 @@ def generate_lam_vals(lambda_base, increment): df.iloc[-int(energy_per_block) :], ) _logger.info( - f"Finished block {x+1} of {num_blocks} for λ = {self._lambda_val}" + f"Finished block {x+1} of {num_blocks} for {lam_sym} = {self._lambda_val}" ) except: raise diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index f3dcb54..216be04 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -21,7 +21,7 @@ __all__ = ["Runner"] - +import os as _os from sire import stream as _stream from sire.system import System as _System @@ -31,6 +31,11 @@ from somd2 import _logger +if _os.platform == "win32": + lam_sym = "lambda" +else: + lam_sym = "λ" + class Runner: """ @@ -456,7 +461,7 @@ def _initialise_simulation(self, system, lambda_value, device=None): has_space=self._has_space, ) except: - _logger.warning(f"System creation at λ = {lambda_value} failed") + _logger.warning(f"System creation at {lam_sym} = {lambda_value} failed") raise def _cleanup_simulation(self): @@ -521,8 +526,9 @@ def run(self): result = job.result() except Exception as e: result = False + _logger.error( - f"Exception raised for λ = {lambda_value}: {e}" + f"Exception raised for {lam_sym} = {lambda_value}: {e}" ) with self._lock: results.append(result) @@ -583,8 +589,8 @@ def _run(sim): return df, lambda_grad, speed except Exception as e: _logger.warning( - f"Minimisation/dynamics at λ = {lambda_value} failed with the " - f"following exception {e}, trying again with minimsation at λ = 0." + f"Minimisation/dynamics at {lam_sym} = {lambda_value} failed with the " + f"following exception {e}, trying again with minimsation at {lam_sym} = 0." ) try: df = sim._run(lambda_minimisation=0.0) @@ -593,8 +599,8 @@ def _run(sim): return df, lambda_grad, speed except Exception as e: _logger.error( - f"Minimisation/dynamics at λ = {lambda_value} failed, even after " - f"minimisation at λ = 0. The following warning was raised: {e}." + f"Minimisation/dynamics at {lam_sym} = {lambda_value} failed, even after " + f"minimisation at {lam_sym} = 0. The following warning was raised: {e}." ) raise else: @@ -605,7 +611,7 @@ def _run(sim): return df, lambda_grad, speed except Exception as e: _logger.error( - f"Dynamics at λ = {lambda_value} failed. The following warning was " + f"Dynamics at {lam_sym} = {lambda_value} failed. The following warning was " f"raised: {e}. This may be due to a lack of minimisation." ) @@ -619,18 +625,20 @@ def _run(sim): ).clone() except: _logger.warning( - f"Unable to load checkpoint file for λ={lambda_value}, starting from scratch." + f"Unable to load checkpoint file for {lam_sym}={lambda_value}, starting from scratch." ) else: system = self._system.clone() if self._config.restart: acc_time = system.time() if acc_time > self._config.runtime - self._config.timestep: - _logger.success(f"λ = {lambda_value} already complete. Skipping.") + _logger.success( + f"{lam_sym} = {lambda_value} already complete. Skipping." + ) return True else: _logger.debug( - f"Restarting λ = {lambda_value} at time {acc_time}, time remaining = {self._config.runtime - acc_time}" + f"Restarting {lam_sym} = {lambda_value} at time {acc_time}, time remaining = {self._config.runtime - acc_time}" ) # GPU platform. if self._is_gpu: @@ -639,11 +647,13 @@ def _run(sim): gpu_num = self._gpu_pool[0] self._remove_gpu_from_pool(gpu_num) if lambda_value is not None: - _logger.info(f"Running λ = {lambda_value} on GPU {gpu_num}") + _logger.info( + f"Running {lam_sym} = {lambda_value} on GPU {gpu_num}" + ) # Assumes that device for non-parallel GPU jobs is 0 else: gpu_num = 0 - _logger.info("Running λ = {lambda_value} on GPU 0") + _logger.info("Running {lam_sym} = {lambda_value} on GPU 0") self._initialise_simulation(system, lambda_value, device=gpu_num) try: df, lambda_grad, speed = _run(self._sim) @@ -660,7 +670,7 @@ def _run(sim): # All other platforms. else: - _logger.info(f"Running λ = {lambda_value}") + _logger.info(f"Running {lam_sym} = {lambda_value}") self._initialise_simulation(system, lambda_value) try: @@ -685,5 +695,5 @@ def _run(sim): filename=self._fnames[lambda_value]["energy_traj"], ) del system - _logger.success(f"λ = {lambda_value} complete") + _logger.success(f"{lam_sym} = {lambda_value} complete") return True diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index 8365992..05ec6b3 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -82,10 +82,9 @@ def test_logfile_creation(): config = Config(output_directory=tmpdir, log_file="test.log") assert config.log_file is not None assert Path.exists(config.output_directory / config.log_file) - Path.unlink(config.output_directory / config.log_file) # Instantiate a runner using the default config. # (All default options, other than platform="cpu".) - runner = Runner(mols, Config(output_directory=tmpdir, log_file="test.log")) + runner = Runner(mols, Config(output_directory=tmpdir, log_file="test1.log")) assert runner._config.log_file is not None assert Path.exists(runner._config.output_directory / runner._config.log_file) From 0d73e87067b61815fed36103ec9beca967c99d12 Mon Sep 17 00:00:00 2001 From: Matthew Date: Tue, 7 Nov 2023 16:59:13 +0000 Subject: [PATCH 13/27] platform not os.platform --- src/somd2/runner/_dynamics.py | 4 ++-- src/somd2/runner/_runner.py | 4 ++-- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/somd2/runner/_dynamics.py b/src/somd2/runner/_dynamics.py index 213d43e..2ab957b 100644 --- a/src/somd2/runner/_dynamics.py +++ b/src/somd2/runner/_dynamics.py @@ -20,7 +20,7 @@ ##################################################################### __all__ = ["Dynamics"] -import os as _os +import platform as _platform from pathlib import Path as _Path from ..config import Config as _Config @@ -29,7 +29,7 @@ from somd2 import _logger -if _os.platform == "win32": +if _platform == "win32": lam_sym = "lambda" else: lam_sym = "λ" diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index 216be04..7d7e134 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -21,7 +21,7 @@ __all__ = ["Runner"] -import os as _os +import platform as _platform from sire import stream as _stream from sire.system import System as _System @@ -31,7 +31,7 @@ from somd2 import _logger -if _os.platform == "win32": +if _platform == "win32": lam_sym = "lambda" else: lam_sym = "λ" From 845db4d79a429eb0adb1a0cc604dd5a055252b29 Mon Sep 17 00:00:00 2001 From: Matthew Date: Wed, 8 Nov 2023 10:05:45 +0000 Subject: [PATCH 14/27] Explicitly delete objects before tempfile cleanup to solve windows issue? --- src/somd2/runner/_runner.py | 5 ++--- tests/runner/test_config.py | 2 ++ 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index 7d7e134..ef2d8af 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -257,9 +257,8 @@ def get_last_config(output_directory): str(self._config.output_directory / "checkpoint_0.s3") ) except: - _logger.error( - f"Unable to load checkpoint file from {self._config.output_directory}." - ) + expdir = self._config.output_directory / "checkpoint_0.s3" + _logger.error(f"Unable to load checkpoint file from {expdir}.") raise else: last_config = dict(system_temp.property("config")) diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index 05ec6b3..cd8529f 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -82,9 +82,11 @@ def test_logfile_creation(): config = Config(output_directory=tmpdir, log_file="test.log") assert config.log_file is not None assert Path.exists(config.output_directory / config.log_file) + del config # Instantiate a runner using the default config. # (All default options, other than platform="cpu".) runner = Runner(mols, Config(output_directory=tmpdir, log_file="test1.log")) assert runner._config.log_file is not None assert Path.exists(runner._config.output_directory / runner._config.log_file) + del runner From 88e7c8da5d4dfc3c260117333c9a002a98b14fd8 Mon Sep 17 00:00:00 2001 From: Matthew Date: Wed, 8 Nov 2023 10:26:46 +0000 Subject: [PATCH 15/27] Disable logfile check on windows --- test_conf.py | 13 +++++++++++++ 1 file changed, 13 insertions(+) create mode 100644 test_conf.py diff --git a/test_conf.py b/test_conf.py new file mode 100644 index 0000000..9da6ed5 --- /dev/null +++ b/test_conf.py @@ -0,0 +1,13 @@ +from somd2.config import Config + + +def configure(inp): + conf = Config(**inp) + return conf + + +# c = configure({"pressure": "1 bar", "minimise": True, "extra_args": {"a": 1}}) +# print(c.equilibrate) + + +cnf = Config() From 8561f5cf28f4b31880cae6375ade24abace580c5 Mon Sep 17 00:00:00 2001 From: Matthew Date: Wed, 8 Nov 2023 11:53:46 +0000 Subject: [PATCH 16/27] Remove wrongly added test, actually disable windows on logfile test --- test_conf.py | 13 ------------- tests/runner/test_config.py | 7 +++++++ 2 files changed, 7 insertions(+), 13 deletions(-) delete mode 100644 test_conf.py diff --git a/test_conf.py b/test_conf.py deleted file mode 100644 index 9da6ed5..0000000 --- a/test_conf.py +++ /dev/null @@ -1,13 +0,0 @@ -from somd2.config import Config - - -def configure(inp): - conf = Config(**inp) - return conf - - -# c = configure({"pressure": "1 bar", "minimise": True, "extra_args": {"a": 1}}) -# print(c.equilibrate) - - -cnf = Config() diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index cd8529f..2463401 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -4,6 +4,9 @@ from somd2.config import Config from somd2.runner import Runner +import platform +import pytest + def test_dynamics_options(): """Validate that dynamics options are set correctly.""" @@ -70,6 +73,10 @@ def test_dynamics_options(): ) == d.integrator().__class__.__name__.lower().replace("integrator", "") +# Skip on windows due to file permission issues +@pytest.mark.skipif( + platform.system() == "Windows", reason="File permission issues on Windows" +) def test_logfile_creation(): # Test that the logfile is created by either the initialisation of the runner or of a config with tempfile.TemporaryDirectory() as tmpdir: From 499c529b78390c818a7d1f147346b04df61b3dfb Mon Sep 17 00:00:00 2001 From: Matthew Date: Wed, 8 Nov 2023 12:32:18 +0000 Subject: [PATCH 17/27] Switch logfile test back on and add explicit logger deletion to config class --- src/somd2/config/_config.py | 4 ++++ tests/runner/test_config.py | 8 +++----- 2 files changed, 7 insertions(+), 5 deletions(-) diff --git a/src/somd2/config/_config.py b/src/somd2/config/_config.py index a597263..3c3d865 100644 --- a/src/somd2/config/_config.py +++ b/src/somd2/config/_config.py @@ -282,6 +282,10 @@ def __eq__(self, other): """Equality operator.""" return self.as_dict() == other.as_dict() + def __del__(self): + """Destructor.""" + _logger.remove() + @staticmethod def from_yaml(path): """ diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index 2463401..9a87fa3 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -74,9 +74,9 @@ def test_dynamics_options(): # Skip on windows due to file permission issues -@pytest.mark.skipif( - platform.system() == "Windows", reason="File permission issues on Windows" -) +# @pytest.mark.skipif( +# platform.system() == "Windows", reason="File permission issues on Windows" +# ) def test_logfile_creation(): # Test that the logfile is created by either the initialisation of the runner or of a config with tempfile.TemporaryDirectory() as tmpdir: @@ -89,11 +89,9 @@ def test_logfile_creation(): config = Config(output_directory=tmpdir, log_file="test.log") assert config.log_file is not None assert Path.exists(config.output_directory / config.log_file) - del config # Instantiate a runner using the default config. # (All default options, other than platform="cpu".) runner = Runner(mols, Config(output_directory=tmpdir, log_file="test1.log")) assert runner._config.log_file is not None assert Path.exists(runner._config.output_directory / runner._config.log_file) - del runner From 08dec8feb79ca0b333a09b764b75f01fe1d65343 Mon Sep 17 00:00:00 2001 From: Matthew Date: Wed, 8 Nov 2023 14:07:04 +0000 Subject: [PATCH 18/27] Removed second half of logfile creation test --- tests/runner/test_config.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index 9a87fa3..13b8747 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -89,9 +89,10 @@ def test_logfile_creation(): config = Config(output_directory=tmpdir, log_file="test.log") assert config.log_file is not None assert Path.exists(config.output_directory / config.log_file) - + """ # Instantiate a runner using the default config. # (All default options, other than platform="cpu".) runner = Runner(mols, Config(output_directory=tmpdir, log_file="test1.log")) assert runner._config.log_file is not None assert Path.exists(runner._config.output_directory / runner._config.log_file) + """ From 471ee73272dc707b9bb0db5446ae256ffe48472e Mon Sep 17 00:00:00 2001 From: Matthew Date: Wed, 8 Nov 2023 14:28:12 +0000 Subject: [PATCH 19/27] returned windows skip to test for now --- tests/runner/test_config.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index 13b8747..28215e2 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -74,9 +74,9 @@ def test_dynamics_options(): # Skip on windows due to file permission issues -# @pytest.mark.skipif( -# platform.system() == "Windows", reason="File permission issues on Windows" -# ) +@pytest.mark.skipif( + platform.system() == "Windows", reason="File permission issues on Windows" +) def test_logfile_creation(): # Test that the logfile is created by either the initialisation of the runner or of a config with tempfile.TemporaryDirectory() as tmpdir: @@ -89,10 +89,9 @@ def test_logfile_creation(): config = Config(output_directory=tmpdir, log_file="test.log") assert config.log_file is not None assert Path.exists(config.output_directory / config.log_file) - """ + # Instantiate a runner using the default config. # (All default options, other than platform="cpu".) runner = Runner(mols, Config(output_directory=tmpdir, log_file="test1.log")) assert runner._config.log_file is not None assert Path.exists(runner._config.output_directory / runner._config.log_file) - """ From ef711771acf0f18a4ade69a5f4eb52e7f7ce0895 Mon Sep 17 00:00:00 2001 From: Matthew Date: Wed, 8 Nov 2023 14:32:41 +0000 Subject: [PATCH 20/27] Proper naming for windows platform --- src/somd2/runner/_dynamics.py | 2 +- src/somd2/runner/_runner.py | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/somd2/runner/_dynamics.py b/src/somd2/runner/_dynamics.py index 2ab957b..47d794d 100644 --- a/src/somd2/runner/_dynamics.py +++ b/src/somd2/runner/_dynamics.py @@ -29,7 +29,7 @@ from somd2 import _logger -if _platform == "win32": +if _platform.system() == "Windows": lam_sym = "lambda" else: lam_sym = "λ" diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index ef2d8af..c0f8b72 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -31,7 +31,7 @@ from somd2 import _logger -if _platform == "win32": +if _platform.system() == "Windows": lam_sym = "lambda" else: lam_sym = "λ" From 4b59cbb0e1dd2e5e8f0fe95ff203d6ca0357c9bc Mon Sep 17 00:00:00 2001 From: Matthew Date: Wed, 8 Nov 2023 14:54:44 +0000 Subject: [PATCH 21/27] another attempt at fixing logging on windows --- tests/runner/test_config.py | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index 28215e2..f12f500 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -1,6 +1,6 @@ import tempfile import sire as sr - +import somd2 from somd2.config import Config from somd2.runner import Runner @@ -74,9 +74,9 @@ def test_dynamics_options(): # Skip on windows due to file permission issues -@pytest.mark.skipif( - platform.system() == "Windows", reason="File permission issues on Windows" -) +# @pytest.mark.skipif( +# platform.system() == "Windows", reason="File permission issues on Windows" +# ) def test_logfile_creation(): # Test that the logfile is created by either the initialisation of the runner or of a config with tempfile.TemporaryDirectory() as tmpdir: @@ -95,3 +95,5 @@ def test_logfile_creation(): runner = Runner(mols, Config(output_directory=tmpdir, log_file="test1.log")) assert runner._config.log_file is not None assert Path.exists(runner._config.output_directory / runner._config.log_file) + + somd2._logger.remove() From c794e7b918a4f99b1a4563bf2e76baded692ac50 Mon Sep 17 00:00:00 2001 From: Matthew Date: Thu, 9 Nov 2023 10:37:31 +0000 Subject: [PATCH 22/27] Added checks for consistency when restarting from checkpoint Config encoded in to checkpoint files must match either the config.yaml file or the config from the lambda=0 window --- src/somd2/runner/_dynamics.py | 2 +- src/somd2/runner/_runner.py | 102 +++++++++++++++++++++------------- tests/runner/test_restart.py | 27 +++++++++ 3 files changed, 92 insertions(+), 39 deletions(-) diff --git a/src/somd2/runner/_dynamics.py b/src/somd2/runner/_dynamics.py index 47d794d..99e1417 100644 --- a/src/somd2/runner/_dynamics.py +++ b/src/somd2/runner/_dynamics.py @@ -338,7 +338,7 @@ def generate_lam_vals(lambda_base, increment): filename=self._filenames["energy_traj"], ) # Also want to add the simulation config to the - # system properties once a block has been succsessfully run. + # system properties once a block has been successfully run. self._system.set_property( "config", self._config.as_dict(sire_compatible=True) ) diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index c0f8b72..a70257f 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -221,6 +221,51 @@ def _check_directory(self): file.unlink() self._fnames = fnames + @staticmethod + def _compare_configs(config1, config2): + if not isinstance(config1, dict): + raise TypeError("'config1' must be of type 'dict'") + if not isinstance(config2, dict): + raise TypeError("'config2' must be of type 'dict'") + + # Define the subset of settings that are allowed to change after restart + allowed_diffs = [ + "runtime", + "restart", + "temperature", + "minimise", + "max_threads", + "equilibration_time", + "equilibration_timestep", + "energy_frequency", + "save_trajectory", + "frame_frequency", + "save_velocities", + "checkpoint_frequency", + "platform", + "max_threads", + "max_gpus", + "run_parallel", + "restart", + "save_trajectories", + "write_config", + "log_level", + "log_file", + "supress_overwrite_warning", + "xtra_args", + ] + for key in config1.keys(): + if key not in allowed_diffs: + # If one is from sire and the other is not, will raise error even though they are the same + if (config1[key] == None and config2[key] == False) or ( + config2[key] == None and config1[key] == False + ): + continue + elif config1[key] != config2[key]: + raise ValueError( + f"{key} has changed since the last run. This is not allowed when using the restart option." + ) + def _verify_restart_config(self): """ Verify that the config file matches the config file used to create the @@ -246,7 +291,8 @@ def get_last_config(output_directory): last_config_file = get_last_config(self._config.output_directory) with open(self._config.output_directory / last_config_file) as file: _logger.debug(f"Opening config file {last_config_file}") - last_config = _yaml.safe_load(file) + self.last_config = _yaml.safe_load(file) + cfg_curr = self._config.as_dict() except: _logger.info( f"""No config files found in {self._config.output_directory}, @@ -261,45 +307,11 @@ def get_last_config(output_directory): _logger.error(f"Unable to load checkpoint file from {expdir}.") raise else: - last_config = dict(system_temp.property("config")) + self.last_config = dict(system_temp.property("config")) + cfg_curr = self._config.as_dict(sire_compatible=True) del system_temp - # Define the subset of settings that are allowed to change after restart - allowed_diffs = [ - "runtime", - "restart", - "temperature", - "minimise", - "max_threads", - "equilibration_time", - "equilibration_timestep", - "energy_frequency", - "save_trajectory", - "frame_frequency", - "save_velocities", - "checkpoint_frequency", - "platform", - "max_threads", - "max_gpus", - "run_parallel", - "restart", - "save_trajectories", - "write_config", - "log_level", - "log_file", - "supress_overwrite_warning", - ] - for key in last_config.keys(): - if key not in allowed_diffs: - _logger.debug(f"Checking {key}") - _logger.debug( - f"""old value: {last_config[key]} - new value: {self._config.as_dict()[key]}""" - ) - if last_config[key] != self._config.as_dict()[key]: - raise ValueError( - f"{key} has changed since the last run. This is not allowed when using the restart option." - ) + self._compare_configs(self.last_config, cfg_curr) def get_options(self): """ @@ -626,6 +638,20 @@ def _run(sim): _logger.warning( f"Unable to load checkpoint file for {lam_sym}={lambda_value}, starting from scratch." ) + else: + try: + self._compare_configs( + self.last_config, dict(system.property("config")) + ) + except: + cfg_here = dict(system.property("config")) + _logger.debug( + f"last config: {self.last_config}, current config: {cfg_here}" + ) + _logger.error( + f"Config for {lam_sym}={lambda_value} does not match previous config." + ) + raise else: system = self._system.clone() if self._config.restart: diff --git a/tests/runner/test_restart.py b/tests/runner/test_restart.py index 528b2c7..cfe4b77 100644 --- a/tests/runner/test_restart.py +++ b/tests/runner/test_restart.py @@ -177,3 +177,30 @@ def test_restart(): with pytest.raises(ValueError): runner_swapendstates = Runner(mols, Config(**config_diffswapendstates)) + + # Need to test restart from sire checkpoint file + # this needs to be done last as it requires unlinking the config files + for file in Path(tmpdir).glob("*.yaml"): + file.unlink() + + # This should work as the config is read from the lambda=0 checkpoint file + runner_noconfig = Runner(mols, Config(**config_new)) + + # remove config again + for file in Path(tmpdir).glob("*.yaml"): + file.unlink() + + # Load the checkpoint file using sire and change the pressure option + sire_checkpoint = sr.stream.load(str(Path(tmpdir) / "checkpoint_0.s3")) + cfg = sire_checkpoint.property("config") + cfg["pressure"] = "0.5 atm" + sire_checkpoint.set_property("config", cfg) + sr.stream.save(sire_checkpoint, str(Path(tmpdir) / "checkpoint_0.s3")) + + # Load the new checkpoint file and make sure the restart fails + with pytest.raises(ValueError): + runner_badconfig = Runner(mols, Config(**config_new)) + + +if __name__ == "__main__": + test_restart() From fe081d5f28de734d5222c6645e0b7b6d2343a573 Mon Sep 17 00:00:00 2001 From: Matthew Date: Thu, 9 Nov 2023 11:54:50 +0000 Subject: [PATCH 23/27] Added lambda value to properties [ci skip] --- src/somd2/runner/_runner.py | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index a70257f..e79e508 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -213,7 +213,7 @@ def _check_directory(self): if len(deleted) > 0: if not self._config.supress_overwrite_warning: _logger.warning( - f"The following files already exist and will be overwritten: {list(set((deleted)))}" + f"The following files already exist and will be overwritten: {list(set((deleted)))} \n" ) input("Press Enter to erase and continue...") # Loop over files to be deleted, ignoring duplicates @@ -652,6 +652,16 @@ def _run(sim): f"Config for {lam_sym}={lambda_value} does not match previous config." ) raise + else: + lambda_encoded = system.property("lambda") + try: + lambda_encoded == lambda_value + except: + fname = self._fnames[lambda_value]["checkpoint"] + raise ValueError( + f"Lambda value from checkpoint file {fname} ({lambda_encoded}) does not match expected value ({lambda_value})." + ) + else: system = self._system.clone() if self._config.restart: From 7873aedcd6d180397e6e397ce6a24779fc5eed72 Mon Sep 17 00:00:00 2001 From: Matthew Date: Tue, 14 Nov 2023 12:03:06 +0000 Subject: [PATCH 24/27] Several changes to bring the code in line with @lohedges suggestions. Changes/removal of comments in both config and test files. Removal of redundant variable in config. lam_sym is now _lam_sym and is only declared in runner.py, being imported yb dynamics.py. Temperature can now no longer be changed between restarts, an appropriate test has been added to ensure this functionality. Extra_args is no longer explicitly checked in the _compare_configs function, and it now ignored by the _to_dict function of _config.py. --- src/somd2/config/_config.py | 5 ++-- src/somd2/runner/_dynamics.py | 24 +++++++++---------- src/somd2/runner/_runner.py | 43 +++++++++++++++++------------------ tests/runner/test_config.py | 6 ++--- tests/runner/test_restart.py | 7 ++++++ 5 files changed, 45 insertions(+), 40 deletions(-) diff --git a/src/somd2/config/_config.py b/src/somd2/config/_config.py index 3c3d865..df5fcc4 100644 --- a/src/somd2/config/_config.py +++ b/src/somd2/config/_config.py @@ -198,7 +198,7 @@ def __init__( Path to a directory to store output files. restart: bool - Whether to restart from a previous simulation - files found in {output-directory}. + Whether to restart from a previous simulation - files found in `output-directory`. write_config: bool Whether to write the configuration options to a YAML file in the output directory. @@ -319,6 +319,8 @@ def as_dict(self, sire_compatible=False): d = {} for attr, value in self.__dict__.items(): + if attr.startswith("_extra") or attr.startswith("extra"): + continue attr_l = attr[1:] if isinstance(value, _Path): d[attr_l] = str(value) @@ -913,7 +915,6 @@ def output_directory(self): @output_directory.setter def output_directory(self, output_directory): - self.cirectory_existed = False if not isinstance(output_directory, _Path): try: output_directory = _Path(output_directory) diff --git a/src/somd2/runner/_dynamics.py b/src/somd2/runner/_dynamics.py index 99e1417..66d5964 100644 --- a/src/somd2/runner/_dynamics.py +++ b/src/somd2/runner/_dynamics.py @@ -28,11 +28,9 @@ from ..io import parquet_append as _parquet_append from somd2 import _logger +import platform as _platform -if _platform.system() == "Windows": - lam_sym = "lambda" -else: - lam_sym = "λ" +from ._runner import _lam_sym class Dynamics: @@ -161,7 +159,7 @@ def _setup_dynamics(self, equilibration=False): pressure = None try: - map = self._config.extra_args + map = self._config._extra_args except: map = None @@ -196,7 +194,7 @@ def _minimisation(self, lambda_min=None): lambda_val. """ if lambda_min is None: - _logger.info(f"Minimising at {lam_sym} = {self._lambda_val}") + _logger.info(f"Minimising at {_lam_sym} = {self._lambda_val}") try: m = self._system.minimisation( cutoff_type=self._config.cutoff_type, @@ -204,14 +202,14 @@ def _minimisation(self, lambda_min=None): lambda_value=self._lambda_val, platform=self._config.platform, vacuum=not self._has_space, - map=self._config.extra_args, + map=self._config._extra_args, ) m.run() self._system = m.commit() except: raise else: - _logger.info(f"Minimising at {lam_sym} = {lambda_min}") + _logger.info(f"Minimising at {_lam_sym} = {lambda_min}") try: m = self._system.minimisation( cutoff_type=self._config.cutoff_type, @@ -219,7 +217,7 @@ def _minimisation(self, lambda_min=None): lambda_value=lambda_min, platform=self._config.platform, vacuum=not self._has_space, - map=self._config.extra_args, + map=self._config._extra_args, ) m.run() self._system = m.commit() @@ -234,7 +232,7 @@ def _equilibration(self): Currently just runs dynamics without any saving """ - _logger.info(f"Equilibrating at {lam_sym} = {self._lambda_val}") + _logger.info(f"Equilibrating at {_lam_sym} = {self._lambda_val}") self._setup_dynamics(equilibration=True) self._dyn.run( self._config.equilibration_time, @@ -287,7 +285,7 @@ def generate_lam_vals(lambda_base, increment): else: lam_arr = self._lambda_array + self._lambda_grad - _logger.info(f"Running dynamics at {lam_sym} = {self._lambda_val}") + _logger.info(f"Running dynamics at {_lam_sym} = {self._lambda_val}") if self._config.checkpoint_frequency.value() > 0.0: ### Calc number of blocks and remainder (surely there's a better way?)### @@ -342,13 +340,15 @@ def generate_lam_vals(lambda_base, increment): self._system.set_property( "config", self._config.as_dict(sire_compatible=True) ) + # Finally, encode lambda value in to properties. + self._system.set_property("lambda", self._lambda_val) else: _parquet_append( f, df.iloc[-int(energy_per_block) :], ) _logger.info( - f"Finished block {x+1} of {num_blocks} for {lam_sym} = {self._lambda_val}" + f"Finished block {x+1} of {num_blocks} for {_lam_sym} = {self._lambda_val}" ) except: raise diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index e79e508..34e80d6 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -30,11 +30,12 @@ from ..io import dict_to_yaml as _dict_to_yaml from somd2 import _logger +import platform as _platform if _platform.system() == "Windows": - lam_sym = "lambda" + _lam_sym = "lambda" else: - lam_sym = "λ" + _lam_sym = "λ" class Runner: @@ -232,7 +233,6 @@ def _compare_configs(config1, config2): allowed_diffs = [ "runtime", "restart", - "temperature", "minimise", "max_threads", "equilibration_time", @@ -252,7 +252,6 @@ def _compare_configs(config1, config2): "log_level", "log_file", "supress_overwrite_warning", - "xtra_args", ] for key in config1.keys(): if key not in allowed_diffs: @@ -472,7 +471,7 @@ def _initialise_simulation(self, system, lambda_value, device=None): has_space=self._has_space, ) except: - _logger.warning(f"System creation at {lam_sym} = {lambda_value} failed") + _logger.warning(f"System creation at {_lam_sym} = {lambda_value} failed") raise def _cleanup_simulation(self): @@ -512,17 +511,17 @@ def run(self): threads_per_worker = ( self._config.max_threads // self._config.num_lambda ) - self._config.extra_args = {"threads": threads_per_worker} + self._config._extra_args = {"threads": threads_per_worker} # (Multi-)GPU platform. elif self._is_gpu: self.max_workers = len(self._gpu_pool) - self._config.extra_args = {} + self._config._extra_args = {} # All other platforms. else: self._max_workers = 1 - self._config.extra_args = {} + self._config._extra_args = {} import concurrent.futures as _futures @@ -539,7 +538,7 @@ def run(self): result = False _logger.error( - f"Exception raised for {lam_sym} = {lambda_value}: {e}" + f"Exception raised for {_lam_sym} = {lambda_value}: {e}" ) with self._lock: results.append(result) @@ -600,8 +599,8 @@ def _run(sim): return df, lambda_grad, speed except Exception as e: _logger.warning( - f"Minimisation/dynamics at {lam_sym} = {lambda_value} failed with the " - f"following exception {e}, trying again with minimsation at {lam_sym} = 0." + f"Minimisation/dynamics at {_lam_sym} = {lambda_value} failed with the " + f"following exception {e}, trying again with minimsation at {_lam_sym} = 0." ) try: df = sim._run(lambda_minimisation=0.0) @@ -610,8 +609,8 @@ def _run(sim): return df, lambda_grad, speed except Exception as e: _logger.error( - f"Minimisation/dynamics at {lam_sym} = {lambda_value} failed, even after " - f"minimisation at {lam_sym} = 0. The following warning was raised: {e}." + f"Minimisation/dynamics at {_lam_sym} = {lambda_value} failed, even after " + f"minimisation at {_lam_sym} = 0. The following warning was raised: {e}." ) raise else: @@ -622,7 +621,7 @@ def _run(sim): return df, lambda_grad, speed except Exception as e: _logger.error( - f"Dynamics at {lam_sym} = {lambda_value} failed. The following warning was " + f"Dynamics at {_lam_sym} = {lambda_value} failed. The following warning was " f"raised: {e}. This may be due to a lack of minimisation." ) @@ -636,7 +635,7 @@ def _run(sim): ).clone() except: _logger.warning( - f"Unable to load checkpoint file for {lam_sym}={lambda_value}, starting from scratch." + f"Unable to load checkpoint file for {_lam_sym}={lambda_value}, starting from scratch." ) else: try: @@ -649,7 +648,7 @@ def _run(sim): f"last config: {self.last_config}, current config: {cfg_here}" ) _logger.error( - f"Config for {lam_sym}={lambda_value} does not match previous config." + f"Config for {_lam_sym}={lambda_value} does not match previous config." ) raise else: @@ -668,12 +667,12 @@ def _run(sim): acc_time = system.time() if acc_time > self._config.runtime - self._config.timestep: _logger.success( - f"{lam_sym} = {lambda_value} already complete. Skipping." + f"{_lam_sym} = {lambda_value} already complete. Skipping." ) return True else: _logger.debug( - f"Restarting {lam_sym} = {lambda_value} at time {acc_time}, time remaining = {self._config.runtime - acc_time}" + f"Restarting {_lam_sym} = {lambda_value} at time {acc_time}, time remaining = {self._config.runtime - acc_time}" ) # GPU platform. if self._is_gpu: @@ -683,12 +682,12 @@ def _run(sim): self._remove_gpu_from_pool(gpu_num) if lambda_value is not None: _logger.info( - f"Running {lam_sym} = {lambda_value} on GPU {gpu_num}" + f"Running {_lam_sym} = {lambda_value} on GPU {gpu_num}" ) # Assumes that device for non-parallel GPU jobs is 0 else: gpu_num = 0 - _logger.info("Running {lam_sym} = {lambda_value} on GPU 0") + _logger.info("Running {_lam_sym} = {lambda_value} on GPU 0") self._initialise_simulation(system, lambda_value, device=gpu_num) try: df, lambda_grad, speed = _run(self._sim) @@ -705,7 +704,7 @@ def _run(sim): # All other platforms. else: - _logger.info(f"Running {lam_sym} = {lambda_value}") + _logger.info(f"Running {_lam_sym} = {lambda_value}") self._initialise_simulation(system, lambda_value) try: @@ -730,5 +729,5 @@ def _run(sim): filename=self._fnames[lambda_value]["energy_traj"], ) del system - _logger.success(f"{lam_sym} = {lambda_value} complete") + _logger.success(f"{_lam_sym} = {lambda_value} complete") return True diff --git a/tests/runner/test_config.py b/tests/runner/test_config.py index f12f500..75bc671 100644 --- a/tests/runner/test_config.py +++ b/tests/runner/test_config.py @@ -84,14 +84,12 @@ def test_logfile_creation(): mols = sr.load(sr.expand(sr.tutorial_url, "merged_molecule.s3")) from pathlib import Path - # Instantiate a runner using the default config. - # (All default options, other than platform="cpu".) + # Test that a logfile is created once a config object is initialised config = Config(output_directory=tmpdir, log_file="test.log") assert config.log_file is not None assert Path.exists(config.output_directory / config.log_file) - # Instantiate a runner using the default config. - # (All default options, other than platform="cpu".) + # Test that a logfile is created once a runner object is initialised runner = Runner(mols, Config(output_directory=tmpdir, log_file="test1.log")) assert runner._config.log_file is not None assert Path.exists(runner._config.output_directory / runner._config.log_file) diff --git a/tests/runner/test_restart.py b/tests/runner/test_restart.py index cfe4b77..e8745a9 100644 --- a/tests/runner/test_restart.py +++ b/tests/runner/test_restart.py @@ -81,6 +81,13 @@ def test_restart(): with pytest.raises(ValueError): runner_timestep = Runner(mols, Config(**config_difftimestep)) + config_difftemperature = config_new.copy() + config_difftemperature["runtime"] = "36fs" + config_difftemperature["temperature"] = "200K" + + with pytest.raises(ValueError): + runner_temperature = Runner(mols, Config(**config_difftemperature)) + config_diffscalefactor = config_new.copy() config_diffscalefactor["runtime"] = "36fs" config_diffscalefactor["charge_scale_factor"] = 0.5 From a832c92db9d96a0b8698fe44154e056c962c2234 Mon Sep 17 00:00:00 2001 From: Matthew Date: Tue, 14 Nov 2023 12:31:41 +0000 Subject: [PATCH 25/27] Removed mention of directory_existed from _config.py - there is no need to raise a warning here as the user is warned in the runnner before any files are removed. [ci skip] --- src/somd2/config/_config.py | 6 ------ 1 file changed, 6 deletions(-) diff --git a/src/somd2/config/_config.py b/src/somd2/config/_config.py index df5fcc4..18eb7d8 100644 --- a/src/somd2/config/_config.py +++ b/src/somd2/config/_config.py @@ -903,10 +903,6 @@ def restart(self): def restart(self, restart): if not isinstance(restart, bool): raise ValueError("'restart' must be of type 'bool'") - if not restart and self.directory_existed: - _logger.warning( - f"Output directory {self.output_directory} already exists files may be overwritten" - ) self._restart = restart @property @@ -927,8 +923,6 @@ def output_directory(self, output_directory): raise ValueError( f"Output directory {output_directory} does not exist and cannot be created" ) - else: - self.directory_existed = True if self.log_file is not None: # Can now add the log file _logger.add(output_directory / self.log_file, level=self.log_level.upper()) From e7b82c3b8305f1994658bf5bc14d948c81023802 Mon Sep 17 00:00:00 2001 From: Matthew Date: Tue, 14 Nov 2023 15:11:37 +0000 Subject: [PATCH 26/27] Adds a systems_are_same function to the runner that checks uids, number of molecules, number of residues and number of atoms, returning true only if all match. This funciton is used to check equivalence between checkpoint files and the original system --- src/somd2/runner/_runner.py | 44 +++++++++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index 34e80d6..92c0717 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -312,6 +312,45 @@ def get_last_config(output_directory): self._compare_configs(self.last_config, cfg_curr) + @staticmethod + def systems_are_same(system0, system1): + """Check for equivalence between a pair of sire systems. + + Parameters + ---------- + system0: sire.system.System + The first system to be compared. + + system1: sire.system.System + The second system to be compared. + """ + if not isinstance(system0, _System): + raise TypeError("'system0' must be of type 'sire.system.System'") + if not isinstance(system1, _System): + raise TypeError("'system1' must be of type 'sire.system.System'") + + # Check for matching uids + if not system0._system.uid() == system1._system.uid(): + reason = "uids do not match" + return False, reason + + # Check for matching number of molecules + if not len(system0.molecules()) == len(system1.molecules()): + reason = "number of molecules do not match" + return False, reason + + # Check for matching number of residues + if not len(system0.residues()) == len(system1.residues()): + reason = "number of residues do not match" + return False, reason + + # Check for matching number of atoms + if not len(system0.atoms()) == len(system1.atoms()): + reason = "number of atoms do not match" + return False, reason + + return True, None + def get_options(self): """ Returns a dictionary of simulation options. @@ -638,6 +677,11 @@ def _run(sim): f"Unable to load checkpoint file for {_lam_sym}={lambda_value}, starting from scratch." ) else: + aresame, reason = self.systems_are_same(self._system, system) + if not aresame: + raise ValueError( + f"Checkpoint file does not match system for the following reason: {reason}." + ) try: self._compare_configs( self.last_config, dict(system.property("config")) From 71ee5dfffc64b2f542be5a0e6c8e5e1ed3c62e02 Mon Sep 17 00:00:00 2001 From: Matthew Date: Tue, 14 Nov 2023 15:50:44 +0000 Subject: [PATCH 27/27] Made systems_are_same private. --- src/somd2/runner/_runner.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/somd2/runner/_runner.py b/src/somd2/runner/_runner.py index 92c0717..cfa8ad5 100644 --- a/src/somd2/runner/_runner.py +++ b/src/somd2/runner/_runner.py @@ -313,7 +313,7 @@ def get_last_config(output_directory): self._compare_configs(self.last_config, cfg_curr) @staticmethod - def systems_are_same(system0, system1): + def _systems_are_same(system0, system1): """Check for equivalence between a pair of sire systems. Parameters @@ -677,7 +677,7 @@ def _run(sim): f"Unable to load checkpoint file for {_lam_sym}={lambda_value}, starting from scratch." ) else: - aresame, reason = self.systems_are_same(self._system, system) + aresame, reason = self._systems_are_same(self._system, system) if not aresame: raise ValueError( f"Checkpoint file does not match system for the following reason: {reason}."