Fixed lockfile check when using a Docker read-only filesystem #3832

jfennick · 2022-09-16T19:36:11Z

Fixes #

Changes made in this Pull Request:

Added OSError in addition to PermissionError

PR Checklist

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

github-actions

Hello there first time contributor! Welcome to the MDAnalysis community! We ask that all contributors abide by our Code of Conduct and that first time contributors introduce themselves on the developer mailing list so we can get to know you. You can learn more about participating here. Please also add yourself to package/AUTHORS as part of this PR.

codecov · 2022-09-16T19:55:00Z

Codecov Report

Base: 94.32% // Head: 94.31% // Decreases project coverage by -0.00% ⚠️

Coverage data is based on head (5b5ec9d) compared to base (463e52c).
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files

@@             Coverage Diff             @@
##           develop    #3832      +/-   ##
===========================================
- Coverage    94.32%   94.31%   -0.01%     
===========================================
  Files          194      194              
  Lines        25062    25064       +2     
  Branches      3390     3391       +1     
===========================================
+ Hits         23639    23640       +1     
- Misses        1374     1375       +1     
  Partials        49       49

Impacted Files	Coverage Δ
package/MDAnalysis/coordinates/XDR.py	`99.29% <83.33%> (-0.71%)`	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

IAlibay · 2022-09-16T22:26:17Z

Thanks @jfennick - since there are no related issues and it'll be hard to test the error, could you explain in more details what was the cause for this fix / any code/docker env details to duplicate the issue?

jfennick · 2022-09-16T22:43:32Z

Sure. Here is the relevant command line + stack trace from my log file.

/Users/jakefennick/workflow_inference_compiler/cachedir/fe47d299645b0f71eff2e8cbf791dcf9$ docker \
    run \
    -i \
    --mount=type=bind,source=/Users/jakefennick/workflow_inference_compiler/cachedir/fe47d299645b0f71eff2e8cbf791dcf9,target=/RvczZO \
    --mount=type=bind,source=/private/tmp/docker_tmpyrn_kcgl,target=/tmp \
    --mount=type=bind,source=/Users/jakefennick/workflow_inference_compiler/cachedir/1b588cb52801df5be9d783bad2d2cb63/npt.gro,target=/var/lib/cwl/stgdce8320f-73ef-4a3a-8db9-ea43afcfd074/npt.gro,readonly \
    --mount=type=bind,source=/Users/jakefennick/workflow_inference_compiler/cachedir/ae541201128048b4defdf1875c5dfa42/prod.trr,target=/var/lib/cwl/stg7d3f8150-98a4-4051-aed6-3f480f6cf22a/prod.trr,readonly \
    --workdir=/RvczZO \
    --read-only=true \
    --user=502:20 \
    --rm \
    --cidfile=/private/tmp/docker_tmp_n10lmap/20220916115735-218731.cid \
    --env=TMPDIR=/tmp \
    --env=HOME=/RvczZO \
    jakefennick/scripts \
    python3 \
    /align_protein_CA_mda.py \
    /var/lib/cwl/stgdce8320f-73ef-4a3a-8db9-ea43afcfd074/npt.gro \
    /var/lib/cwl/stg7d3f8150-98a4-4051-aed6-3f480f6cf22a/prod.trr \
    prod_align_protein_CA.trr
<Universe with 40893 atoms>
Traceback (most recent call last):
  File "/align_protein_CA_mda.py", line 15, in <module>
    trr.trajectory = TRR.TRRReader(input_trr_path) # This loads all frames.
  File "/miniconda/lib/python3.8/site-packages/MDAnalysis/lib/util.py", line 2495, in wrapper
    return func(self, *args, **kwargs)
  File "/miniconda/lib/python3.8/site-packages/MDAnalysis/coordinates/XDR.py", line 157, in __init__
    self._load_offsets()
  File "/miniconda/lib/python3.8/site-packages/MDAnalysis/coordinates/XDR.py", line 198, in _load_offsets
    with fasteners.InterProcessLock(lock_name) as filelock:
  File "/miniconda/lib/python3.8/site-packages/fasteners/process_lock.py", line 157, in __enter__
    gotten = self.acquire()
  File "/miniconda/lib/python3.8/site-packages/fasteners/process_lock.py", line 135, in acquire
    self._do_open()
  File "/miniconda/lib/python3.8/site-packages/fasteners/process_lock.py", line 107, in _do_open
    self.lockfile = open(self.path, 'a')
OSError: [Errno 30] Read-only file system: b'/var/lib/cwl/stg7d3f8150-98a4-4051-aed6-3f480f6cf22a/.prod.trr_offsets.lock'

