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

Fixed another symlinking issue with Docker + staged files from InitialWorkDirRequirement #1708

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

jfennick
Copy link
Contributor

After two weeks of debugging, I think I finally fixed some additional symlinking issues. I will attempt to add a minimal example and/or tests soon, and failing that I will link the example which this PR solves from my project once I get it on my CI.

Copy link
Member

@mr-c mr-c left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on this!

cwltool/job.py Outdated
os.remove(host_outdir_tgt)
except PermissionError:
pass
os.chmod(host_outdir_tgt, mode)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

https://github.com/common-workflow-language/cwltool/runs/7851533013?check_suite_focus=true#step:8:146

 File "/home/runner/work/cwltool/cwltool/cwltool/job.py", line 104, in relink_initialworkdir
      os.chmod(host_outdir_tgt, mode)
  PermissionError: [Errno 1] Operation not permitted

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, I'm still working on the precise change necessary which both makes my code work and which satisfies your CI. Part of the problem is that your CI still doesn't work on my local machine. For example, if I do a fresh clone of your main branch (i.e. not my fork), and run pytest in a fresh conda environment, I get:

    def test_single_process_subwf_subwf_inline_step() -> None:
        """Test --single-process on an inline sub-sub-workflow step."""
        err_code, stdout, stderr = get_main_output(
            [
                "--single-process",
                "step1/stepX/stepY",
                get_data("tests/subgraph/count-lines17-wf.cwl.json"),
                get_data("tests/wf/wc-job.json"),
            ]
        )
        assert err_code == 0
>       assert (
            json.loads(stdout)["output"]["checksum"]
            == "sha1$3596ea087bfdaf52380eae441077572ed289d657"
        )
E       AssertionError: assert 'sha1$9aefd46...8d192af7bcac4' == 'sha1$3596ea0...7572ed289d657'
E         - sha1$3596ea087bfdaf52380eae441077572ed289d657
E         + sha1$9aefd466231cf30b4e9ae5234d58d192af7bcac4

I'm not really sure what's happening here. Maybe it's because I'm running on a Macbook?

Copy link
Member

@mr-c mr-c Aug 16, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, macs do have different wc command outputs; I'll make a PR to use a docker container for those tests.

@codecov
Copy link

codecov bot commented Aug 16, 2022

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (e4b5d92) 83.77% compared to head (a699296) 83.67%.

Files Patch % Lines
cwltool/job.py 83.33% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1708      +/-   ##
==========================================
- Coverage   83.77%   83.67%   -0.11%     
==========================================
  Files          46       46              
  Lines        8201     8212      +11     
  Branches     2178     2178              
==========================================
+ Hits         6870     6871       +1     
- Misses        855      863       +8     
- Partials      476      478       +2     

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

@lgtm-com
Copy link

lgtm-com bot commented Aug 16, 2022

This pull request introduces 2 alerts when merging 1ef91d9 into 8763bed - view on LGTM.com

new alerts:

  • 2 for Variable defined multiple times

@lgtm-com
Copy link

lgtm-com bot commented Oct 12, 2022

This pull request introduces 2 alerts when merging ce48bdd into 6f0e1d9 - view on LGTM.com

new alerts:

  • 2 for Variable defined multiple times

@mr-c
Copy link
Member

mr-c commented Jun 26, 2023

@jfennick
Copy link
Contributor Author

Yeah, there's still something wrong here. Every month or two when I have time I return to this, but I'm still slowly working on it.

@@ -236,7 +236,7 @@ def add_file_or_directory_volume(
"""Append volume a file/dir mapping to the runtime option list."""
if not volume.resolved.startswith("_:"):
_check_docker_machine_path(volume.resolved)
self.append_volume(runtime, volume.resolved, volume.target)
self.append_volume(runtime, volume.resolved, volume.target, writable=volume.staged)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the goal here? This results in many files that should be mounted read-only to not be..

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The goal overall was to fix a bug I was encountering with a specific CommandLineTool (Yank) which we are no longer using. I still believe there is a bug in cwltool, but I can't seem to simultaneously fix the bug and get the test suite to pass. This is very low priority work for me, so feel free to close if you like.

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.

2 participants