Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Transfer of fasteners dependency to filelock #4800

Open
wants to merge 26 commits into
base: develop
Choose a base branch
from

Conversation

talagayev
Copy link
Member

@talagayev talagayev commented Nov 22, 2024

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


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

changes fasteners to filelock dependency
changes fasteners to filelock dependency
changes in the action the fasteners dependency to filelock
adjusted input fasteners to filelock
changed fasteners to filelock
changed fasteners to filelock
changed fasteners to filelock
@talagayev talagayev changed the title Transfer of fasteners dependency to fielock Transfer of fasteners dependency to fielock Nov 22, 2024
Copy link

codecov bot commented Nov 22, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 93.65%. Comparing base (a27591c) to head (33c0ea4).

Additional details and impacted files
@@             Coverage Diff             @@
##           develop    #4800      +/-   ##
===========================================
- Coverage    93.68%   93.65%   -0.03%     
===========================================
  Files          177      189      +12     
  Lines        21743    22809    +1066     
  Branches      3055     3055              
===========================================
+ Hits         20370    21362     +992     
- Misses         927     1002      +75     
+ Partials       446      445       -1     

☔ 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
Copy link
Member Author

@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.

@orbeckst orbeckst changed the title Transfer of fasteners dependency to fielock Transfer of fasteners dependency to filelock Nov 25, 2024
changed read offsets from True to False, retained the warning
changed and to or pytest warning in test
@talagayev
Copy link
Member Author

@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
Copy link
Contributor

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 modification
add skip exception for azure
@pep8speaks
Copy link

pep8speaks commented Dec 8, 2024

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

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2024-12-12 22:30:32 UTC

@talagayev
Copy link
Member Author

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.

Ah that makes sense, but yes strange that it is structured that way.
I assume the options would be to modfiy the pytest or make an exception for azure windows, which I tried
to apply now and then it works fine, due to the exception for this specific pytest and azure windows.

As for running it on windows, locally it works fine on my PC and it mainly appears only for azure windows, but could
be connected to the different permissions maybe.

@talagayev talagayev marked this pull request as ready for review December 9, 2024 12:08
@orbeckst
Copy link
Member

@yuxuanzhuang would you be able to manage this PR?

You can recruit yourself as a reviewer or see that you can get someone else to look.

If you don't have the bandwidth please un-assign yourself and let me know. Thanks.

@orbeckst
Copy link
Member

@talagayev you definitely also need a CHANGELOG entry.

@talagayev
Copy link
Member Author

@talagayev you definitely also need a CHANGELOG entry.

Ah yes, forgot about that, since it was in the draft and I didn't adjust the CHANGELOG in the draft. will do it now :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

replace fasteners
4 participants