For purposes of reproducibility/distribution with Common Workflow Language scripts, I am embedding align_protein_CA_mda.py within my Docker image jakefennick/scripts. My script works fine outside of Docker, but a Docker image is of course a read-only filesystem which causes MDAnalysis to raise an OSError, not a PermissionError.

For testing purposes, align_protein_CA_mda.py is rather trivial and should work with any standard gro and trr files.

jfennick · 2022-09-16T22:49:34Z

I guess I should clarify that cwltool automatically generates the above command and it includes the --read-only=true flag.

tylerjereddy · 2022-09-17T02:10:10Z

package/MDAnalysis/coordinates/XDR.py

@@ -197,7 +197,7 @@ def _load_offsets(self):
        try:
            with fasteners.InterProcessLock(lock_name) as filelock:
                pass
-        except PermissionError:
+        except (PermissionError, OSError):


A reproducer for the original handled PermissionError is straightforward, like below, I wonder if we could cook something up for the other error? May not be worth much energy though.

--- a/testsuite/MDAnalysisTests/coordinates/test_xdr.py +++ b/testsuite/MDAnalysisTests/coordinates/test_xdr.py @@ -904,3 +904,14 @@ class TestTRRReader_offsets(_GromacsReader_offsets): 9155712, 10300176 ]) _reader = mda.coordinates.TRR.TRRReader + + +def test_offset_readonly_handle(tmpdir): + with tmpdir.as_cwd(): + curr_path = os.getcwd() + new_gro = os.path.join(curr_path, os.path.basename(GRO)) + new_xtc = os.path.join(curr_path, os.path.basename(XTC)) + shutil.copyfile(GRO, new_gro) + shutil.copyfile(XTC, new_xtc) + os.chmod(curr_path, 0o555) + u = mda.Universe(new_gro, new_xtc)

zemanj

Checking for OSError only seems too broad to me (for details see my code comment).

zemanj · 2022-09-18T08:15:26Z

package/MDAnalysis/coordinates/XDR.py

@@ -197,7 +197,7 @@ def _load_offsets(self):
        try:
            with fasteners.InterProcessLock(lock_name) as filelock:
                pass
-        except PermissionError:
+        except (PermissionError, OSError):


According to the Python docs, PermissionError is a subclass of OSError, so your code is currently equivalent to only checking for OSError.
I'd advise against a simple check for OSError only, though, as this exception class covers many kinds of OS-related errors.
To narrow down the cause of the exception, it might be better to catch OSError, then check if it's a PermissionError or the errno member of the exception is the one you encountered. If so, raise the warning as before, otherwise just raise for the exception to bubble up.
For checking the errno, there should be a suitable constant in the errno module of the standard library so you don't have to hard-code numbers here.

EDIT: It's errno.EROFS.

zemanj

Awesome, just a few nitpicks. If you re-raise an exception, make sure to only use a bare raise statement without arguments. If you use raise e, the original exception trace won't be preserved.

package/MDAnalysis/coordinates/XDR.py

zemanj

Looks good to me. @tylerjereddy already commented on testing this so I'll approve once he's ok with it.

IAlibay

I'm going to put a short hold on new contributions for a few days re: licensing (sorry about this) - I'll release a blog post explaining why tonight / tomorrow hopefully, nothing big just need to work out what's our plan before we continue

jfennick · 2022-10-03T20:28:51Z

Any movement on this issue?

unblock

IAlibay · 2022-10-04T00:55:37Z

Sorry about this @jfennick - I didn't get much headway on the alluded to blog post (fingers crossed end of this week). I'm dismissing my block, but could I ask you that you introduce yourself on the mailing list as per #3832 (review) before we merge this?

We will need to trace contributors in the near future, so we need a way to reach out to everyone.

jfennick · 2022-10-13T15:23:11Z

Is there anything else I need to do to get this merged? If licensing issues are impeding drive-by contributions, then in the future perhaps I'll simply submit Issues instead of PRs.

IAlibay · 2022-10-14T11:02:56Z

@zemanj you have a blocking review here, could you have another look please?

IAlibay · 2022-10-14T11:04:39Z

Is there anything else I need to do to get this merged? If licensing issues are impeding drive-by contributions, then in the future perhaps I'll simply submit Issues instead of PRs.

