diff --git a/CHANGELOG.md b/CHANGELOG.md index d35f2601..ab1cb37c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,26 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ## [Unreleased] +### Changed + +- **`--timeout` precedence restored to the v1 rule; default unified at 7200s** (**behavior change**): The v2 rewrite spread timeout handling across five layers, and the sentinel was materialized into a concrete value at the CLI before the resolver ever saw it. Because the resolver then detected "the user did not pass `--timeout`" by comparing the value against the default, an explicit `--timeout 7200` was indistinguishable from no flag at all and silently lost to a model card `timeout` — in v1 it won. The CLI now forwards the user's value verbatim and precedence is applied in one place: default < model card < explicit `--timeout`, where `-1` means "not specified" and falls through, `0` means "no timeout" and beats the levels below it. Separately, the distributed path defaulted to 3600s while local execution used 7200s and `docs/cli-reference.md` documented 7200 as *the* default; SLURM runs now get the same 2h default as local runs. Resolution moved from `execution/container_runner_helpers.py` to `core/timeout.py` (re-exported from its old home) so the deployment layer shares one rule instead of reimplementing it, and `build_orchestrator`/`run_orchestrator` now agree that a manifest `timeout` of `null` means "the model card specified none". + + **Upgrade note:** a `build_manifest.json` written by 2.1.x or earlier stored `-1` as filler for every model *without* a `timeout` key (`model.get("timeout", -1)`). Since a model card's `-1` is now a real value meaning "no timeout", replaying such a manifest runs those models unbounded rather than at the 7200s default. Rebuild the manifest, or pass an explicit `--timeout`, when reusing one written before this change. + +### Fixed + +- **`--timeout 0` crashed instead of disabling the timeout**: The CLI mapped `0` to `None`, and three consumers were unprepared for it. `execution/container_runner.py` and `deployment/slurm.py` guarded with a bare `timeout > 0`, raising `TypeError: '>' not supported between instances of 'NoneType' and 'int'` on the self-managed-launcher and in-allocation paths. The SLURM job template used `{{ timeout | default(3600) }}`, but Jinja's `default` filter only substitutes for *undefined* values, so `None` rendered the literal string `--timeout None` into the generated script, which Typer then rejected. Only an `int` crosses layer boundaries now, and the sentinel is mapped to `subprocess`/`communicate` semantics by a single named guard, `core.timeout.subprocess_timeout()` — needed because `subprocess` reads `timeout=0` as "expire immediately", not "no timeout". A fourth site with the same latent bug (the model script invocation in `container_runner.py`, which reaches `communicate()`) was fixed at the same time. Regression tests added for each. + +- **SLURM runs lost their wall-clock timeout cap by default**: `_execute_distributed` forwarded the CLI's `-1` sentinel straight into `DeploymentConfig.timeout` instead of resolving it first, unlike the local path (which resolves in `container_runner.py`). `subprocess_timeout(-1)` maps to `None`, so the SLURM in-allocation path (`slurm.py`'s `_run_inside_existing_allocation`) ran with no timeout at all on any run that didn't pass an explicit `--timeout`. `_execute_distributed` now calls `resolve_run_timeout()` before building `DeploymentConfig`, restoring the 7200s default cap. The generated SLURM job script's `--timeout` argument (`job.sh.j2`) no longer uses `{{ timeout | default(3600) }}`, which only ever caught `undefined`, not `None`. + +- **A model card's `timeout` was ignored on SLURM**: SLURM resolves the timeout twice — once in `_execute_distributed`, and again inside the job, where the generated script re-invokes `madengine run`. Rendering the *resolved* value into that script made the inner run see an explicit `--timeout`, which correctly outranks the model card, so a model declaring `"timeout": 3600` silently ran under the 7200s default instead. (Before the precedence fix above this was masked: the rendered `7200` happened to equal the constant the old resolver compared against, so it read as "no flag passed".) `DeploymentConfig` now carries the two values separately — `timeout`, the resolved cap on madengine's own wait, and `cli_timeout`, the sentinel forwarded verbatim for the in-job run to resolve against the model card itself. Local runs were never affected. + +- **A model card's `timeout` was never enforced on Kubernetes**: `job.yaml.j2` never referenced the timeout at all, so K8s jobs ran the model script unbounded regardless of what the model card or `--timeout` said. Unlike SLURM, K8s has no inner `madengine run` inside the pod to re-resolve against the card, so `k8s_template_context.py` now calls `resolve_run_timeout()` at render time and the template wraps the model script in `timeout {{ timeout }}`, treating a non-positive resolved value as "run unbounded" per v1 precedence. A script killed by the timeout surfaces as exit code 124 with an explicit `model script timed out after Ns` line in the pod log. Both the launcher and direct-script paths are covered. + +- **A failing or timed-out K8s model discarded its own results**: The pod script runs under `set -e` and copies artifacts to the results PVC only after the model returns, so a non-zero model exit aborted the container before its post-scripts, `perf.csv`, and logs were published — the runs most worth diagnosing were the ones that left nothing behind. Both invocation branches in `job.yaml.j2` now capture the exit code and defer to the single `exit ${MODEL_EXIT_CODE:-0}` at the end of the script. Pre-existing on the unbounded path; the timeout wrapper added above would otherwise have extended it to timeouts. + +- **Programmatically omitting `timeout` overrode model cards**: `RunOrchestrator.execute()`, `ContainerRunner.run_container()`, and `ContainerRunner.run_models_from_manifest()` each defaulted the parameter to `DEFAULT_RUN_TIMEOUT`, but `resolve_run_timeout()` reads any non-negative value as an explicit `--timeout`. A caller that omitted the argument therefore forced 7200s over every model card, contradicting the documented precedence. All three now default to the `-1` sentinel, which still resolves to 7200s when no card specifies one. The CLI was unaffected — it always passes a value. + ## [2.1.3] - 2026-07-15 ### Added diff --git a/docs/cli-reference.md b/docs/cli-reference.md index 0e9c869b..8aea3257 100644 --- a/docs/cli-reference.md +++ b/docs/cli-reference.md @@ -223,7 +223,7 @@ madengine run [OPTIONS] | `--tags` | `-t` | TEXT | `[]` | Model tags to run (can specify multiple) | | `--manifest-file` | `-m` | TEXT | `""` | Build manifest file path (for pre-built images) | | `--registry` | `-r` | TEXT | `None` | Docker registry URL | -| `--timeout` | | INT | `-1` | Timeout in seconds (-1=default 7200s, 0=no timeout) | +| `--timeout` | | INT | `-1` | Timeout in seconds. `-1` means "not passed" and falls through to the model card's `timeout`, or 7200s if it has none; `0` disables the timeout; a positive value overrides the model card. See [Usage — Custom Timeouts](usage.md#custom-timeouts). | | `--additional-context` | `-c` | TEXT | `"{}"` | Additional context as JSON string | | `--additional-context-file` | `-f` | TEXT | `None` | File containing additional context JSON | | `--keep-alive` | | FLAG | `False` | Keep Docker containers alive after run (local Docker only; ignored with a warning on SLURM/K8s) | diff --git a/docs/configuration.md b/docs/configuration.md index f3f46d29..6168ae43 100644 --- a/docs/configuration.md +++ b/docs/configuration.md @@ -335,7 +335,9 @@ Format: Comma-separated list with hyphen ranges. ### Timeout Settings -Set a per-model timeout (seconds) in `models.json`: +Set a per-model timeout (seconds) in `models.json`. Omit the field to get the +7200s (2 hour) default. Use `0` (or any non-positive value) to run the model +without a timeout: ```json { @@ -349,6 +351,10 @@ Or use the command-line option, which overrides the model's timeout: madengine run --tags model --timeout 7200 ``` +Full precedence rules, including the `-1` sentinel and how the timeout is +applied on SLURM and Kubernetes, are in +[Usage — Custom Timeouts](usage.md#custom-timeouts). + ### Local Data Mirroring Force local data caching: diff --git a/docs/usage.md b/docs/usage.md index a79d495b..d06b8baf 100644 --- a/docs/usage.md +++ b/docs/usage.md @@ -446,6 +446,23 @@ madengine run --tags model --timeout 7200 madengine run --tags model --timeout 0 ``` +Precedence, lowest to highest: the 7200s default, then a model card's `timeout` +field, then `--timeout`. `--timeout -1` (the default) means "not passed" and +falls through to the level below, so an explicit `--timeout 7200` still +overrides a model card timeout even though it equals the default. A resolved +timeout of `0` or less means no timeout — including a model card that sets +`"timeout": 0` or `-1`. + +The same default and precedence apply to distributed runs. On SLURM the +submitting process caps its own wait at the resolved timeout, and forwards +`--timeout` unresolved to the job, so a model card's value still wins there. +On Kubernetes the timeout is resolved when the Job manifest is rendered — the +pod has no inner `madengine` to resolve it — and the model script is wrapped in +`timeout`, so a model that overruns is killed with exit code 124 and logs +`model script timed out after Ns`. A pod whose model fails or times out still +runs its post-scripts and copies its artifacts to the results PVC before +exiting on the model's code, so failed runs remain diagnosable. + ### Debugging ```bash diff --git a/docs/wiki/index.html b/docs/wiki/index.html index 4c5204d4..3afc99b5 100644 --- a/docs/wiki/index.html +++ b/docs/wiki/index.html @@ -655,15 +655,20 @@

