change XTC/TRR tests for lock file presence

[orbeckst]

Fixes #5382

Changes made in this Pull Request:

• 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 filelock)
• 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
• 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

LLM / AI generated code disclosure

LLMs or other AI-powered tools (beyond simple IDE use cases) were used in this contribution: no

PR Checklist

• Issue raised/referenced?
• Tests updated/added?
• Documentation updated/added?
• package/CHANGELOG file updated?
• Is your name in package/AUTHORS? (If it is not, add it!)
• LLM/AI disclosure was updated.

Developers Certificate of Origin

I certify that I can submit this code contribution as described in the Developer Certificate of Origin, under the MDAnalysis LICENSE.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.86%. Comparing base (32b7808) to head (e495ba5).
⚠️ Report is 10 commits behind head on develop.

Additional details and impacted files

@@           Coverage Diff            @@
##           develop    #5423   +/-   ##
========================================
  Coverage    93.85%   93.86%           
========================================
  Files          182      182           
  Lines        22509    22510    +1     
  Branches      3202     3202           
========================================
+ Hits         21125    21128    +3     
+ Misses         922      921    -1     
+ Partials       462      461    -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[orbeckst]

Not sure what the linter is complaining about. Pre-commit (with black 24.10) was happy.

The feedback from linters/black is close to useless to me as it just says "2 files would be reformatted, 218 files would be left unchanged." without telling me which one. (And please don't make me read the list of 200 files that it touched to find the one that does not say "already well formatted, good job."...).

[orbeckst]

It looks as if on Win, the lock file is being removed (?!). https://dev.azure.com/mdanalysis/mdanalysis/_build/results?buildId=9343&view=logs&j=dc60759d-7bf7-580f-541c-8b624e38610f&t=c864e5cd-d904-5b0e-978b-27ea9778a5fc&l=843

_______________ TestXTCReader_offsets.test_offset_lock_created ________________
[gw1] win32 -- Python 3.11.9 C:\hostedtoolcache\windows\Python\3.11.9\x64\python.exe

self = <MDAnalysisTests.coordinates.test_xdr.TestXTCReader_offsets object at 0x00000214C33F2550>

    @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.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)
    
        # released lock
        assert not lock.is_locked
    
        # 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:
>       assert os.path.exists(lock_file_path)
E       AssertionError: assert False
E        +  where False = <function exists at 0x00000214A835C220>('D:/a/1/s/testsuite/MDAnalysisTests/data\\.adk_oplsaa.xtc_offsets.lock')
E        +    where <function exists at 0x00000214A835C220> = <module 'ntpath' (frozen)>.exists
E        +      where <module 'ntpath' (frozen)> = os.path

D:\a\1\s\testsuite\MDAnalysisTests\coordinates\test_xdr.py:1070: AssertionError

[read-the-docs-community]

Documentation build overview

📚 MDAnalysis | 🛠️ Build #33671639 | 📁 Comparing e495ba5 against latest (c1b821b)

  🔍 Preview build  

10 files changed · ± 10 modified

± Modified

• genindex.html
• index.html
• documentation_pages/analysis/dihedrals.html
• documentation_pages/analysis/rms.html
• documentation_pages/analysis/wbridge_analysis.html
• documentation_pages/coordinates/XDR.html
• documentation_pages/lib/util.html
• _modules/MDAnalysis/analysis/rdf.html
• _modules/MDAnalysis/coordinates/XDR.html
• _modules/MDAnalysis/core/universe.html

[orbeckst]

We might want to consider switching from FileLock to SoftFileLock because the latter works on NFS, which is typical in HPC environments. See eg https://py-filelock.readthedocs.io/en/latest/concepts.html#what-filelock-doesn-t-protect-against

I am happy to postpone this decision, though, and just get this PR merged so that our CI passes reliably again.

[RMeli]

The feedback from linters/black is close to useless to me as it just says "2 files would be reformatted, 218 files would be left unchanged." without telling me which one. (And please don't make me read the list of 200 files that it touched to find the one that does not say "already well formatted, good job."...).

Good point, I'll see if this can be improved.

[RMeli]

Are the timeouts expected?

[RMeli]

/azp run

[azure-pipelines]

Azure Pipelines successfully started running 1 pipeline(s).

[IAlibay]

Are the timeouts expected?

Seems like a consistent failure with the IMD tests, which is odd given that it's meant to all be accessing localhost only.

[bitflicker64]

hey, took a look at this and ported the diff onto a fork to re-run the lock tests under current filelock

fork PR with the same XDR/test changes: bitflicker64#1

focused run on test_xdr offsets/lock tests with filelock 3.31.0 - both TestXTCReader_offsets::test_offset_lock_created and the TRR twin passed (29 related tests green):
https://github.com/bitflicker64/mdanalysis/actions/runs/29683554932

the POSIX keep-lockfile + Windows delete-on-release split matches what filelock does now (tox-dev/filelock#577 / >=3.29.5), and the xfail for older versions makes sense. docs / intersphinx / CHANGELOG look fine from here

on the remaining red CI here: the timeouts @RMeli / @IAlibay flagged look like IMD + imdclient 0.2.4 (default timeout went 5→600), not this FileLock change. suite doesn’t use pytest-flaky either - only pytest-timeout + xdist - and these read more like hard hangs that burn the full --timeout=200 than intermittent flakes. happy to dig on that separately if useful

softfilelock/NFS as a follow-up also seems right rather than blocking this

cc @orbeckst @IAlibay @yuxuanzhuang

[orbeckst]

@IAlibay the latest 0.2.4 release of imdclient ships with an increased timeout (issue Becksteinlab/imdclient#111 , PR Becksteinlab/imdclient#120 ). I am going to quickly raise an issue.

[orbeckst]

@yuxuanzhuang @IAlibay @RMeli could you please review this PR? The imdclient issue is being addressed in PR #5443 . Thank you.

[IAlibay]

Before I even review this PR - @orbeckst this PR is in direct contradiction to our own AI policy. It's unclear to me how we should proceed.

At the very minimum, please provide details on what parts of the code being pushed is AI generated (and how).

[orbeckst]

The yes is a mistake. It should be no — I just made a mistake when I put up the PR. I thought I had edited it but apparently hadn't. Will do now.

[IAlibay]

Just the one thing, otherwise lgtm

[IAlibay]

I don't think splitting a URL up is beneficial to understanding. Please revert the black changeand add a fmt: skip?

[orbeckst]

Ok, can do; the linter was unhappy with the original long line. I hadn’t considered locally exempting format checks.

[orbeckst]

fixed — and now the CI is all 💚

[yuxuanzhuang]

lgtm!