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

use_relative_output_paths #608

Merged
merged 21 commits into from
Nov 3, 2022
Merged

use_relative_output_paths #608

merged 21 commits into from
Nov 3, 2022

Conversation

mlin
Copy link
Collaborator

@mlin mlin commented Oct 23, 2022

#604 #606

@rhpvorderman Here's a refactor based on my earlier comments, WDYT? (Opened a new PR because I didn't have permission to push to your branch, but keeping the commit history will ensure your co-authorship when we merge)

@coveralls
Copy link

coveralls commented Oct 23, 2022

Pull Request Test Coverage Report for Build 3384144984

  • 35 of 36 (97.22%) changed or added relevant lines in 1 file are covered.
  • 4 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.05%) to 95.161%

Changes Missing Coverage Covered Lines Changed/Added Lines %
WDL/runtime/task.py 35 36 97.22%
Files with Coverage Reduction New Missed Lines %
WDL/runtime/download.py 1 88.8%
WDL/runtime/workflow.py 1 97.17%
WDL/runtime/task.py 2 95.5%
Totals Coverage Status
Change from base Build 3376681751: -0.05%
Covered Lines: 7197
Relevant Lines: 7563

💛 - Coveralls

@rhpvorderman
Copy link
Contributor

I did a test. Unfortunately, when using the call_cache it outputs everything in the same directory, without keeping the directory structure. This was not the case before the refactor.
I am checking the code to see if this can be fixed easily.

rel_link = os.path.relpath(target, run_dir)
p = rel_link.find("/out/")
rel_link = rel_link[p + 5 :] if p > 0 else None
# if neither of the above cases applies, then fall back to just the target basename
Copy link
Contributor

Choose a reason for hiding this comment

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

A case is missing for call cached entries. These also have an outdir. I wonder if it is possible to "know" that the code is taking a path from the cache.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@rhpvorderman Good catch, I'll look into this

@mlin
Copy link
Collaborator Author

mlin commented Oct 30, 2022

@rhpvorderman updated, please give this a try? I think the logic is now very similar to your original, just with a few added heuristics to make it harder (but not impossible) to confuse it with other subdirectories named work/ or out/.

@rhpvorderman
Copy link
Contributor

rhpvorderman commented Oct 31, 2022

@mlin, this works great! I tested it with call caching and output hardlinks, and both seem to work fine. Thanks!

@mlin mlin merged commit ec4ccee into main Nov 3, 2022
@mlin mlin deleted the mlin/relativeoutputpaths branch November 3, 2022 09:13
@rhpvorderman
Copy link
Contributor

@mlin, thanks again for your efforts!

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.

None yet

3 participants