Add --runDir option; make --workDir/--coordinationDir create-if-missing (#5516) - #5560
Add --runDir option; make --workDir/--coordinationDir create-if-missing (#5516)#5560annagiroti wants to merge 11 commits into
Conversation
…og warning --workDir and --coordinationDir get created automatically instead of raising when missing, matching --batchLogsDir's existing behavior. ensure_dir_exists moves to toil.lib.io and backstops this in Leader.run(). The "no log file" warning on job failure now checks the batch system's own logs first, so it isn't shown next to log content Toil already found. Adds tests for dir auto-creation and --runDir derivation/override precedence.
| Test that --runDir derives the CWL image cache location. The job | ||
| store/work dir derivation is covered by | ||
| commonTests.TestDeriveRunDirDefaults; cachedir is CWL-specific and | ||
| isn't. |
There was a problem hiding this comment.
Usually you want a docstring to have a one-line summary, then a blank line, and then any further description.
The further description right now is mostly about what isn't here and why it shouldn't be here. I don't think that the reader is at risk of thinking that those things should have been here, so trying to disabuse them of that notion seems more likely to confuse them than to help, because they're going to read this and then be confused as to why anyone would think they would have thought that. I would just cut everything that isn't about this test itself.
| "--sleep", | ||
| "2", |
There was a problem hiding this comment.
This looks like it might wait for 2 seconds that we don't actually need to wait for.
| def run(self) -> Any: | ||
| """ | ||
| Run the leader process to issue and manage jobs. | ||
|
|
||
| :raises: toil.exceptions.FailedJobsException if failed jobs remain after running. | ||
|
|
||
| :return: The return value of the root job's run function. | ||
| """ | ||
|
|
||
| # Toil.getToilWorkDir and get_local_workflow_coordination_dir already | ||
| # create these directories during Toil.__enter__, before the Leader | ||
| # exists. These calls are a defensive backstop for any code path that | ||
| # reaches Leader.run() without having gone through Toil.__enter__ first. | ||
| ensure_dir_exists(self.config.workDir, "--workDir") | ||
| ensure_dir_exists(self.config.coordination_dir, "--coordinationDir") |
There was a problem hiding this comment.
But ensure_dir_exists() is allowed to just exit the whole process if it e.g. doesn't have permission, right? Is someone's Python-based workflow going to be expecting that behavior from inside the Toil object, rather than an exception?
I might let ensure_dir_exists() propagate its various permission errors upward.
On the other hand, having it handle the exit makes the user-facing behavior in what's now the "normal" case of running a workflow nicer (because there's no stack trace), without us needing to put catches everywhere.
| # exists. These calls are a defensive backstop for any code path that | ||
| # reaches Leader.run() without having gone through Toil.__enter__ first. |
There was a problem hiding this comment.
Do we think those codepaths are possible or legal? Outside Toil's own testing where we might independently instantiate a Leader?
I think we're not actually allowed to run a Leader for a workflow when not inside the Toil object context manager, so the thing we're trying to handle isn't possible and we shouldn't handle it (unless it's to complain that it has happened and something has gone terribly wrong).
| if batch_system_id is None: | ||
| logger.warning( | ||
| "No log file is present, despite job failing: %s. " | ||
| "Toil does not retain worker logs by default; rerun with " | ||
| "--writeLogs=PATH or --writeLogsGzip=PATH to save failed " | ||
| "jobs' logs to disk. Toil was not able to look for logs " | ||
| "from the batch system for this job; check the batch " | ||
| "system's own tools or logs directly.", | ||
| replacement_job, | ||
| ) | ||
| else: | ||
| logger.warning( | ||
| "No log file is present, despite job failing: %s. " | ||
| "Toil does not retain worker logs by default; rerun with " | ||
| "--writeLogs=PATH or --writeLogsGzip=PATH to save failed " | ||
| "jobs' logs to disk. Toil looked for the batch system's " | ||
| "own logs (see --batchLogsDir) but found none; check the " | ||
| "batch system's own tools or logs directly if you are " | ||
| "running on a grid engine.", | ||
| replacement_job, | ||
| ) |
There was a problem hiding this comment.
These two similar messages should probably be composed by pasting together shared and variable strings.
Also, if a job fails, and there's a log from the Slurm task but we don't have a log sent back from the Toil worker itself, then something has gone wrong. "Normal" failed jobs due to user error still fail in a way that the worker is able to report in. We still want to log something to alert the user that the worker failed to report in like it was supposed to, even if we have some logging about it. We might want to suggest that the user read the logging we do have.
| self.runDir, self.jobStore, self.workDir, self.coordination_dir = ( | ||
| derive_run_dir_defaults( | ||
| self.runDir, self.jobStore, self.workDir, self.coordination_dir | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Maybe this really wants to be a method that just gets to use self?
2 participants
Changelog Entry
Added
--runDir/TOIL_RUN_DIRto derive the job store, work dir, coordination dir, and CWL/WDL image cache locations under one directory.--workDir/--coordinationDirare now created automatically if missing instead of raising, and Toil logs the resolved run paths at startup.Known follow-up: the job store path derived from
--runDiris fixed, so concurrent workflows sharing one--runDirwill collide. Flagged with TODOs pending a fix.Addresses #5516
To be copied to the draft changelog by merger:
Reviewer Checklist
issues/XXXX-fix-the-thingin the Toil repo, or from an external repo.camelCasethat want to be insnake_case.docs/running/{cliOptions,cwl,wdl}.rstMerger Checklist