fix(timeout): restore v1 precedence, unify default, fix distributed timeout resolution

[coketaste]

Summary

The v2 rewrite spread timeout handling across five layers and, in doing so, lost several v1 guarantees. This PR restores a single source of truth (core/timeout.py) and fixes every place that drifted from it: local runs, SLURM, Kubernetes, and the documentation describing all three.

Precedence, lowest to highest: default (7200s) < model card timeout < explicit --timeout.

-1 on the CLI means "not passed" and falls through. 0 (CLI or model card) means "no timeout." A model card's value is otherwise taken as-is, including negative numbers.

Bugs fixed

• Explicit --timeout equal to the default was invisible.
  The old resolver detected "unspecified" by comparing the CLI value against 7200, so --timeout 7200 looked identical to no flag and silently lost to a model card's timeout. In v1, the explicit flag always won.

• --timeout 0 crashed.
  The CLI mapped 0 to None before it reached consumers. container_runner.py and slurm.py guarded with a bare timeout > 0, raising TypeError on None. The SLURM job template's {{ timeout | default(3600) }} only substitutes for undefined, not None, so it rendered the literal string --timeout None, which Typer then rejected.

• A model card's "timeout": -1 was silently capped.
  The resolver guarded the model card with >= 0, so a card asking for no timeout fell through to the 7200s default instead. Two real MAD-internal models (migx_onnxrt_resnet50_v1_5_benchmarks, pyt_huggingface_msft_phi3.5) set -1 for exactly that and were being capped without warning. The card is now taken as-is, matching v1, where any non-positive value means unbounded.

• Distributed default was inconsistent and sometimes missing entirely.
  SLURM and Kubernetes defaulted to 3600s while local execution and the documentation said 7200s. Worse, _execute_distributed forwarded the CLI's unresolved -1 sentinel straight into DeploymentConfig, so subprocess_timeout(-1) mapped to None and SLURM in-allocation runs had no wall-clock cap at all on any run that did not pass --timeout explicitly.

• SLURM ignored the model card's timeout on the resolved path.
  SLURM resolves the timeout twice: once for the submitting process's own wait, and again inside the job, which re-invokes madengine run. Rendering the already resolved value into that inner invocation made it look like an explicit --timeout, which correctly outranks the model card. As a result, a model declaring "timeout": 3600 silently ran under the 7200s default instead. DeploymentConfig now carries the resolved cap and the raw CLI sentinel separately, so the inner run resolves against the card itself.

• Kubernetes never enforced the timeout at all.
  job.yaml.j2 never referenced it, so Kubernetes jobs ran unbounded regardless of the model card or --timeout. Unlike SLURM, Kubernetes has no inner madengine process to re-resolve inside the pod, so the timeout is now resolved at manifest render time and the model script is wrapped in timeout, terminating with exit code 124 on overrun.

Changes

• core/timeout.py is now the single source of truth:

  • resolve_run_timeout() applies default < model card < explicit CLI precedence.
  • subprocess_timeout() is the sole guard that maps timeout values onto subprocess/communicate semantics. This is important because timeout=0 means "expire immediately," not "no timeout."
  • DEFAULT_RUN_TIMEOUT = 7200 unifies the default everywhere.

• The CLI (cli/commands/run.py) now forwards the user's value verbatim instead of resolving it early. It only builds a display string.

• resolve_run_timeout() moved from execution/container_runner_helpers.py to core/timeout.py (the old import path remains re-exported).

• run_orchestrator._execute_distributed() now resolves the sentinel before building DeploymentConfig, restoring the default cap for SLURM and Kubernetes.

• DeploymentConfig now includes:

  • timeout: the resolved value used to cap madengine's own wait.
  • cli_timeout: the raw CLI sentinel forwarded to the inner SLURM job.

• job.sh.j2 now renders the resolved value directly instead of using {{ timeout | default(3600) }}.

• job.yaml.j2 and k8s_template_context.py now resolve against the model card at render time and enforce the result with timeout in the generated script.

• build_orchestrator and run_orchestrator manifest writers

[Copilot]

Pull request overview

This PR centralizes and restores the v1 timeout sentinel/precedence behavior after the v2 rewrite, aiming to make timeout handling consistent across local and distributed execution (SLURM/K8s) with a unified default of 7200 seconds. It introduces madengine.core.timeout as the single source of truth for resolving run timeouts and for mapping timeout values onto subprocess semantics.

Changes:

• Added core.timeout.resolve_run_timeout() and core.timeout.subprocess_timeout() and updated call sites to use a shared sentinel contract (-1 unspecified, 0 no timeout, >0 explicit seconds).
• Unified defaults to DEFAULT_RUN_TIMEOUT = 7200 across orchestration, execution, and deployment paths, and updated SLURM job template timeout rendering.
• Added/updated regression tests plus documentation and changelog entries covering precedence and sentinel behavior.

Reviewed changes

