-
Notifications
You must be signed in to change notification settings - Fork 240
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
workDir and jobStore now default to (shared) tmp-outdir-prefix #5154
workDir and jobStore now default to (shared) tmp-outdir-prefix #5154
Conversation
Implements #5143. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code changes look good but I think as is this makes the documentation confusing for the non-CWL case.
docs/running/cliOptions.rst
Outdated
the workflow logs. Default is determined by the | ||
variables (TMPDIR, TEMP, TMP) via mkdtemp. This | ||
the workflow logs. Default is the temporary output | ||
directory (see ``--tmp-outdir-prefix``). This |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There's only a notion of --tmp-outdir-prefix
in the CWL runner; Python workflows an the WDL runner don't have it, and this documentation is meant to cover all three. So this probably needs to say that for CWL workflows the default --workDir
comes from --tmp-outdir-prefix
, but for other cases it still comes from mkdtemp
.
Also, the help that Toil actually prints is here, and it also has the material about mkdtemp
, so if in CWL mode that doesn't apply, we might want to change that text too. There we have access to a boolean cwl
flag, so we could describe the CWL-related behavior when it is set and the non-CWL behavior otherwise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I reverted the change to the original text, and added a line about CWL workflows and --tmp-outdir-prefix
. I don't know if you want it to be more elaborate.
I didn't touch the help that Toil prints. It was not completely clear to me where these changes had to be made, taking the cwl
boolean into account.
This commit makes the following changes to the behaviour of the given command-line options: * `tmp-outdir-prefix` defaults to `tmpdir-prefix`, unless given on the command-line * `workDir` defaults to `tmp-outdir-prefix`, unless given on the command-line * `jobStore` defaults to `tmp-outdir-prefix`, unless given on the command-line * `coordinationDir` defaults to the default tmpdir-prefix, ignoring `tmpdir-prefix` when given on the command-line (rationale: this is a book-keeping location, that must be on a local 100% posix-compliant file system, because it uses file locks).
Updated the CLI documention for the `--workDir` option.
The jobstore cannot be put inside the working directory, because it may need to be retained (e.g. when --stats is set). It now gets its own (temporary) directory, if not specified with the --jobstore option.
We do not need to create a working directory on the head node. We only need to create our jobstore here.
There's no need to set `options.tmp_outdir_prefix` here. It is not done in the current `master` branch either.
Improved the documentation of the `--workDir` option, by adding an explanation that the `--tmp-outdir-prefix` will be used for CWL workflows.
ec7b5ca
to
f71ddf3
Compare
@stxue1 is going to fix up the help here and merge this in. |
Testing the equivalent over at https://github.com/DataBiosphere/toil/tree/issues/5143-workdir-and-jobstore-in-tmp-outdir-prefix |
Changelog Entry
To be copied to the draft changelog by merger:
Reviewer Checklist
issues/XXXX-fix-the-thing
in the Toil repo, or from an external repo.camelCase
that want to be insnake_case
.docs/running/{cliOptions,cwl,wdl}.rst
Merger Checklist