-
Notifications
You must be signed in to change notification settings - Fork 247
Add --runDir option; make --workDir/--coordinationDir create-if-missing (#5516) #5560
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
base: master
Are you sure you want to change the base?
Changes from all commits
c0b50ec
422b251
0b37e2f
3255854
6e76eba
a62076f
b13080e
6e8f230
53c262c
c5ae340
de58bc2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -34,6 +34,9 @@ | |
| BatchJobExitReason, | ||
| UpdatedBatchJobInfo, | ||
| ) | ||
| from toil.batchSystems.abstractGridEngineBatchSystem import ( | ||
| AbstractGridEngineBatchSystem, | ||
| ) | ||
| from toil.bus import ( | ||
| JobCompletedMessage, | ||
| JobFailedMessage, | ||
|
|
@@ -53,6 +56,7 @@ | |
| TemporaryID, | ||
| ) | ||
| from toil.jobStores.abstractJobStore import AbstractJobStore, NoSuchJobException, TOIL_WORKER_NO_JOB_STORE_EXIT_CODE | ||
| from toil.lib.io import ensure_dir_exists | ||
| from toil.lib.throttle import LocalThrottle | ||
| from toil.provisioners.abstractProvisioner import AbstractProvisioner | ||
| from toil.provisioners.clusterScaler import ScalerThread, NonScalableBatchSystemError | ||
|
|
@@ -262,6 +266,22 @@ def run(self) -> Any: | |
|
|
||
| :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. | ||
|
Comment on lines
+272
to
+273
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
| ensure_dir_exists(self.config.workDir, "--workDir") | ||
| ensure_dir_exists(self.config.coordination_dir, "--coordinationDir") | ||
|
annagiroti marked this conversation as resolved.
|
||
| 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() | ||
| ) | ||
|
|
||
| self.jobStore.write_kill_flag(kill=False) | ||
|
|
||
| with enlighten.get_manager( | ||
|
|
@@ -1593,12 +1613,11 @@ def process_finished_job_description( | |
| # If the batch system returned a non-zero exit code then the worker | ||
| # is assumed not to have captured the failure of the job, so we | ||
| # reduce the try count here. | ||
| if replacement_job.logJobStoreFileID is None: | ||
| logger.warning( | ||
| "No log file is present, despite job failing: %s", | ||
| replacement_job, | ||
| ) | ||
|
|
||
| # Search for the batch system's own logs first, so the | ||
| # "no log file" warning below is only shown when Toil | ||
| # genuinely found nothing, and can say so specifically. | ||
| found_batch_system_log = False | ||
| if batch_system_id is not None: | ||
| # Look for any standard output/error files created by the batch system. | ||
| # They will only appear if the batch system actually supports | ||
|
|
@@ -1620,6 +1639,7 @@ def process_finished_job_description( | |
| else: | ||
| with log_stream: | ||
| if os.path.getsize(log_file) > 0: | ||
| found_batch_system_log = True | ||
| StatsAndLogging.logWithFormatting( | ||
| f'Log from job "{job_store_id}"', | ||
| log_stream, | ||
|
|
@@ -1655,6 +1675,32 @@ def process_finished_job_description( | |
| % log_file | ||
| ) | ||
|
|
||
| if ( | ||
| replacement_job.logJobStoreFileID is None | ||
| and not found_batch_system_log | ||
| ): | ||
| 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, | ||
| ) | ||
|
Comment on lines
+1682
to
+1702
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
|
|
||
| # Tell the job to reset itself after a failure. | ||
| # It needs to know the failure reason if available; some are handled specially. | ||
| replacement_job.setupJobAfterFailure( | ||
|
|
||
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.
Maybe this really wants to be a method that just gets to use
self?