Sorry about the long time to merge, the licensing stuff is not a blocker here (I dismissed my review some time back alongside #3832 (comment)). I'll give @zemanj until next week to re-review otherwise I'll take over and approve.

hmacdope · 2022-10-26T02:34:16Z

@IAlibay just scrolling through and thought I would give this a bump.

IAlibay

lgtm sorry this took so long @jfennick I'm not familiar with this part of the code so I didn't want to dismiss things rapidly

comments were addressed

IAlibay

Actually, I completely forgot - could you add yourself to AUTHORS @jfennick ?

IAlibay · 2022-10-26T14:56:13Z

Sorry about the last two commits, just thought it was faster for me to just add you to the changelog and fix the merge conflicts in one.

jfennick · 2022-10-26T14:58:18Z

Sorry about the last two commits, just thought it was faster for me to just add you to the changelog and fix the merge conflicts in one.

No problem. Whatever works.

IAlibay · 2022-10-26T15:54:48Z

/azp run

azure-pipelines · 2022-10-26T15:54:57Z

Azure Pipelines successfully started running 1 pipeline(s).

IAlibay · 2022-10-26T16:55:49Z

CI failure is being solved in #3886 I'm going to go ahead with the squash merge to not hold things up.

Added OSError

c7fa13e

github-actions bot added the Component-Readers label Sep 16, 2022

github-actions bot reviewed Sep 16, 2022

View reviewed changes

tylerjereddy reviewed Sep 17, 2022

View reviewed changes

zemanj requested changes Sep 18, 2022

View reviewed changes

Check OSError errno

41342f6

zemanj previously requested changes Sep 18, 2022

View reviewed changes

package/MDAnalysis/coordinates/XDR.py Show resolved Hide resolved

package/MDAnalysis/coordinates/XDR.py Outdated Show resolved Hide resolved

re-raise exception properly

51e3b75

jfennick requested a review from zemanj September 18, 2022 20:54

zemanj reviewed Sep 18, 2022

View reviewed changes

IAlibay previously requested changes Sep 18, 2022

View reviewed changes

IAlibay approved these changes Oct 26, 2022

View reviewed changes

IAlibay requested changes Oct 26, 2022

View reviewed changes

Added to package/AUTHORS

186fbfe

IAlibay approved these changes Oct 26, 2022

View reviewed changes

jfennick requested a review from IAlibay October 26, 2022 14:47

IAlibay added 2 commits October 26, 2022 15:54

Merge branch 'develop' into develop

f08e1c3

Update CHANGELOG

5b5ec9d

IAlibay merged commit c9685fb into MDAnalysis:develop Oct 26, 2022

IAlibay added the defect label Sep 25, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixed lockfile check when using a Docker read-only filesystem #3832

Fixed lockfile check when using a Docker read-only filesystem #3832

jfennick commented Sep 16, 2022

github-actions bot left a comment

codecov bot commented Sep 16, 2022 •

edited

Loading

IAlibay commented Sep 16, 2022

jfennick commented Sep 16, 2022

jfennick commented Sep 16, 2022

tylerjereddy Sep 17, 2022

zemanj left a comment

zemanj Sep 18, 2022 •

edited

Loading

zemanj left a comment

zemanj left a comment

IAlibay left a comment •

edited

Loading

jfennick commented Oct 3, 2022

IAlibay commented Oct 4, 2022

jfennick commented Oct 13, 2022

IAlibay commented Oct 14, 2022

IAlibay commented Oct 14, 2022

hmacdope commented Oct 26, 2022

IAlibay left a comment

IAlibay left a comment

IAlibay commented Oct 26, 2022

jfennick commented Oct 26, 2022

IAlibay commented Oct 26, 2022

azure-pipelines bot commented Oct 26, 2022

IAlibay commented Oct 26, 2022

Fixed lockfile check when using a Docker read-only filesystem #3832

Fixed lockfile check when using a Docker read-only filesystem #3832

Conversation

jfennick commented Sep 16, 2022

PR Checklist

github-actions bot left a comment

Choose a reason for hiding this comment

codecov bot commented Sep 16, 2022 • edited Loading

Codecov Report

IAlibay commented Sep 16, 2022

jfennick commented Sep 16, 2022

jfennick commented Sep 16, 2022

tylerjereddy Sep 17, 2022

Choose a reason for hiding this comment

zemanj left a comment

Choose a reason for hiding this comment

zemanj Sep 18, 2022 • edited Loading

Choose a reason for hiding this comment

zemanj left a comment

Choose a reason for hiding this comment

zemanj left a comment

Choose a reason for hiding this comment

IAlibay left a comment • edited Loading

Choose a reason for hiding this comment

jfennick commented Oct 3, 2022

IAlibay commented Oct 4, 2022

jfennick commented Oct 13, 2022

IAlibay commented Oct 14, 2022

IAlibay commented Oct 14, 2022

hmacdope commented Oct 26, 2022

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay left a comment

Choose a reason for hiding this comment

IAlibay commented Oct 26, 2022

jfennick commented Oct 26, 2022

IAlibay commented Oct 26, 2022

azure-pipelines bot commented Oct 26, 2022

IAlibay commented Oct 26, 2022

codecov bot commented Sep 16, 2022 •

edited

Loading

zemanj Sep 18, 2022 •

edited

Loading

IAlibay left a comment •

edited

Loading