From b17f93c1b60dfc4033c66421b969aa2ff0710cb1 Mon Sep 17 00:00:00 2001 From: Haider Al-Tahan Date: Tue, 22 Sep 2026 08:42:07 +0300 Subject: [PATCH] Fail the job when an evaluation produces no result Each evaluation's exit status was ignored, so a container that fails to start, a missing venv or a crashed harness all printed "Evaluation finished" and left the array element COMPLETED with no result file. The loss only surfaced later, as a gap in `collect --check`. Record each harness's exit status and whether it wrote anything, count the failures and exit non-zero. The read loop is redirected rather than piped so the counter survives past `done`, and the evalchemy branch's `cd` is scoped to a subshell now that the loop body runs in the current shell. Also exit non-zero when sbatch rejects the submission, instead of logging an error and returning normally. --- oellm/main.py | 4 +- oellm/resources/template.sbatch | 54 +++++++++- tests/test_job_failure_reporting.py | 162 ++++++++++++++++++++++++++++ 3 files changed, 214 insertions(+), 6 deletions(-) create mode 100644 tests/test_job_failure_reporting.py diff --git a/oellm/main.py b/oellm/main.py index c4ed101d..b6a58312 100644 --- a/oellm/main.py +++ b/oellm/main.py @@ -538,10 +538,12 @@ def schedule_evals( except subprocess.CalledProcessError as e: logging.error(f"Failed to submit job: {e}") logging.error(f"sbatch stderr: {e.stderr}") - except FileNotFoundError: + raise SystemExit(1) from e + except FileNotFoundError as e: logging.error( "sbatch command not found. Please make sure you are on a system with SLURM installed." ) + raise SystemExit(1) from e def collect_results( diff --git a/oellm/resources/template.sbatch b/oellm/resources/template.sbatch index 6ae5ae76..7036b5e4 100644 --- a/oellm/resources/template.sbatch +++ b/oellm/resources/template.sbatch @@ -60,9 +60,14 @@ if [ "$START_INDEX" -gt "$END_INDEX" ]; then exit 0 fi +# Evaluations in this job that produced no result. Becomes the job's exit +# status so a failed eval cannot be mistaken for a completed one. +FAILED_EVALS=0 + # Use `tail` and `head` to slice the CSV file for the tasks assigned to this job. # The +1 on START_INDEX accounts for the header row. -tail -n +$((START_INDEX + 1)) "$CSV_PATH" | head -n $((END_INDEX - START_INDEX + 1)) | \ +# Redirected rather than piped into the loop: a pipeline runs the loop body in a +# subshell, and FAILED_EVALS would be discarded when it exits. while IFS=, read -r model_path task_path n_shot eval_suite do # Remove trailing carriage returns if script is edited on Windows @@ -135,20 +140,31 @@ do fi }} + # Set by every branch below: the harness exit status, and where to look for + # what it wrote. lm_eval appends a timestamp to --output_path, so the result + # is a directory plus a filename pattern rather than an exact path. + EVAL_STATUS=0 + EVAL_OUT_DIR="" + EVAL_OUT_PAT="" + case "$suite_normalized" in lm_eval|lm-eval|lm-eval-harness) echo echo "----------------------------------------------------" echo "lm_eval Execution" + RUN_ID=$(openssl rand -hex 5) + EVAL_OUT_DIR="{evals_dir}" + EVAL_OUT_PAT="$RUN_ID*.json" run_python -m lm_eval --model hf \ --model_args pretrained="$model_path",trust_remote_code=True \ --tasks "$task_path" \ --num_fewshot "$n_shot" \ - --output_path "{evals_dir}/$(openssl rand -hex 5).json" \ + --output_path "$EVAL_OUT_DIR/$RUN_ID.json" \ --trust_remote_code \ ${{LM_EVAL_INCLUDE_PATH:+--include_path $$LM_EVAL_INCLUDE_PATH}} \ --batch_size auto \ ${{LIMIT:+--limit $LIMIT}} + EVAL_STATUS=$? echo "----------------------------------------------------" ;; lighteval|light-eval) @@ -171,6 +187,8 @@ do RESULTS_SUBDIR="{evals_dir}/$(openssl rand -hex 5)" mkdir -p "$RESULTS_SUBDIR" + EVAL_OUT_DIR="$RESULTS_SUBDIR" + EVAL_OUT_PAT="*" if [ -n "$VENV_PATH" ]; then source "$VENV_PATH/bin/activate" @@ -180,6 +198,7 @@ do --load-tasks-multilingual \ --output-dir "$RESULTS_SUBDIR" \ ${{LIMIT:+--max-samples $LIMIT}} + EVAL_STATUS=$? else singularity exec $SINGULARITY_ARGS \ $SINGULARITY_ENV_ARGS \ @@ -193,13 +212,19 @@ do --load-tasks-multilingual \ --output-dir "$RESULTS_SUBDIR" \ ${{LIMIT:+--max-samples $LIMIT}} + EVAL_STATUS=$? fi ;; evalchemy) EVALCHEMY_WORK_DIR="{evalchemy_dir}" RESULTS_SUBDIR="{evals_dir}/$(openssl rand -hex 5)" + EVAL_OUT_DIR="$RESULTS_SUBDIR" + EVAL_OUT_PAT="*" if [ -n "$VENV_PATH" ]; then source "$VENV_PATH/bin/activate" + # subshell: the loop no longer runs in one, so this cd would + # otherwise leak into the next evaluation in this job + ( cd "$EVALCHEMY_WORK_DIR" accelerate launch --num-processes "$GPUS_PER_NODE" --num-machines 1 \ $MULTI_GPU_FLAG -m eval.eval \ @@ -209,6 +234,8 @@ do --batch_size auto \ --output_path "$RESULTS_SUBDIR" \ ${{LIMIT:+--limit $LIMIT}} + ) + EVAL_STATUS=$? else echo "[error] evalchemy suite requires --venv_path (not supported in container mode)." exit 1 @@ -216,12 +243,29 @@ do ;; *) echo "[warning] Unknown evaluation suite '$eval_suite'. Skipping." + EVAL_STATUS=1 ;; esac - echo "----------------------------------------------------" - echo "Evaluation finished for model: $model_path" + # An evaluation counts as done only if the harness exited 0 and wrote + # something. Both halves matter: a container that fails to start and a + # missing venv both exit 0 while producing no result. + if [ "$EVAL_STATUS" -ne 0 ]; then + echo "[error] $task_path ($eval_suite) exited with status $EVAL_STATUS" + FAILED_EVALS=$((FAILED_EVALS + 1)) + elif [ -n "$EVAL_OUT_DIR" ] && [ -z "$(find "$EVAL_OUT_DIR" -type f -name "$EVAL_OUT_PAT" 2>/dev/null | head -n 1)" ]; then + echo "[error] $task_path ($eval_suite) exited 0 but wrote no result to $EVAL_OUT_DIR/$EVAL_OUT_PAT" + FAILED_EVALS=$((FAILED_EVALS + 1)) + else + echo "----------------------------------------------------" + echo "Evaluation finished for model: $model_path" + fi + +done < <(tail -n +$((START_INDEX + 1)) "$CSV_PATH" | head -n $((END_INDEX - START_INDEX + 1))) -done +if [ "$FAILED_EVALS" -gt 0 ]; then + echo "Job $SLURM_ARRAY_TASK_ID finished with $FAILED_EVALS failed evaluation(s)." + exit 1 +fi echo "Job $SLURM_ARRAY_TASK_ID finished." diff --git a/tests/test_job_failure_reporting.py b/tests/test_job_failure_reporting.py new file mode 100644 index 00000000..5f5936c9 --- /dev/null +++ b/tests/test_job_failure_reporting.py @@ -0,0 +1,162 @@ +"""The generated job script must not report a failed evaluation as finished. + +Every eval is a subprocess whose exit status the script used to ignore, so a +container that fails to start, a missing venv or a crashed harness all ended +with "Evaluation finished" and a zero exit code. The array element was then +COMPLETED with no result file, and only a later `collect --check` noticed. + +These tests render a real script through `schedule_evals` and run it against a +stub harness, so they exercise the template rather than a copy of its logic. +""" + +import os +import stat +import subprocess +import sys +from pathlib import Path +from unittest.mock import patch + +import pytest + +from oellm.main import schedule_evals + +# Stub `python` implementations placed on PATH by the fake venv below. +HARNESS_OK = """#!/bin/bash +# lm_eval creates the results directory itself and appends a timestamp to the +# path it is given; mimic both so the script's result check sees a real file. +for arg in "$@"; do + if [ "$prev" = "--output_path" ]; then + mkdir -p "$(dirname "$arg")" + touch "${arg%.json}_2026.json" + fi + prev="$arg" +done +exit 0 +""" +HARNESS_CRASHES = """#!/bin/bash +echo "boom" >&2 +exit 1 +""" +HARNESS_SILENT = """#!/bin/bash +exit 0 +""" + + +def _fake_venv(tmp_path: Path, harness: str) -> Path: + """A venv whose `activate` puts a stub `python` ahead of the real one.""" + venv = tmp_path / "venv" + (venv / "bin").mkdir(parents=True) + (venv / "bin" / "activate").write_text(f'export PATH="{venv}/bin:$PATH"\n') + python = venv / "bin" / "python" + python.write_text(harness) + python.chmod(python.stat().st_mode | stat.S_IEXEC) + return venv + + +def _render(tmp_path: Path, venv: Path) -> Path: + with ( + patch("oellm.main._load_cluster_env"), + patch("oellm.main._num_jobs_in_queue", return_value=0), + patch.dict(os.environ, {"EVAL_OUTPUT_DIR": str(tmp_path), "GPUS_PER_NODE": "1"}), + ): + schedule_evals( + models="EleutherAI/pythia-70m", + tasks="hellaswag", + n_shot=0, + skip_checks=True, + venv_path=str(venv), + dry_run=True, + ) + scripts = list(tmp_path.glob("**/submit_evals.sbatch")) + assert len(scripts) == 1 + return scripts[0] + + +def _run(script: Path) -> subprocess.CompletedProcess: + return subprocess.run( + ["bash", str(script)], + env={ + **os.environ, + "SLURM_ARRAY_TASK_ID": "0", + "SLURM_ARRAY_JOB_ID": "1", + "SLURM_JOB_ID": "1", + }, + capture_output=True, + text=True, + ) + + +@pytest.mark.parametrize( + "harness, expected_error", + [ + pytest.param(HARNESS_CRASHES, "exited with status 1", id="harness-crashes"), + pytest.param(HARNESS_SILENT, "wrote no result", id="harness-writes-nothing"), + ], +) +def test_failed_evaluation_fails_the_job(tmp_path, harness, expected_error): + """A crash and a silent no-op are both failures, not finished evaluations.""" + script = _render(tmp_path, _fake_venv(tmp_path, harness)) + result = _run(script) + + assert result.returncode == 1 + assert expected_error in result.stdout + assert "1 failed evaluation" in result.stdout + assert "Evaluation finished" not in result.stdout + + +def test_successful_evaluation_passes(tmp_path): + """The happy path still exits 0 and reports the evaluation as finished.""" + script = _render(tmp_path, _fake_venv(tmp_path, HARNESS_OK)) + result = _run(script) + + assert result.returncode == 0 + assert "Evaluation finished" in result.stdout + assert "[error]" not in result.stdout + + +def test_failure_count_survives_the_read_loop(tmp_path): + """Both rows of a two-eval job are counted. + + The loop reads the CSV by redirection rather than a pipe; a pipeline would + run the body in a subshell and lose the counter at `done`. + """ + venv = _fake_venv(tmp_path, HARNESS_CRASHES) + with ( + patch("oellm.main._load_cluster_env"), + patch("oellm.main._num_jobs_in_queue", return_value=0), + patch.dict(os.environ, {"EVAL_OUTPUT_DIR": str(tmp_path), "GPUS_PER_NODE": "1"}), + ): + schedule_evals( + models="EleutherAI/pythia-70m", + tasks="hellaswag,arc_easy", + n_shot=0, + skip_checks=True, + venv_path=str(venv), + max_array_len=1, + dry_run=True, + ) + script = next(iter(tmp_path.glob("**/submit_evals.sbatch"))) + result = _run(script) + + assert result.returncode == 1 + assert "2 failed evaluation" in result.stdout + + +def test_sbatch_failure_exits_non_zero(tmp_path): + """A rejected submission must not look like a successful schedule.""" + error = subprocess.CalledProcessError(1, ["sbatch"], stderr="AssocMaxSubmitJobLimit") + with ( + patch("oellm.main._load_cluster_env"), + patch("oellm.main._num_jobs_in_queue", return_value=0), + patch("oellm.main.subprocess.run", side_effect=error), + patch.dict(os.environ, {"EVAL_OUTPUT_DIR": str(tmp_path)}), + pytest.raises(SystemExit) as excinfo, + ): + schedule_evals( + models="EleutherAI/pythia-70m", + tasks="hellaswag", + n_shot=0, + skip_checks=True, + venv_path=str(Path(sys.prefix)), + ) + assert excinfo.value.code == 1