Transfer of `fasteners` dependency to `filelock` #4800

talagayev · 2024-11-22T23:43:54Z

Fixes #4797

Changes made in this Pull Request:

Removal of fasteners dependency from XDR.py and all installation files
Addition of filelock as an replacement dependency of fasteners

PR Checklist

Tests?
Docs?
CHANGELOG updated?
Issue raised/referenced?

Developers certificate of origin

I certify that this contribution is covered by the LGPLv2.1+ license as defined in our LICENSE and adheres to the Developer Certificate of Origin.

📚 Documentation preview 📚: https://mdanalysis--4800.org.readthedocs.build/en/4800/

changes fasteners to filelock dependency

changes in the action the fasteners dependency to filelock

adjusted input fasteners to filelock

changed fasteners to filelock

codecov · 2024-11-22T23:53:26Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.62%. Comparing base (bdfb2c9) to head (7b580c1).
Report is 1 commits behind head on develop.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #4800      +/-   ##
===========================================
- Coverage    93.66%   93.62%   -0.05%     
===========================================
  Files          177      189      +12     
  Lines        21795    22861    +1066     
  Branches      3067     3067              
===========================================
+ Hits         20414    21403     +989     
- Misses         929     1006      +77     
  Partials       452      452

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

test _read_offsets adjustment

modification of test_offset_lock_created to adjust to new dependency

returned the warning and read_offsets for tests

required removal of asser_equal to not get the error, since now the files, dont't end with lock and the removal of the end would be a duplicate of the other assert_equal

talagayev · 2024-11-23T10:39:45Z

@yuxuanzhuang I would ping you since you worked on the XDR.py PR to get your opinion regarding the lines:

https://github.com/talagayev/mdanalysis/blob/b66e3d21680909f4b4a15379d77602090f879fd0/package/MDAnalysis/coordinates/XDR.py#L204-L206

and if they require modification.

Other than that the only problem that I encounter now is the Azure failing, due to Windows runners gw0 and gw1 crashing during the test_persistent_offsets_readonly, which I currently cannot explain.

changed read offsets from True to False, retained the warning

changed and to or pytest warning in test

talagayev · 2024-11-27T03:48:04Z

@IAlibay I would probably ping you in this PR to get your opinion, due to the problem with test_persistent_offsets_readonly, which has an problem with the Azure pipelines and windows, possibly similar to #4122, can't say how similar that case is to the one that was then, if it was also connected with crashing of runners.

yuxuanzhuang · 2024-12-08T04:53:43Z

The issue is that filelock hangs when attempting to acquire a lock in Windows.

Refer to the following line of code:
https://github.com/tox-dev/filelock/blob/57f488ff8fdc2193572efe102408fb63cfefe4e4/src/filelock/_windows.py#L28

When the lock file cannot be written in Windows, instead of raising a PermissionError, the library keeps retrying to acquire again:
https://github.com/tox-dev/filelock/blob/57f488ff8fdc2193572efe102408fb63cfefe4e4/src/filelock/_api.py#L329

In contrast, on UNIX systems https://github.com/tox-dev/filelock/blob/57f488ff8fdc2193572efe102408fb63cfefe4e4/src/filelock/_unix.py#L42, if the file cannot be written, the operation fails immediately, raising an appropriate exception.

Not sure why platforms are implemented differently though.

test path

test path modification

moved back

add skip exception for azure

pep8speaks · 2024-12-08T22:12:02Z

Hello @talagayev! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

In the file testsuite/MDAnalysisTests/coordinates/test_xdr.py:

Line 990:80: E501 line too long (96 > 79 characters)

Comment last updated at 2025-01-11 15:59:37 UTC

pep

small black adjustments

talagayev · 2025-01-08T22:21:32Z

Sorry for the late reply and not working on the PR, was mainly on holidays and afterwards working on an publication. @yuxuanzhuang I adjusted now the two pytests for test_persistent_offsets_readonly with the mock.patch as you suggested and for test_offset_lock_created it's basically testing if it is locked and then unlocked, tried a couple of versions of this pytest, some led to the file unlocking step taking a lot of time or getting stuck, but this version now works.

Currently the linters / black action fails, thus I ran the parts added through black again, but didn't touch any remainder of test_xdr.py to meddle with the other pytest parts

orbeckst

My comments have been addressed. I have one non-blocking question/suggestion. Thanks!!

