From cd214b2a9dbd2b305fe6b1c4145575a055923f2c Mon Sep 17 00:00:00 2001 From: Avni Gandhi Date: Thu, 28 May 2026 12:43:36 -0700 Subject: [PATCH 01/20] add injected container cpu/memory accounting for cwl and wdl --- src/toil/cwl/cwltoil.py | 103 ++++++++++++++++++- src/toil/lib/interpreter.py | 194 +++++++++++++++++++++++++++++++++++ src/toil/test/cwl/cwlTest.py | 111 ++++++++++++++++++++ src/toil/wdl/wdltoil.py | 157 +++------------------------- 4 files changed, 418 insertions(+), 147 deletions(-) create mode 100644 src/toil/lib/interpreter.py diff --git a/src/toil/cwl/cwltoil.py b/src/toil/cwl/cwltoil.py index ab8ea5beb1..18366c2ab3 100644 --- a/src/toil/cwl/cwltoil.py +++ b/src/toil/cwl/cwltoil.py @@ -31,13 +31,21 @@ import pprint import shutil import stat +import subprocess import sys import textwrap import uuid # This is also in configargparse but MyPy doesn't know it from argparse import RawDescriptionHelpFormatter -from collections.abc import Callable, Iterator, Mapping, MutableMapping, MutableSequence +from collections.abc import ( + Callable, + Generator, + Iterator, + Mapping, + MutableMapping, + MutableSequence, +) from tempfile import NamedTemporaryFile, TemporaryFile, gettempdir from threading import Thread from typing import IO, Any, Literal, Optional, Protocol, TextIO, TypeVar, Union, cast @@ -67,6 +75,8 @@ fill_in_defaults, shortname, ) +from cwltool.docker import DockerCommandLineJob, PodmanCommandLineJob +from cwltool.job import ContainerCommandLineJob from cwltool.secrets import SecretStore from cwltool.singularity import SingularityCommandLineJob from cwltool.software_requirements import ( @@ -102,6 +112,12 @@ from toil.common import Config, Toil, addOptions, InconsistentConfigurationError from toil.cwl import check_cwltool_version from toil.lib.directory import DirectoryContents, decode_directory, encode_directory +from toil.lib.interpreter import ( + add_injections, + command_line_to_shell_script, + handle_injection_messages_from_outdir, + shell_script_to_command_line, +) from toil.lib.misc import call_command from toil.lib.trs import resolve_workflow from toil.provisioners.clusterScaler import JobTooBigError @@ -1089,6 +1105,32 @@ def run_jobs( return super().run_jobs(process, job_order_object, logger, runtime_context) +class ToilContainerCommandLineJob(ContainerCommandLineJob): + """Container job that collects resource stats from injected in-container code.""" + + def _execute( + self, + runtime: list[str], + env: MutableMapping[str, str], + runtimeContext: cwltool.context.RuntimeContext, + monitor_function: Callable[["subprocess.Popen[str]"], None] | None = None, + ) -> None: + super()._execute(runtime, env, runtimeContext, monitor_function) + handle_injection_messages_from_outdir(self.outdir) + + +class ToilDockerCommandLineJob(ToilContainerCommandLineJob, DockerCommandLineJob): + """Docker container job with Toil runtime injection support.""" + + +class ToilPodmanCommandLineJob(ToilContainerCommandLineJob, PodmanCommandLineJob): + """Podman container job with Toil runtime injection support.""" + + +class ToilSingularityCommandLineJob(ToilContainerCommandLineJob, SingularityCommandLineJob): + """Singularity container job with Toil runtime injection support.""" + + class ToilTool: """Mixin to hook Toil into a cwltool tool type.""" @@ -1144,7 +1186,64 @@ def __str__(self) -> str: class ToilCommandLineTool(ToilTool, cwltool.command_line_tool.CommandLineTool): - """Subclass the cwltool command line tool to provide the custom ToilPathMapper.""" + """Subclass the cwltool command line tool to provide the custom ToilPathMapper + and add the monitoring code to the job's container command line.""" + + def make_job_runner( + self, runtimeContext: cwltool.context.RuntimeContext + ) -> type[cwltool.job.JobBase]: + """Use Toil container job classes that collect injected runtime messages.""" + parent_class = super().make_job_runner(runtimeContext) + if parent_class is DockerCommandLineJob: + return ToilDockerCommandLineJob + if parent_class is PodmanCommandLineJob: + return ToilPodmanCommandLineJob + if parent_class is SingularityCommandLineJob: + return ToilSingularityCommandLineJob + return parent_class + + def _uses_container(self, runtimeContext: cwltool.context.RuntimeContext) -> bool: + if not runtimeContext.use_container: + return False + docker_req, _ = self.get_requirement("DockerRequirement") + if docker_req is not None: + return True + if runtimeContext.find_default_container is not None: + return runtimeContext.find_default_container(self) is not None + return runtimeContext.default_container is not None + + @staticmethod + def _file_mounts_from_pathmapper( + job: ContainerCommandLineJob, + ) -> list[tuple[str, str]]: + file_mounts: list[tuple[str, str]] = [] + for location in job.pathmapper.files(): + ent = job.pathmapper.mapper(location) + if ent.type == "File" and not ent.resolved.startswith("_:"): + file_mounts.append((ent.resolved, ent.target)) + return file_mounts + + def job( + self, + job_order: CWLObjectType, + output_callbacks: Callable[[CWLObjectType | None, str], None], + runtimeContext: cwltool.context.RuntimeContext, + ) -> Generator[ + cwltool.job.JobBase | cwltool.command_line_tool.CallbackJob, None, None + ]: + """ + Override the job method to inject resource (cpu & memory) monitoring code into + the job's container command line. + """ + for job in super().job(job_order, output_callbacks, runtimeContext): + if isinstance(job, ContainerCommandLineJob) and self._uses_container( + runtimeContext + ): + file_mounts = self._file_mounts_from_pathmapper(job) + script = command_line_to_shell_script(job.command_line) + script = add_injections(script, file_mounts) + job.command_line = shell_script_to_command_line(script) + yield job def _initialworkdir( self, j: cwltool.job.JobBase | None, builder: cwltool.builder.Builder diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py new file mode 100644 index 0000000000..35ab399a37 --- /dev/null +++ b/src/toil/lib/interpreter.py @@ -0,0 +1,194 @@ +import glob +import logging +import os +import platform +import shlex +import textwrap +from typing import Iterable + +from toil.lib.resources import ResourceMonitor + +logger = logging.getLogger(__name__) + +### +# Runtime code injection system +# When a workflow steps runs inside a container, the Toil worker process on the host +# often cannot see how much CPU and RAM that step actually used. This system allows +# the step to inject code into the container that will write resource usage information +# to files in a directory, which the Toil worker process can then read and use to +# update the resource usage stats. +### + +# Runtime code injected in the container communicates back to the rest of the +# runtime through files in this directory. +INJECTED_MESSAGE_DIR = ".toil_runtime" + + +# Helper function to convert a CWL command from argv list to one shell script string +# that can be executed in the container. +def command_line_to_shell_script(command_line: list[str]) -> str: + """ + Extract or synthesize the inner shell script from a cwltool argv list. + + cwltool uses ``["/bin/sh", "-c", script]`` when ShellCommandRequirement is + present, and a flat argv list otherwise. + """ + if ( + len(command_line) >= 3 + and command_line[0] == "/bin/sh" + and command_line[1] == "-c" + ): + return command_line[2] + return " ".join(shlex.quote(arg) for arg in command_line) # this is the shell script string + + +# Helper function to convert a shell script back into an argv list that can be used by cwltool. +def shell_script_to_command_line(script: str) -> list[str]: + """Wrap a shell script for cwltool/docker (injection requires bash).""" + return ["/bin/bash", "-c", script] + +# Core function to inject the resource usage monitoring code into the command line for the docker swarm container. +# Takes the command string and the file mounts and returns the modified command string. +def add_injections( + command_string: str, + file_mounts: Iterable[tuple[str, str]], + message_dir: str = INJECTED_MESSAGE_DIR, +) -> str: + """ + Inject extra Bash code from the Toil runtime into the command for the container. + + Currently doesn't implement the MiniWDL plugin system, but does add + resource usage monitoring to Docker containers. + """ + + parts = [] + # We're running on Docker Swarm, so we need to monitor CPU usage + # and so on from inside the container, since it won't be attributed + # to Toil child processes in the leader's self-monitoring. + # TODO: Mount this from a file Toil installs instead or something. + script = textwrap.dedent( + """\ + function _toil_resource_monitor () { + # Turn off error checking and echo in here + set +ex + MESSAGE_DIR="${1}" + mkdir -p "${MESSAGE_DIR}" + + function sample_cpu_usec() { + if [[ -f /sys/fs/cgroup/cpu.stat ]] ; then + awk '{ if ($1 == "usage_usec") {print $2} }' /sys/fs/cgroup/cpu.stat + elif [[ -f /sys/fs/cgroup/cpuacct/cpuacct.stat ]] ; then + echo $(( $(head -n 1 /sys/fs/cgroup/cpuacct/cpuacct.stat | cut -f2 -d' ') * 10000 )) + fi + } + + function sample_memory_bytes() { + if [[ -f /sys/fs/cgroup/memory.stat ]] ; then + awk '{ if ($1 == "anon") { print $2 } }' /sys/fs/cgroup/memory.stat + elif [[ -f /sys/fs/cgroup/memory/memory.stat ]] ; then + awk '{ if ($1 == "total_rss") { print $2 } }' /sys/fs/cgroup/memory/memory.stat + fi + } + + while true ; do + printf "CPU\\t" >> ${MESSAGE_DIR}/resources.tsv + sample_cpu_usec >> ${MESSAGE_DIR}/resources.tsv + printf "Memory\\t" >> ${MESSAGE_DIR}/resources.tsv + sample_memory_bytes >> ${MESSAGE_DIR}/resources.tsv + sleep 1 + done + } + """ + ) + parts.append(script) + # Launch in a subshell so that it doesn't interfere with Bash "wait" in the main shell + parts.append(f"(_toil_resource_monitor {message_dir} &)") + + if platform.system() == "Darwin": + # With gRPC FUSE file sharing, files immediately downloaded before + # being mounted may appear as size 0 in the container due to a race + # condition. Check for this and produce an approperiate error. + + script = textwrap.dedent( + """\ + function _toil_check_size () { + TARGET_FILE="${1}" + GOT_SIZE="$(stat -c %s "${TARGET_FILE}")" + EXPECTED_SIZE="${2}" + if [[ "${GOT_SIZE}" != "${EXPECTED_SIZE}" ]] ; then + echo >&2 "Toil Error:" + echo >&2 "File size visible in container for ${TARGET_FILE} is size ${GOT_SIZE} but should be size ${EXPECTED_SIZE}" + echo >&2 "Are you using gRPC FUSE file sharing in Docker Desktop?" + echo >&2 "It doesn't work: see ." + exit 1 + fi + } + """ + ) + parts.append(script) + for host_path, job_path in file_mounts: + expected_size = os.path.getsize(host_path) + if expected_size != 0: + parts.append(f'_toil_check_size "{job_path}" {expected_size}') + + parts.append(command_string) + + return "\n".join(parts) + +# Helper function to parse the injected resource usage monitoring code and update the resource usage stats. +def handle_message_file(file_path: str) -> None: + """ + Handle a message file received from in-container injected code. + + Takes the host-side path of the file. + """ + if os.path.basename(file_path) == "resources.tsv": + # This is a TSV of resource usage info. + first_cpu_usec: int | None = None + last_cpu_usec: int | None = None + max_memory_bytes: int | None = None + + for line in open(file_path): + if not line.endswith("\n"): + # Skip partial lines + continue + # For each full line we got + parts = line.strip().split("\t") + if len(parts) != 2: + # Skip odd-shaped lines + continue + if parts[0] == "CPU": + # Parse CPU usage + cpu_usec = int(parts[1]) + # Update summary stats + if first_cpu_usec is None: + first_cpu_usec = cpu_usec + last_cpu_usec = cpu_usec + elif parts[0] == "Memory": + # Parse memory usage + memory_bytes = int(parts[1]) + # Update summary stats + if max_memory_bytes is None or max_memory_bytes < memory_bytes: + max_memory_bytes = memory_bytes + + if max_memory_bytes is not None: + logger.info( + "Container used at about %s bytes of memory at peak", + max_memory_bytes, + ) + # Treat it as if used by a child process + ResourceMonitor.record_extra_memory(max_memory_bytes // 1024) + if last_cpu_usec is not None: + assert first_cpu_usec is not None + cpu_seconds = (last_cpu_usec - first_cpu_usec) / 1000000 + logger.info("Container used about %s seconds of CPU time", cpu_seconds) + # Treat it as if used by a child process + ResourceMonitor.record_extra_cpu(cpu_seconds) + +# Helper function that is a convenience wrapper around the handle_message_file function. +def handle_injection_messages_from_outdir(outdir: str) -> None: + """Read and handle any message files left in the job outdir by injected code.""" + message_dir = os.path.join(outdir, INJECTED_MESSAGE_DIR) + for file_path in glob.glob(os.path.join(message_dir, "*")): + if os.path.isfile(file_path): + handle_message_file(file_path) diff --git a/src/toil/test/cwl/cwlTest.py b/src/toil/test/cwl/cwlTest.py index 712d553eec..1d459c34c0 100644 --- a/src/toil/test/cwl/cwlTest.py +++ b/src/toil/test/cwl/cwlTest.py @@ -40,6 +40,7 @@ from schema_salad.exceptions import ValidationException +from toil.common import Toil from toil.cwl.utils import ( DirectoryStructure, download_structure, @@ -66,6 +67,7 @@ from toil.test import pneeds_torque as needs_torque from toil.test import pneeds_wes_server as needs_wes_server from toil.test import pslow as slow +from toil.utils.toilStats import get_stats, process_data log = logging.getLogger(__name__) CONFORMANCE_TEST_TIMEOUT = 10000 @@ -482,6 +484,37 @@ def test_s3_as_secondary_file(self, tmp_path: Path) -> None: with open(out["output"]["location"][len("file://") :]) as f: assert f.read().strip() == "When is s4 coming out?" + @needs_docker + @pytest.mark.docker + @pytest.mark.online + @pytest.mark.timeout(180) + def test_cpu_memory_monitoring(self, tmp_path: Path) -> None: + """Container CPU and memory usage is recorded in Toil job stats.""" + from toil.cwl import cwltoil + + with get_data("test/cwl/waste_cpu_memory.cwl") as cwl_file: + with get_data("test/cwl/empty.json") as inputs_file: + job_store = tmp_path / "jobStore" + main_args = [ + "--outdir", + str(tmp_path / "output_dir"), + str(cwl_file), + str(inputs_file), + "--jobStore", + str(job_store), + "--stats", + ] + cwltoil.main(main_args) + + resumed_job_store = Toil.resumeJobStore(str(job_store)) + stats = get_stats(resumed_job_store) + collated_stats = process_data(resumed_job_store.config, stats) + + # The tool burns ~30s of CPU and ~1 GiB of RAM inside Docker. + # Per-job stats use KiB for memory and include injected container usage. + assert collated_stats.jobs.total_clock >= 30 + assert collated_stats.jobs.max_memory >= 1024 * 1024 + @needs_docker @pytest.mark.docker @pytest.mark.online @@ -2299,6 +2332,84 @@ def test_download_structure(tmp_path: Path) -> None: ) +@needs_cwl +@pytest.mark.cwl +@pytest.mark.cwl_small +def test_cwl_resource_message_parsing_records_cpu_and_memory( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """ + Resource monitor messages emitted from a container are translated into + Toil's extra CPU and memory accounting. + """ + from toil.lib import interpreter + + message_file = tmp_path / "resources.tsv" + # Include malformed and partial lines to exercise parser robustness. + message_file.write_text( + "CPU\t1000000\n" + "Memory\t1024\n" + "CPU\t4000000\n" + "Memory\t2048\n" + "Odd\tline\n" + "CPU\t5000000", + encoding="utf-8", + ) + + recorded_memory_ki: list[int] = [] + recorded_cpu_seconds: list[float] = [] + monkeypatch.setattr( + interpreter.ResourceMonitor, + "record_extra_memory", + lambda peak_ki: recorded_memory_ki.append(peak_ki), + ) + monkeypatch.setattr( + interpreter.ResourceMonitor, + "record_extra_cpu", + lambda seconds: recorded_cpu_seconds.append(seconds), + ) + + interpreter.handle_message_file(str(message_file)) + + assert recorded_memory_ki == [2] + assert recorded_cpu_seconds == [3.0] + + +@needs_cwl +@pytest.mark.cwl +@pytest.mark.cwl_small +def test_cwl_job_injection_wraps_container_command( + tmp_path: Path, monkeypatch: pytest.MonkeyPatch +) -> None: + """ + Container jobs are wrapped with injected runtime monitoring code. + """ + from toil.cwl import cwltoil + + host_input = tmp_path / "input.txt" + host_input.write_text("hello", encoding="utf-8") + + class FakeMapperEntry: + type = "File" + + def __init__(self, resolved: str, target: str) -> None: + self.resolved = resolved + self.target = target + + class FakePathMapper: + def __init__(self, entry: FakeMapperEntry) -> None: + self._entry = entry + + def files(self) -> list[str]: + return ["file://fake-input"] + + def mapper(self, _location: str) -> FakeMapperEntry: + return self._entry + + job = object.__new__(cwltoil.ToilDockerCommandLineJob) + job.command_line = ["echo", "hello"] + job.pathmapper = FakePathMapper(FakeMapperEntry(str(host_input), "/work/input.txt")) + @needs_cwl @pytest.mark.cwl @pytest.mark.timeout(300) diff --git a/src/toil/wdl/wdltoil.py b/src/toil/wdl/wdltoil.py index 2f7010140d..0988767983 100755 --- a/src/toil/wdl/wdltoil.py +++ b/src/toil/wdl/wdltoil.py @@ -133,6 +133,11 @@ from toil.lib.trs import resolve_workflow from toil.lib.url import URLAccess from toil.provisioners.clusterScaler import JobTooBigError +from toil.lib.interpreter import ( + INJECTED_MESSAGE_DIR, + add_injections, + handle_message_file, +) logger = logging.getLogger(__name__) @@ -3871,98 +3876,6 @@ def __init__( self._cache_key = cache_key self._mount_spec = mount_spec - ### - # Runtime code injection system - ### - - # WDL runtime code injected in the container communicates back to the rest - # of the runtime through files in this directory. - INJECTED_MESSAGE_DIR = ".toil_wdl_runtime" - - def add_injections(self, command_string: str, task_container: TaskContainer) -> str: - """ - Inject extra Bash code from the Toil WDL runtime into the command for the container. - - Currently doesn't implement the MiniWDL plugin system, but does add - resource usage monitoring to Docker containers. - """ - - parts = [] - - if isinstance(task_container, SwarmContainer): - # We're running on Docker Swarm, so we need to monitor CPU usage - # and so on from inside the container, since it won't be attributed - # to Toil child processes in the leader's self-monitoring. - # TODO: Mount this from a file Toil installs instead or something. - script = textwrap.dedent( - """\ - function _toil_resource_monitor () { - # Turn off error checking and echo in here - set +ex - MESSAGE_DIR="${1}" - mkdir -p "${MESSAGE_DIR}" - - function sample_cpu_usec() { - if [[ -f /sys/fs/cgroup/cpu.stat ]] ; then - awk '{ if ($1 == "usage_usec") {print $2} }' /sys/fs/cgroup/cpu.stat - elif [[ -f /sys/fs/cgroup/cpuacct/cpuacct.stat ]] ; then - echo $(( $(head -n 1 /sys/fs/cgroup/cpuacct/cpuacct.stat | cut -f2 -d' ') * 10000 )) - fi - } - - function sample_memory_bytes() { - if [[ -f /sys/fs/cgroup/memory.stat ]] ; then - awk '{ if ($1 == "anon") { print $2 } }' /sys/fs/cgroup/memory.stat - elif [[ -f /sys/fs/cgroup/memory/memory.stat ]] ; then - awk '{ if ($1 == "total_rss") { print $2 } }' /sys/fs/cgroup/memory/memory.stat - fi - } - - while true ; do - printf "CPU\\t" >> ${MESSAGE_DIR}/resources.tsv - sample_cpu_usec >> ${MESSAGE_DIR}/resources.tsv - printf "Memory\\t" >> ${MESSAGE_DIR}/resources.tsv - sample_memory_bytes >> ${MESSAGE_DIR}/resources.tsv - sleep 1 - done - } - """ - ) - parts.append(script) - # Launch in a subshell so that it doesn't interfere with Bash "wait" in the main shell - parts.append(f"(_toil_resource_monitor {self.INJECTED_MESSAGE_DIR} &)") - - if isinstance(task_container, SwarmContainer) and platform.system() == "Darwin": - # With gRPC FUSE file sharing, files immediately downloaded before - # being mounted may appear as size 0 in the container due to a race - # condition. Check for this and produce an approperiate error. - - script = textwrap.dedent( - """\ - function _toil_check_size () { - TARGET_FILE="${1}" - GOT_SIZE="$(stat -c %s "${TARGET_FILE}")" - EXPECTED_SIZE="${2}" - if [[ "${GOT_SIZE}" != "${EXPECTED_SIZE}" ]] ; then - echo >&2 "Toil Error:" - echo >&2 "File size visible in container for ${TARGET_FILE} is size ${GOT_SIZE} but should be size ${EXPECTED_SIZE}" - echo >&2 "Are you using gRPC FUSE file sharing in Docker Desktop?" - echo >&2 "It doesn't work: see ." - exit 1 - fi - } - """ - ) - parts.append(script) - for host_path, job_path in task_container.input_path_map.items(): - expected_size = os.path.getsize(host_path) - if expected_size != 0: - parts.append(f'_toil_check_size "{job_path}" {expected_size}') - - parts.append(command_string) - - return "\n".join(parts) - def handle_injection_messages( self, outputs_library: ToilWDLStdLibTaskOutputs ) -> None: @@ -3971,60 +3884,11 @@ def handle_injection_messages( """ message_files = outputs_library._glob( - WDL.Value.String(os.path.join(self.INJECTED_MESSAGE_DIR, "*")) + WDL.Value.String(os.path.join(INJECTED_MESSAGE_DIR, "*")) ) logger.debug("Handling message files: %s", message_files) for message_file in message_files.value: - self.handle_message_file(message_file.value) - - def handle_message_file(self, file_path: str) -> None: - """ - Handle a message file received from in-container injected code. - - Takes the host-side path of the file. - """ - if os.path.basename(file_path) == "resources.tsv": - # This is a TSV of resource usage info. - first_cpu_usec: int | None = None - last_cpu_usec: int | None = None - max_memory_bytes: int | None = None - - for line in open(file_path): - if not line.endswith("\n"): - # Skip partial lines - continue - # For each full line we got - parts = line.strip().split("\t") - if len(parts) != 2: - # Skip odd-shaped lines - continue - if parts[0] == "CPU": - # Parse CPU usage - cpu_usec = int(parts[1]) - # Update summary stats - if first_cpu_usec is None: - first_cpu_usec = cpu_usec - last_cpu_usec = cpu_usec - elif parts[0] == "Memory": - # Parse memory usage - memory_bytes = int(parts[1]) - # Update summary stats - if max_memory_bytes is None or max_memory_bytes < memory_bytes: - max_memory_bytes = memory_bytes - - if max_memory_bytes is not None: - logger.info( - "Container used at about %s bytes of memory at peak", - max_memory_bytes, - ) - # Treat it as if used by a child process - ResourceMonitor.record_extra_memory(max_memory_bytes // 1024) - if last_cpu_usec is not None: - assert first_cpu_usec is not None - cpu_seconds = (last_cpu_usec - first_cpu_usec) / 1000000 - logger.info("Container used about %s seconds of CPU time", cpu_seconds) - # Treat it as if used by a child process - ResourceMonitor.record_extra_cpu(cpu_seconds) + handle_message_file(message_file.value) ### # Helper functions to work out what containers runtime we can use @@ -4501,8 +4365,11 @@ def get_path_in_container(inode: AnyINode) -> AnyINode | None: .value ) - # Do any command injection we might need to do - command_string = self.add_injections(command_string, task_container) + # Do command injection for docker container resource monitoring + # when a docker container is used + if isinstance(task_container, SwarmContainer): + file_mounts = task_container.input_path_map.items() + command_string = add_injections(command_string, file_mounts) # Grab the standard out and error paths. MyPy complains if we call # them because in the current MiniWDL version they are untyped. From 62e252b297bdecf11e471bb708e1003cc624fe28 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 1 Jul 2026 14:00:30 -0400 Subject: [PATCH 02/20] Change output hook and avoid hooking container types that don't need it --- src/toil/cwl/cwltoil.py | 72 +++++++++++++++++++---------------------- 1 file changed, 33 insertions(+), 39 deletions(-) diff --git a/src/toil/cwl/cwltoil.py b/src/toil/cwl/cwltoil.py index 18366c2ab3..1b816f64e2 100644 --- a/src/toil/cwl/cwltoil.py +++ b/src/toil/cwl/cwltoil.py @@ -1105,32 +1105,6 @@ def run_jobs( return super().run_jobs(process, job_order_object, logger, runtime_context) -class ToilContainerCommandLineJob(ContainerCommandLineJob): - """Container job that collects resource stats from injected in-container code.""" - - def _execute( - self, - runtime: list[str], - env: MutableMapping[str, str], - runtimeContext: cwltool.context.RuntimeContext, - monitor_function: Callable[["subprocess.Popen[str]"], None] | None = None, - ) -> None: - super()._execute(runtime, env, runtimeContext, monitor_function) - handle_injection_messages_from_outdir(self.outdir) - - -class ToilDockerCommandLineJob(ToilContainerCommandLineJob, DockerCommandLineJob): - """Docker container job with Toil runtime injection support.""" - - -class ToilPodmanCommandLineJob(ToilContainerCommandLineJob, PodmanCommandLineJob): - """Podman container job with Toil runtime injection support.""" - - -class ToilSingularityCommandLineJob(ToilContainerCommandLineJob, SingularityCommandLineJob): - """Singularity container job with Toil runtime injection support.""" - - class ToilTool: """Mixin to hook Toil into a cwltool tool type.""" @@ -1189,20 +1163,10 @@ class ToilCommandLineTool(ToilTool, cwltool.command_line_tool.CommandLineTool): """Subclass the cwltool command line tool to provide the custom ToilPathMapper and add the monitoring code to the job's container command line.""" - def make_job_runner( - self, runtimeContext: cwltool.context.RuntimeContext - ) -> type[cwltool.job.JobBase]: - """Use Toil container job classes that collect injected runtime messages.""" - parent_class = super().make_job_runner(runtimeContext) - if parent_class is DockerCommandLineJob: - return ToilDockerCommandLineJob - if parent_class is PodmanCommandLineJob: - return ToilPodmanCommandLineJob - if parent_class is SingularityCommandLineJob: - return ToilSingularityCommandLineJob - return parent_class - def _uses_container(self, runtimeContext: cwltool.context.RuntimeContext) -> bool: + """ + Returns True if this tool will be run inside a container. + """ if not runtimeContext.use_container: return False docker_req, _ = self.get_requirement("DockerRequirement") @@ -1212,10 +1176,16 @@ def _uses_container(self, runtimeContext: cwltool.context.RuntimeContext) -> boo return runtimeContext.find_default_container(self) is not None return runtimeContext.default_container is not None + # TODO: Why is this on ToilCommandLineTool? @staticmethod def _file_mounts_from_pathmapper( job: ContainerCommandLineJob, ) -> list[tuple[str, str]]: + """ + Get all the container mounts that will be used for a containerized job. + + :returns: a list of (host path, container path) tuples, one per mount. + """ file_mounts: list[tuple[str, str]] = [] for location in job.pathmapper.files(): ent = job.pathmapper.mapper(location) @@ -1245,6 +1215,30 @@ def job( job.command_line = shell_script_to_command_line(script) yield job + def collect_output_ports( + self, + ports: CommentedSeq | set[CWLObjectType], + builder: cwltool.builder.Builder, + outdir: str, + rcode: int, + compute_checksum: bool = True, + jobname: str = "", + readers: MutableMapping[str, CWLFileType | CWLDirectoryType] | None = None, + ) -> cwltool.command_line_tool.OutputPortsType: + """ + Hook output collection to also collect resource usage statistics. + """ + handle_injection_messages_from_outdir(outdir) + return super().collect_output_ports( + ports, + builder, + outdir, + rcode, + compute_checksum, + jobname, + readers, + ) + def _initialworkdir( self, j: cwltool.job.JobBase | None, builder: cwltool.builder.Builder ) -> None: From 8da4efddf80d88683d80a51113c04b5c2355c886 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 1 Jul 2026 15:19:30 -0400 Subject: [PATCH 03/20] Fix type checking --- src/toil/cwl/cwltoil.py | 27 +++++++++++++++++++++++---- src/toil/test/cwl/cwlTest.py | 14 ++++++++++---- 2 files changed, 33 insertions(+), 8 deletions(-) diff --git a/src/toil/cwl/cwltoil.py b/src/toil/cwl/cwltoil.py index 1b816f64e2..b98b9e0572 100644 --- a/src/toil/cwl/cwltoil.py +++ b/src/toil/cwl/cwltoil.py @@ -84,6 +84,7 @@ get_container_from_software_requirements, ) from cwltool.stdfsaccess import StdFsAccess, abspath +from cwltool.udocker import UDockerCommandLineJob from cwltool.utils import ( adjustDirObjs, aslist, @@ -1158,6 +1159,13 @@ def __str__(self) -> str: """Return string representation of this tool type.""" return f'{self.__class__.__name__}({repr(getattr(self, "tool", {}).get("id", "???"))})' +# We need to know which cwltool container implementations run the container +# under the calling process's child tree. Unlisted container implementations +# are assumed to run it elsewhere through a deamon. +# +# We have to list it this way around (and not list the implementations that use +# a daemon) because some of these extend DockerCommandLineJob. +CHILD_PROCESS_CONTAINER_JOBS = (PodmanCommandLineJob, SingularityCommandLineJob, UDockerCommandLineJob) class ToilCommandLineTool(ToilTool, cwltool.command_line_tool.CommandLineTool): """Subclass the cwltool command line tool to provide the custom ToilPathMapper @@ -1206,16 +1214,27 @@ def job( the job's container command line. """ for job in super().job(job_order, output_callbacks, runtimeContext): - if isinstance(job, ContainerCommandLineJob) and self._uses_container( - runtimeContext + if ( + isinstance(job, ContainerCommandLineJob) and + not isinstance(job, CHILD_PROCESS_CONTAINER_JOBS) and + self._uses_container( + runtimeContext + ) ): + # This job is going to run in a container that makes it not be + # a descendant of our process. So we need to inject code to + # count its resource usage. + + # That code also checks to make sure files mounted on Docker + # for Mac are intact, so we need to tell it ablut the file + # mounts we are going to use. file_mounts = self._file_mounts_from_pathmapper(job) script = command_line_to_shell_script(job.command_line) script = add_injections(script, file_mounts) job.command_line = shell_script_to_command_line(script) yield job - def collect_output_ports( + def collect_output_ports( self, ports: CommentedSeq | set[CWLObjectType], builder: cwltool.builder.Builder, @@ -1223,7 +1242,7 @@ def collect_output_ports( rcode: int, compute_checksum: bool = True, jobname: str = "", - readers: MutableMapping[str, CWLFileType | CWLDirectoryType] | None = None, + readers: MutableMapping[str, CWLObjectType] | None = None, ) -> cwltool.command_line_tool.OutputPortsType: """ Hook output collection to also collect resource usage statistics. diff --git a/src/toil/test/cwl/cwlTest.py b/src/toil/test/cwl/cwlTest.py index 1d459c34c0..ff4e186a2b 100644 --- a/src/toil/test/cwl/cwlTest.py +++ b/src/toil/test/cwl/cwlTest.py @@ -512,6 +512,9 @@ def test_cpu_memory_monitoring(self, tmp_path: Path) -> None: # The tool burns ~30s of CPU and ~1 GiB of RAM inside Docker. # Per-job stats use KiB for memory and include injected container usage. + assert hasattr(collated_stats, "jobs") + assert hasattr(collated_stats.jobs, "total_clock") + assert hasattr(collated_stats.jobs, "max_memory") assert collated_stats.jobs.total_clock >= 30 assert collated_stats.jobs.max_memory >= 1024 * 1024 @@ -2343,6 +2346,7 @@ def test_cwl_resource_message_parsing_records_cpu_and_memory( Toil's extra CPU and memory accounting. """ from toil.lib import interpreter + from toil.lib.resources import ResourceMonitor message_file = tmp_path / "resources.tsv" # Include malformed and partial lines to exercise parser robustness. @@ -2359,12 +2363,12 @@ def test_cwl_resource_message_parsing_records_cpu_and_memory( recorded_memory_ki: list[int] = [] recorded_cpu_seconds: list[float] = [] monkeypatch.setattr( - interpreter.ResourceMonitor, + ResourceMonitor, "record_extra_memory", lambda peak_ki: recorded_memory_ki.append(peak_ki), ) monkeypatch.setattr( - interpreter.ResourceMonitor, + ResourceMonitor, "record_extra_cpu", lambda seconds: recorded_cpu_seconds.append(seconds), ) @@ -2385,6 +2389,8 @@ def test_cwl_job_injection_wraps_container_command( Container jobs are wrapped with injected runtime monitoring code. """ from toil.cwl import cwltoil + from cwltool.docker import DockerCommandLineJob + from cwltool.pathmapper import PathMapper host_input = tmp_path / "input.txt" host_input.write_text("hello", encoding="utf-8") @@ -2406,9 +2412,9 @@ def files(self) -> list[str]: def mapper(self, _location: str) -> FakeMapperEntry: return self._entry - job = object.__new__(cwltoil.ToilDockerCommandLineJob) + job = object.__new__(DockerCommandLineJob) job.command_line = ["echo", "hello"] - job.pathmapper = FakePathMapper(FakeMapperEntry(str(host_input), "/work/input.txt")) + job.pathmapper = cast(PathMapper, FakePathMapper(FakeMapperEntry(str(host_input), "/work/input.txt"))) @needs_cwl @pytest.mark.cwl From 52d319eda59f6afde8c17e25e413243ef4524de6 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 1 Jul 2026 16:04:54 -0400 Subject: [PATCH 04/20] Re-document interpreter.py --- src/toil/lib/interpreter.py | 92 ++++++++++++++++++++++++++----------- 1 file changed, 65 insertions(+), 27 deletions(-) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index 35ab399a37..63f41c5298 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -1,3 +1,30 @@ +""" +Runtime code injection system + +When a workflow steps runs inside a container like Docker, where the contained +process is not a descendant of Toil, the Toil worker process on the host cannot +see how much CPU and RAM that step actually used. + +This system allows Toil to inject code into the container that will colklect +and export resource usage information back to Toil. + +It also allows Toil to do other checks on the container system. +""" + +# Copyright (C) 2026 Regents of the University of California +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + import glob import logging import os @@ -10,22 +37,14 @@ logger = logging.getLogger(__name__) -### -# Runtime code injection system -# When a workflow steps runs inside a container, the Toil worker process on the host -# often cannot see how much CPU and RAM that step actually used. This system allows -# the step to inject code into the container that will write resource usage information -# to files in a directory, which the Toil worker process can then read and use to -# update the resource usage stats. -### - -# Runtime code injected in the container communicates back to the rest of the -# runtime through files in this directory. +# Code injected into the container communicates back to the rest of Toil +# through files in this directory. INJECTED_MESSAGE_DIR = ".toil_runtime" -# Helper function to convert a CWL command from argv list to one shell script string -# that can be executed in the container. +# We mostly want to work with shell script strings, but CWL works with command +# argument lists, so we use these functions to convert back and forth. + def command_line_to_shell_script(command_line: list[str]) -> str: """ Extract or synthesize the inner shell script from a cwltool argv list. @@ -42,30 +61,45 @@ def command_line_to_shell_script(command_line: list[str]) -> str: return " ".join(shlex.quote(arg) for arg in command_line) # this is the shell script string -# Helper function to convert a shell script back into an argv list that can be used by cwltool. def shell_script_to_command_line(script: str) -> list[str]: """Wrap a shell script for cwltool/docker (injection requires bash).""" return ["/bin/bash", "-c", script] -# Core function to inject the resource usage monitoring code into the command line for the docker swarm container. -# Takes the command string and the file mounts and returns the modified command string. +# Main function + def add_injections( command_string: str, file_mounts: Iterable[tuple[str, str]], message_dir: str = INJECTED_MESSAGE_DIR, ) -> str: """ - Inject extra Bash code from the Toil runtime into the command for the container. + Add resource usage monitoring and file mount checking code to a command. + + The command is expected to be about to run in a container, under a + container system that does not itself attribute resource usage to the + calling Toil process (such as Docker, which uses a daemon). + + :param command_string: shell command or script to modify + :param file_mounts: collection of (host path, container path) tuples for + files mounted into the container. Code will be added to require that + the container sees the complete file that the host sees. + :param message_dir: directory relative to the working directory that the + command should record resource usage to + + :returns: shell command string (possibly containing multiple commands) that + runs the original command and reports resource usage. - Currently doesn't implement the MiniWDL plugin system, but does add - resource usage monitoring to Docker containers. """ parts = [] - # We're running on Docker Swarm, so we need to monitor CPU usage - # and so on from inside the container, since it won't be attributed - # to Toil child processes in the leader's self-monitoring. - # TODO: Mount this from a file Toil installs instead or something. + # We're running on Docker or another platform where Toil isn't an ancestor + # process, so we need to monitor CPU usage and so on from inside the + # container, since it won't be attributed to Toil child processes in the + # leader's self-monitoring. + + # TODO: Mount this script from a file Toil installs instead or something + # instead of injecting it in every command line, which makes it show up in + # logs. script = textwrap.dedent( """\ function _toil_resource_monitor () { @@ -135,10 +169,11 @@ def add_injections( return "\n".join(parts) -# Helper function to parse the injected resource usage monitoring code and update the resource usage stats. +# Helper functions to parse resource usage output + def handle_message_file(file_path: str) -> None: """ - Handle a message file received from in-container injected code. + Handle a message file received from injected code from :meth:`add_injections()`. Takes the host-side path of the file. """ @@ -185,9 +220,12 @@ def handle_message_file(file_path: str) -> None: # Treat it as if used by a child process ResourceMonitor.record_extra_cpu(cpu_seconds) -# Helper function that is a convenience wrapper around the handle_message_file function. def handle_injection_messages_from_outdir(outdir: str) -> None: - """Read and handle any message files left in the job outdir by injected code.""" + """ + Handle any message files in the job outdir. + + Files would have been left by injected code from :meth:`add_injections()`. + """ message_dir = os.path.join(outdir, INJECTED_MESSAGE_DIR) for file_path in glob.glob(os.path.join(message_dir, "*")): if os.path.isfile(file_path): From 65d3078377b4309820d9b23a40250557d1c00ed9 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 1 Jul 2026 16:09:24 -0400 Subject: [PATCH 05/20] Address interpreter.py review comments --- src/toil/lib/interpreter.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index 63f41c5298..666e20bb37 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -49,12 +49,12 @@ def command_line_to_shell_script(command_line: list[str]) -> str: """ Extract or synthesize the inner shell script from a cwltool argv list. - cwltool uses ``["/bin/sh", "-c", script]`` when ShellCommandRequirement is - present, and a flat argv list otherwise. + cwltool uses ``["/bin/sh", "-c", script]`` in some cases, so that pattern + is handled specially. """ if ( - len(command_line) >= 3 - and command_line[0] == "/bin/sh" + len(command_line) == 3 + and command_line[0] in ("/bin/sh", "/bin/bash") and command_line[1] == "-c" ): return command_line[2] @@ -62,7 +62,11 @@ def command_line_to_shell_script(command_line: list[str]) -> str: def shell_script_to_command_line(script: str) -> list[str]: - """Wrap a shell script for cwltool/docker (injection requires bash).""" + """ + Wrap a shell script as a list of arguments for launchign a process. + + The resulting command required Bash to be available. + """ return ["/bin/bash", "-c", script] # Main function @@ -175,7 +179,10 @@ def handle_message_file(file_path: str) -> None: """ Handle a message file received from injected code from :meth:`add_injections()`. - Takes the host-side path of the file. + Records the described usage as if it had occurred within a child process, + by talking to the :class:`toil.lib.resources.ResourceMonitor` system. + + :param file_path: the host-side path of the message file. """ if os.path.basename(file_path) == "resources.tsv": # This is a TSV of resource usage info. From cac82970b0e0d6731e65029022c4eb16ec6d69de Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 1 Jul 2026 16:12:40 -0400 Subject: [PATCH 06/20] Note dependency on default runner --- src/toil/test/cwl/cwlTest.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/src/toil/test/cwl/cwlTest.py b/src/toil/test/cwl/cwlTest.py index ff4e186a2b..a314018180 100644 --- a/src/toil/test/cwl/cwlTest.py +++ b/src/toil/test/cwl/cwlTest.py @@ -503,6 +503,9 @@ def test_cpu_memory_monitoring(self, tmp_path: Path) -> None: "--jobStore", str(job_store), "--stats", + # TODO: this relies on the default container engine for + # toil-cwl-runner being Docker, because we don't have a + # --docker option to make it explicit. ] cwltoil.main(main_args) From 4cf943a63b5c137a784a2eafeebf7c2538637d02 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 1 Jul 2026 16:16:11 -0400 Subject: [PATCH 07/20] Remove incomplete or unwanted tests --- src/toil/test/cwl/cwlTest.py | 43 +----------------------------------- 1 file changed, 1 insertion(+), 42 deletions(-) diff --git a/src/toil/test/cwl/cwlTest.py b/src/toil/test/cwl/cwlTest.py index a314018180..011fc2a7c3 100644 --- a/src/toil/test/cwl/cwlTest.py +++ b/src/toil/test/cwl/cwlTest.py @@ -2352,14 +2352,11 @@ def test_cwl_resource_message_parsing_records_cpu_and_memory( from toil.lib.resources import ResourceMonitor message_file = tmp_path / "resources.tsv" - # Include malformed and partial lines to exercise parser robustness. message_file.write_text( "CPU\t1000000\n" "Memory\t1024\n" "CPU\t4000000\n" - "Memory\t2048\n" - "Odd\tline\n" - "CPU\t5000000", + "Memory\t2048\n", encoding="utf-8", ) @@ -2381,44 +2378,6 @@ def test_cwl_resource_message_parsing_records_cpu_and_memory( assert recorded_memory_ki == [2] assert recorded_cpu_seconds == [3.0] - -@needs_cwl -@pytest.mark.cwl -@pytest.mark.cwl_small -def test_cwl_job_injection_wraps_container_command( - tmp_path: Path, monkeypatch: pytest.MonkeyPatch -) -> None: - """ - Container jobs are wrapped with injected runtime monitoring code. - """ - from toil.cwl import cwltoil - from cwltool.docker import DockerCommandLineJob - from cwltool.pathmapper import PathMapper - - host_input = tmp_path / "input.txt" - host_input.write_text("hello", encoding="utf-8") - - class FakeMapperEntry: - type = "File" - - def __init__(self, resolved: str, target: str) -> None: - self.resolved = resolved - self.target = target - - class FakePathMapper: - def __init__(self, entry: FakeMapperEntry) -> None: - self._entry = entry - - def files(self) -> list[str]: - return ["file://fake-input"] - - def mapper(self, _location: str) -> FakeMapperEntry: - return self._entry - - job = object.__new__(DockerCommandLineJob) - job.command_line = ["echo", "hello"] - job.pathmapper = cast(PathMapper, FakePathMapper(FakeMapperEntry(str(host_input), "/work/input.txt"))) - @needs_cwl @pytest.mark.cwl @pytest.mark.timeout(300) From ef5d54701f452d1efa722ad233e50bd1a5f00dfe Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 1 Jul 2026 16:19:09 -0400 Subject: [PATCH 08/20] Make interpreter.py able to be about more than just the code injection system --- src/toil/lib/interpreter.py | 25 +++++++++++++++---------- 1 file changed, 15 insertions(+), 10 deletions(-) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index 666e20bb37..8e1ef032d4 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -1,14 +1,5 @@ """ -Runtime code injection system - -When a workflow steps runs inside a container like Docker, where the contained -process is not a descendant of Toil, the Toil worker process on the host cannot -see how much CPU and RAM that step actually used. - -This system allows Toil to inject code into the container that will colklect -and export resource usage information back to Toil. - -It also allows Toil to do other checks on the container system. +Shared code for workflow language interpreters implemented with Toil. """ # Copyright (C) 2026 Regents of the University of California @@ -37,6 +28,20 @@ logger = logging.getLogger(__name__) + +#### +# Shell command injection system +# +# When a workflow steps runs inside a container like Docker, where the contained +# process is not a descendant of Toil, the Toil worker process on the host cannot +# see how much CPU and RAM that step actually used. +# +# This system allows Toil to inject code into the container that will colklect +# and export resource usage information back to Toil. +# +# It also allows Toil to do other checks on the container system. +### + # Code injected into the container communicates back to the rest of Toil # through files in this directory. INJECTED_MESSAGE_DIR = ".toil_runtime" From 79401f3f3b324330ba39af5282e0d51067ea3457 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 1 Jul 2026 19:40:22 -0400 Subject: [PATCH 09/20] Stop requiring a fixed Bash path and stop unpacking what could be real user argument lists --- src/toil/lib/interpreter.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index 8e1ef032d4..d5e683e3af 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -54,15 +54,9 @@ def command_line_to_shell_script(command_line: list[str]) -> str: """ Extract or synthesize the inner shell script from a cwltool argv list. - cwltool uses ``["/bin/sh", "-c", script]`` in some cases, so that pattern - is handled specially. + We don't want to disturb user CWL command line binding lists that + explicitly ask for things like ["bash", "-c"]. """ - if ( - len(command_line) == 3 - and command_line[0] in ("/bin/sh", "/bin/bash") - and command_line[1] == "-c" - ): - return command_line[2] return " ".join(shlex.quote(arg) for arg in command_line) # this is the shell script string @@ -72,7 +66,7 @@ def shell_script_to_command_line(script: str) -> list[str]: The resulting command required Bash to be available. """ - return ["/bin/bash", "-c", script] + return ["bash", "-c", script] # Main function From 02dc3a6f27d3d03f4547faceec61e8135c1f13bf Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 1 Jul 2026 19:43:44 -0400 Subject: [PATCH 10/20] Add Anthropic Claude's idea of what waste_cpu_memory.cwl probably was supposed to be --- src/toil/test/cwl/waste_cpu_memory.cwl | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) create mode 100644 src/toil/test/cwl/waste_cpu_memory.cwl diff --git a/src/toil/test/cwl/waste_cpu_memory.cwl b/src/toil/test/cwl/waste_cpu_memory.cwl new file mode 100644 index 0000000000..340876f591 --- /dev/null +++ b/src/toil/test/cwl/waste_cpu_memory.cwl @@ -0,0 +1,22 @@ +cwlVersion: v1.2 +class: CommandLineTool + +doc: "Burn ~30s of CPU and allocate ~1 GiB of RAM inside a container, for stats monitoring tests." + +requirements: + - class: DockerRequirement + dockerPull: python:3-slim + +baseCommand: + - python3 + - -c + - | + import time + # Hold ~1 GiB in RAM for the duration of the run. + data = bytearray(1024 * 1024 * 1024) + end = time.monotonic() + 30 + while time.monotonic() < end: + pass + +inputs: [] +outputs: [] From 3a9bccf2bb6fb4eb4f53dc8bb7ad619b6454911f Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Mon, 6 Jul 2026 18:05:03 -0400 Subject: [PATCH 11/20] POSIX-ify injected shell code --- src/toil/lib/interpreter.py | 35 +++++++++++++++++++---------------- 1 file changed, 19 insertions(+), 16 deletions(-) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index d5e683e3af..f221acca76 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -57,16 +57,16 @@ def command_line_to_shell_script(command_line: list[str]) -> str: We don't want to disturb user CWL command line binding lists that explicitly ask for things like ["bash", "-c"]. """ - return " ".join(shlex.quote(arg) for arg in command_line) # this is the shell script string + return " ".join(shlex.quote(arg) for arg in command_line) def shell_script_to_command_line(script: str) -> list[str]: """ Wrap a shell script as a list of arguments for launchign a process. - The resulting command required Bash to be available. + The resulting command only requires a POSIX shell as sh. """ - return ["bash", "-c", script] + return ["sh", "-c", script] # Main function @@ -82,6 +82,8 @@ def add_injections( container system that does not itself attribute resource usage to the calling Toil process (such as Docker, which uses a daemon). + All shell code added is compatible with POSIX sh. + :param command_string: shell command or script to modify :param file_mounts: collection of (host path, container path) tuples for files mounted into the container. Code will be added to require that @@ -103,35 +105,36 @@ def add_injections( # TODO: Mount this script from a file Toil installs instead or something # instead of injecting it in every command line, which makes it show up in # logs. + + # Use https://www.shellcheck.net/ with #!/bin/sh at the top to make sure + # this is all sh-compatible. + script = textwrap.dedent( """\ - function _toil_resource_monitor () { + _toil_resource_monitor () { # Turn off error checking and echo in here set +ex MESSAGE_DIR="${1}" mkdir -p "${MESSAGE_DIR}" - function sample_cpu_usec() { - if [[ -f /sys/fs/cgroup/cpu.stat ]] ; then + sample_cpu_usec() { + if [ -f /sys/fs/cgroup/cpu.stat ] ; then awk '{ if ($1 == "usage_usec") {print $2} }' /sys/fs/cgroup/cpu.stat - elif [[ -f /sys/fs/cgroup/cpuacct/cpuacct.stat ]] ; then + elif [ -f /sys/fs/cgroup/cpuacct/cpuacct.stat ] ; then echo $(( $(head -n 1 /sys/fs/cgroup/cpuacct/cpuacct.stat | cut -f2 -d' ') * 10000 )) fi } - function sample_memory_bytes() { - if [[ -f /sys/fs/cgroup/memory.stat ]] ; then + sample_memory_bytes() { + if [ -f /sys/fs/cgroup/memory.stat ] ; then awk '{ if ($1 == "anon") { print $2 } }' /sys/fs/cgroup/memory.stat - elif [[ -f /sys/fs/cgroup/memory/memory.stat ]] ; then + elif [ -f /sys/fs/cgroup/memory/memory.stat ] ; then awk '{ if ($1 == "total_rss") { print $2 } }' /sys/fs/cgroup/memory/memory.stat fi } while true ; do - printf "CPU\\t" >> ${MESSAGE_DIR}/resources.tsv - sample_cpu_usec >> ${MESSAGE_DIR}/resources.tsv - printf "Memory\\t" >> ${MESSAGE_DIR}/resources.tsv - sample_memory_bytes >> ${MESSAGE_DIR}/resources.tsv + (printf "CPU\\t" ; sample_cpu_usec ; printf "Memory\\t" ; sample_memory_bytes) >> "${MESSAGE_DIR}"/resources.tsv sleep 1 done } @@ -148,11 +151,11 @@ def add_injections( script = textwrap.dedent( """\ - function _toil_check_size () { + _toil_check_size () { TARGET_FILE="${1}" GOT_SIZE="$(stat -c %s "${TARGET_FILE}")" EXPECTED_SIZE="${2}" - if [[ "${GOT_SIZE}" != "${EXPECTED_SIZE}" ]] ; then + if [ "${GOT_SIZE}" != "${EXPECTED_SIZE}" ] ; then echo >&2 "Toil Error:" echo >&2 "File size visible in container for ${TARGET_FILE} is size ${GOT_SIZE} but should be size ${EXPECTED_SIZE}" echo >&2 "Are you using gRPC FUSE file sharing in Docker Desktop?" From 95868c0f40ce0fec52dcbe9ba81a09a17f4588e9 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Mon, 6 Jul 2026 18:12:51 -0400 Subject: [PATCH 12/20] Stop assuming stat --- src/toil/lib/interpreter.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index f221acca76..ffe69d180f 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -149,9 +149,15 @@ def add_injections( # being mounted may appear as size 0 in the container due to a race # condition. Check for this and produce an approperiate error. + # We can't guarantee that the container has a stat binary, and we can't + # measure the size of a file with pure shell without reading it all, so + # we poll for stat and skip the check if it isn't there. script = textwrap.dedent( """\ _toil_check_size () { + if ! which stat >/dev/null 2>/dev/null ; then + return + fi TARGET_FILE="${1}" GOT_SIZE="$(stat -c %s "${TARGET_FILE}")" EXPECTED_SIZE="${2}" From b36cda79647ec788a0dd509ab67698414f8d9a7b Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Mon, 6 Jul 2026 18:22:19 -0400 Subject: [PATCH 13/20] Read cpu stats the same way for cgroups v1 as v2 Accoridng to https://github.com/torvalds/linux/blob/8cdeaa50eae8dad34885515f62559ee83e7e8dda/kernel/sched/cpuacct.c#L262 we have user and system fields here, and actually probably we need to be including both. --- src/toil/lib/interpreter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index ffe69d180f..7455ba2f82 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -121,7 +121,7 @@ def add_injections( if [ -f /sys/fs/cgroup/cpu.stat ] ; then awk '{ if ($1 == "usage_usec") {print $2} }' /sys/fs/cgroup/cpu.stat elif [ -f /sys/fs/cgroup/cpuacct/cpuacct.stat ] ; then - echo $(( $(head -n 1 /sys/fs/cgroup/cpuacct/cpuacct.stat | cut -f2 -d' ') * 10000 )) + echo $(( ( $(awk '{ if ($1 == "user") {print $2} }' /sys/fs/cgroup/cpuacct/cpuacct.stat) + $(awk '{ if ($1 == "system") {print $2} }' /sys/fs/cgroup/cpuacct/cpuacct.stat) ) * 10000 )) fi } From 2eebbfc9717ce491dfae281d41992a93c8aee74a Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Mon, 6 Jul 2026 18:32:00 -0400 Subject: [PATCH 14/20] Mechanically translate awk to shell --- src/toil/lib/interpreter.py | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index 7455ba2f82..0b2c4a2599 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -117,19 +117,30 @@ def add_injections( MESSAGE_DIR="${1}" mkdir -p "${MESSAGE_DIR}" + get_field () { + INFILE="${1}" + FIELD_NAME="${2}" + while IFS=' ' read -r KEY VALUE ; do + if [ "${KEY}" = "${FIELD_NAME}" ] ; then + echo "${VALUE}" + return + fi + done < "${INFILE}" + } + sample_cpu_usec() { if [ -f /sys/fs/cgroup/cpu.stat ] ; then - awk '{ if ($1 == "usage_usec") {print $2} }' /sys/fs/cgroup/cpu.stat + get_field /sys/fs/cgroup/cpu.stat usage_usec elif [ -f /sys/fs/cgroup/cpuacct/cpuacct.stat ] ; then - echo $(( ( $(awk '{ if ($1 == "user") {print $2} }' /sys/fs/cgroup/cpuacct/cpuacct.stat) + $(awk '{ if ($1 == "system") {print $2} }' /sys/fs/cgroup/cpuacct/cpuacct.stat) ) * 10000 )) + echo $(( ( $(get_field /sys/fs/cgroup/cpuacct/cpuacct.stat user) + $(get_field /sys/fs/cgroup/cpuacct/cpuacct.stat system) ) * 10000 )) fi } sample_memory_bytes() { if [ -f /sys/fs/cgroup/memory.stat ] ; then - awk '{ if ($1 == "anon") { print $2 } }' /sys/fs/cgroup/memory.stat + get_field /sys/fs/cgroup/memory.stat anon elif [ -f /sys/fs/cgroup/memory/memory.stat ] ; then - awk '{ if ($1 == "total_rss") { print $2 } }' /sys/fs/cgroup/memory/memory.stat + get_field /sys/fs/cgroup/memory/memory.stat total_rss fi } From f17e682387f228f7340a65c68d25d442fbd34e75 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Mon, 6 Jul 2026 19:14:05 -0400 Subject: [PATCH 15/20] Complain about how we need to add a mount somehow --- src/toil/cwl/cwltoil.py | 17 ++++++++++++++--- src/toil/lib/interpreter.py | 20 ++++++++++---------- src/toil/wdl/wdltoil.py | 8 +++++++- 3 files changed, 31 insertions(+), 14 deletions(-) diff --git a/src/toil/cwl/cwltoil.py b/src/toil/cwl/cwltoil.py index b98b9e0572..df26ac5351 100644 --- a/src/toil/cwl/cwltoil.py +++ b/src/toil/cwl/cwltoil.py @@ -114,9 +114,10 @@ from toil.cwl import check_cwltool_version from toil.lib.directory import DirectoryContents, decode_directory, encode_directory from toil.lib.interpreter import ( + INJECTED_MESSAGE_DIR, add_injections, command_line_to_shell_script, - handle_injection_messages_from_outdir, + handle_injection_messages_from, shell_script_to_command_line, ) from toil.lib.misc import call_command @@ -1230,7 +1231,17 @@ def job( # mounts we are going to use. file_mounts = self._file_mounts_from_pathmapper(job) script = command_line_to_shell_script(job.command_line) - script = add_injections(script, file_mounts) + # TODO: we really need to somehow add another mount to the + # container (maybe by making _file_mounts_from_pathmapper() + # actually the canonical source for mounts?) so we can put the + # messages there, because in CWL we're not allowed to drop + # extra stuff in the working directory. We can't just go a + # level up from it because that's probably not mounted, and we + # can't just work in a temp directory and move the data after + # the user code because that won't handle an OOM kill. So right + # now we fail initial_workdir_empty_writable_docker, presumably + # for having this here. + script = add_injections(script, file_mounts, INJECTED_MESSAGE_DIR) job.command_line = shell_script_to_command_line(script) yield job @@ -1247,7 +1258,7 @@ def collect_output_ports( """ Hook output collection to also collect resource usage statistics. """ - handle_injection_messages_from_outdir(outdir) + handle_injection_messages_from(os.path.join(outdir, INJECTED_MESSAGE_DIR)) return super().collect_output_ports( ports, builder, diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index 0b2c4a2599..49f7d13523 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -42,11 +42,11 @@ # It also allows Toil to do other checks on the container system. ### -# Code injected into the container communicates back to the rest of Toil -# through files in this directory. +# Code injected into containers container communicates back to the rest of Toil +# through files in a directory named this. +# Where the directory actually is is interpreter-specific. INJECTED_MESSAGE_DIR = ".toil_runtime" - # We mostly want to work with shell script strings, but CWL works with command # argument lists, so we use these functions to convert back and forth. @@ -73,7 +73,7 @@ def shell_script_to_command_line(script: str) -> list[str]: def add_injections( command_string: str, file_mounts: Iterable[tuple[str, str]], - message_dir: str = INJECTED_MESSAGE_DIR, + message_dir: str, ) -> str: """ Add resource usage monitoring and file mount checking code to a command. @@ -88,8 +88,8 @@ def add_injections( :param file_mounts: collection of (host path, container path) tuples for files mounted into the container. Code will be added to require that the container sees the complete file that the host sees. - :param message_dir: directory relative to the working directory that the - command should record resource usage to + :param message_dir: directory, absolute or relative to the working + directory that the command should record resource usage to :returns: shell command string (possibly containing multiple commands) that runs the original command and reports resource usage. @@ -246,13 +246,13 @@ def handle_message_file(file_path: str) -> None: # Treat it as if used by a child process ResourceMonitor.record_extra_cpu(cpu_seconds) -def handle_injection_messages_from_outdir(outdir: str) -> None: +def handle_injection_messages_from(message_dir: str) -> None: """ - Handle any message files in the job outdir. + Handle any message files in the given directory. Files would have been left by injected code from :meth:`add_injections()`. """ - message_dir = os.path.join(outdir, INJECTED_MESSAGE_DIR) - for file_path in glob.glob(os.path.join(message_dir, "*")): + for filename in os.listdir(message_dir): + file_path = os.path.join(message_dir, filename) if os.path.isfile(file_path): handle_message_file(file_path) diff --git a/src/toil/wdl/wdltoil.py b/src/toil/wdl/wdltoil.py index 0988767983..883cc524af 100755 --- a/src/toil/wdl/wdltoil.py +++ b/src/toil/wdl/wdltoil.py @@ -4369,7 +4369,13 @@ def get_path_in_container(inode: AnyINode) -> AnyINode | None: # when a docker container is used if isinstance(task_container, SwarmContainer): file_mounts = task_container.input_path_map.items() - command_string = add_injections(command_string, file_mounts) + # For WDL we're allowed to drop things in the current working + # directory. + command_string = add_injections( + command_string, + file_mounts, + INJECTED_MESSAGE_DIR + ) # Grab the standard out and error paths. MyPy complains if we call # them because in the current MiniWDL version they are untyped. From f5b868ed0cb54b0d3f66912fb3e15f1945264ca1 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Mon, 6 Jul 2026 19:41:07 -0400 Subject: [PATCH 16/20] Hide the usage data in the temp directory --- src/toil/cwl/cwltoil.py | 24 ++++++++++++------------ 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/src/toil/cwl/cwltoil.py b/src/toil/cwl/cwltoil.py index df26ac5351..f09e70f190 100644 --- a/src/toil/cwl/cwltoil.py +++ b/src/toil/cwl/cwltoil.py @@ -1231,17 +1231,15 @@ def job( # mounts we are going to use. file_mounts = self._file_mounts_from_pathmapper(job) script = command_line_to_shell_script(job.command_line) - # TODO: we really need to somehow add another mount to the - # container (maybe by making _file_mounts_from_pathmapper() - # actually the canonical source for mounts?) so we can put the - # messages there, because in CWL we're not allowed to drop - # extra stuff in the working directory. We can't just go a - # level up from it because that's probably not mounted, and we - # can't just work in a temp directory and move the data after - # the user code because that won't handle an OOM kill. So right - # now we fail initial_workdir_empty_writable_docker, presumably - # for having this here. - script = add_injections(script, file_mounts, INJECTED_MESSAGE_DIR) + # Since we're not allowed to drop stuff in the working + # directory, we put the resource usage info in the temp + # directory, which cwltool mounts, as divined by Anthropic + # Claude. See + # + script = add_injections(script, file_mounts, os.path.join(job.CONTAINER_TMPDIR, INJECTED_MESSAGE_DIR)) + # TODO: Can we do the collection somewhere where we have the + # job? For now just hide the value on us. + self.job_tmpdir = job.tmpdir job.command_line = shell_script_to_command_line(script) yield job @@ -1258,7 +1256,9 @@ def collect_output_ports( """ Hook output collection to also collect resource usage statistics. """ - handle_injection_messages_from(os.path.join(outdir, INJECTED_MESSAGE_DIR)) + if hasattr(self, "job_tmpdir"): + assert isinstance(self.job_tmpdir, str) + handle_injection_messages_from(os.path.join(self.job_tmpdir, INJECTED_MESSAGE_DIR)) return super().collect_output_ports( ports, builder, From a3f3cbb2af0af7f6b6a6eabeacf1eeec904931a7 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Tue, 7 Jul 2026 12:47:45 -0400 Subject: [PATCH 17/20] Make sure to stop the background monitoring process in the container --- src/toil/lib/interpreter.py | 39 +++++++++++++++++++++++++++++-------- 1 file changed, 31 insertions(+), 8 deletions(-) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index 49f7d13523..dfba936751 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -31,11 +31,11 @@ #### # Shell command injection system -# +# # When a workflow steps runs inside a container like Docker, where the contained # process is not a descendant of Toil, the Toil worker process on the host cannot # see how much CPU and RAM that step actually used. -# +# # This system allows Toil to inject code into the container that will colklect # and export resource usage information back to Toil. # @@ -73,7 +73,7 @@ def shell_script_to_command_line(script: str) -> list[str]: def add_injections( command_string: str, file_mounts: Iterable[tuple[str, str]], - message_dir: str, + message_dir: str, ) -> str: """ Add resource usage monitoring and file mount checking code to a command. @@ -114,8 +114,8 @@ def add_injections( _toil_resource_monitor () { # Turn off error checking and echo in here set +ex - MESSAGE_DIR="${1}" - mkdir -p "${MESSAGE_DIR}" + TOIL_MESSAGE_DIR="${1}" + mkdir -p "${TOIL_MESSAGE_DIR}" get_field () { INFILE="${1}" @@ -145,15 +145,35 @@ def add_injections( } while true ; do - (printf "CPU\\t" ; sample_cpu_usec ; printf "Memory\\t" ; sample_memory_bytes) >> "${MESSAGE_DIR}"/resources.tsv + (printf "CPU\\t" ; sample_cpu_usec ; printf "Memory\\t" ; sample_memory_bytes) >> "${TOIL_MESSAGE_DIR}"/resources.tsv sleep 1 done } """ ) parts.append(script) - # Launch in a subshell so that it doesn't interfere with Bash "wait" in the main shell - parts.append(f"(_toil_resource_monitor {message_dir} &)") + + # Launch in a subshell so that it doesn't interfere with "wait" in the + # main shell. + # + # We need another subshell as a background job, so we can get the PID which + # is used as the PGID for the background process group, so we can stop it + # later without job control. + parts.append(f"(set +m; (_toil_resource_monitor {message_dir} &) ) &") + + # We use a loop to try and wait on the whole process group the background + # monitoring is in by polling it, to make sure it's gone before we exit. + # See + # + # TODO: What if the user also traps EXIT? Then this won't work. + stopper_script = textwrap.dedent( + """\ + TOIL_BG_PGID="${!}" + wait + trap "while kill -0 -'${TOIL_BG_PGID}' 2>/dev/null ; do kill -9 -'${TOIL_BG_PGID}' 2>/dev/null ; done" EXIT + """ + ) + parts.append(stopper_script) if platform.system() == "Darwin": # With gRPC FUSE file sharing, files immediately downloaded before @@ -252,6 +272,9 @@ def handle_injection_messages_from(message_dir: str) -> None: Files would have been left by injected code from :meth:`add_injections()`. """ + if not os.path.isdir(message_dir): + # It's possible for the container to never have made this directory + return for filename in os.listdir(message_dir): file_path = os.path.join(message_dir, filename) if os.path.isfile(file_path): From 17916a0627b79a5a5e72f067fa9cba7ad09d13c1 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Tue, 7 Jul 2026 16:26:35 -0400 Subject: [PATCH 18/20] Make bucket creation handle retry better if the bucket got created --- src/toil/jobStores/aws/jobStore.py | 5 +++++ src/toil/lib/aws/s3.py | 23 ++++++++++++++++------- 2 files changed, 21 insertions(+), 7 deletions(-) diff --git a/src/toil/jobStores/aws/jobStore.py b/src/toil/jobStores/aws/jobStore.py index 32df188d7e..739f44f375 100644 --- a/src/toil/jobStores/aws/jobStore.py +++ b/src/toil/jobStores/aws/jobStore.py @@ -195,6 +195,11 @@ def initialize(self, config: Config) -> None: Create bucket, raise if it already exists. Set options from config. + + Note that polling for bucket existence only happens once; there's not a + good way to guard against races to create the bucket that also handles + the possibility that a request to AWS could return an error but still + create the bucket. """ logger.debug( f"Instantiating {self.__class__} for region {self.region} with bucket: '{self.bucket_name}'" diff --git a/src/toil/lib/aws/s3.py b/src/toil/lib/aws/s3.py index 9f8d7b4c4c..c8ba5c5ace 100644 --- a/src/toil/lib/aws/s3.py +++ b/src/toil/lib/aws/s3.py @@ -90,6 +90,10 @@ def create_s3_bucket( Create an AWS S3 bucket, using the given Boto3 S3 session, with the given name, in the given region. + If the bucket already exists, uses it. (It is difficult to implement + retries if this function is not idempotent and AWS can successfully create + the bucket but produce an error on the client.) + Supports the us-east-1 region, where bucket creation is special. *ALL* S3 bucket creation should use this function. @@ -107,13 +111,18 @@ def create_s3_bucket( False. """ logger.info("Creating bucket '%s' in region %s.", bucket_name, region) - if region == "us-east-1": # see https://github.com/boto/boto3/issues/125 - bucket = s3_resource.create_bucket(Bucket=bucket_name) - else: - bucket = s3_resource.create_bucket( - Bucket=bucket_name, - CreateBucketConfiguration={"LocationConstraint": region}, - ) + try: + if region == "us-east-1": # see https://github.com/boto/boto3/issues/125 + bucket = s3_resource.create_bucket(Bucket=bucket_name) + else: + bucket = s3_resource.create_bucket( + Bucket=bucket_name, + CreateBucketConfiguration={"LocationConstraint": region}, + ) + except s3_resource.meta.client.exceptions.BucketAlreadyOwnedByYou: + # We could have created the bucket, then failed, then retried. So use + # it. + bucket = s3_resource.Bucket(bucket_name) # wait until the bucket exists before adding tags bucket.wait_until_exists() From 4158f87757d7df48449012b9cfa537f4dadefe43 Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Wed, 8 Jul 2026 13:28:40 -0400 Subject: [PATCH 19/20] Pass through Interrupted exit codes in hopes of passing the fail-with-the-right-exit-code WDL test more --- src/toil/lib/interpreter.py | 4 +++- src/toil/worker.py | 17 ++++++++++++++++- 2 files changed, 19 insertions(+), 2 deletions(-) diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index dfba936751..c02daf0f24 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -166,11 +166,13 @@ def add_injections( # See # # TODO: What if the user also traps EXIT? Then this won't work. + # TODO: This also doesn't (always? ever?) work to prevent Docker Swarm from + # reporting a container as running with an exit code. stopper_script = textwrap.dedent( """\ TOIL_BG_PGID="${!}" wait - trap "while kill -0 -'${TOIL_BG_PGID}' 2>/dev/null ; do kill -9 -'${TOIL_BG_PGID}' 2>/dev/null ; done" EXIT + trap "while kill -0 -${TOIL_BG_PGID} 2>/dev/null ; do kill -9 -${TOIL_BG_PGID} 2>/dev/null ; done" EXIT """ ) parts.append(stopper_script) diff --git a/src/toil/worker.py b/src/toil/worker.py index b9529db792..9e866d5b01 100644 --- a/src/toil/worker.py +++ b/src/toil/worker.py @@ -19,6 +19,7 @@ import os import pickle import random +import re import shutil import signal import socket @@ -740,10 +741,24 @@ def blockFn() -> bool: failure_exit_code = e.code else: try: - from WDL.runtime.error import CommandFailed + from WDL.runtime.error import CommandFailed, Interrupted if isinstance(e, CommandFailed): failure_exit_code = e.exit_status + elif isinstance(e, Interrupted): + # Try to work around + # https://github.com/chanzuckerberg/miniwdl/issues/902 at + # least enough to pass the tests. + # If we observed a container supposedly running with a + # nonzero exit code, MiniWDL will bail out with an + # Interrupted with "exit code = {exit_code}" in the + # message. We need to parse it out and use it to at least + # cover the cases where that code was indeed supposed to be + # a failure and needs to make it to the leader. + found = re.search("exit code = ([0-9]+)", str(e)) + if found: + failure_exit_code = int(found.group(1)) + except ImportError: # WDL dependency not available pass From e5e2390038235785b638dd14b60242b3be52ef1c Mon Sep 17 00:00:00 2001 From: Adam Novak Date: Thu, 9 Jul 2026 11:02:28 -0700 Subject: [PATCH 20/20] Move injection back into job class but just one this time --- src/toil/cwl/cwltoil.py | 197 +++++++++++++++++++---------------- src/toil/lib/interpreter.py | 2 + src/toil/test/cwl/cwlTest.py | 5 +- 3 files changed, 114 insertions(+), 90 deletions(-) diff --git a/src/toil/cwl/cwltoil.py b/src/toil/cwl/cwltoil.py index f09e70f190..f3622e4898 100644 --- a/src/toil/cwl/cwltoil.py +++ b/src/toil/cwl/cwltoil.py @@ -92,6 +92,7 @@ get_listing, normalizeFilesDirs, visit_class, + OutputCallbackType, ) from cwl_utils.types import ( CWLDirectoryType, @@ -1168,106 +1169,126 @@ def __str__(self) -> str: # a daemon) because some of these extend DockerCommandLineJob. CHILD_PROCESS_CONTAINER_JOBS = (PodmanCommandLineJob, SingularityCommandLineJob, UDockerCommandLineJob) -class ToilCommandLineTool(ToilTool, cwltool.command_line_tool.CommandLineTool): - """Subclass the cwltool command line tool to provide the custom ToilPathMapper - and add the monitoring code to the job's container command line.""" - def _uses_container(self, runtimeContext: cwltool.context.RuntimeContext) -> bool: - """ - Returns True if this tool will be run inside a container. - """ - if not runtimeContext.use_container: - return False - docker_req, _ = self.get_requirement("DockerRequirement") - if docker_req is not None: - return True - if runtimeContext.find_default_container is not None: - return runtimeContext.find_default_container(self) is not None - return runtimeContext.default_container is not None - - # TODO: Why is this on ToilCommandLineTool? - @staticmethod - def _file_mounts_from_pathmapper( - job: ContainerCommandLineJob, - ) -> list[tuple[str, str]]: +class ToilDockerCommandLineJob(DockerCommandLineJob): + """Container job that collects resource stats from injected in-container code.""" + + def _execute( + self, + runtime: list[str], + env: MutableMapping[str, str], + runtimeContext: cwltool.context.RuntimeContext, + monitor_function: Callable[["subprocess.Popen[str]"], None] | None = None, + ) -> None: + # This job is going to run in a container that makes it not be a + # descendant of our process. So we need to inject code to count its + # resource usage. + + # First we need the command that's actually going to run. To be fully + # spec-compliant, we need to make sure Docker ENTRYPOINT keeps working. + # So we need to fetch it out, put it in the script, and remember to + # override it with a shell later. See + # + + # We know the selected container image is the last element in runtime. + # Inspect it to get its entrypoint list + image_config = json.loads( + subprocess.check_output( + [ + self.docker_exec, + "inspect", + "-f", + "{{json .Config}}", + runtime[-1], + ] + ) + ) + # Null entrypoints are allowed; Docker doesn't seem to put literal + # nulls but who knows how it might change. + entrypoint: list[str] = image_config.get("Entrypoint") or [] + + # We also need to ensure that if the CWL baseCommand is empty, the + # image command list runs. + default_command: list[str] = image_config.get("Cmd") or [] + + # Make a command string that does the expected user work + script = command_line_to_shell_script(entrypoint + (self.command_line or default_command)) + + # Our injected code also checks to make sure files mounted on Docker + # for Mac are intact, so we need to tell it about the file + # mounts we are going to use. + file_mounts = self._file_mounts_from_pathmapper() + + # Since we're not allowed to drop stuff in the working directory, we + # put the resource usage info in the temp directory, which cwltool + # mounts, as divined by Anthropic Claude. See + # + script = add_injections(script, file_mounts, os.path.join(self.CONTAINER_TMPDIR, INJECTED_MESSAGE_DIR)) + self.command_line = shell_script_to_command_line(script) + + # Adjust the runtime to clear the entrypoint. + # See . + # Stick the argument and value in before the last element. + runtime[-1:-1] = ["--entrypoint", ""] + + # By the time the base class _execute() returns, the temp directory + # holding the resource usage information has already been deleted. So + # we need to hook into one of the functions it calls, from somewhere + # where we have access to our tmpdir (like here). + def message_handler_output_callback( + output_callbacks: OutputCallbackType | None, + outputs: CWLObjectType | None, + processStatus: str, + ) -> None: + """ + Output callback that processes resource usage messages. + + Needs to be partial'd into a callback chain. + """ + handle_injection_messages_from(os.path.join(self.tmpdir, INJECTED_MESSAGE_DIR)) + if output_callbacks: + output_callbacks(outputs, processStatus) + # Hook into the output callback chain + self.output_callback = functools.partial(message_handler_output_callback, self.output_callback) + + # Run the container + super()._execute(runtime, env, runtimeContext, monitor_function) + # By the time we get here, the temp directory where we hide the + # resource usage info has already been deleted. + + def _file_mounts_from_pathmapper(self) -> list[tuple[str, str]]: """ Get all the container mounts that will be used for a containerized job. :returns: a list of (host path, container path) tuples, one per mount. """ file_mounts: list[tuple[str, str]] = [] - for location in job.pathmapper.files(): - ent = job.pathmapper.mapper(location) + for location in self.pathmapper.files(): + ent = self.pathmapper.mapper(location) if ent.type == "File" and not ent.resolved.startswith("_:"): file_mounts.append((ent.resolved, ent.target)) return file_mounts - def job( - self, - job_order: CWLObjectType, - output_callbacks: Callable[[CWLObjectType | None, str], None], - runtimeContext: cwltool.context.RuntimeContext, - ) -> Generator[ - cwltool.job.JobBase | cwltool.command_line_tool.CallbackJob, None, None - ]: - """ - Override the job method to inject resource (cpu & memory) monitoring code into - the job's container command line. - """ - for job in super().job(job_order, output_callbacks, runtimeContext): - if ( - isinstance(job, ContainerCommandLineJob) and - not isinstance(job, CHILD_PROCESS_CONTAINER_JOBS) and - self._uses_container( - runtimeContext - ) - ): - # This job is going to run in a container that makes it not be - # a descendant of our process. So we need to inject code to - # count its resource usage. - - # That code also checks to make sure files mounted on Docker - # for Mac are intact, so we need to tell it ablut the file - # mounts we are going to use. - file_mounts = self._file_mounts_from_pathmapper(job) - script = command_line_to_shell_script(job.command_line) - # Since we're not allowed to drop stuff in the working - # directory, we put the resource usage info in the temp - # directory, which cwltool mounts, as divined by Anthropic - # Claude. See - # - script = add_injections(script, file_mounts, os.path.join(job.CONTAINER_TMPDIR, INJECTED_MESSAGE_DIR)) - # TODO: Can we do the collection somewhere where we have the - # job? For now just hide the value on us. - self.job_tmpdir = job.tmpdir - job.command_line = shell_script_to_command_line(script) - yield job - - def collect_output_ports( - self, - ports: CommentedSeq | set[CWLObjectType], - builder: cwltool.builder.Builder, - outdir: str, - rcode: int, - compute_checksum: bool = True, - jobname: str = "", - readers: MutableMapping[str, CWLObjectType] | None = None, - ) -> cwltool.command_line_tool.OutputPortsType: + +class ToilCommandLineTool(ToilTool, cwltool.command_line_tool.CommandLineTool): + """Subclass the cwltool command line tool to provide the custom ToilPathMapper + and add the monitoring code to the job's container command line.""" + + def make_job_runner( + self, runtimeContext: cwltool.context.RuntimeContext + ) -> type[cwltool.job.JobBase]: """ - Hook output collection to also collect resource usage statistics. + Use a job class that monitors container resource usage if needed. + + We only act on container systems where the container isn't a descendant + process of us. """ - if hasattr(self, "job_tmpdir"): - assert isinstance(self.job_tmpdir, str) - handle_injection_messages_from(os.path.join(self.job_tmpdir, INJECTED_MESSAGE_DIR)) - return super().collect_output_ports( - ports, - builder, - outdir, - rcode, - compute_checksum, - jobname, - readers, - ) + parent_class = super().make_job_runner(runtimeContext) + if parent_class is DockerCommandLineJob: + # There's only one daemon-containerized container system: it's + # Docker itself (and not any of the subclasses of this), + return ToilDockerCommandLineJob + return parent_class def _initialworkdir( self, j: cwltool.job.JobBase | None, builder: cwltool.builder.Builder @@ -1309,7 +1330,7 @@ def _initialworkdir( # Notice that we have downloaded our inputs. Explain which files # those are here and what the task will expect to call them. self._toil_job.files_downloaded_hook(host_and_job_paths) - + class ToilExpressionTool(ToilTool, cwltool.command_line_tool.ExpressionTool): """Subclass the cwltool expression tool to provide the custom ToilPathMapper.""" diff --git a/src/toil/lib/interpreter.py b/src/toil/lib/interpreter.py index c02daf0f24..8041625ed6 100644 --- a/src/toil/lib/interpreter.py +++ b/src/toil/lib/interpreter.py @@ -225,6 +225,7 @@ def handle_message_file(file_path: str) -> None: :param file_path: the host-side path of the message file. """ + logger.debug("Handling message file %s", file_path) if os.path.basename(file_path) == "resources.tsv": # This is a TSV of resource usage info. first_cpu_usec: int | None = None @@ -276,6 +277,7 @@ def handle_injection_messages_from(message_dir: str) -> None: """ if not os.path.isdir(message_dir): # It's possible for the container to never have made this directory + logger.debug("No message files found") return for filename in os.listdir(message_dir): file_path = os.path.join(message_dir, filename) diff --git a/src/toil/test/cwl/cwlTest.py b/src/toil/test/cwl/cwlTest.py index 011fc2a7c3..db199a283c 100644 --- a/src/toil/test/cwl/cwlTest.py +++ b/src/toil/test/cwl/cwlTest.py @@ -496,6 +496,7 @@ def test_cpu_memory_monitoring(self, tmp_path: Path) -> None: with get_data("test/cwl/empty.json") as inputs_file: job_store = tmp_path / "jobStore" main_args = [ + "--logDebug", "--outdir", str(tmp_path / "output_dir"), str(cwl_file), @@ -518,8 +519,8 @@ def test_cpu_memory_monitoring(self, tmp_path: Path) -> None: assert hasattr(collated_stats, "jobs") assert hasattr(collated_stats.jobs, "total_clock") assert hasattr(collated_stats.jobs, "max_memory") - assert collated_stats.jobs.total_clock >= 30 - assert collated_stats.jobs.max_memory >= 1024 * 1024 + assert collated_stats.jobs.total_clock >= 30, f"Total clock of {collated_stats.jobs.total_clock} is too low" + assert collated_stats.jobs.max_memory >= 1024 * 1024, f"Max memory of {collated_stats.jobs.max_memory} is too low" @needs_docker @pytest.mark.docker