Skip to content

respect tmp-outdir-prefix when containerless #467

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

Merged
merged 5 commits into from
May 15, 2018

Conversation

jxtx
Copy link
Contributor

@jxtx jxtx commented Jul 13, 2017

Otherwise, the --tmp-outdir-prefix option passed on the command line is not respected, and outputs are instead written to the system temp directory.

@cwl-bot
Copy link

cwl-bot commented Jul 13, 2017

Can one of the admins verify this patch?

kapilkd13 pushed a commit to kapilkd13/cwltool that referenced this pull request Jul 14, 2017
0f28896 Merge pull request common-workflow-language#408 from kapilkd13/master
18fc8d6 Merge branch 'master' into master
d71b2f7 Merge pull request common-workflow-language#467 from kapilkd13/inlineJS-conformance
1f5f0de Update conformance_test_v1.0.yaml
8abc3f7 Merge pull request common-workflow-language#472 from common-workflow-language/no-draft-links
7134321 remove links to pre-v1.0 from homepage
2a88775 Merge pull request common-workflow-language#471 from common-workflow-language/changelong-tweak
0e0cb4b clarify that these change descriptions are specific to each of the standards
da56971 added conformance test for InlineJavascriptRequirement
485be33 Merge pull request common-workflow-language#465 from common-workflow-language/video_link
d874786 Add preview & link to intro video
397769d Merge pull request common-workflow-language#463 from common-workflow-language/hmenager-patch-1
cab882c version links
ca21bdf moar
84bd265 Update cli-description-languages-comparison.md
0cd5307 Merge pull request common-workflow-language#464 from common-workflow-language/bd2k
1cd58d3 Update cli-description-languages-comparison.md
4834980 Add BD2K as a participating organization
80e52b5 Update cli-description-languages-comparison.md
4f1fcd5 Update cli-description-languages-comparison.md
145bd0b Update cli-description-languages-comparison.md
c9ed818 Update cli-description-languages-comparison.md
1da94c2 Update cli-description-languages-comparison.md
1ff1819 Update cli-description-languages-comparison.md
24d782b Update cli-description-languages-comparison.md
e793622 Create cli-description-languages-comparison.md
2625c13 adding conformance test: Testing warning instead of error when default path is not present
52d9fcb changes to enable nolinkcheck in case of default field, related to cwltool issue #2

git-subtree-dir: cwltool/schemas
git-subtree-split: 0f28896
@mr-c
Copy link
Member

mr-c commented Jul 14, 2017

Jenkins, add to test list

@mr-c
Copy link
Member

mr-c commented Jul 14, 2017

Jenkins, add to testlist

tetron pushed a commit that referenced this pull request Jul 23, 2017
added conformance test for InlineJavascriptRequirement
@mr-c mr-c requested a review from tetron July 26, 2017 21:35
@tetron
Copy link
Member

tetron commented Mar 22, 2018

I think the reason it deletes outdir from kwargs because it sets self.outdir in the constructor. That is supposed to respect tmp_outdir_prefix, if it doesn't then that is a bug. If kwargs["outdir"] isn't removed, it would be passed to the workflow steps, and then each step would use the same value of outputdir instead of using separate ones for each step.

@codecov
Copy link

codecov bot commented May 11, 2018

Codecov Report

Merging #467 into master will increase coverage by 0.13%.
The diff coverage is 25%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #467      +/-   ##
==========================================
+ Coverage   62.93%   63.07%   +0.13%     
==========================================
  Files          28       28              
  Lines        4589     4579      -10     
  Branches     1233     1225       -8     
==========================================
  Hits         2888     2888              
+ Misses       1399     1390       -9     
+ Partials      302      301       -1
Impacted Files Coverage Δ
cwltool/singularity.py 18.88% <0%> (+0.13%) ⬆️
cwltool/workflow.py 65.8% <100%> (-0.19%) ⬇️
cwltool/executors.py 71.96% <100%> (+2.8%) ⬆️
cwltool/process.py 73.98% <100%> (ø) ⬆️
cwltool/job.py 56.08% <60%> (+0.37%) ⬆️
cwltool/docker.py 15.52% <7.4%> (+0.27%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d598e2d...1cdc519. Read the comment docs.

@mr-c mr-c changed the title Retain "outdir" in WorkflowJob.job. respect tmp-outdir-prefix when containerless May 11, 2018
@mr-c
Copy link
Member

mr-c commented May 11, 2018

@jxtx @tetron I believe I found the real bug; --tmp-outdir-prefix was only being consulted for containerized steps. Please review and test

@@ -622,7 +623,8 @@ def _init_job(self, joborder, **kwargs):
builder.tmpdir = builder.fs_access.docker_compatible_realpath(kwargs.get("docker_tmpdir") or "/tmp")
builder.stagedir = builder.fs_access.docker_compatible_realpath(kwargs.get("docker_stagedir") or "/var/lib/cwl")
else:
builder.outdir = builder.fs_access.realpath(kwargs.get("outdir") or tempfile.mkdtemp())
builder.outdir = builder.fs_access.realpath(kwargs.get("outdir")
or tempfile.mkdtemp(prefix=kwargs["tmp_outdir_prefix"]))
if self.tool[u"class"] != 'Workflow':
builder.tmpdir = builder.fs_access.realpath(kwargs.get("tmpdir") or tempfile.mkdtemp())
Copy link
Member

Choose a reason for hiding this comment

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

Does this need to be fixed as well?

@tetron
Copy link
Member

tetron commented May 14, 2018

@mr-c I think you're right. There might be a few other places with a similar bug?

@mr-c
Copy link
Member

mr-c commented May 15, 2018

Jenkins, test this please

jxtx and others added 5 commits May 15, 2018 00:48
Otherwise, the `--tmp-outdir-prefix` option passed on the command line is not respected, and outputs are instead written to the system temp directory.
@mr-c mr-c merged commit 33c2358 into common-workflow-language:master May 15, 2018
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.

4 participants