orbeckst · 2025-01-08T23:44:19Z

testsuite/MDAnalysisTests/coordinates/test_xdr.py

+        # Acquire the lock
+        lock.acquire()
+
+        time.sleep(5)  # wait 5 seconds before checking if the file is locked


Why did you choose 5 seconds? Could 1s be sufficient — the less time we spend in a test, the better.

Add a comment why delay is needed.

I was debating if to have the sleep overall or not. @yuxuanzhuang suggested 10 seconds, I thought 5 seconds would be the optimum, but yes for tests speed it makes sense to reduce it. Overall now that I see I think that the time.sleep() is overall redundant here, since @yuxuanzhuang meant it in a different sense. So I would tend to remove it overall, since it is not necessary to have it in the test and just makes the test 5 seconds longer

This is not exactly the same test as I suggested.

One possible approach could be to spawn a file lock in a separate process for 10 seconds and then verify that the read_offsets function takes more than 10 seconds to complete, indicating it’s waiting for the lock to be released.

I think it’s fine to revert to the original test that simply verifies the creation of the offset lock file.

The other part of the PR looks good to me.

This is not exactly the same test as I suggested.

One possible approach could be to spawn a file lock in a separate process for 10 seconds and then verify that the read_offsets function takes more than 10 seconds to complete, indicating it’s waiting for the lock to be released.

I think it’s fine to revert to the original test that simply verifies the creation of the offset lock file.

The other part of the PR looks good to me.

@yuxuanzhuang yes somehow I think I tried it first before the holidays with the separate process, but had troubles implementing it, since I had the case that the tests ran indefinetly and after the holidays the idea of the 10 seconds kinda got lost overtime, which is why the pytest looks different than what you had in mind.

As for reverting you mean reverting back to the current version of the test in the develop branch:

mdanalysis/testsuite/MDAnalysisTests/coordinates/test_xdr.py

Lines 1016 to 1019 in 263bbe6

def test_offset_lock_created(self):

assert os.path.exists(

XDR.offsets_filename(self.filename, ending="lock")

)

or any iteration from the previous commits in this PR?

The one in the develop branch.

@yuxuanzhuang hm, now the problem appears again regardin the ending="lock" which is not present and thus leads to an error and by removing the ending="lock" it just checks that the file exists but not that it is locked.

I’m not sure, but the test was originally checking the existence of lock file for self.filename, whereas it should have been testing self.traj instead. The test passed previously more as a side effect of directly reading offsets from self.filename in the other test.

talagayev · 2025-01-08T23:55:34Z

My comments have been addressed. I have one non-blocking question/suggestion. Thanks!!

Nice, thank you @orbeckst, the only thing I am not sure about, would be the linters / black (pull_request) failing, is there something I can do about it?

yuxuanzhuang · 2025-01-09T17:51:13Z

testsuite/MDAnalysisTests/coordinates/test_xdr.py

+        # Acquire the lock
+        lock.acquire()
+
+        time.sleep(5)  # wait 5 seconds before checking if the file is locked


This is not exactly the same test as I suggested.

One possible approach could be to spawn a file lock in a separate process for 10 seconds and then verify that the read_offsets function takes more than 10 seconds to complete, indicating it’s waiting for the lock to be released.

I think it’s fine to revert to the original test that simply verifies the creation of the offset lock file.

The other part of the PR looks good to me.

remove sleep

reverted to pytest in develop

check without lock

return ending="lock"

yuxuanzhuang

It appears that the lock file only exists when it is actively locked on Windows machines.

talagayev · 2025-01-10T23:26:16Z

It appears that the lock file only exists when it is actively locked on Windows machines.

quite an interesting case that it behaves differently on Windows compared to the other systems 🤔

yuxuanzhuang · 2025-01-12T03:31:02Z

@orbeckst Let me know you need to have another look at this PR, otherwise, I think it is good to get merged.

Extra things need to be done:

Once this PR is getting towards merging, please also

raise an issue to update the User Guide in the Contributing Code section, where the explicit package dependencies are listed — and it would be wonderful if you could then start a PR to address the issue!

raise an issue to update the conda-forge feedstock https://github.com/conda-forge/mdanalysis-feedstock/ to change the package dependencies, specifically the recipe meta.yaml.

orbeckst · 2025-01-12T18:47:49Z

