change XTC/TRR tests for lock file presence (#5423)

* fix #5382
* change XTC/TRR tests for now persistent lock file
  - filelock >= 3.29.5 will now never remove the lockfile (avoids a race condition on POSIX):
    changed the correspond test (xfail for older versions of fielock)
* filelock also apparently changed behavior on Windows: it used to leave the lock file around but now it 
  seems to be removing it (based on the windows runners): 
  - changed the test to account for this new behavior
* docs updates for offsets
  - updated doc string XDRBaseReader._load_offsets to indicate that we use filelock
  - added more docs to XDR page (more details on offsets)
  - add filelock docs to intersphinx mapping
* cleaned up lockfile context managers (follow their best-practice docs)
* keep long string in sphinx conf.py
  - exempt from black
  - exempt from flake8
  - shortened other offending lines
* update CHANGELOG

Author: orbeckst

package/CHANGELOG

@@ -23,6 +23,7 @@ The rules for this file:
  * 2.11.0
 
 Fixes
+ * Fix FileLock tests for XTC and TRR: lock file is no longer removed (#5382)
  * InterRDF now correctly returns bins in parallel (PR #5344)
  * `Merge()` no longer raises a TypeError on Universes that have a `cmaps`
    attribute; cmaps are now combined like the other connection attributes

package/MDAnalysis/coordinates/XDR.py

@@ -32,13 +32,42 @@
 MDAnalysis.coordinates.XTC: Read and write GROMACS XTC trajectory files.
 MDAnalysis.coordinates.TRR: Read and write GROMACS TRR trajectory files.
 MDAnalysis.lib.formats.libmdaxdr: Low level xdr format reader
+
+
+XDR reader class
+----------------
+
+The :class:`XDRBaseReader` contains common functionality for the TRR and XTC
+reader for GROMACS files, which are implemented in the
+:mod:`MDAnalysis.lib.formats.libmdaxdr` module.
+
+Both formats have in common that they do not allow
+native random frame access. Therefore, we first scan the whole trajectory to
+build an index of frames in the file ("offsets") as a look-up for seeking to
+frames. This process is initially slow so we save the offsets to a hidden file
+next to the trajectory (if possible) and then read the offset file when the
+trajectory is opened the next time, as described under :ref:`Offsets<offsets-label>`.
+
+.. autoclass:: XDRBaseReader
+   :members:
+   :inherited-members:
+   :private-members:
+
+
+Functions
+---------
+
+.. autofunction:: offsets_filename
+
+.. autofunction:: read_numpy_offsets
+
 """
 
 import errno
 import numpy as np
 from os.path import getctime, getsize, isfile, split, join
 import warnings
-from filelock import FileLock
+import filelock
 
 from . import base
 from ..lib.mdamath import triclinic_box
@@ -95,27 +124,30 @@ class XDRBaseReader(base.ReaderBase):
     """Base class for libmdaxdr file formats xtc and trr
 
     This class handles integration of XDR based formats into MDAnalysis. The
-    XTC and TRR classes only implement `_write_next_frame` and
-    `_frame_to_ts`.
+    XTC and TRR classes only implement :meth:`_write_next_frame` and
+    :meth:`_frame_to_ts`.
 
     .. _offsets-label:
 
     Notes
     -----
     XDR based readers store persistent offsets on disk. The offsets are used to
     enable access to random frames efficiently. These offsets will be generated
-    automatically the  first time the  trajectory is opened.  Generally offsets
-    are stored  in hidden  `*_offsets.npz` files.  Afterwards opening  the same
+    automatically the first time the trajectory is opened.  Generally offsets
+    are stored in hidden ``*_offsets.npz`` files.  Afterwards opening the same
     file again is fast. It sometimes can happen that the stored offsets get out
     off sync with the trajectory they refer to. For this the offsets also store
     the number of atoms, size of the file and last modification time. If any of
-    them change  the offsets are recalculated.  Writing of the offset  file can
-    fail when the  directory where the trajectory file resides  is not writable
-    or if the  disk is full. In this  case a warning message will  be shown but
+    them change the offsets are recalculated.  Writing of the offset file can
+    fail when the directory where the trajectory file resides is not writable
+    or if the disk is full. In this case a warning message will be shown but
     the offsets will nevertheless be used during the lifetime of the trajectory
-    Reader. However, the  next time the trajectory is opened,  the offsets will
+    Reader. However, the next time the trajectory is opened, the offsets will
     have to be rebuilt again.
 
+    See :meth:`_load_offsets` for further details.
+
+
     .. versionchanged:: 1.0.0
        XDR offsets read from trajectory if offsets file read-in fails
     .. versionchanged:: 2.0.0
@@ -124,6 +156,7 @@ class XDRBaseReader(base.ReaderBase):
        Use a direct read into ts attributes
     .. versionchanged:: 2.9.0
        Changed fasteners.InterProcessLock() to filelock.FileLock
+
     """
 
     @store_init_arguments
@@ -204,15 +237,52 @@ def close(self):
         self._xdr.close()
 
     def _load_offsets(self):
-        """load frame offsets from file, reread them from the trajectory if that
-        fails. To prevent the competition of generating the same offset file
-        from multiple processes, an `InterProcessLock` is used."""
+        """load frame offsets from file or recalculate if necessary
+
+        Frame offsets are cached in an offsets file, which is stored as a
+        hidden file in the same directory as the trajectory. If the file does
+        not exist we generate the offsets and store them.
+
+        If the data in the offset file are outdated (older than the trajectory
+        file or different number of frames from the trajectory or different
+        file size) then the offset file is also regenerated.
+
+        .. Note::
+
+           Generating offsets can take minutes for large trajectories because
+           the whole file must be scanned. During this time, code appears to
+           hang.
+
+        You can force regenerating offsets with the `refresh_offsets` keyword
+        argument for :class:`~MDAnalysis.core.universe.Universe`, for
+        example,::
+
+           u = mda.Universe(TOPOLOGY, XTC, refresh_offsets=True)
+
+        To prevent the competition of generating the same offset file from
+        multiple processes, a :attr:`filelock.FileLock` is used, which is
+        implemented via a lock file (in the same directory as the offset file
+        and ending in ".lock"). This lock file is *not* automatically deleted
+        because doing so could lead to race conditions.
+
+        Once this method completes, the
+        :attr:`~MDAnalysis.lib.formats.libmdaxdr.XTCFile.offsets` attribute of
+        the underlying reader contains current offsets for the trajectory.
+
+
+        .. SeeAlso::
+           - :func:`offsets_filename`
+           - :meth:`_read_offsets`
+           - :func:`read_numpy_offsets`
+
+        """
         fname = offsets_filename(self.filename)
         lock_name = offsets_filename(self.filename, ending="lock")
 
         #  check if the location of the lock is writable.
+        lock = filelock.FileLock(lock_name)
         try:
-            with FileLock(lock_name) as filelock:
+            with lock:
                 pass
         except OSError as e:
             if isinstance(e, PermissionError) or e.errno == errno.EROFS:
@@ -225,7 +295,7 @@ def _load_offsets(self):
             else:
                 raise
 
-        with FileLock(lock_name) as filelock:
+        with lock:
             if not isfile(fname):
                 self._read_offsets(store=True)
                 return
@@ -267,7 +337,24 @@ def _load_offsets(self):
                 self._xdr.set_offsets(data["offsets"])
 
     def _read_offsets(self, store=False):
-        """read frame offsets from trajectory"""
+        """read frame offsets from trajectory
+
+        Scan the trajectory for frames and build an index that relates frame
+        number to the position in the file, thus enabling direct seeking to
+        specific frames. The trajectory scan can take minutes for large
+        trajectories.
+
+        Parameters
+        ----------
+        store : bool
+            Save the frame index ("offsets") to a file with name generated from
+            the trajectory name (:attr:`filename`) with function
+            :func:`offsets_filename`. The offsets file also contains, ctime,
+            file size, number of frames, and number of atoms of the trajectory.
+            The file format is a compressed numpy array (:func:`numpy.savez`).
+
+            If saving the file fails for any reasons, only a warning is issued.
+        """
         fname = offsets_filename(self.filename)
         offsets = self._xdr.offsets
         if store:

package/doc/sphinx/source/conf.py

@@ -3,7 +3,8 @@
 # MDAnalysis documentation build configuration file, created by
 # sphinx-quickstart on Mon Sep 27 09:39:55 2010.
 #
-# This file is execfile()d with the current directory set to its containing dir.
+# This file is execfile()d with the current directory set to its containing
+# dir.
 #
 # Note that not all possible configuration values are present in this
 # autogenerated file.
@@ -29,13 +30,13 @@
 # make sure sphinx always uses the current branch
 sys.path.insert(0, os.path.abspath("../../.."))
 
-# -- General configuration -----------------------------------------------------
+# -- General configuration ----------------------------------------------------
 
 # If your documentation needs a minimal Sphinx version, state it here.
 # needs_sphinx = '1.0'
 
-# Add any Sphinx extension module names here, as strings. They can be extensions
-# coming with Sphinx (named 'sphinx.ext.*') or your custom ones.
+# Add any Sphinx extension module names here, as strings. They can be
+# extensions coming with Sphinx (named 'sphinx.ext.*') or your custom ones.
 extensions = [
     "sphinx.ext.autodoc",
     "sphinx.ext.intersphinx",
@@ -70,7 +71,8 @@ class KeyStyle(UnsrtStyle):
 register_plugin("pybtex.style.labels", "keylabel", KeyLabelStyle)
 register_plugin("pybtex.style.formatting", "MDA", KeyStyle)
 
-mathjax_path = "https://cdnjs.cloudflare.com/ajax/libs/mathjax/2.7.0/MathJax.js?config=TeX-AMS-MML_HTMLorMML"
+
+mathjax_path = "https://cdnjs.cloudflare.com/ajax/libs/mathjax/2.7.0/MathJax.js?config=TeX-AMS-MML_HTMLorMML"  # noqa: E501; fmt: skip
 
 # for sitemap with https://github.com/jdillard/sphinx-sitemap
 # This sitemap is correct both for the development and release docs, which
@@ -96,8 +98,8 @@ class KeyStyle(UnsrtStyle):
 
 # General information about the project.
 # (take the list from AUTHORS)
-# Ordering: (1) Naveen (2) Elizabeth, then all contributors in alphabetical order
-#           (last) Oliver
+# Ordering: (1) Naveen (2) Elizabeth, then all contributors in alphabetical
+#           order (last) Oliver
 author_list = mda.__authors__
 authors = ", ".join(author_list[:-1]) + ", and " + author_list[-1]
 project = "MDAnalysis"
@@ -130,7 +132,8 @@ class KeyStyle(UnsrtStyle):
 # directories to ignore when looking for source files.
 exclude_patterns = ["_build"]
 
-# The reST default role (used for this markup: `text`) to use for all documents.
+# The reST default role (used for this markup: `text`) to use for all
+# documents.
 # default_role = None
 
 # If true, '()' will be appended to :func: etc. cross-reference text.
@@ -156,7 +159,7 @@ class KeyStyle(UnsrtStyle):
 # to prevent including of member entries in toctree
 toc_object_entries = False
 
-# -- Options for HTML output ---------------------------------------------------
+# -- Options for HTML output --------------------------------------------------
 
 # The theme to use for HTML and HTML Help pages.  See the documentation for
 # a list of builtin themes.
@@ -246,7 +249,7 @@ class KeyStyle(UnsrtStyle):
 htmlhelp_basename = "MDAnalysisdoc"
 
 
-# -- Options for LaTeX output --------------------------------------------------
+# -- Options for LaTeX output -------------------------------------------------
 
 # The paper size ('letter' or 'a4').
 # latex_paper_size = 'letter'
@@ -255,7 +258,8 @@ class KeyStyle(UnsrtStyle):
 # latex_font_size = '10pt'
 
 # Grouping the document tree into LaTeX files. List of tuples
-# (source start file, target name, title, author, documentclass [howto/manual]).
+# (source start file, target name, title, author, documentclass
+# [howto/manual]).
 latex_documents = [
     ("MDAnalysis.tex", "MDAnalysis Documentation", authors, "manual"),
 ]
@@ -284,14 +288,14 @@ class KeyStyle(UnsrtStyle):
 # latex_domain_indices = True
 
 
-# -- Options for manual page output --------------------------------------------
+# -- Options for manual page output -------------------------------------------
 
 # One entry per manual page. List of tuples
 # (source start file, name, description, authors, manual section).
 man_pages = [("mdanalysis", "MDAnalysis Documentation", [authors], 1)]
 
 
-# -- Options for Epub output ---------------------------------------------------
+# -- Options for Epub output --------------------------------------------------
 
 # Bibliographic Dublin Core info.
 epub_title = "MDAnalysis"
@@ -352,4 +356,5 @@ class KeyStyle(UnsrtStyle):
     "imdclient": ("https://imdclient.readthedocs.io/en/stable/", None),
     "pooch": ("https://www.fatiando.org/pooch/latest/", None),
     "requests": ("https://requests.readthedocs.io/en/latest/", None),
+    "filelock": ("https://py-filelock.readthedocs.io/en/latest/", None),
 }

package/doc/sphinx/source/documentation_pages/coordinates/XDR.rst

@@ -1,3 +1 @@
 .. automodule:: MDAnalysis.coordinates.XDR
-   :members:
-   :inherited-members:

testsuite/MDAnalysisTests/coordinates/test_xdr.py

@@ -22,12 +22,13 @@
 #
 import pytest
 from unittest.mock import patch
+from packaging.version import Version
 
 import re
 import os
 import shutil
 import sys
-from filelock import FileLock
+import filelock
 from pathlib import Path
 
 import numpy as np
@@ -1028,7 +1029,7 @@ def test_persistent_offsets_readonly(self, tmpdir, trajectory):
         ref_offset = trajectory._xdr.offsets
         # Mock filelock acquire to raise an error
         with patch.object(
-            FileLock, "acquire", side_effect=PermissionError
+            filelock.FileLock, "acquire", side_effect=PermissionError
         ):  # Simulate failure
             with pytest.warns(UserWarning, match="Cannot write lock"):
                 reader = self._reader(filename)
@@ -1048,21 +1049,32 @@ def test_persistent_offsets_readonly(self, tmpdir, trajectory):
             False,
         )
 
+    @pytest.mark.xfail(
+        Version(filelock.__version__) < Version("3.29.5"),
+        reason="unsecure version of filelock",
+    )
     def test_offset_lock_created(self):
         lock_file_path = XDR.offsets_filename(self.filename, ending="lock")
 
-        with FileLock(lock_file_path) as lock:
+        with filelock.FileLock(lock_file_path) as lock:
             # Lock acquired in context manager, so lock file should exist
             assert lock.is_locked
             assert os.path.exists(lock_file_path)
 
-            # Explicitly release lock, file should be deleted on UNIX
-            lock.release()
-            assert not lock.is_locked
-            if not sys.platform.startswith("win"):
-                # As of filelock>=3.21.0, filelock explicitly deletes lockfile
-                # upon release on UNIX. filelock does not do that on windows.
-                assert not os.path.exists(lock_file_path)
+        # released lock
+        assert not lock.is_locked
+
+        # validate the expected behavior of how filelock handles lock files for
+        # released locks:
+        if sys.platform.startswith("win"):
+            # on Windows, the lockfile remained (filelock ~3.21.0) but recent
+            # versions (at least 3.29.7) appear to remove the lockfile
+            assert not os.path.exists(lock_file_path)
+        else:
+            # for filelock>=3.21.0,<3.29.5, the lockfile was deleted on POSIX,
+            # but this can lead to race conditions. Secure versions of filelock
+            # keep the lockfile (see GH tox-dev/filelock#574)
+            assert os.path.exists(lock_file_path)
 
 
 class TestXTCReader_offsets(_GromacsReader_offsets):