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 12 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.
| # 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).
Remove dead ensure_dir_exists calls in Leader.run() (unreachable codepath), compose the duplicate missing-log warning messages from shared/variable strings, and simplify test_cwl_run_dir.
adamnovak
left a comment
adamnovak
left a comment
There was a problem hiding this comment.
I think the missing-log-file messages still aren't quite right.
The behavior where derive_run_dir_defaults() sometimes promises that the directories it returns exist, and sometimes doesn't, seems weird and under-justified; is there a good reason for this I am missing? Is it just to make sure that a failed directory creation can't print a wrong error message?
I found a few other things I don't like about the comments, that I probably should have found the first time.
| Creates the derived work dir and coordination dir; the job store | ||
| creates itself. |
There was a problem hiding this comment.
This doesn't clearly communicate that work_dir and coordination_dir will be created, if they didn't exist, only when they are defaulted from run_dir, and not when they are passed in. I think it does mean that (it talks about "derived" directories), but that's weird so if it does that it should mean that harder and more obviously.
Right now the tests test that behavior, but it's not clear to me why that's the right design. The caller is going to need to make sure the directories exist before using them whether they are determined by this function or passed through it, so this design means there need to be at least two places where we worry about directory creation.
If we made this function either always or never create the directories, we'd need to worry about it in fewer places.
| if isinstance(self.batchSystem, AbstractGridEngineBatchSystem): | ||
| # The batch system isn't available yet when Toil logs the other | ||
| # resolved run paths (see Toil._log_resolved_paths), so log this | ||
| # one here instead, now that it exists. Only grid batch systems | ||
| # actually write their own logs to this directory. | ||
| logger.info( | ||
| "Resolved batch logs dir: %s", self.batchSystem.get_batch_logs_dir() | ||
| ) |
There was a problem hiding this comment.
We could leave it like this, but it would be nicer if, if only some batch systems use a batch logs directory, we exposed that in the batch system API itself (maybe by making get_batch_logs_dir() return None when not used?), instead of needing to know here what implementations actually use it.
| # returning logs to the machine that submitted jobs, or if | ||
| # --workDir / TOIL_WORKDIR is on a shared file system. | ||
| # They live directly in the Toil work directory because that is | ||
| # guaranteed to exist on the leader and workers. |
There was a problem hiding this comment.
This comment has, independent of the current changes, gone out of date, because the logs can be in --batchLogsDir instead now.
| if batch_system_id is None: | ||
| variable_message = ( | ||
| "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." | ||
| ) | ||
| else: | ||
| variable_message = ( | ||
| "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." | ||
| ) |
There was a problem hiding this comment.
Neither of these is really quite applicable for when we're using a batch system like mesos (or I think even single_machine) which we know in advance don't keep batch logs for us using this system.
When Toil thinks the batch system should have sent batch logs, and it can't find them, it should complain. It can mention --batchLogsDir, and maybe suggest making sure it's on shared storage.
When Toil thinks the batch system shouldn't send batch logs for this job (maybe because batch_system_id is None, which I think could mean we're looking at a batch system that doesn't use the concept or a local job that didn't go to the cluster), it shouldn't complain if they aren't there.
Encouraging the user to ask the batch system themselves can be useful, because e.g. Mesos will have logs for them somewhere. It won't apply to every batch system, though; I don't think single_machine has anything. We could just deal with that, or we could try to figure out when the user has something to find. If we build a system to figure out when it's applicable we might as well make that system let the batch system explain to the user where they need to look ("check the Mesos web UI", etc.).
| "Toil does not retain worker logs by default; rerun " | ||
| "with --writeLogs=PATH or --writeLogsGzip=PATH to " | ||
| "save failed jobs' logs to disk. " |
There was a problem hiding this comment.
I don't understand how --writeLogs is expected to help here. The log we didn't find for the failing job is the one that would get shipped back to the leader for writing to the --writeLogs directory. Adding the flag would only get a log to appear here in very weird circumstances involving worker failure after saving logs for a job it thinks has succeeded, and that log would not really help a lot for failures that happen after it is written.
The message here makes it sound like in general failed job logs are not available when --writeLogs isn't passed, but that's not true.
| # user didn't set explicitly; explicit flags always win. The | ||
| # jobStore-derivation branch inside this is only reachable for | ||
| # direct callers of the flag-based parser (jobstore_as_flag=True) | ||
| # that leave --jobStore unset. The CWL/WDL runners fill in their | ||
| # own jobStore default before calling setOptions, and the plain | ||
| # `toil` entry point requires jobStore as a positional argument, | ||
| # so neither ever reaches it in practice. |
There was a problem hiding this comment.
I can't tell what aspect of the code here this long description of interactions between other pieces of code is meant to justify. If we need to explain these interactions, I don't think here's the right place.
| # user didn't set explicitly; explicit flags always win. The | |
| # jobStore-derivation branch inside this is only reachable for | |
| # direct callers of the flag-based parser (jobstore_as_flag=True) | |
| # that leave --jobStore unset. The CWL/WDL runners fill in their | |
| # own jobStore default before calling setOptions, and the plain | |
| # `toil` entry point requires jobStore as a positional argument, | |
| # so neither ever reaches it in practice. | |
| # user didn't set explicitly; explicit flags always win. |
| Log the resolved job store, work dir, and coordination dir paths | ||
| at INFO, once, so users can find them without tracing flag | ||
| precedence themselves. Skips the batch logs dir; the batch system | ||
| isn't built yet here, and grid systems log their own dir on startup. |
There was a problem hiding this comment.
Do grid engine batch systems really log their own batch logs directory on startup? If so, why are we also logging it on their behalf in Leader.run()?
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