All ok from my side.

Please do the follow-up issues.

talagayev · 2025-01-12T20:40:37Z

@yuxuanzhuang I could raise the Issue for the User Guide, but for the conda-forge I think it makes more sense for you as a coredev to raise the Issue, or do you want to raise both or should I raise both Issues?

yuxuanzhuang · 2025-01-12T23:08:19Z

Thanks @talagayev! I will raise issues in both locations.

talagayev · 2025-01-12T23:12:34Z

Thanks @talagayev! I will raise issues in both locations.

Perfect, thank you @yuxuanzhuang with helping with the PR and thank you @orbeckst for looking into the PR :)

talagayev · 2025-01-12T23:24:13Z

@yuxuanzhuang the Userguide Issue would be quite a fast and straightforward PR. The question would be, should I quickly do it tomorrow or would it be more reasonable to keep it for now in case of GSOC, since I have a feeling that there are not so many beginner friendly PRs left for newcomers currently and the parallelization ones that are not tackled are the ones, that I already tried tackling locally and had struggles to find a solution to get them parallelized, so those would probably be complicated for beginners 🤔

IAlibay · 2025-01-13T01:05:45Z

@yuxuanzhuang the Userguide Issue would be quite a fast and straightforward PR. The question would be, should I quickly do it tomorrow or would it be more reasonable to keep it for now in case of GSOC, since I have a feeling that there are not so many beginner friendly PRs left for newcomers currently and the parallelization ones that are not tackled are the ones, that I already tried tackling locally and had struggles to find a solution to get them parallelized, so those would probably be complicated for beginners 🤔

Now please - we shouldn't sit on anything that deal with dependencies, especially when it's a non-optional one.

talagayev · 2025-01-13T09:15:15Z

@yuxuanzhuang the Userguide Issue would be quite a fast and straightforward PR. The question would be, should I quickly do it tomorrow or would it be more reasonable to keep it for now in case of GSOC, since I have a feeling that there are not so many beginner friendly PRs left for newcomers currently and the parallelization ones that are not tackled are the ones, that I already tried tackling locally and had struggles to find a solution to get them parallelized, so those would probably be complicated for beginners 🤔

Now please - we shouldn't sit on anything that deal with dependencies, especially when it's a non-optional one.

Ah I see @yuxuanzhuang already created a PR for it, so all good :)

talagayev added 7 commits November 23, 2024 00:30

Update pyproject.toml

0d02218

changes fasteners to filelock dependency

Update requirements.txt

833f58b

changes fasteners to filelock dependency

Update action.yaml

0e45f80

changes in the action the fasteners dependency to filelock

Update action.yaml

0505cc5

adjusted input fasteners to filelock

Update environment.yml

ea5a61c

changed fasteners to filelock

Update azure-pipelines.yml

40251b6

changed fasteners to filelock

Update XDR.py

c98a11f

changed fasteners to filelock

talagayev changed the title ~~Transfer of fasteners dependency to fielock~~ Transfer of fasteners dependency to fielock Nov 22, 2024

talagayev added 4 commits November 23, 2024 01:49

Update XDR.py

97927bb

test _read_offsets adjustment

Update test_xdr.py

f8f05f2

modification of test_offset_lock_created to adjust to new dependency

Update XDR.py

3cd7307

returned the warning and read_offsets for tests

Update test_xdr.py

b66e3d2

required removal of asser_equal to not get the error, since now the files, dont't end with lock and the removal of the end would be a duplicate of the other assert_equal

orbeckst changed the title ~~Transfer of fasteners dependency to fielock~~ Transfer of fasteners dependency to filelock Nov 25, 2024

orbeckst requested a review from yuxuanzhuang November 25, 2024 20:12

talagayev added 3 commits November 26, 2024 13:04

Update XDR.py

e71943f

changed read offsets from True to False, retained the warning

Update test_xdr.py

ce3656a

changed and to or pytest warning in test

Merge branch 'MDAnalysis:develop' into fasteners_to_filelock

aa9f83f

talagayev added 4 commits December 8, 2024 20:02

Update test_xdr.py

307027b

test path

Update test_xdr.py

77abba0

test path modification

Update test_xdr.py

fc52a18

moved back

Update test_xdr.py

e1fe6fb

add skip exception for azure

talagayev added 4 commits December 8, 2024 23:43