CLI — run

--verbose / --no-verbose

Timeout resolution

+

Precedence, lowest to highest: the 7200 s default, then the model card's +timeout field, then an explicit --timeout.

- + - - - - + + + +
ValueResolved timeout
--timeoutResolved timeout
-1 (default)7200 s (2 hours)
0Disabled (no timeout)
model card timeout fieldUsed when CLI is default (-1)
Explicit positive intThat many seconds, overrides model card
-1 (default)Not passed; falls through to the model card's timeout, or 7200 s (2 hours) if the card has none
0Disabled (no timeout), overriding the model card
Explicit positive intThat many seconds, overrides the model card — including --timeout 7200, which is distinguishable from not passing the flag
< -1Rejected with exit code 4 (INVALID_ARGS)
+

A model card may also set "timeout": 0 (or any non-positive value) +to declare "no timeout". Resolution lives in +core/timeout.py and is shared by local, SLURM, and K8s runs.

@@ -842,7 +847,7 @@

Key field notes

FieldNotes n_gpus"-1" = use all GPUs on the host (MAD_SYSTEM_NGPUS). Positive int = that many GPUs. Used for perf CSV metadata. -timeoutUsed when CLI --timeout=-1 (default). Explicit CLI value always wins. +timeoutSeconds. Overrides the 7200 s default; overridden in turn by an explicit --timeout. A non-positive value (0, -1) means "no timeout". Omit the field to take the default. skip_gpu_archComma-separated GPU arch names (e.g. "gfx908,A100"). Model is skipped if detected arch matches. Disable with --disable-skip-gpu-arch. multiple_resultsPath to CSV file (relative to model dir) with per-result rows that are appended to perf.csv individually. DOCKER_IMAGE_NAME in env_varsRequired for slurm_multi: specifies the registry image for parallel srun docker pull on compute nodes. Also set automatically by DockerBuilder after a successful push. diff --git a/src/madengine/cli/commands/run.py b/src/madengine/cli/commands/run.py index c961cbe5..37c90ce0 100644 --- a/src/madengine/cli/commands/run.py +++ b/src/madengine/cli/commands/run.py @@ -24,6 +24,7 @@ ConfigurationError, ExecutionError, ) +from madengine.core.timeout import DEFAULT_RUN_TIMEOUT from ..constants import ( ExitCode, @@ -188,14 +189,14 @@ def run( effective_additional_context = repr(merged) effective_additional_context_file = None - # Convert -1 (default) to actual default timeout value (7200 seconds = 2 hours) - if timeout == -1: - timeout = 7200 - # 0 means "no timeout" per the help text — map to None so subprocess never expires - elif timeout == 0: - timeout = None - - timeout_display = "disabled" if timeout is None else f"{timeout}s" + # The sentinel is passed through untouched (-1 unspecified, 0 no timeout); + # resolve_run_timeout applies precedence against the model card downstream. + if timeout == 0: + timeout_display = "disabled" + elif timeout == -1: + timeout_display = f"{DEFAULT_RUN_TIMEOUT}s (default)" + else: + timeout_display = f"{timeout}s" try: # Check if we're doing execution-only or full workflow diff --git a/src/madengine/core/timeout.py b/src/madengine/core/timeout.py index 68e83834..00502dba 100644 --- a/src/madengine/core/timeout.py +++ b/src/madengine/core/timeout.py @@ -1,14 +1,79 @@ #!/usr/bin/env python3 -"""Module to define the Timeout class. +"""Module to define the Timeout class and run-timeout resolution. -This module provides the Timeout class to handle timeouts. +This module provides the Timeout class to handle timeouts, plus the single +definition of how a run timeout is resolved and how it maps onto subprocess +semantics. + +Resolution follows madengine v1: the default is overridden by the model card, +which is overridden by an explicit ``--timeout``. Only the CLI has a sentinel, +``-1``, meaning "not passed". Any non-positive resolved timeout runs unbounded, +which both consumers below already implement. Copyright (c) Advanced Micro Devices, Inc. All rights reserved. """ # built-in modules import signal +import typing from typing import Optional +# Default run timeout (2 hours). Single source of truth for local and +# distributed execution alike. +DEFAULT_RUN_TIMEOUT = 7200 + + +def resolve_run_timeout( + model_info: typing.Dict, + cli_timeout: typing.Optional[int], + default_timeout: int = DEFAULT_RUN_TIMEOUT, +) -> int: + """Resolve the effective run timeout. + + Precedence, lowest to highest: default < model card < CLI. A model card's + ``timeout`` is taken as-is, including a non-positive one, which means the + author asked for no timeout. A CLI timeout of -1 means ``--timeout`` was + not passed and falls through to the level below. + + ``None`` in the model card is ignored so that manifests written by older + builds (which store ``null`` for an absent timeout) still load. + + Args: + model_info: Model info dict; may have a "timeout" key. + cli_timeout: Timeout from the CLI; -1 if not passed. + default_timeout: Value used when neither level specifies one. + + Returns: + int: Effective timeout in seconds; non-positive means no timeout. + """ + timeout = default_timeout + + model_timeout = model_info.get("timeout") + if model_timeout is not None: + timeout = model_timeout + + if cli_timeout is not None and cli_timeout >= 0: + timeout = cli_timeout + + return timeout + + +def subprocess_timeout(timeout: typing.Optional[int]) -> Optional[int]: + """Map a resolved timeout onto ``subprocess``/``communicate`` semantics. + + ``subprocess`` treats ``timeout=0`` as "expire immediately", not as "no + timeout", so a non-positive timeout cannot be passed through directly and + becomes ``None`` instead. + + Args: + timeout: Resolved timeout in seconds; non-positive means no timeout. + + Returns: + Optional[int]: Seconds to pass to subprocess, or None for no timeout. + """ + if timeout is None or timeout <= 0: + return None + return timeout + class Timeout: """Class to handle timeouts. diff --git a/src/madengine/deployment/base.py b/src/madengine/deployment/base.py index 69e1367e..53261482 100644 --- a/src/madengine/deployment/base.py +++ b/src/madengine/deployment/base.py @@ -19,6 +19,8 @@ from jinja2 import Environment, FileSystemLoader from rich.console import Console +from madengine.core.timeout import DEFAULT_RUN_TIMEOUT + # Regex for parsing "performance: " log lines. # Value: optional sign, integer/decimal, scientific notation (e or E). @@ -67,7 +69,15 @@ class DeploymentConfig: target: str # "slurm", "k8s" (NOT "local" - that uses container_runner) manifest_file: str additional_context: Dict[str, Any] = field(default_factory=dict) - timeout: int = 3600 + timeout: int = DEFAULT_RUN_TIMEOUT + # The CLI's --timeout verbatim (-1 unspecified, 0 no timeout), for the + # generated job script to forward to the madengine it re-invokes. Distinct + # from `timeout` above, which is this process's own wall-clock cap and must + # already be resolved. Resolving both alike would flatten the sentinel into + # a concrete value, which the inner CLI cannot distinguish from an explicit + # --timeout and would therefore rank above the model card. Defaults to the + # sentinel so a config built without it forwards "unspecified". + cli_timeout: int = -1 monitor: bool = True cleanup_on_failure: bool = True diff --git a/src/madengine/deployment/k8s_template_context.py b/src/madengine/deployment/k8s_template_context.py index e38b251a..6063291d 100644 --- a/src/madengine/deployment/k8s_template_context.py +++ b/src/madengine/deployment/k8s_template_context.py @@ -31,6 +31,7 @@ from madengine.core.additional_context_defaults import DEFAULT_GUEST_OS from madengine.core.dataprovider import Data from madengine.core.errors import ConfigurationError +from madengine.core.timeout import resolve_run_timeout from madengine.utils.gpu_config import resolve_runtime_gpus from madengine.utils.path_utils import get_madengine_root @@ -545,7 +546,11 @@ def _prepare_template_context( "nnodes": nnodes, "nproc_per_node": nproc_per_node, "master_port": master_port, - "timeout": self.config.timeout, + # Resolved here, not taken from config.timeout: that value caps the + # submitting process's wait on the Job and never saw the model card. + # K8s has no inner madengine to re-resolve against the card (unlike + # SLURM), so the card's timeout has to be applied at render time. + "timeout": resolve_run_timeout(model_info, self.config.cli_timeout), # Environment - Merge base env vars with data/tools env vars "env_vars": self._prepare_env_vars(model_info), # Volumes diff --git a/src/madengine/deployment/slurm.py b/src/madengine/deployment/slurm.py index 088a3fb2..7dd2d43c 100644 --- a/src/madengine/deployment/slurm.py +++ b/src/madengine/deployment/slurm.py @@ -29,6 +29,7 @@ ) from .config_loader import ConfigLoader, apply_deployment_config from .slurm_node_selector import SlurmNodeSelector +from madengine.core.timeout import subprocess_timeout from madengine.utils.gpu_config import resolve_runtime_gpus from madengine.utils.run_details import get_build_number, get_pipeline from madengine.utils.path_utils import scripts_base_dir_from @@ -712,7 +713,9 @@ def debug(self, msg): "shared_workspace": self.slurm_config.get("shared_workspace"), "shared_data": self.config.additional_context.get("shared_data"), "results_dir": self.slurm_config.get("results_dir"), - "timeout": self.config.timeout, + # The sentinel, not config.timeout: the job script re-invokes + # madengine, and that run applies model-card precedence itself. + "timeout": self.config.cli_timeout, "live_output": self.config.additional_context.get("live_output", False), "tags": " ".join(model_info.get("tags", [])), "multiple_results": model_info.get("multiple_results"), @@ -1288,7 +1291,7 @@ def _run_inside_existing_allocation(self) -> DeploymentResult: # Don't capture output - let it stream directly to console result = subprocess.run( ["bash", str(self.script_path)], - timeout=self.config.timeout if self.config.timeout > 0 else None, + timeout=subprocess_timeout(self.config.timeout), ) if result.returncode == 0: diff --git a/src/madengine/deployment/templates/kubernetes/job.yaml.j2 b/src/madengine/deployment/templates/kubernetes/job.yaml.j2 index 320d049f..c7599dee 100644 --- a/src/madengine/deployment/templates/kubernetes/job.yaml.j2 +++ b/src/madengine/deployment/templates/kubernetes/job.yaml.j2 @@ -300,14 +300,40 @@ spec: {% endfor %} {% endif %} - # Execute launcher with tool chain - MODEL_START_TIME=$(date +%s.%N) + # Execute launcher with tool chain. + # The command goes into a file so `timeout` can wrap it: a tool chain + # may begin with an environment assignment, which `timeout` would + # otherwise try to exec as the program name. + cat > /tmp/run_model.sh << 'MODEL_EOF' {% if launcher_tool_chain and launcher_tool_chain != "bash /tmp/run_launcher.sh" %} {{ launcher_tool_chain }} {% else %} bash /tmp/run_launcher.sh {% endif %} - MODEL_EXIT_CODE=$? + MODEL_EOF + MODEL_START_TIME=$(date +%s.%N) + # The model's exit code is captured, not acted on: `set -e` is in + # effect, so a bare invocation would abort the container before the + # post-scripts and artifact copy below. The script exits on it once, + # at the end, after the results have been published. + {% if timeout > 0 %} + echo "⏰ Setting timeout to {{ timeout }} seconds." + if timeout {{ timeout }} bash /tmp/run_model.sh; then + MODEL_EXIT_CODE=0 + else + MODEL_EXIT_CODE=$? + if [ "$MODEL_EXIT_CODE" -eq 124 ]; then + echo "ERROR: model script timed out after {{ timeout }}s" + fi + fi + {% else %} + echo "⏰ No timeout set; the model script runs unbounded." + if bash /tmp/run_model.sh; then + MODEL_EXIT_CODE=0 + else + MODEL_EXIT_CODE=$? + fi + {% endif %} MODEL_END_TIME=$(date +%s.%N) MODEL_DURATION=$(awk "BEGIN {printf \"%.6f\", $MODEL_END_TIME - $MODEL_START_TIME}") echo "test_duration: ${MODEL_DURATION}s" @@ -480,14 +506,42 @@ spec: {% endfor %} {% endif %} - # Execute script with tool chain - MODEL_START_TIME=$(date +%s.%N) + # Execute script with tool chain. + # The command goes into a file so `timeout` can wrap it: a tool + # chain may begin with an environment assignment, which `timeout` + # would otherwise try to exec as the program name. The heredoc + # terminator sits at the block scalar's own indent so that it + # lands in column 0 of the rendered shell script. + cat > /tmp/run_model.sh << 'MODEL_EOF' {% if direct_script_tool_chain and direct_script_tool_chain != "bash " ~ model_script %} {{ direct_script_tool_chain }} {% else %} bash {{ model_script }} {% endif %} - MODEL_EXIT_CODE=$? + MODEL_EOF + MODEL_START_TIME=$(date +%s.%N) + # The model's exit code is captured, not acted on: `set -e` is + # in effect, so a bare invocation would abort the container + # before the post-scripts and artifact copy below. The script + # exits on it once, at the end, after publishing the results. + {% if timeout > 0 %} + echo "⏰ Setting timeout to {{ timeout }} seconds." + if timeout {{ timeout }} bash /tmp/run_model.sh; then + MODEL_EXIT_CODE=0 + else + MODEL_EXIT_CODE=$? + if [ "$MODEL_EXIT_CODE" -eq 124 ]; then + echo "ERROR: model script timed out after {{ timeout }}s" + fi + fi + {% else %} + echo "⏰ No timeout set; the model script runs unbounded." + if bash /tmp/run_model.sh; then + MODEL_EXIT_CODE=0 + else + MODEL_EXIT_CODE=$? + fi + {% endif %} MODEL_END_TIME=$(date +%s.%N) MODEL_DURATION=$(awk "BEGIN {printf \"%.6f\", $MODEL_END_TIME - $MODEL_START_TIME}") echo "test_duration: ${MODEL_DURATION}s" diff --git a/src/madengine/deployment/templates/slurm/job.sh.j2 b/src/madengine/deployment/templates/slurm/job.sh.j2 index 4cdddba0..c692acf3 100644 --- a/src/madengine/deployment/templates/slurm/job.sh.j2 +++ b/src/madengine/deployment/templates/slurm/job.sh.j2 @@ -677,7 +677,7 @@ echo "[DEBUG] $(date -Iseconds) Node ${SLURM_PROCID} ($(hostname)): about to run # Environment variables (MASTER_ADDR, MAD_MULTI_NODE_RUNNER, etc.) are inherited $MAD_CLI_COMMAND run \ --manifest-file "$EXEC_MANIFEST" \ - --timeout {{ timeout | default(3600) }} \ + --timeout {{ timeout }} \ {% if shared_data %}--force-mirror-local {{ shared_data }}{% endif %} \ {% if live_output %}--live-output{% endif %} \ > "${NODE_LOG_OUT}" 2>> "${NODE_LOG_ERR}" @@ -766,7 +766,7 @@ echo "" # Environment variables (MASTER_ADDR, MAD_MULTI_NODE_RUNNER, etc.) are inherited $MAD_CLI_COMMAND run \ {% if manifest_file %}--manifest-file "$EXEC_MANIFEST"{% else %}--tags {{ tags }}{% endif %} \ - --timeout {{ timeout | default(3600) }} \ + --timeout {{ timeout }} \ {% if shared_data %}--force-mirror-local {{ shared_data }}{% endif %} \ {% if live_output %}--live-output{% endif %} diff --git a/src/madengine/execution/container_runner.py b/src/madengine/execution/container_runner.py index eab4af7d..d0bc67c5 100644 --- a/src/madengine/execution/container_runner.py +++ b/src/madengine/execution/container_runner.py @@ -23,7 +23,7 @@ from madengine.core.console import Console, redact_secrets from madengine.core.context import Context from madengine.core.docker import Docker -from madengine.core.timeout import Timeout +from madengine.core.timeout import Timeout, subprocess_timeout from madengine.core.dataprovider import Data from madengine.utils.ops import PythonicTee, file_print from madengine.reporting.update_perf_csv import ( @@ -985,7 +985,7 @@ def _run_self_managed( shell=True, cwd=script_dir, env=env, - timeout=timeout if timeout > 0 else None, + timeout=subprocess_timeout(timeout), ) run_results["test_duration"] = time.time() - test_start_time @@ -1100,7 +1100,7 @@ def run_container( keep_alive: bool = False, keep_model_dir: bool = False, skip_model_run: bool = False, - timeout: int = 7200, + timeout: int = -1, tools_json_file: str = "scripts/common/tools.json", phase_suffix: str = "", generate_sys_env_details: bool = True, @@ -1114,7 +1114,8 @@ def run_container( keep_alive: Whether to keep container alive after execution keep_model_dir: Whether to keep model directory after execution skip_model_run: Whether to skip the model script invocation - timeout: Execution timeout in seconds + timeout: Execution timeout in seconds; -1 (unspecified) defers to + the model card, then to DEFAULT_RUN_TIMEOUT tools_json_file: Path to tools configuration file phase_suffix: Suffix for log file name (e.g., ".run" or "") generate_sys_env_details: Whether to collect system environment details @@ -1600,11 +1601,12 @@ def run_container( else: self.rich_console.print("[bold blue]Running model...[/bold blue]") # Use the container timeout (default 7200s) for script execution - # to prevent indefinite hangs + # to prevent indefinite hangs. A resolved timeout of 0 means + # "no timeout", which communicate() spells as None. try: model_output = model_docker.sh( f"cd {model_dir} && {script_name} {model_args}", - timeout=timeout, + timeout=subprocess_timeout(timeout), ) except RuntimeError as run_err: # On script failure, collect lightweight diagnostics from the @@ -2764,7 +2766,7 @@ def run_models_from_manifest( self, manifest_file: str, registry: str = None, - timeout: int = 7200, + timeout: int = -1, keep_alive: bool = False, keep_model_dir: bool = False, skip_model_run: bool = False, @@ -2777,7 +2779,8 @@ def run_models_from_manifest( Args: manifest_file: Path to build_manifest.json registry: Optional registry override - timeout: Execution timeout per model in seconds + timeout: Execution timeout per model in seconds; -1 (unspecified) + defers to each model card, then to DEFAULT_RUN_TIMEOUT keep_alive: Whether to keep containers alive after execution keep_model_dir: Whether to keep model directory after execution skip_model_run: Whether to skip the model script invocation diff --git a/src/madengine/execution/container_runner_helpers.py b/src/madengine/execution/container_runner_helpers.py index 120d3144..c8ea6e47 100644 --- a/src/madengine/execution/container_runner_helpers.py +++ b/src/madengine/execution/container_runner_helpers.py @@ -8,6 +8,10 @@ import re import typing +# Timeout resolution lives in core.timeout so the deployment layer can share it; +# re-exported here for existing importers. +from madengine.core.timeout import resolve_run_timeout # noqa: F401 + # Default substrings matched in container run logs post-hoc (see ContainerRunner). DEFAULT_LOG_ERROR_PATTERNS: typing.Tuple[str, ...] = ( "OutOfMemoryError", @@ -200,35 +204,6 @@ def resolve_run_status( return "FAILURE", "no performance metrics" -def resolve_run_timeout( - model_info: typing.Dict, - cli_timeout: int, - default_cli_timeout: int = 7200, -) -> int: - """ - Resolve effective run timeout from model config and CLI. - - - If model has a timeout and CLI is using default (7200), use model's timeout. - - If CLI timeout is explicitly set (not default), it overrides model timeout. - - Args: - model_info: Model info dict; may have "timeout" key. - cli_timeout: Timeout from CLI. - default_cli_timeout: Value considered "default" for CLI (typically 7200). - - Returns: - Effective timeout in seconds. - """ - if ( - "timeout" in model_info - and model_info["timeout"] is not None - and model_info["timeout"] > 0 - and cli_timeout == default_cli_timeout - ): - return model_info["timeout"] - return cli_timeout - - def _docker_image_ref_for_log_naming(docker_image: str) -> str: """ Reduce a Docker image reference to a stable filename-safe log naming component. diff --git a/src/madengine/orchestration/build_orchestrator.py b/src/madengine/orchestration/build_orchestrator.py index 17e836e8..48428b89 100644 --- a/src/madengine/orchestration/build_orchestrator.py +++ b/src/madengine/orchestration/build_orchestrator.py @@ -567,7 +567,9 @@ def _execute_with_prebuilt_image( "training_precision": model.get("training_precision", ""), "multiple_results": model.get("multiple_results", ""), "tags": model.get("tags", []), - "timeout": model.get("timeout", -1), + # None (JSON null) = the card specified none; a card's -1 is + # a real value meaning "no timeout", not filler. + "timeout": model.get("timeout"), "args": model.get("args", ""), "slurm": model.get("slurm", {}), "distributed": model_distributed, diff --git a/src/madengine/orchestration/run_orchestrator.py b/src/madengine/orchestration/run_orchestrator.py index 498401c4..152aae3a 100644 --- a/src/madengine/orchestration/run_orchestrator.py +++ b/src/madengine/orchestration/run_orchestrator.py @@ -25,6 +25,7 @@ from madengine.core.auth import load_credentials from madengine.core.context import Context from madengine.core.dataprovider import Data +from madengine.core.timeout import resolve_run_timeout from madengine.core.errors import ( BuildError, ConfigurationError, @@ -132,7 +133,7 @@ def execute( manifest_file: Optional[str] = None, tags: Optional[list] = None, registry: Optional[str] = None, - timeout: int = 3600, + timeout: int = -1, ) -> Dict: """ Execute run workflow. @@ -149,7 +150,8 @@ def execute( manifest_file: Path to build_manifest.json tags: Model tags to build (triggers build phase if no manifest) registry: Optional registry override - timeout: Execution timeout in seconds + timeout: Execution timeout in seconds; -1 (unspecified) defers to + the model card, then to DEFAULT_RUN_TIMEOUT Returns: Execution results dict @@ -453,7 +455,9 @@ def _create_manifest_from_local_image( "owner": model.get("owner", ""), "training_precision": model.get("training_precision", ""), "args": model.get("args", ""), # Required field for docker run - "timeout": model.get("timeout", None), # Optional timeout override + # None (JSON null) = the card specified none; a card's -1 is a + # real value meaning "no timeout", so it cannot double as filler. + "timeout": model.get("timeout"), "data": data_str, "cred": model.get("cred", ""), "deprecated": model.get("deprecated", False), @@ -730,7 +734,19 @@ def _execute_distributed(self, target: str, manifest_file: str) -> Dict: target=target, manifest_file=manifest_file, additional_context=self.additional_context, - timeout=getattr(self.args, "timeout", 3600), + # Two different values, deliberately. `timeout` caps this process's + # own wait on the deployment, so the sentinel has to be resolved + # here -- left raw, subprocess_timeout(-1) is None and the SLURM + # in-allocation path runs unbounded. `cli_timeout` is what the + # generated job script forwards to the madengine it re-invokes, and + # must stay verbatim: that inner run resolves against the model card + # itself, and a concrete value here would read as an explicit + # --timeout and outrank the card. No model card is consulted at this + # level, hence the empty dict. + timeout=resolve_run_timeout( + {}, getattr(self.args, "timeout", -1) + ), + cli_timeout=getattr(self.args, "timeout", -1), monitor=self.additional_context.get("monitor", True), cleanup_on_failure=self.additional_context.get("cleanup_on_failure", True), ) diff --git a/tests/e2e/test_execution_features.py b/tests/e2e/test_execution_features.py index 4d7fd601..2147b109 100644 --- a/tests/e2e/test_execution_features.py +++ b/tests/e2e/test_execution_features.py @@ -39,6 +39,9 @@ class TestCustomTimeoutsFunctionality: ("dummy_timeout", "dummy_timeout_dummy", "360", ""), ("dummy", "dummy_dummy", "120", "--timeout 120"), ("dummy_timeout", "dummy_timeout_dummy", "120", "--timeout 120"), + # An explicit --timeout that happens to equal the default still + # beats the model card: the sentinel, not the value, marks "unset". + ("dummy_timeout", "dummy_timeout_dummy", "7200", "--timeout 7200"), ], ) def test_timeout_value_in_log( diff --git a/tests/unit/test_cli.py b/tests/unit/test_cli.py index 164ac7a5..312ca8c2 100644 --- a/tests/unit/test_cli.py +++ b/tests/unit/test_cli.py @@ -460,6 +460,40 @@ def test_run_invalid_timeout_exits_invalid_args(self, runner: CliRunner) -> None ) assert result.exit_code == ExitCode.INVALID_ARGS + @pytest.mark.parametrize("cli_timeout", ["0", "-1", "120"]) + def test_run_forwards_timeout_sentinel_unchanged( + self, runner: CliRunner, cli_timeout: str + ) -> None: + """The CLI must hand the sentinel to the orchestrator as an int, verbatim. + + Regression: --timeout 0 used to be rewritten to None here, and every + downstream consumer then had to defend against it. Two did not, and + raised TypeError on `timeout > 0` (container_runner self-managed path + and slurm.py). Precedence against the model card belongs to + resolve_run_timeout, not to this layer. + """ + run_module = importlib.import_module("madengine.cli.commands.run") + mock_orch = MagicMock() + mock_orch.execute.return_value = {"successful_runs": [], "failed_runs": []} + with patch.object(run_module, "RunOrchestrator", return_value=mock_orch): + runner.invoke( + app, + [ + "run", + "--tags", + "some_model", + "--timeout", + cli_timeout, + "--additional-context", + '{"gpu_vendor": "AMD", "guest_os": "UBUNTU"}', + ], + ) + + assert mock_orch.execute.called, "orchestrator was never invoked" + forwarded = mock_orch.execute.call_args.kwargs["timeout"] + assert forwarded == int(cli_timeout) + assert isinstance(forwarded, int), f"None/str leaked downstream: {forwarded!r}" + def test_run_help_exits_zero(self, runner: CliRunner) -> None: """CLI help is reachable in CI without GPU.""" result = runner.invoke(app, ["run", "--help"]) diff --git a/tests/unit/test_container_runner.py b/tests/unit/test_container_runner.py index aae79321..2b2692b2 100644 --- a/tests/unit/test_container_runner.py +++ b/tests/unit/test_container_runner.py @@ -523,3 +523,184 @@ def test_skip_model_run_false_does_exec_script(self): assert any( "run.sh" in c and "cd " in c for c in docker_sh_calls ), f"Model script was not executed: {docker_sh_calls}" + + +class TestRunModelsFromManifestDefaultTimeoutIsSentinel: + """The manifest entry point must forward the sentinel, not a concrete 7200. + + Same regression as run_container(): a DEFAULT_RUN_TIMEOUT default here would + reach run_container() as an explicit CLI timeout and outrank every card. + """ + + def _forwarded_timeout(self, tmp_path, **kwargs): + manifest_path = str(tmp_path / "build_manifest.json") + manifest = { + "built_images": {"img1": {"docker_image": "local/img1", "dockerfile": "D"}}, + "built_models": { + "img1": {"name": "test/model", "tags": "t1", "n_gpus": "1", "args": ""} + }, + } + with open(manifest_path, "w") as f: + json.dump(manifest, f) + + ctx = MagicMock() + ctx.ctx = {"docker_env_vars": {}} + ctx.ensure_runtime_context = MagicMock() + mock_console = MagicMock() + mock_console.sh.return_value = "testhost" + runner = ContainerRunner(context=ctx, console=mock_console) + runner.perf_csv_path = str(tmp_path / "perf.csv") + runner.set_credentials({}) + + with patch.object( + runner, "run_container", return_value={"status": "SUCCESS"} + ) as mock_run: + runner.run_models_from_manifest(manifest_file=manifest_path, **kwargs) + + mock_run.assert_called_once() + return mock_run.call_args.kwargs["timeout"] + + def test_omitted_timeout_forwards_the_sentinel(self, tmp_path): + assert self._forwarded_timeout(tmp_path) == -1 + + def test_explicit_timeout_forwarded_verbatim(self, tmp_path): + assert self._forwarded_timeout(tmp_path, timeout=120) == 120 + + @pytest.mark.parametrize("sentinel", [0, -1]) + def test_no_timeout_sentinels_forwarded_verbatim(self, tmp_path, sentinel): + assert self._forwarded_timeout(tmp_path, timeout=sentinel) == sentinel + + +class TestRunContainerDefaultTimeoutIsSentinel: + """Omitting `timeout` must not outrank a model card. + + Regression: the parameter defaulted to DEFAULT_RUN_TIMEOUT, but + resolve_run_timeout() reads any non-negative value as an explicit + --timeout, so a programmatic caller that omitted the argument silently + forced 7200s over the card. The default is the -1 sentinel instead, which + still resolves to 7200s when no card timeout exists. + """ + + def _resolved_timeout(self, model_info, **kwargs): + """Run run_container with mocks and return what reached subprocess.""" + harness = TestRunContainerSkipModelRun() + runner = harness._make_runner() + docker_sh_timeouts = [] + + from contextlib import contextmanager + + from madengine.core.docker import Docker + + @contextmanager + def noop_timeout(_): + yield + + with patch.object(ContainerRunner, "_resolve_docker_image", return_value="ci-dummy"), \ + patch.object(ContainerRunner, "get_gpu_arg", return_value=""), \ + patch.object(ContainerRunner, "get_cpu_arg", return_value=""), \ + patch.object(ContainerRunner, "get_env_arg", return_value=""), \ + patch.object(ContainerRunner, "get_mount_arg", return_value=""), \ + patch.object(ContainerRunner, "gather_system_env_details"), \ + patch.object(ContainerRunner, "ensure_perf_csv_exists"), \ + patch("madengine.utils.rocm_path_resolver.finalize_container_rocm_path"), \ + patch("madengine.execution.container_runner._print_run_env_table"), \ + patch("madengine.execution.container_runner.Timeout", noop_timeout), \ + patch.object(Docker, "__init__", return_value=None), \ + patch.object(Docker, "sh", + side_effect=lambda cmd, **kw: docker_sh_timeouts.append( + (cmd, kw.get("timeout")) + ) or "ok"), \ + patch.object(Docker, "__del__", return_value=None), \ + patch("builtins.open", mock_open(read_data="")): + runner.run_container( + model_info=model_info, docker_image="ci-dummy", **kwargs + ) + + model_runs = [ + t for cmd, t in docker_sh_timeouts if "run.sh" in cmd and cmd.startswith("cd ") + ] + assert len(model_runs) == 1, docker_sh_timeouts + return model_runs[0] + + def _model_info(self, card_timeout=None): + info = { + "name": "dummy", + "scripts": "scripts/dummy/run.sh", + "args": "", + "n_gpus": "1", + "tags": [], + } + if card_timeout is not None: + info["timeout"] = card_timeout + return info + + def test_model_card_wins_when_timeout_omitted(self): + assert self._resolved_timeout(self._model_info(card_timeout=360)) == 360 + + def test_card_asking_for_no_timeout_is_honored_when_omitted(self): + assert self._resolved_timeout(self._model_info(card_timeout=-1)) is None + + def test_default_still_applies_without_a_card_timeout(self): + from madengine.core.timeout import DEFAULT_RUN_TIMEOUT + + assert self._resolved_timeout(self._model_info()) == DEFAULT_RUN_TIMEOUT + + def test_explicit_timeout_still_outranks_the_card(self): + assert ( + self._resolved_timeout(self._model_info(card_timeout=360), timeout=120) + == 120 + ) + + +class TestSelfManagedLauncherTimeout: + """`--timeout 0` (no timeout) must reach subprocess.run as None, not 0. + + Regression: the call site read `timeout if timeout > 0 else None`, which + raised TypeError once the CLI started handing down None for "no timeout", + and would have expired the run instantly under the int sentinel: + subprocess spells "no timeout" as None, and treats 0 as "expire now". + """ + + def _make_runner(self): + runner = ContainerRunner.__new__(ContainerRunner) + runner.context = MagicMock() + runner.context.ctx = {} + runner.console = MagicMock() + runner.rich_console = MagicMock() + runner.live_output = False + runner.additional_context = {} + return runner + + def _invoke(self, tmp_path, timeout): + script = tmp_path / "run.sh" + script.write_text("#!/bin/bash\nexit 0\n") + run_results = {} + with patch( + "madengine.execution.container_runner.subprocess.run", + return_value=subprocess.CompletedProcess("", 0), + ) as mock_run: + self._make_runner()._run_self_managed( + model_info={"name": "dummy", "scripts": str(script), "args": ""}, + build_info={}, + log_file_path=str(tmp_path / "run.live.log"), + timeout=timeout, + run_results=run_results, + pre_encapsulate_post_scripts={}, + run_env={}, + ) + mock_run.assert_called_once() + return mock_run.call_args.kwargs["timeout"] + + @pytest.mark.parametrize("timeout", [0, -1]) + def test_no_timeout_sentinels_become_none(self, tmp_path, timeout): + assert self._invoke(tmp_path, timeout) is None + + def test_legacy_none_does_not_raise_type_error(self, tmp_path): + # The bare `timeout > 0` this replaced raised TypeError on None, which + # is what the CLI used to send for --timeout 0. The sentinel contract + # keeps None out of here now, but manifests and older callers still + # carry it, so the guard must absorb it rather than crash. + assert self._invoke(tmp_path, None) is None + + def test_positive_timeout_passed_through(self, tmp_path): + assert self._invoke(tmp_path, 120) == 120 diff --git a/tests/unit/test_execution.py b/tests/unit/test_execution.py index dc18121e..0b4e612d 100644 --- a/tests/unit/test_execution.py +++ b/tests/unit/test_execution.py @@ -1,12 +1,18 @@ """Unit tests for execution: container_runner_helpers and dockerfile_utils.""" +import json + import pytest -from madengine.core.timeout import Timeout +from madengine.core.timeout import ( + DEFAULT_RUN_TIMEOUT, + Timeout, + resolve_run_timeout, + subprocess_timeout, +) from madengine.execution.container_runner_helpers import ( _docker_image_ref_for_log_naming, make_run_log_file_path, - resolve_run_timeout, ) from madengine.execution.dockerfile_utils import ( GPU_ARCH_VARIABLES, @@ -40,33 +46,91 @@ def test_positive_seconds_raises_on_expiry(self): # ---- container_runner_helpers ---- class TestResolveRunTimeout: - """resolve_run_timeout behavior.""" + """resolve_run_timeout: default < model card < CLI (v1 precedence). - def test_model_timeout_used_when_cli_default(self): - assert resolve_run_timeout({"timeout": 3600}, 7200) == 3600 - assert resolve_run_timeout({"timeout": 100}, 7200) == 100 + Only the CLI has a sentinel: -1 means --timeout was not passed. A model + card timeout is taken as-is, and any non-positive result means no timeout. + """ - def test_cli_timeout_used_when_explicit(self): - assert resolve_run_timeout({"timeout": 3600}, 6000) == 6000 - assert resolve_run_timeout({"timeout": 3600}, 100) == 100 - - def test_cli_default_returned_when_no_model_timeout(self): - assert resolve_run_timeout({}, 7200) == 7200 - assert resolve_run_timeout({"name": "x"}, 3600) == 3600 + def test_default_when_nothing_specified(self): + assert resolve_run_timeout({}, -1) == DEFAULT_RUN_TIMEOUT + assert resolve_run_timeout({"name": "x"}, -1) == DEFAULT_RUN_TIMEOUT - @pytest.mark.parametrize("model_timeout", [None, 0]) - def test_falsy_model_timeout_ignored_uses_cli(self, model_timeout): - assert resolve_run_timeout({"timeout": model_timeout}, 7200) == 7200 + def test_model_timeout_overrides_default(self): + assert resolve_run_timeout({"timeout": 360}, -1) == 360 + assert resolve_run_timeout({"timeout": 100}, -1) == 100 - def test_custom_default_cli(self): - assert resolve_run_timeout({"timeout": 100}, 5000, default_cli_timeout=5000) == 100 - assert resolve_run_timeout({"timeout": 100}, 7200, default_cli_timeout=5000) == 7200 + def test_cli_timeout_overrides_model(self): + assert resolve_run_timeout({"timeout": 360}, 120) == 120 + assert resolve_run_timeout({"timeout": 3600}, 6000) == 6000 - def test_no_timeout_sentinel_none_passthrough(self): - # --timeout 0 is converted to None by the CLI; resolve_run_timeout must - # pass None through unchanged (model timeout must NOT override "no timeout"). - assert resolve_run_timeout({"timeout": 3600}, None) is None - assert resolve_run_timeout({}, None) is None + def test_explicit_cli_equal_to_default_still_wins(self): + # Regression: the old resolver detected "CLI is default" by comparing + # against 7200, so an explicit --timeout 7200 silently lost to the model + # card. With the -1 sentinel the two are distinguishable. + assert resolve_run_timeout({"timeout": 360}, DEFAULT_RUN_TIMEOUT) == DEFAULT_RUN_TIMEOUT + + def test_non_positive_means_no_timeout(self): + # v1 read the model card unconditionally and handed the value to + # Timeout/subprocess, where anything non-positive runs unbounded. Real + # MAD cards set -1 for exactly that, so it must not fall through to the + # 7200s default. + assert resolve_run_timeout({"timeout": -1}, -1) == -1 + assert resolve_run_timeout({"timeout": 0}, -1) == 0 + assert resolve_run_timeout({"timeout": 360}, 0) == 0 + assert resolve_run_timeout({}, 0) == 0 + + def test_none_in_manifest_treated_as_unset(self): + # Manifests store null when the card specified no timeout. + assert resolve_run_timeout({"timeout": None}, -1) == DEFAULT_RUN_TIMEOUT + assert resolve_run_timeout({"timeout": None}, 120) == 120 + + @pytest.mark.parametrize( + "card_timeout, expected", + [ + (None, DEFAULT_RUN_TIMEOUT), # no "timeout" key in the model card + (-1, -1), # explicit "no timeout" + (0, 0), + (360, 360), + ], + ) + def test_manifest_round_trip_preserves_intent(self, card_timeout, expected): + """A card's timeout must survive being written to a manifest and read back. + + Regression: the manifest writers filled an absent timeout with -1, which + collided with a card explicitly setting -1 to mean "no timeout". A card + with no timeout field then resolved to unbounded instead of the 7200s + default. The filler is None (JSON null), which cannot be a real value. + """ + model_card = {} if card_timeout is None else {"timeout": card_timeout} + # Mirrors the manifest writers in build_orchestrator/run_orchestrator. + manifest_entry = json.loads(json.dumps({"timeout": model_card.get("timeout")})) + assert resolve_run_timeout(manifest_entry, -1) == expected + + def test_custom_default(self): + assert resolve_run_timeout({}, -1, default_timeout=5000) == 5000 + assert resolve_run_timeout({"timeout": 100}, -1, default_timeout=5000) == 100 + + def test_always_returns_int(self): + # No None ever escapes the resolver — downstream consumers rely on this. + for model, cli in (({}, -1), ({"timeout": None}, -1), ({"timeout": 0}, -1), ({}, 0)): + assert isinstance(resolve_run_timeout(model, cli), int) + + +class TestSubprocessTimeout: + """subprocess_timeout: resolved timeout -> subprocess/communicate semantics. + + subprocess treats timeout=0 as "expire immediately", not "no timeout", so + any non-positive value must become None. + """ + + @pytest.mark.parametrize("value", [0, -1, None]) + def test_no_timeout_values_become_none(self, value): + assert subprocess_timeout(value) is None + + @pytest.mark.parametrize("value", [1, 120, 7200]) + def test_positive_passes_through(self, value): + assert subprocess_timeout(value) == value class TestDockerImageRefForLogNaming: diff --git a/tests/unit/test_k8s.py b/tests/unit/test_k8s.py index 4a1f84a4..1599be4c 100644 --- a/tests/unit/test_k8s.py +++ b/tests/unit/test_k8s.py @@ -5,8 +5,16 @@ Integration/e2e tests stay in their own modules. """ +import json +import subprocess +from pathlib import Path +from unittest.mock import MagicMock + import pytest +import yaml +from madengine.core.timeout import DEFAULT_RUN_TIMEOUT +from madengine.deployment.base import DeploymentConfig, create_jinja_env from madengine.deployment.k8s_names import ( sanitize_k8s_container_name, sanitize_k8s_label_value, @@ -274,3 +282,216 @@ def test_invalid_mode_falls_back_to_lite(self): pre_scripts = [] mixin.gather_system_env_details(pre_scripts, "my_model", rocenv_mode="bogus") assert pre_scripts[0]["args"] == "my_model_env lite UBUNTU" + + +# --------------------------------------------------------------------------- +# Run timeout on the K8s path + + +def _k8s_template_context(model_timeout=None, cli_timeout=-1, tmp_path=None): + """Template context for a minimal single-node job, without touching a cluster. + + Builds the context off the same mixin the deployment uses, so the timeout + the template sees is the one a real render would get. + """ + from madengine.deployment.k8s_scripts import KubernetesScriptsMixin + from madengine.deployment.k8s_template_context import ( + KubernetesTemplateContextMixin, + ) + + class _Harness(KubernetesTemplateContextMixin, KubernetesScriptsMixin): + pass + + model_info = { + "name": "dummy", + "scripts": "scripts/dummy/run.sh", + "args": "", + "n_gpus": "1", + } + if model_timeout is not None: + model_info["timeout"] = model_timeout + + manifest = { + "built_images": {"dummy": {"docker_image": "dummy:latest"}}, + "built_models": {"dummy": model_info}, + "context": {"gpu_vendor": "AMD", "guest_os": "UBUNTU"}, + } + manifest_path = tmp_path / "build_manifest.json" + manifest_path.write_text(json.dumps(manifest)) + + k8s_config = {"namespace": "ns"} + harness = _Harness() + harness.config = DeploymentConfig( + target="k8s", + manifest_file=str(manifest_path), + additional_context={"k8s": k8s_config}, + cli_timeout=cli_timeout, + ) + harness.k8s_config = k8s_config + harness.console = MagicMock() + harness.manifest = manifest + harness.namespace = "ns" + harness.job_name = "j" + harness.job_label = "j" + harness.main_container_name = "c" + harness.configmap_name = "cm" + harness.service_name = "s" + harness.gpu_resource_name = "amd.com/gpu" + harness.data = None + + return harness._prepare_template_context( + model_info, {"registry_image": "dummy:latest"} + ) + + +def _render_k8s_job_script(context): + """Render job.yaml.j2 and return the main container's shell script.""" + template_dir = ( + Path(__file__).resolve().parents[2] + / "src" + / "madengine" + / "deployment" + / "templates" + / "kubernetes" + ) + rendered = ( + create_jinja_env(template_dir).get_template("job.yaml.j2").render(**context) + ) + job = list(yaml.safe_load_all(rendered))[0] + return job["spec"]["template"]["spec"]["containers"][0]["args"][0] + + +class TestK8sRunTimeoutResolution: + """The model card's timeout must reach the K8s job, following v1 precedence. + + Unlike SLURM, no inner madengine re-resolves inside the pod, so the card has + to be applied at render time -- config.timeout only bounds the submitting + process's wait on the Job and never saw the card. + """ + + def test_default_when_neither_card_nor_cli_specifies(self, tmp_path): + ctx = _k8s_template_context(tmp_path=tmp_path) + assert ctx["timeout"] == DEFAULT_RUN_TIMEOUT + + def test_model_card_overrides_default(self, tmp_path): + ctx = _k8s_template_context(model_timeout=360, tmp_path=tmp_path) + assert ctx["timeout"] == 360 + + def test_cli_overrides_model_card(self, tmp_path): + ctx = _k8s_template_context( + model_timeout=360, cli_timeout=120, tmp_path=tmp_path + ) + assert ctx["timeout"] == 120 + + @pytest.mark.parametrize("card_timeout", [0, -1]) + def test_model_card_can_ask_for_no_timeout(self, card_timeout, tmp_path): + ctx = _k8s_template_context(model_timeout=card_timeout, tmp_path=tmp_path) + assert ctx["timeout"] == card_timeout + + def test_cli_zero_disables_a_model_card_timeout(self, tmp_path): + ctx = _k8s_template_context(model_timeout=360, cli_timeout=0, tmp_path=tmp_path) + assert ctx["timeout"] == 0 + + +class TestK8sJobScriptTimeout: + """The rendered job script must actually enforce the resolved timeout.""" + + def test_model_script_is_wrapped_in_timeout(self, tmp_path): + ctx = _k8s_template_context(model_timeout=360, tmp_path=tmp_path) + script = _render_k8s_job_script(ctx) + assert "timeout 360 bash /tmp/run_model.sh" in script + + def test_non_positive_timeout_runs_unbounded(self, tmp_path): + ctx = _k8s_template_context(model_timeout=0, tmp_path=tmp_path) + script = _render_k8s_job_script(ctx) + assert "timeout 0 " not in script + assert "No timeout set" in script + assert "bash /tmp/run_model.sh" in script + + def test_rendered_script_is_valid_bash(self, tmp_path): + """The heredoc terminator must land in column 0 after YAML dedent.""" + for card_timeout in (360, 0): + ctx = _k8s_template_context(model_timeout=card_timeout, tmp_path=tmp_path) + script_path = tmp_path / f"job_{card_timeout}.sh" + script_path.write_text(_render_k8s_job_script(ctx)) + result = subprocess.run( + ["bash", "-n", str(script_path)], capture_output=True, text=True + ) + assert result.returncode == 0, result.stderr + + +def _run_model_invocation_block(script, model_exit_code, tmp_path, name): + """Execute just the model-invocation block of a rendered job script. + + Takes the lines from MODEL_START_TIME to MODEL_END_TIME verbatim, runs them + under `set -e` against a stub model script exiting with `model_exit_code`, + and reports whether execution reached the end of the block (i.e. whether the + container would go on to run post-scripts and copy artifacts). + """ + lines = script.splitlines() + start = next(i for i, l in enumerate(lines) if l.strip().startswith("MODEL_START_TIME=")) + end = next(i for i, l in enumerate(lines) if l.strip().startswith("MODEL_END_TIME=")) + block = "\n".join(l.strip() for l in lines[start:end]) + + stub = tmp_path / f"run_model_{name}.sh" + stub.write_text(f"#!/bin/bash\nexit {model_exit_code}\n") + harness = tmp_path / f"harness_{name}.sh" + harness.write_text( + "set -e\n" + f"cp {stub} /tmp/run_model.sh\n" + f"{block}\n" + 'echo "REACHED_ARTIFACT_COPY exit=$MODEL_EXIT_CODE"\n' + ) + return subprocess.run( + ["bash", str(harness)], capture_output=True, text=True, timeout=60 + ) + + +class TestK8sJobScriptPublishesResultsOnFailure: + """A failed or timed-out model must not abort the container early. + + The script runs under `set -e` and copies artifacts to the results PVC only + after the model returns, so a bare invocation (or an early `exit`) would + throw away perf.csv and the logs for exactly the runs worth diagnosing. + Both branches capture the exit code and defer to the single exit at the end. + """ + + @pytest.mark.parametrize("model_timeout", [360, 0]) + @pytest.mark.parametrize("model_exit_code", [0, 1, 124]) + def test_execution_continues_past_the_model( + self, model_timeout, model_exit_code, tmp_path + ): + ctx = _k8s_template_context(model_timeout=model_timeout, tmp_path=tmp_path) + script = _render_k8s_job_script(ctx) + result = _run_model_invocation_block( + script, model_exit_code, tmp_path, f"{model_timeout}_{model_exit_code}" + ) + assert ( + f"REACHED_ARTIFACT_COPY exit={model_exit_code}" in result.stdout + ), f"aborted early: rc={result.returncode} out={result.stdout!r} err={result.stderr!r}" + + def test_timeout_exit_code_is_reported(self, tmp_path): + """A real `timeout` kill (124) must be labelled, not just propagated.""" + ctx = _k8s_template_context(model_timeout=1, tmp_path=tmp_path) + script = _render_k8s_job_script(ctx) + lines = script.splitlines() + start = next( + i for i, l in enumerate(lines) if l.strip().startswith("MODEL_START_TIME=") + ) + end = next( + i for i, l in enumerate(lines) if l.strip().startswith("MODEL_END_TIME=") + ) + block = "\n".join(l.strip() for l in lines[start:end]) + + harness = tmp_path / "harness_real_timeout.sh" + harness.write_text( + "set -e\n" + 'printf "#!/bin/bash\\nsleep 30\\n" > /tmp/run_model.sh\n' + f"{block}\n" + 'echo "REACHED_ARTIFACT_COPY exit=$MODEL_EXIT_CODE"\n' + ) + result = subprocess.run( + ["bash", str(harness)], capture_output=True, text=True, timeout=60 + ) + assert "model script timed out after 1s" in result.stdout + assert "REACHED_ARTIFACT_COPY exit=124" in result.stdout diff --git a/tests/unit/test_orchestration.py b/tests/unit/test_orchestration.py index aaf24fcc..642006da 100644 --- a/tests/unit/test_orchestration.py +++ b/tests/unit/test_orchestration.py @@ -360,6 +360,128 @@ def test_distributed_warns_on_local_only_flags(self, tmp_path): assert "--skip-model-run" in printed assert "--keep-model-dir" not in printed # was False, must not appear + @pytest.mark.parametrize( + "cli_timeout,expected_config_timeout", + [ + (-1, 7200), # unspecified -> shared default, not left as -1 + (0, 0), # explicit "no timeout" passed through + (120, 120), # explicit timeout passed through + ], + ) + def test_distributed_resolves_timeout_sentinel( + self, tmp_path, cli_timeout, expected_config_timeout + ): + """_execute_distributed must resolve the CLI sentinel before building + DeploymentConfig. + + Regression: unlike the local path (which calls resolve_run_timeout() in + container_runner.py), the distributed path forwarded args.timeout to + DeploymentConfig verbatim. A default run (--timeout unspecified, i.e. + -1) therefore left DeploymentConfig.timeout == -1, which + subprocess_timeout() maps to None -- silently dropping the wall-clock + cap on the SLURM in-allocation path instead of applying the intended + 7200s default. + """ + from unittest.mock import MagicMock, patch + from madengine.orchestration.run_orchestrator import RunOrchestrator + + mock_args = MagicMock() + mock_args.keep_alive = False + mock_args.keep_model_dir = False + mock_args.skip_model_run = False + mock_args.timeout = cli_timeout + mock_args.additional_context = None + mock_args.live_output = False + + orchestrator = RunOrchestrator(mock_args) + orchestrator.additional_context = {} + orchestrator.rich_console = MagicMock() + + fake_result = MagicMock() + fake_result.is_success = True + fake_result.deployment_id = "test-id" + fake_result.logs_path = None + fake_result.metrics = {"successful_runs": [], "failed_runs": []} + + with patch("madengine.deployment.factory.DeploymentFactory.create") as mock_create: + mock_deploy = MagicMock() + mock_deploy.execute.return_value = fake_result + mock_create.return_value = mock_deploy + + orchestrator._execute_distributed("slurm", str(tmp_path / "manifest.json")) + + deployment_config = mock_create.call_args.args[0] + assert deployment_config.timeout == expected_config_timeout + assert isinstance(deployment_config.timeout, int) + # The sentinel itself must also survive, unresolved, for the job script + # to forward to the madengine it re-invokes. + assert deployment_config.cli_timeout == cli_timeout + + def test_model_card_timeout_survives_the_distributed_round_trip(self, tmp_path): + """A model card's timeout must still win on SLURM when no --timeout is given. + + Regression: _execute_distributed resolved the sentinel and the template + rendered that resolved value, so the job re-invoked madengine with an + explicit --timeout 7200. Precedence (correctly) ranks an explicit CLI + timeout above the model card, so the card's own value was discarded -- + only on distributed targets, and only because madengine had synthesized + the value it was now treating as user intent. + """ + from madengine.core.timeout import resolve_run_timeout + + model_card = {"name": "foo", "timeout": 3600} + + # Hop 1: the orchestrator, which has no model card in hand. + rendered = -1 # DeploymentConfig.cli_timeout for an unspecified --timeout + # Hop 2: the in-job madengine, resolving against the card. + assert resolve_run_timeout(model_card, rendered) == 3600 + # ... matching what the single-hop local path produces. + assert resolve_run_timeout(model_card, -1) == 3600 + + @pytest.mark.parametrize( + "kwargs,expected", + [ + ({}, -1), # omitted -> sentinel, so the model card can still win + ({"timeout": 120}, 120), + ({"timeout": 0}, 0), + ({"timeout": -1}, -1), + ], + ) + def test_execute_forwards_timeout_sentinel_to_local( + self, tmp_path, kwargs, expected + ): + """execute()'s own default must be the sentinel, not DEFAULT_RUN_TIMEOUT. + + Regression: the parameter defaulted to 7200, which _execute_local hands + to resolve_run_timeout() as an explicit CLI timeout. A programmatic + caller that omitted `timeout` therefore silently outranked every model + card, contradicting the documented precedence. + """ + manifest_path = tmp_path / "build_manifest.json" + manifest_path.write_text( + json.dumps( + { + "deployment_config": {"target": "local"}, + "context": {}, + "built_images": {}, + } + ) + ) + + mock_args = MagicMock() + mock_args.additional_context = None + mock_args.live_output = False + mock_args.output = str(tmp_path / "perf.csv") + + orchestrator = RunOrchestrator(mock_args) + + with patch.object(RunOrchestrator, "_cleanup_model_dir_copies"), \ + patch.object(RunOrchestrator, "_execute_local") as mock_local: + mock_local.return_value = {"successful_runs": [], "failed_runs": []} + orchestrator.execute(manifest_file=str(manifest_path), **kwargs) + + assert mock_local.call_args.args[1] == expected + @pytest.mark.unit class TestCreateManifestFromLocalImage: diff --git a/tests/unit/test_slurm_job_template.py b/tests/unit/test_slurm_job_template.py index 6ce19628..a0a39104 100644 --- a/tests/unit/test_slurm_job_template.py +++ b/tests/unit/test_slurm_job_template.py @@ -18,11 +18,13 @@ import json import re +import subprocess from pathlib import Path from unittest.mock import patch import pytest +from madengine.core.timeout import DEFAULT_RUN_TIMEOUT from madengine.deployment.base import DeploymentConfig from madengine.deployment.slurm import SlurmDeployment @@ -45,6 +47,8 @@ def _build_deployment( tmp_path: Path, slurm_overrides: dict = None, distributed_overrides: dict = None, + timeout: int = None, + cli_timeout: int = None, ) -> SlurmDeployment: """SlurmDeployment over a minimal torchrun manifest, output_dir under tmp_path.""" manifest = { @@ -81,6 +85,9 @@ def _build_deployment( } distributed_config.update(distributed_overrides or {}) + cfg_kwargs = {} if timeout is None else {"timeout": timeout} + if cli_timeout is not None: + cfg_kwargs["cli_timeout"] = cli_timeout cfg = DeploymentConfig( target="slurm", manifest_file=str(manifest_path), @@ -91,6 +98,7 @@ def _build_deployment( "slurm": slurm_config, "distributed": distributed_config, }, + **cfg_kwargs, ) return SlurmDeployment(cfg) @@ -199,3 +207,95 @@ def test_directive_present_by_default(self, tmp_path): def test_directive_omitted_when_opted_out(self, tmp_path): script = _render(_build_deployment(tmp_path, {"skip_gpus_directive": True})) assert "--gpus-per-node" not in script + + +# --------------------------------------------------------------------------- +# 4. The --timeout the job script passes back to madengine + +class TestTimeoutForwarding: + """The rendered `madengine run --timeout N` must always carry a valid int. + + The template used `{{ timeout | default(3600) }}`, but Jinja's default filter + only substitutes for *undefined* — a None slipped straight through and + rendered the literal `--timeout None`, which Typer then rejected. + """ + + @staticmethod + def _timeout_args(script: str) -> list: + return re.findall(r"--timeout (\S+)", script) + + def test_no_timeout_renders_zero_not_none(self, tmp_path): + # --timeout 0 (no timeout) is the case that used to render "None". + script = _render(_build_deployment(tmp_path, cli_timeout=0)) + args = self._timeout_args(script) + assert args, "job script does not forward --timeout at all" + assert all(a == "0" for a in args), args + assert "--timeout None" not in script + + def test_explicit_timeout_forwarded(self, tmp_path): + script = _render(_build_deployment(tmp_path, cli_timeout=120)) + assert all(a == "120" for a in self._timeout_args(script)) + + def test_unspecified_sentinel_forwarded_verbatim(self, tmp_path): + # -1 must survive to the inner CLI so it can apply model-card precedence + # there, rather than being flattened to a concrete default here. + script = _render(_build_deployment(tmp_path, cli_timeout=-1)) + assert all(a == "-1" for a in self._timeout_args(script)) + + def test_resolved_process_cap_does_not_leak_into_the_job(self, tmp_path): + """config.timeout caps *this* process; only cli_timeout reaches the job. + + Regression: the template read config.timeout, so a default run rendered + --timeout 7200 into the job script. The inner madengine cannot tell that + from a user-supplied --timeout 7200, so it outranked the model card and + a model declaring "timeout": 3600 silently ran with a 2h cap instead. + """ + deployment = _build_deployment( + tmp_path, timeout=DEFAULT_RUN_TIMEOUT, cli_timeout=-1 + ) + assert all(a == "-1" for a in self._timeout_args(_render(deployment))) + + def test_default_config_forwards_the_sentinel(self, tmp_path): + # A config built without an explicit CLI timeout forwards "unspecified", + # leaving the model card free to win inside the job. + script = _render(_build_deployment(tmp_path)) + assert all(a == "-1" for a in self._timeout_args(script)) + + +# --------------------------------------------------------------------------- +# 5. The timeout handed to subprocess on the in-allocation path + +class TestInAllocationTimeout: + """`_run_inside_existing_allocation` must not pass a sentinel to subprocess. + + Regression: the call site read `self.config.timeout if ... > 0 else None`, + which raised TypeError once the CLI started sending None for "no timeout". + subprocess spells "no timeout" as None and reads 0 as "expire now", so + both sentinels have to be mapped, not compared inline. + """ + + def _invoke(self, tmp_path, timeout): + deployment = _build_deployment(tmp_path) + # Set on the config directly: None is one of the values under test, so + # it cannot be routed through _build_deployment's "omit the kwarg" flag. + deployment.config.timeout = timeout + deployment.inside_allocation = False # skip the allocation-size check + deployment.script_path = tmp_path / "job.sh" + deployment.script_path.write_text("#!/bin/bash\nexit 0\n") + with patch( + "madengine.deployment.slurm.subprocess.run", + return_value=subprocess.CompletedProcess([], 0), + ) as mock_run: + deployment._run_inside_existing_allocation() + mock_run.assert_called_once() + return mock_run.call_args.kwargs["timeout"] + + @pytest.mark.parametrize("timeout", [0, -1, None]) + def test_no_timeout_values_become_none(self, tmp_path, timeout): + assert self._invoke(tmp_path, timeout) is None + + def test_positive_timeout_passed_through(self, tmp_path): + assert self._invoke(tmp_path, 120) == 120 + + def test_default_config_carries_the_shared_default(self, tmp_path): + assert _build_deployment(tmp_path).config.timeout == DEFAULT_RUN_TIMEOUT