From 50ed3ad6128d6492097054ce07c9bd3e35a0c681 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Mon, 9 Nov 2020 23:58:41 +0100 Subject: [PATCH 01/28] Set up CI with Azure Pipelines [skip ci] --- azure-pipelines.yml | 9 --------- 1 file changed, 9 deletions(-) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 0560cc04718..a58fbd4525e 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -26,15 +26,6 @@ jobs: Python37-32bit-full: PYTHON_VERSION: '3.7' PYTHON_ARCH: 'x86' - Python36-64bit-full: - PYTHON_VERSION: '3.6' - PYTHON_ARCH: 'x64' - Python37-64bit-full: - PYTHON_VERSION: '3.7' - PYTHON_ARCH: 'x64' - Python38-64bit-full: - PYTHON_VERSION: '3.8' - PYTHON_ARCH: 'x64' steps: - task: UsePythonVersion@0 inputs: From bcdd917c9bd8ae937291a619268c3dcf8abd3f67 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Thu, 26 Aug 2021 17:39:18 +0200 Subject: [PATCH 02/28] use fastener file locker --- package/MDAnalysis/coordinates/XDR.py | 31 ++++++++++++++++++++++++++- 1 file changed, 30 insertions(+), 1 deletion(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index da348b032c9..7a39a2b0248 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -38,6 +38,7 @@ import numpy as np from os.path import getctime, getsize, isfile, split, join import warnings +import fasteners from . import base from ..lib.mdamath import triclinic_box @@ -64,6 +65,28 @@ def offsets_filename(filename, ending='npz'): ending=ending)) +def offset_lock_filename(filename, ending='lock'): + """Return offset lock filename for XDR files. For this + the filename is appended + with `_offsets.{ending}`. + + Parameters + ---------- + filename : str + filename of trajectory + ending : str (optional) + fileending of offsets file + + Returns + ------- + offset_lock_filename : str + + """ + head, tail = split(filename) + return join(head, '.{tail}_offsets.{ending}'.format(tail=tail, + ending=ending)) + + def read_numpy_offsets(filename): """read offsets into dictionary. @@ -181,6 +204,9 @@ def _load_offsets(self): """load frame offsets from file, reread them from the trajectory if that fails""" fname = offsets_filename(self.filename) + lock_name = offset_lock_filename(self.filename) + lock = fasteners.InterProcessLock(lock_name) + lock.acquire() if not isfile(fname): self._read_offsets(store=True) @@ -217,15 +243,18 @@ def _load_offsets(self): self._read_offsets(store=True) else: self._xdr.set_offsets(data['offsets']) + lock.release() + def _read_offsets(self, store=False): """read frame offsets from trajectory""" + fname = offsets_filename(self.filename) offsets = self._xdr.offsets if store: ctime = getctime(self.filename) size = getsize(self.filename) try: - np.savez(offsets_filename(self.filename), + np.savez(fname, offsets=offsets, size=size, ctime=ctime, n_atoms=self._xdr.n_atoms) except Exception as e: From cdfc90157cee89e73a0ee9d2527199f1b46681d5 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Thu, 26 Aug 2021 17:44:47 +0200 Subject: [PATCH 03/28] recover azure --- azure-pipelines.yml | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 7cb944527ff..90fa4a18b07 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -26,6 +26,18 @@ jobs: Python37-32bit-full: PYTHON_VERSION: '3.7' PYTHON_ARCH: 'x86' + Python36-64bit-full: + PYTHON_VERSION: '3.6' + PYTHON_ARCH: 'x64' + Python37-64bit-full: + PYTHON_VERSION: '3.7' + PYTHON_ARCH: 'x64' + Python38-64bit-full: + PYTHON_VERSION: '3.8' + PYTHON_ARCH: 'x64' + Python39-64bit-full: + PYTHON_VERSION: '3.9' + PYTHON_ARCH: 'x64' steps: - task: UsePythonVersion@0 inputs: From 0db6651f912f1588c684a33ff381122815e3cc9c Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Thu, 26 Aug 2021 18:10:03 +0200 Subject: [PATCH 04/28] fastener requirement --- .travis.yml | 4 ++-- azure-pipelines.yml | 1 + package/requirements.txt | 1 + package/setup.py | 1 + 4 files changed, 5 insertions(+), 2 deletions(-) diff --git a/.travis.yml b/.travis.yml index 74e12a70c4d..a0c5fc72edc 100644 --- a/.travis.yml +++ b/.travis.yml @@ -25,7 +25,7 @@ matrix: - conda activate base - conda update --yes conda - conda install --yes pip pytest==6.1.2 mmtf-python biopython networkx cython matplotlib-base "scipy>=1.6" griddataformats hypothesis gsd codecov -c conda-forge -c biobuilds - - pip install pytest-xdist threadpoolctl + - pip install pytest-xdist threadpoolctl fasteners - conda info install: - (cd package/ && python setup.py develop) && (cd testsuite/ && python setup.py install) @@ -45,7 +45,7 @@ matrix: if: type = cron before_install: - python -m pip install cython "numpy>=1.19.2" scipy - - python -m pip install --no-build-isolation hypothesis matplotlib pytest pytest-cov pytest-xdist tqdm threadpoolctl + - python -m pip install --no-build-isolation hypothesis matplotlib pytest pytest-cov pytest-xdist tqdm threadpoolctl fasteners install: - cd package - python setup.py install diff --git a/azure-pipelines.yml b/azure-pipelines.yml index 90fa4a18b07..9829b8147dd 100644 --- a/azure-pipelines.yml +++ b/azure-pipelines.yml @@ -60,6 +60,7 @@ jobs: h5py tqdm threadpoolctl + fasteners displayName: 'Install dependencies' # TODO: recent rdkit is not on PyPI - script: >- diff --git a/package/requirements.txt b/package/requirements.txt index 78d8f40ca6d..3e676ce5d63 100644 --- a/package/requirements.txt +++ b/package/requirements.txt @@ -21,3 +21,4 @@ sphinx_rtd_theme sphinx_sitemap threadpoolctl tqdm +fasteners diff --git a/package/setup.py b/package/setup.py index 12d9428365a..536a8875ad1 100755 --- a/package/setup.py +++ b/package/setup.py @@ -597,6 +597,7 @@ def long_description(readme): 'matplotlib>=1.5.1', 'tqdm>=4.43.0', 'threadpoolctl', + 'fasteners' ] if not os.name == 'nt': From 8d6b7675d5a94522636d6fd088cf4c7990cdc705 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Thu, 26 Aug 2021 18:10:19 +0200 Subject: [PATCH 05/28] change lock file func --- package/MDAnalysis/coordinates/XDR.py | 29 +++++---------------------- 1 file changed, 5 insertions(+), 24 deletions(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index 7a39a2b0248..49831e9adfa 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -45,7 +45,8 @@ def offsets_filename(filename, ending='npz'): - """Return offset filename for XDR files. For this the filename is appended + """Return offset or its lock filename for XDR files. + For this the filename is appended with `_offsets.{ending}`. Parameters @@ -65,28 +66,6 @@ def offsets_filename(filename, ending='npz'): ending=ending)) -def offset_lock_filename(filename, ending='lock'): - """Return offset lock filename for XDR files. For this - the filename is appended - with `_offsets.{ending}`. - - Parameters - ---------- - filename : str - filename of trajectory - ending : str (optional) - fileending of offsets file - - Returns - ------- - offset_lock_filename : str - - """ - head, tail = split(filename) - return join(head, '.{tail}_offsets.{ending}'.format(tail=tail, - ending=ending)) - - def read_numpy_offsets(filename): """read offsets into dictionary. @@ -204,7 +183,9 @@ def _load_offsets(self): """load frame offsets from file, reread them from the trajectory if that fails""" fname = offsets_filename(self.filename) - lock_name = offset_lock_filename(self.filename) + lock_name = offsets_filename(self.filename, + ending='lock') + lock = fasteners.InterProcessLock(lock_name) lock.acquire() From a27be21fd48b02e6bb86f508d4367188d0743b17 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Thu, 26 Aug 2021 18:16:55 +0200 Subject: [PATCH 06/28] add valueerror check for offset --- package/MDAnalysis/coordinates/XDR.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index 49831e9adfa..729a886dd53 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -84,7 +84,7 @@ def read_numpy_offsets(filename): """ try: return {k: v for k, v in np.load(filename).items()} - except IOError: + except (ValueError, IOError): warnings.warn("Failed to load offsets file {}\n".format(filename)) return False From 1f5f192186430d12a16be3d05cdfdd95d518b624 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Thu, 26 Aug 2021 18:24:41 +0200 Subject: [PATCH 07/28] add fasteners dep to github ci --- .github/workflows/gh-ci.yaml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.github/workflows/gh-ci.yaml b/.github/workflows/gh-ci.yaml index 4b393a8dc7c..9aeb1d7d1f5 100644 --- a/.github/workflows/gh-ci.yaml +++ b/.github/workflows/gh-ci.yaml @@ -21,7 +21,7 @@ defaults: shell: bash -l {0} env: - MDA_CONDA_MIN_DEPS: "pip pytest mmtf-python biopython networkx cython matplotlib-base scipy griddataformats hypothesis gsd codecov threadpoolctl" + MDA_CONDA_MIN_DEPS: "pip pytest mmtf-python biopython networkx cython matplotlib-base scipy griddataformats hypothesis gsd codecov threadpoolctl fasteners" MDA_CONDA_EXTRA_DEPS: "seaborn>=0.7.0 clustalw=2.1 netcdf4 scikit-learn joblib>=0.12 chemfiles-python>=0.9 tqdm>=4.43.0 tidynamics>=1.0.0 rdkit>=2020.03.1 h5py openmm" MDA_PIP_MIN_DEPS: 'coverage<5 pytest-cov==2.10.1 pytest-xdist' MDA_PIP_EXTRA_DEPS: 'duecredit parmed' From 86e6e3dfcb12851e1dd07c11489e71a84cd94c64 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Wed, 22 Sep 2021 13:29:25 +0200 Subject: [PATCH 08/28] documents for file lock --- package/MDAnalysis/coordinates/XDR.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index 729a886dd53..ef20e150911 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -84,6 +84,8 @@ def read_numpy_offsets(filename): """ try: return {k: v for k, v in np.load(filename).items()} + + # `ValueError` is encountered when the offset file is corrupted. except (ValueError, IOError): warnings.warn("Failed to load offsets file {}\n".format(filename)) return False @@ -181,13 +183,15 @@ def close(self): def _load_offsets(self): """load frame offsets from file, reread them from the trajectory if that - fails""" + fails. To prevent the competition of generating the same offset file + from multiple processes, an `InterProcessLock` is used.""" fname = offsets_filename(self.filename) lock_name = offsets_filename(self.filename, ending='lock') - lock = fasteners.InterProcessLock(lock_name) - lock.acquire() + try: + lock = fasteners.InterProcessLock(lock_name) + lock.acquire() if not isfile(fname): self._read_offsets(store=True) From 9440a1951f733cc84278bd366bc52f60cc18ab09 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Wed, 22 Sep 2021 13:41:06 +0200 Subject: [PATCH 09/28] use context manager filelock --- package/MDAnalysis/coordinates/XDR.py | 75 +++++++++++++-------------- 1 file changed, 35 insertions(+), 40 deletions(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index ef20e150911..02d501b1ef5 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -189,47 +189,42 @@ def _load_offsets(self): lock_name = offsets_filename(self.filename, ending='lock') - try: - lock = fasteners.InterProcessLock(lock_name) - lock.acquire() - - if not isfile(fname): - self._read_offsets(store=True) - return - - # if offsets file read correctly, data will be a dictionary of offsets - # if not, data will be False - # if False, offsets should be read from the trajectory - # this warning can be avoided by loading Universe like: - # u = mda.Universe(data.TPR, data.XTC, refresh_offsets=True) - # refer to Issue #1893 - data = read_numpy_offsets(fname) - if not data: - warnings.warn("Reading offsets from {} failed, " - "reading offsets from trajectory instead\n" - "Consider setting 'refresh_offsets=True' " - "when loading your Universe".format(fname)) - self._read_offsets(store=True) - return - - ctime_ok = size_ok = n_atoms_ok = False - - try: - ctime_ok = getctime(self.filename) == data['ctime'] - size_ok = getsize(self.filename) == data['size'] - n_atoms_ok = self._xdr.n_atoms == data['n_atoms'] - except KeyError: - # we tripped over some old offset formated file - pass - - if not (ctime_ok and size_ok and n_atoms_ok): - warnings.warn("Reload offsets from trajectory\n " - "ctime or size or n_atoms did not match") - self._read_offsets(store=True) - else: - self._xdr.set_offsets(data['offsets']) - lock.release() + with fasteners.InterProcessLock(lock_name): + if not isfile(fname): + self._read_offsets(store=True) + return + + # if offsets file read correctly, data will be a dictionary of offsets + # if not, data will be False + # if False, offsets should be read from the trajectory + # this warning can be avoided by loading Universe like: + # u = mda.Universe(data.TPR, data.XTC, refresh_offsets=True) + # refer to Issue #1893 + data = read_numpy_offsets(fname) + if not data: + warnings.warn("Reading offsets from {} failed, " + "reading offsets from trajectory instead\n" + "Consider setting 'refresh_offsets=True' " + "when loading your Universe".format(fname)) + self._read_offsets(store=True) + return + + ctime_ok = size_ok = n_atoms_ok = False + try: + ctime_ok = getctime(self.filename) == data['ctime'] + size_ok = getsize(self.filename) == data['size'] + n_atoms_ok = self._xdr.n_atoms == data['n_atoms'] + except KeyError: + # we tripped over some old offset formated file + pass + + if not (ctime_ok and size_ok and n_atoms_ok): + warnings.warn("Reload offsets from trajectory\n " + "ctime or size or n_atoms did not match") + self._read_offsets(store=True) + else: + self._xdr.set_offsets(data['offsets']) def _read_offsets(self, store=False): """read frame offsets from trajectory""" From bd5a9b0e00b282ec29fc6dad024e10d4199948b2 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Wed, 22 Sep 2021 13:44:53 +0200 Subject: [PATCH 10/28] filelock changelog --- package/CHANGELOG | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/package/CHANGELOG b/package/CHANGELOG index 4ac7f080d21..fe6bd32ce48 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -139,6 +139,10 @@ Fixes would create a test artifact (Issue #2979, PR #2981) * Fix syntax warning over comparison of literals using is (Issue #3066) * new `Results` class now can be pickled/unpickled. (PR #3309) + * Fix the competition when generating offsets from multiple processes by + using a interprocess file lock. (Issue #1988, PR #3375) + * Fix the error when trying to load a corrupted offset file (Issue #3230 + PR #3375) Enhancements * LAMMPSDumpReader can now read coordinates in all the different LAMMPS From b9f161fadecdece211d1cc47e405ad4b017b792d Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Wed, 22 Sep 2021 13:47:24 +0200 Subject: [PATCH 11/28] add versionchanged for filelock --- package/MDAnalysis/coordinates/XDR.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index 02d501b1ef5..89f5e2b7ea3 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -117,6 +117,8 @@ class XDRBaseReader(base.ReaderBase): .. versionchanged:: 1.0.0 XDR offsets read from trajectory if offsets file read-in fails + .. versionchanged:: 2.0.0 + Add a InterProcessLock when generating offsets """ def __init__(self, filename, convert_units=True, sub=None, From 45995003bdeedbac3fe14e4340b6f31e9806b4d4 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Wed, 22 Sep 2021 14:16:04 +0200 Subject: [PATCH 12/28] check lock file permission --- package/MDAnalysis/coordinates/XDR.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index 89f5e2b7ea3..7b29b64cdc4 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -191,7 +191,16 @@ def _load_offsets(self): lock_name = offsets_filename(self.filename, ending='lock') - with fasteners.InterProcessLock(lock_name): + # check if the location of the lock is writable. + try: + filelock = fasteners.InterProcessLock(lock_name) + filelock.acquire() + filelock.release() + except PermissionError: + head, tail = split(self.filename) + filelock = fasteners.InterProcessLock('/tmp/' + tail +'.lock') + + with filelock: if not isfile(fname): self._read_offsets(store=True) return From 1a6129d4a62c2f040a9a76d9745ac24fbc1146d1 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Wed, 22 Sep 2021 14:40:09 +0200 Subject: [PATCH 13/28] add test --- .../MDAnalysisTests/coordinates/test_xdr.py | 4 ++++ .../parallelism/test_multiprocessing.py | 19 +++++++++++++++++++ 2 files changed, 23 insertions(+) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 40d0b6c2974..9f4af18fac0 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -819,6 +819,10 @@ def test_persistent_offsets_readonly(self, tmpdir): self._reader(filename) assert_equal(os.path.exists(XDR.offsets_filename(filename)), False) + def test_offset_lock_created(self): + assert_equal(os.path.exists(XDR.offsets_filename(self.filename, + ending='lock')), True) + class TestXTCReader_offsets(_GromacsReader_offsets): __test__ = True diff --git a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py index 12e63ba31b5..fd9f70af32b 100644 --- a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py +++ b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py @@ -23,6 +23,7 @@ import multiprocessing import numpy as np +import os import pytest import pickle from numpy.testing import assert_equal @@ -30,6 +31,7 @@ import MDAnalysis as mda from MDAnalysis.coordinates.core import get_reader_for from MDAnalysis.analysis.rms import RMSD +from MDAnalysis.coordinates import XDR from MDAnalysisTests.datafiles import ( CRD, @@ -125,6 +127,23 @@ def test_universe_unpickle_in_new_process(): assert_equal(ref, res) +def create_universe(): + return mda.Universe(GRO, XTC) + + +def test_creating_multiple_universe_without_offset(): + # test if they can be created without generating + # the offset simultaneously. + try: + os.remove(XDR.offsets_filename(XTC)) + except OSError: + pass + p = multiprocessing.Pool(2) + _ = [p.apply(create_universe) + for i in range(3)] + p.close() + + @pytest.fixture(params=[ # formatname, filename ('CRD', CRD, dict()), From 35c88bf4547512c6c47ed5dd66d5c22076805586 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Wed, 22 Sep 2021 14:59:50 +0200 Subject: [PATCH 14/28] ignore lock file --- .gitignore | 1 + package/MDAnalysis/coordinates/XDR.py | 2 +- testsuite/MDAnalysisTests/coordinates/test_xdr.py | 5 +++++ 3 files changed, 7 insertions(+), 1 deletion(-) diff --git a/.gitignore b/.gitignore index 70f1106a6a2..0760f8129e4 100644 --- a/.gitignore +++ b/.gitignore @@ -50,3 +50,4 @@ benchmarks/results *~ .idea .vscode +*.lock diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index 7b29b64cdc4..3e5714710db 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -198,7 +198,7 @@ def _load_offsets(self): filelock.release() except PermissionError: head, tail = split(self.filename) - filelock = fasteners.InterProcessLock('/tmp/' + tail +'.lock') + filelock = fasteners.InterProcessLock('/tmp/' + tail + '.lock') with filelock: if not isfile(fname): diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 9f4af18fac0..5e436b4e9c2 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -26,6 +26,7 @@ import errno import numpy as np import os +from os.path import split import shutil import subprocess @@ -819,6 +820,10 @@ def test_persistent_offsets_readonly(self, tmpdir): self._reader(filename) assert_equal(os.path.exists(XDR.offsets_filename(filename)), False) + # test if the offset lock file is created in tmp folder + head, tail = split(filename) + assert_equal(os.path.exists('/tmp/' + tail + '.lock'), True) + def test_offset_lock_created(self): assert_equal(os.path.exists(XDR.offsets_filename(self.filename, ending='lock')), True) From 4fd183be43565643a74130116867642b37f792ee Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Mon, 1 Nov 2021 23:05:52 +0100 Subject: [PATCH 15/28] move changelog to 2.1.0 --- package/CHANGELOG | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/package/CHANGELOG b/package/CHANGELOG index 0b8668e6cb2..4f1a01ae829 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -13,7 +13,8 @@ The rules for this file: * release numbers follow "Semantic Versioning" http://semver.org ------------------------------------------------------------------------------ -??/??/?? IAlibay, melomcr, mdd31, ianmkenney, richardjgowers, hmacdope +??/??/?? IAlibay, melomcr, mdd31, ianmkenney, richardjgowers, hmacdope, + yuxuanzhuang * 2.1.0 @@ -22,6 +23,10 @@ Fixes * TRRWriter preserves timestep data (Issue #3350). * Fixed Universe creation from a custom object which only provided a topology, previously raised a TypeError. (Issue #3443) + * Fix the competition when generating offsets from multiple processes by + using a interprocess file lock. (Issue #1988, PR #3375) + * Fix the error when trying to load a corrupted offset file (Issue #3230 + PR #3375) Enhancements * Add option for custom compiler flags for C/C++ on build and remove march @@ -145,10 +150,6 @@ Fixes would create a test artifact (Issue #2979, PR #2981) * Fix syntax warning over comparison of literals using is (Issue #3066) * new `Results` class now can be pickled/unpickled. (PR #3309) - * Fix the competition when generating offsets from multiple processes by - using a interprocess file lock. (Issue #1988, PR #3375) - * Fix the error when trying to load a corrupted offset file (Issue #3230 - PR #3375) Enhancements * LAMMPSDumpReader can now read coordinates in all the different LAMMPS From acde1a64d30f84c0b868c06ed77d375349f71307 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Mon, 1 Nov 2021 23:33:16 +0100 Subject: [PATCH 16/28] remove tmp lock file creation --- package/MDAnalysis/coordinates/XDR.py | 11 ++++++----- package/setup.py | 2 +- testsuite/MDAnalysisTests/coordinates/test_xdr.py | 11 ++++++----- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index 3e5714710db..564bc6d798a 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -193,12 +193,13 @@ def _load_offsets(self): # check if the location of the lock is writable. try: - filelock = fasteners.InterProcessLock(lock_name) - filelock.acquire() - filelock.release() + with fasteners.InterProcessLock(lock_name) as filelock: + pass except PermissionError: - head, tail = split(self.filename) - filelock = fasteners.InterProcessLock('/tmp/' + tail + '.lock') + warnings.warn(f"Cannot write lock/offset file in same location as " + "{self.filename}. Using slow offset calculation.") + self._read_offsets(store=True) + return with filelock: if not isfile(fname): diff --git a/package/setup.py b/package/setup.py index 23b8612503c..e827f4cd398 100755 --- a/package/setup.py +++ b/package/setup.py @@ -599,7 +599,7 @@ def long_description(readme): 'matplotlib>=1.5.1', 'tqdm>=4.43.0', 'threadpoolctl', - 'fasteners' + 'fasteners', ] if not os.name == 'nt': diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index d30ad5965dd..645b78e7587 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -851,12 +851,13 @@ def test_persistent_offsets_readonly(self, tmpdir): filename = str(tmpdir.join(os.path.basename(self.filename))) # try to write a offsets file - self._reader(filename) + with (pytest.warns(UserWarning, match="Couldn't save offsets") and + pytest.warns(UserWarning, match="Cannot write")): + self._reader(filename) assert_equal(os.path.exists(XDR.offsets_filename(filename)), False) - - # test if the offset lock file is created in tmp folder - head, tail = split(filename) - assert_equal(os.path.exists('/tmp/' + tail + '.lock'), True) + # check the lock file is not created as well. + assert_equal(os.path.exists(XDR.offsets_filename(filename, + ending='.lock')), False) def test_offset_lock_created(self): assert_equal(os.path.exists(XDR.offsets_filename(self.filename, From d1540df152455ff1c5de9a723e389a350b761821 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Mon, 1 Nov 2021 23:36:39 +0100 Subject: [PATCH 17/28] period --- package/MDAnalysis/coordinates/XDR.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index 564bc6d798a..b29ec580bc7 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -215,9 +215,9 @@ def _load_offsets(self): data = read_numpy_offsets(fname) if not data: warnings.warn("Reading offsets from {} failed, " - "reading offsets from trajectory instead\n" + "reading offsets from trajectory instead.\n" "Consider setting 'refresh_offsets=True' " - "when loading your Universe".format(fname)) + "when loading your Universe.".format(fname)) self._read_offsets(store=True) return From c42c12c18870733c4bd390b9466542904575da33 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Mon, 1 Nov 2021 23:38:32 +0100 Subject: [PATCH 18/28] test assert --- testsuite/MDAnalysisTests/coordinates/test_xdr.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 645b78e7587..71e82bfd531 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -860,8 +860,8 @@ def test_persistent_offsets_readonly(self, tmpdir): ending='.lock')), False) def test_offset_lock_created(self): - assert_equal(os.path.exists(XDR.offsets_filename(self.filename, - ending='lock')), True) + assert os.path.exists(XDR.offsets_filename(self.filename, + ending='lock')) class TestXTCReader_offsets(_GromacsReader_offsets): From f4aded12e99f3a69d76bae1a10ed79648cb9e1d2 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Mon, 1 Nov 2021 23:52:35 +0100 Subject: [PATCH 19/28] init filelock --- package/MDAnalysis/coordinates/XDR.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package/MDAnalysis/coordinates/XDR.py b/package/MDAnalysis/coordinates/XDR.py index b29ec580bc7..e3109e413ae 100644 --- a/package/MDAnalysis/coordinates/XDR.py +++ b/package/MDAnalysis/coordinates/XDR.py @@ -201,7 +201,7 @@ def _load_offsets(self): self._read_offsets(store=True) return - with filelock: + with fasteners.InterProcessLock(lock_name) as filelock: if not isfile(fname): self._read_offsets(store=True) return From 7ea02bb6059e267a14fabcd6896538f0d5c846c5 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Sat, 20 Nov 2021 18:47:20 +0100 Subject: [PATCH 20/28] test offset ioerror --- .../MDAnalysisTests/coordinates/test_xdr.py | 23 ++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 71e82bfd531..eba7ab8f34e 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -750,7 +750,16 @@ def test_nonexistant_offsets_file(self, traj): saved_offsets = XDR.read_numpy_offsets(outfile_offsets) assert_equal(saved_offsets, False) - def test_reload_offsets_if_offsets_readin_fails(self, trajectory): + def test_nonexistant_offsets_file(self, traj): + # assert that a corrupted file returns False during read-in + # Issue #3230 + outfile_offsets = XDR.offsets_filename(traj) + with patch.object(np, "load") as np_load_mock: + np_load_mock.side_effect = ValueError + saved_offsets = XDR.read_numpy_offsets(outfile_offsets) + assert_equal(saved_offsets, False) + + def test_reload_offsets_if_offsets_readin_io_fails(self, trajectory): # force the np.load call that is called in read_numpy_offsets # during _load_offsets to give an IOError # ensure that offsets are then read-in from the trajectory @@ -762,6 +771,18 @@ def test_reload_offsets_if_offsets_readin_fails(self, trajectory): self.ref_offsets, err_msg="error loading frame offsets") + def test_reload_offsets_if_offsets_readin_value_fails(self, trajectory): + # force the np.load call that is called in read_numpy_offsets + # during _load_offsets to give an ValueError (Issue #3230) + # ensure that offsets are then read-in from the trajectory + with patch.object(np, "load") as np_load_mock: + np_load_mock.side_effect = ValueError + trajectory._load_offsets() + assert_almost_equal( + trajectory._xdr.offsets, + self.ref_offsets, + err_msg="error loading frame offsets") + def test_persistent_offsets_size_mismatch(self, traj): # check that stored offsets are not loaded when trajectory # size differs from stored size From e3e45715f63fcbe2312d628fe578bb9d9cf746b8 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Thu, 17 Feb 2022 18:41:10 +0100 Subject: [PATCH 21/28] add comments to racing test --- testsuite/MDAnalysisTests/coordinates/test_xdr.py | 3 ++- .../MDAnalysisTests/parallelism/test_multiprocessing.py | 5 ++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index eba7ab8f34e..96f61261298 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -777,7 +777,8 @@ def test_reload_offsets_if_offsets_readin_value_fails(self, trajectory): # ensure that offsets are then read-in from the trajectory with patch.object(np, "load") as np_load_mock: np_load_mock.side_effect = ValueError - trajectory._load_offsets() + with pytest.warns(UserWarning, match="Failed to load offsets"): + trajectory._load_offsets() assert_almost_equal( trajectory._xdr.offsets, self.ref_offsets, diff --git a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py index fd9f70af32b..dcd8edb3b22 100644 --- a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py +++ b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py @@ -134,15 +134,18 @@ def create_universe(): def test_creating_multiple_universe_without_offset(): # test if they can be created without generating # the offset simultaneously. + # The tested XTC file is way too short to induce a race scenario try: os.remove(XDR.offsets_filename(XTC)) except OSError: pass p = multiprocessing.Pool(2) - _ = [p.apply(create_universe) + universes = [p.apply(create_universe) for i in range(3)] p.close() + assert_equal(universes[0], universes[1]) + @pytest.fixture(params=[ # formatname, filename From 411bc99ac9ad36b31cd319f4f5e49bb4883a4fa6 Mon Sep 17 00:00:00 2001 From: Yuxuan Zhuang Date: Thu, 17 Feb 2022 18:53:35 +0100 Subject: [PATCH 22/28] fix race condition --- testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py index dcd8edb3b22..340c897fb3d 100644 --- a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py +++ b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py @@ -144,7 +144,8 @@ def test_creating_multiple_universe_without_offset(): for i in range(3)] p.close() - assert_equal(universes[0], universes[1]) + assert_equal(universes[0].trajectory._xdr.offsets, + universes[1].trajectory._xdr.offsets) @pytest.fixture(params=[ From 03bf8057cc18e65f0836f11e8e68bdf73313ff00 Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Fri, 25 Feb 2022 10:58:40 -0700 Subject: [PATCH 23/28] addressed my comments --- .../MDAnalysisTests/coordinates/test_xdr.py | 6 +-- .../parallelism/test_multiprocessing.py | 38 +++++++++++-------- 2 files changed, 26 insertions(+), 18 deletions(-) diff --git a/testsuite/MDAnalysisTests/coordinates/test_xdr.py b/testsuite/MDAnalysisTests/coordinates/test_xdr.py index 96f61261298..236db03cbda 100644 --- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -742,15 +742,15 @@ def test_offsets(self, trajectory, traj): def test_reload_offsets(self, traj): self._reader(traj, refresh_offsets=True) - def test_nonexistant_offsets_file(self, traj): - # assert that a nonexistant file returns False during read-in + def test_nonexistent_offsets_file(self, traj): + # assert that a nonexistent file returns False during read-in outfile_offsets = XDR.offsets_filename(traj) with patch.object(np, "load") as np_load_mock: np_load_mock.side_effect = IOError saved_offsets = XDR.read_numpy_offsets(outfile_offsets) assert_equal(saved_offsets, False) - def test_nonexistant_offsets_file(self, traj): + def test_nonexistent_offsets_file(self, traj): # assert that a corrupted file returns False during read-in # Issue #3230 outfile_offsets = XDR.offsets_filename(traj) diff --git a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py index 340c897fb3d..c1cd10d81e0 100644 --- a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py +++ b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py @@ -24,14 +24,15 @@ import numpy as np import os +import shutil import pytest import pickle from numpy.testing import assert_equal import MDAnalysis as mda +import MDAnalysis.coordinates from MDAnalysis.coordinates.core import get_reader_for from MDAnalysis.analysis.rms import RMSD -from MDAnalysis.coordinates import XDR from MDAnalysisTests.datafiles import ( CRD, @@ -87,6 +88,19 @@ def u(request): top, trj = request.param return mda.Universe(top, trj) +@pytest.fixture(scope="function") +def temp_xtc(tmp_path): + fresh_xtc = tmp_path / "testing.xtc" + shutil.copy(XTC, fresh_xtc) + # In principle there is no possibility that the offset for + # fresh_xtc exists as it is a fresh copy for every test. However, + # the code is left for documentation. + try: + os.remove(MDAnalysis.coordinates.XDR.offsets_filename(fresh_xtc)) + except OSError: + pass + return fresh_xtc + # Define target functions here # inside test functions doesn't work @@ -127,22 +141,16 @@ def test_universe_unpickle_in_new_process(): assert_equal(ref, res) -def create_universe(): - return mda.Universe(GRO, XTC) - - -def test_creating_multiple_universe_without_offset(): +def test_creating_multiple_universe_without_offset(temp_xtc, ncopies=3): # test if they can be created without generating # the offset simultaneously. - # The tested XTC file is way too short to induce a race scenario - try: - os.remove(XDR.offsets_filename(XTC)) - except OSError: - pass - p = multiprocessing.Pool(2) - universes = [p.apply(create_universe) - for i in range(3)] - p.close() + # The tested XTC file is way too short to induce a race scenario but the + # test is included as documentation for the scenario that used to create + # a problem (see PR #3375 and issues #3230, #1988) + + args = (GRO, str(temp_xtc)) + with multiprocessing.Pool(2) as p: + universes = [p.apply(mda.Universe, args) for i in range(ncopies)] assert_equal(universes[0].trajectory._xdr.offsets, universes[1].trajectory._xdr.offsets) From 53b6a3bd40fffdb73f06c6237e5a30220a371e08 Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Sat, 26 Feb 2022 23:20:04 -0700 Subject: [PATCH 24/28] add fasteners to GitHub actions for CI --- .github/actions/setup-deps/action.yaml | 3 +++ 1 file changed, 3 insertions(+) diff --git a/.github/actions/setup-deps/action.yaml b/.github/actions/setup-deps/action.yaml index 4cb45480dbc..61ff81c7f55 100644 --- a/.github/actions/setup-deps/action.yaml +++ b/.github/actions/setup-deps/action.yaml @@ -20,6 +20,8 @@ inputs: default: 'codecov' cython: default: 'cython' + fasteners: + default: 'fasteners' griddataformats: default: 'griddataformats' gsd: @@ -88,6 +90,7 @@ runs: ${{ inputs.biopython }} ${{ inputs.codecov }} ${{ inputs.cython }} + ${{ inputs.fasteners }} ${{ inputs.griddataformats }} ${{ inputs.gsd }} ${{ inputs.hypothesis }} From 4af0ca8e34d0db46cf975228905bceed98f132ff Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Mon, 28 Feb 2022 11:14:50 -0700 Subject: [PATCH 25/28] sorted requirements.txt alphabetically --- package/requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/package/requirements.txt b/package/requirements.txt index 3e676ce5d63..ac2a416fbac 100644 --- a/package/requirements.txt +++ b/package/requirements.txt @@ -1,6 +1,7 @@ biopython codecov cython +fasteners griddataformats gsd hypothesis @@ -21,4 +22,3 @@ sphinx_rtd_theme sphinx_sitemap threadpoolctl tqdm -fasteners From b93e5bf5d12d02b593f5177b5ad4819c63edeccd Mon Sep 17 00:00:00 2001 From: Oliver Beckstein Date: Wed, 2 Mar 2022 16:41:53 -0700 Subject: [PATCH 26/28] update/fix CHANGELOG for PR #3375 - add change: fasteners now core dependency - removed duplicated 'fix' lines --- package/CHANGELOG | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/package/CHANGELOG b/package/CHANGELOG index ae2c4dd09d7..55b03eb0849 100644 --- a/package/CHANGELOG +++ b/package/CHANGELOG @@ -21,7 +21,7 @@ The rules for this file: Fixes * Prevents attempts to close an already closed NamedStream (Issue #3386) - * Added missing exe_err import in hole.py. Fixed a bug in HoleAnalysis where + * Added missing exe_err import in hole.py. Fixed a bug in HoleAnalysis where kwarg was passed instead of object to generate path (Issue #3493, PR #3496) * Fixes creation of vmd surfaces in hole2 when using non-contiguous frames with start/stop/step (Issue #3476) @@ -31,10 +31,6 @@ Fixes * TRRWriter preserves timestep data (Issue #3350). * Fixed Universe creation from a custom object which only provided a topology, previously raised a TypeError. (Issue #3443) - * Fix the competition when generating offsets from multiple processes by - using a interprocess file lock. (Issue #1988, PR #3375) - * Fix the error when trying to load a corrupted offset file (Issue #3230 - PR #3375) * Documentation fixes (Issues #3224, #2667) * Fixed string attributes (e.g. resname, segid) not working on Residues and Segments created using add_Residue and add_Segment (Issue #3437) @@ -54,6 +50,7 @@ Enhancements * Added MDAnalysis.lib.distances.minimize_vectors Changes + * `fasteners` package is now a core dependency (Issues #3230, #1988, PR #3375) * Indexing a group (AtomGroup, ResidueGroup, or SegmentGroup) with None raises a TypeError (Issue #3092, PR #3517) * TRZReader now defaults to a `dt` of 1.0 (instead of 0.0 previously) when From cec7d6bb2a16576e3332bc747ddc66ff3a247d20 Mon Sep 17 00:00:00 2001 From: "yuxuan.zhuang@dbb.su.se" Date: Wed, 6 Apr 2022 12:04:46 +0200 Subject: [PATCH 27/28] use apply_async --- testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py index c1cd10d81e0..4df5c8d0938 100644 --- a/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py +++ b/testsuite/MDAnalysisTests/parallelism/test_multiprocessing.py @@ -150,7 +150,9 @@ def test_creating_multiple_universe_without_offset(temp_xtc, ncopies=3): args = (GRO, str(temp_xtc)) with multiprocessing.Pool(2) as p: - universes = [p.apply(mda.Universe, args) for i in range(ncopies)] + universes = [p.apply_async(mda.Universe, args) for i in range(ncopies)] + universes = [universe.get() for universe in universes] + assert_equal(universes[0].trajectory._xdr.offsets, universes[1].trajectory._xdr.offsets) From 7a2e30b66ffdecb218ceab94c54d653a4accd3c8 Mon Sep 17 00:00:00 2001 From: IAlibay Date: Mon, 16 May 2022 13:26:21 +0100 Subject: [PATCH 28/28] Add fasteners to env file for docs build --- maintainer/conda/environment.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/maintainer/conda/environment.yml b/maintainer/conda/environment.yml index f9756fa77b6..ad3d10da4ed 100644 --- a/maintainer/conda/environment.yml +++ b/maintainer/conda/environment.yml @@ -6,6 +6,7 @@ dependencies: - chemfiles>=0.10 - codecov - cython + - fasteners - griddataformats - gsd - h5py