Update test_xdr.py

fbcd060

Update test_xdr.py

9c2ed87

Update test_xdr.py

2d3cf13

Update test_xdr.py

78b61e9

talagayev added 3 commits January 8, 2025 21:12

Update test_xdr.py

73e1d03

pep

Update test_xdr.py

a6bb5fe

Update test_xdr.py

a5d0f3a

small black adjustments

orbeckst approved these changes Jan 8, 2025

View reviewed changes

yuxuanzhuang requested changes Jan 9, 2025

View reviewed changes

talagayev and others added 9 commits January 9, 2025 20:25

Update test_xdr.py

fe59638

remove sleep

Update test_xdr.py

d1df5ac

reverted to pytest in develop

Update test_xdr.py

8bb1c77

check without lock

Update test_xdr.py

42539bd

return ending="lock"

check lock file exists for traj

3c5cb66

use mock to test cannot write offsets

2fb9b69

Merge branch 'develop' into fasteners_to_filelock

40d09cc

skip lock file test for windows

9b8209a

import sys

fd537cc

yuxuanzhuang approved these changes Jan 10, 2025

View reviewed changes

test filelock permission error

7b580c1

yuxuanzhuang merged commit 6842fd7 into MDAnalysis:develop Jan 12, 2025
22 of 24 checks passed

yuxuanzhuang mentioned this pull request Jan 12, 2025

Replace fasteners dependency to filelock MDAnalysis/UserGuide#400

Open

talagayev deleted the fasteners_to_filelock branch January 12, 2025 23:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Transfer of `fasteners` dependency to `filelock` #4800

Transfer of `fasteners` dependency to `filelock` #4800

talagayev commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

talagayev commented Nov 23, 2024

talagayev commented Nov 27, 2024

yuxuanzhuang commented Dec 8, 2024

pep8speaks commented Dec 8, 2024 •

edited

Loading

talagayev commented Jan 8, 2025

orbeckst left a comment

orbeckst Jan 8, 2025

talagayev Jan 8, 2025

yuxuanzhuang Jan 9, 2025

talagayev Jan 9, 2025

yuxuanzhuang Jan 9, 2025

talagayev Jan 9, 2025

yuxuanzhuang Jan 10, 2025

talagayev commented Jan 8, 2025

yuxuanzhuang Jan 9, 2025

yuxuanzhuang left a comment

talagayev commented Jan 10, 2025

yuxuanzhuang commented Jan 12, 2025

orbeckst commented Jan 12, 2025

talagayev commented Jan 12, 2025

yuxuanzhuang commented Jan 12, 2025

talagayev commented Jan 12, 2025

talagayev commented Jan 12, 2025

IAlibay commented Jan 13, 2025

talagayev commented Jan 13, 2025

	def test_offset_lock_created(self):
	assert os.path.exists(
	XDR.offsets_filename(self.filename, ending="lock")
	)

Transfer of fasteners dependency to filelock #4800

Transfer of fasteners dependency to filelock #4800

Conversation

talagayev commented Nov 22, 2024 • edited Loading

PR Checklist

Developers certificate of origin

codecov bot commented Nov 22, 2024 • edited Loading

Codecov Report

talagayev commented Nov 23, 2024

talagayev commented Nov 27, 2024

yuxuanzhuang commented Dec 8, 2024

pep8speaks commented Dec 8, 2024 • edited Loading

Comment last updated at 2025-01-11 15:59:37 UTC

talagayev commented Jan 8, 2025

orbeckst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

talagayev commented Jan 8, 2025

Choose a reason for hiding this comment

yuxuanzhuang left a comment

Choose a reason for hiding this comment

talagayev commented Jan 10, 2025

yuxuanzhuang commented Jan 12, 2025

orbeckst commented Jan 12, 2025

talagayev commented Jan 12, 2025

yuxuanzhuang commented Jan 12, 2025

talagayev commented Jan 12, 2025

talagayev commented Jan 12, 2025

IAlibay commented Jan 13, 2025

talagayev commented Jan 13, 2025

Transfer of `fasteners` dependency to `filelock` #4800

Transfer of `fasteners` dependency to `filelock` #4800

talagayev commented Nov 22, 2024 •

edited

Loading

codecov bot commented Nov 22, 2024 •

edited

Loading

pep8speaks commented Dec 8, 2024 •

edited

Loading