Copilot reviewed 16 out of 16 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
tests/unit/test_slurm_job_template.py	Adds regression coverage for SLURM job script timeout forwarding and in-allocation subprocess timeout mapping.
tests/unit/test_orchestration.py	Verifies distributed path resolves/handles timeout sentinel when building DeploymentConfig.
tests/unit/test_execution.py	Updates unit coverage for new core.timeout resolution and subprocess mapping helpers.
tests/unit/test_container_runner.py	Adds coverage ensuring subprocess timeout uses None for “no timeout” sentinels.
tests/unit/test_cli.py	Ensures CLI forwards timeout sentinel verbatim as an int downstream.
tests/e2e/test_execution_features.py	Adds E2E assertion that explicit --timeout 7200 still beats model card timeout.
src/madengine/orchestration/run_orchestrator.py	Unifies defaults and updates distributed execution timeout handling.
src/madengine/execution/container_runner.py	Switches timeout defaults to DEFAULT_RUN_TIMEOUT and uses subprocess_timeout() at subprocess call sites.
src/madengine/execution/container_runner_helpers.py	Re-exports resolve_run_timeout from core.timeout for backward-compatible imports.
src/madengine/deployment/templates/slurm/job.sh.j2	Stops using Jinja default() filter and renders the passed timeout value directly.
src/madengine/deployment/slurm.py	Uses subprocess_timeout() when running inside an existing allocation.
src/madengine/deployment/base.py	Aligns DeploymentConfig.timeout default with DEFAULT_RUN_TIMEOUT.
src/madengine/core/timeout.py	Adds shared default constant plus resolver + subprocess mapping functions alongside Timeout class.
src/madengine/cli/commands/run.py	Stops materializing the timeout sentinel early; updates user-facing timeout display.
docs/usage.md	Documents precedence and sentinel semantics (including explicit --timeout 7200 behavior).
CHANGELOG.md	Records behavior changes and bug fixes related to timeout precedence and sentinels.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 18 out of 18 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

Previously missed (1) — in code that hasn't changed since the last review.

src/madengine/execution/container_runner.py:1103

• ContainerRunner.run_container() defaults timeout to DEFAULT_RUN_TIMEOUT (7200) but then passes it into resolve_run_timeout(model_info, timeout) where any non-negative value is treated as an explicit CLI timeout. Calling run_container() without a timeout argument will therefore force 7200s and prevent a model card timeout from taking precedence. Defaulting this parameter to the CLI sentinel -1 preserves the same effective default while allowing model-card overrides.

This issue also appears on line 2768 of the same file.

        timeout: int = DEFAULT_RUN_TIMEOUT,

src/madengine/execution/container_runner.py:2768

• ContainerRunner.run_models_from_manifest() has the same issue as run_container(): the default timeout value is a concrete DEFAULT_RUN_TIMEOUT, but downstream resolution interprets any non-negative value as an explicit CLI timeout and will prevent model-card timeout from overriding it when the caller omits the argument. Default this to the CLI sentinel -1 instead.

        timeout: int = DEFAULT_RUN_TIMEOUT,

[Copilot]

Pull request overview

Copilot reviewed 21 out of 21 changed files in this pull request and generated no new comments.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/madengine/execution/container_runner.py:1103

• run_container() now defaults timeout to DEFAULT_RUN_TIMEOUT (7200). Because run_container() immediately calls resolve_run_timeout(model_info, timeout), a caller that omits timeout will unintentionally treat 7200 as an explicit CLI timeout and override any model-card timeout, violating the documented precedence (default < model card < explicit CLI). Use the -1 sentinel as the default here so model cards can still override when callers don't specify a timeout.

        build_info: typing.Dict = None,
        keep_alive: bool = False,
        keep_model_dir: bool = False,
        skip_model_run: bool = False,
        timeout: int = DEFAULT_RUN_TIMEOUT,
        tools_json_file: str = "scripts/common/tools.json",

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/madengine/deployment/templates/kubernetes/job.yaml.j2:526

• The direct-script path has the same early-exit in the timeout>0 branch, which skips the later artifact copy and the final exit ${MODEL_EXIT_CODE:-0}. Also, in the timeout<=0 branch bash /tmp/run_model.sh is not guarded; under set -e a non-zero model exit would abort the script before MODEL_EXIT_CODE is captured. Prefer capturing the exit code in an if ...; then ... else ... fi and exit only once at the end of the container script.

                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
                    exit $MODEL_EXIT_CODE

[Copilot]

Pull request overview

Copilot reviewed 23 out of 23 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

Previously missed (1) — in code that hasn't changed since the last review.

src/madengine/execution/container_runner.py:988

• subprocess_timeout(timeout) correctly maps non-positive values to None, but the log message printed earlier in this block still says "Setting timeout to {timeout} seconds" even when the effective subprocess timeout is disabled (e.g., model card timeout 0/-1 or CLI --timeout 0). This makes logs misleading and can complicate debugging of unbounded runs. Consider computing the mapped subprocess timeout once, printing a "no timeout" message when it is None, and passing that variable into subprocess.run.

                        timeout=subprocess_timeout(timeout),