From 63b5f5693370fa3475ffc067ca5661fa9cffdf3f Mon Sep 17 00:00:00 2001 From: shreyaskommuri Date: Mon, 27 Jul 2026 11:07:34 -0700 Subject: [PATCH 1/3] Make Sleep's was_run_successful check the real exit code SleepTestDefinition never overrode was_run_successful(), so it fell back to the base TestDefinition default, which unconditionally returns is_successful=True regardless of what actually happened. Every other workload checked (Fio, vLLM, NCCL, NixlEP, DynamoMocker, NemoRun, SGLang) implements a real check; Sleep did not. Confirmed via a real (non-dry-run) standalone run with a deliberately-failing command (`sleep -1`): CloudAI's own results table reported PASSED with exit code 0, no stdout.txt/stderr.txt even captured. Fix scoped to standalone, the only system with an actual reproduction here: - SleepStandaloneCommandGenStrategy.gen_exec_command() now redirects stdout/stderr to files and captures the real exit code, following the same pattern FioStandaloneCommandGenStrategy already uses for stdout/stderr redirection. - SleepTestDefinition.was_run_successful() reads that exit code file. If it's absent (Slurm/LSF/Kubernetes command-gen strategies don't capture one yet), it falls back to the previous always-true behavior rather than fail runs that never had a chance to produce it - additive, not a behavior change for those systems. Slurm/LSF/Kubernetes have the same underlying was_run_successful gap in principle, but I don't have those systems available to verify a fix against, so this PR is standalone-only; the issue documents the gap generally for whoever picks up the other systems. Separately noticed, out of scope here: even with was_run_successful now correctly returning FAILED, handle_dry_run_and_run's non-DSE path (src/cloudai/cli/handlers.py) unconditionally returns 0 regardless of job outcome, so the overall `cloudai run` process exit code still doesn't reflect a failed test. That's a broader gap affecting every workload's exit code, not Sleep-specific, and not fixed by this PR. Issue: NVIDIA/cloudai#980 Tested: uv run pytest -q (1767 passed, 18 skipped); uv run ruff check src/cloudai/workloads/sleep/sleep.py src/cloudai/workloads/sleep/standalone_command_gen_strategy.py tests/workloads/sleep/test_sleep.py tests/workloads/sleep/test_command_gen_strategy_standalone.py; uv run ruff format --check (same files); manually re-ran the exact broken repro from the issue against this fix - results table now correctly shows FAILED with "sleep exited with code 1. Check .../stderr.txt for details." instead of silently reporting PASSED. AI was used for context and guidance while investigating, implementing, and testing this fix. Signed-off-by: shreyaskommuri --- src/cloudai/workloads/sleep/sleep.py | 33 ++++++++- .../sleep/standalone_command_gen_strategy.py | 20 ++++-- .../test_command_gen_strategy_standalone.py | 8 ++- tests/workloads/sleep/test_sleep.py | 72 +++++++++++++++++++ 4 files changed, 124 insertions(+), 9 deletions(-) create mode 100644 tests/workloads/sleep/test_sleep.py diff --git a/src/cloudai/workloads/sleep/sleep.py b/src/cloudai/workloads/sleep/sleep.py index 216981314..ba92fb755 100644 --- a/src/cloudai/workloads/sleep/sleep.py +++ b/src/cloudai/workloads/sleep/sleep.py @@ -1,5 +1,5 @@ # SPDX-FileCopyrightText: NVIDIA CORPORATION & AFFILIATES -# Copyright (c) 2024-2025 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# Copyright (c) 2024-2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. # SPDX-License-Identifier: Apache-2.0 # # Licensed under the Apache License, Version 2.0 (the "License"); @@ -15,9 +15,11 @@ # limitations under the License. -from cloudai.core import Installable +from cloudai.core import Installable, JobStatusResult, TestRun from cloudai.models.workload import CmdArgs, TestDefinition +EXIT_CODE_FILE_NAME = "exit_code.txt" + class SleepCmdArgs(CmdArgs): """Sleep test command arguments.""" @@ -34,3 +36,30 @@ class SleepTestDefinition(TestDefinition): @property def installables(self) -> list[Installable]: return [] + + def was_run_successful(self, tr: TestRun) -> JobStatusResult: + exit_code_path = tr.output_path / EXIT_CODE_FILE_NAME + if not exit_code_path.is_file(): + # Not every system's command-gen strategy captures an exit code yet + # (currently only standalone does); fall back to the previous + # behavior rather than fail runs that never had a chance to produce + # this file. + return JobStatusResult(is_successful=True) + + exit_code_text = exit_code_path.read_text().strip() + try: + exit_code = int(exit_code_text) + except ValueError: + return JobStatusResult( + is_successful=False, + error_message=f"Could not parse exit code from {exit_code_path}: {exit_code_text!r}.", + ) + + if exit_code != 0: + stderr_path = tr.output_path / "stderr.txt" + return JobStatusResult( + is_successful=False, + error_message=f"sleep exited with code {exit_code}. Check {stderr_path} for details.", + ) + + return JobStatusResult(is_successful=True) diff --git a/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py b/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py index b4494b971..5994041bc 100644 --- a/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py +++ b/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py @@ -14,6 +14,7 @@ # See the License for the specific language governing permissions and # limitations under the License. +import shlex from typing import cast import toml @@ -21,7 +22,7 @@ from cloudai.core import CommandGenStrategy from cloudai.models.scenario import TestRunDetails -from .sleep import SleepTestDefinition +from .sleep import EXIT_CODE_FILE_NAME, SleepTestDefinition class SleepStandaloneCommandGenStrategy(CommandGenStrategy): @@ -31,13 +32,22 @@ def _generate_sleep_command(self) -> str: tdef: SleepTestDefinition = cast(SleepTestDefinition, self.test_run.test) return f"sleep {tdef.cmd_args.seconds}" - def store_test_run(self) -> None: + def store_test_run(self, full_cmd: str) -> None: test_cmd = self._generate_sleep_command() self.test_run.output_path.mkdir(parents=True, exist_ok=True) with (self.test_run.output_path / self.TEST_RUN_DUMP_FILE_NAME).open("w", encoding="utf-8") as f: - trd = TestRunDetails.from_test_run(self.test_run, test_cmd=test_cmd, full_cmd=test_cmd) + trd = TestRunDetails.from_test_run(self.test_run, test_cmd=test_cmd, full_cmd=full_cmd) toml.dump(trd.model_dump(), f) def gen_exec_command(self) -> str: - self.store_test_run() - return self._generate_sleep_command() + stdout_path = self.test_run.output_path / "stdout.txt" + stderr_path = self.test_run.output_path / "stderr.txt" + exit_code_path = self.test_run.output_path / EXIT_CODE_FILE_NAME + + full_cmd = ( + f"{self._generate_sleep_command()} " + f"> {shlex.quote(str(stdout_path))} 2> {shlex.quote(str(stderr_path))}; " + f"echo $? > {shlex.quote(str(exit_code_path))}" + ) + self.store_test_run(full_cmd) + return full_cmd diff --git a/tests/workloads/sleep/test_command_gen_strategy_standalone.py b/tests/workloads/sleep/test_command_gen_strategy_standalone.py index 95964f422..ad63cd89d 100644 --- a/tests/workloads/sleep/test_command_gen_strategy_standalone.py +++ b/tests/workloads/sleep/test_command_gen_strategy_standalone.py @@ -22,6 +22,7 @@ from cloudai.models.scenario import TestRunDetails from cloudai.systems.standalone import StandaloneSystem from cloudai.workloads.sleep import SleepCmdArgs, SleepStandaloneCommandGenStrategy, SleepTestDefinition +from cloudai.workloads.sleep.sleep import EXIT_CODE_FILE_NAME def test_gen_exec_command_writes_test_run_details(tmp_path: Path, standalone_system: StandaloneSystem) -> None: @@ -36,10 +37,13 @@ def test_gen_exec_command_writes_test_run_details(tmp_path: Path, standalone_sys strategy = SleepStandaloneCommandGenStrategy(standalone_system, tr) command = strategy.gen_exec_command() - assert command == "sleep 60" + assert command == ( + f"sleep 60 > {tr.output_path / 'stdout.txt'} 2> {tr.output_path / 'stderr.txt'}; " + f"echo $? > {tr.output_path / EXIT_CODE_FILE_NAME}" + ) dump_path = tr.output_path / CommandGenStrategy.TEST_RUN_DUMP_FILE_NAME assert dump_path.is_file() details = TestRunDetails.model_validate(toml.load(dump_path)) assert details.test_cmd == "sleep 60" - assert details.full_cmd == "sleep 60" + assert details.full_cmd == command diff --git a/tests/workloads/sleep/test_sleep.py b/tests/workloads/sleep/test_sleep.py new file mode 100644 index 000000000..cb602d210 --- /dev/null +++ b/tests/workloads/sleep/test_sleep.py @@ -0,0 +1,72 @@ +# SPDX-FileCopyrightText: NVIDIA CORPORATION & AFFILIATES +# Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# 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. + +from pathlib import Path + +from cloudai.core import TestRun +from cloudai.workloads.sleep import SleepCmdArgs, SleepTestDefinition +from cloudai.workloads.sleep.sleep import EXIT_CODE_FILE_NAME + + +def _sleep_test_run(tmp_path: Path) -> TestRun: + tdef = SleepTestDefinition( + name="sleep_test", + description="Simple sleep test", + test_template_name="Sleep", + cmd_args=SleepCmdArgs(seconds=1), + ) + return TestRun(name="sleep-job", test=tdef, num_nodes=1, nodes=[], output_path=tmp_path / "output") + + +def test_was_run_successful_without_exit_code_file_assumes_success(tmp_path: Path) -> None: + """Systems that don't yet capture an exit code (Slurm, LSF, Kubernetes) keep the old behavior.""" + tr = _sleep_test_run(tmp_path) + + result = tr.test.was_run_successful(tr) + + assert result.is_successful is True + + +def test_was_run_successful_with_zero_exit_code(tmp_path: Path) -> None: + tr = _sleep_test_run(tmp_path) + tr.output_path.mkdir(parents=True) + (tr.output_path / EXIT_CODE_FILE_NAME).write_text("0\n") + + result = tr.test.was_run_successful(tr) + + assert result.is_successful is True + + +def test_was_run_successful_with_nonzero_exit_code(tmp_path: Path) -> None: + tr = _sleep_test_run(tmp_path) + tr.output_path.mkdir(parents=True) + (tr.output_path / EXIT_CODE_FILE_NAME).write_text("1\n") + + result = tr.test.was_run_successful(tr) + + assert result.is_successful is False + assert "exited with code 1" in result.error_message + + +def test_was_run_successful_with_unparseable_exit_code(tmp_path: Path) -> None: + tr = _sleep_test_run(tmp_path) + tr.output_path.mkdir(parents=True) + (tr.output_path / EXIT_CODE_FILE_NAME).write_text("not-a-number\n") + + result = tr.test.was_run_successful(tr) + + assert result.is_successful is False + assert "Could not parse exit code" in result.error_message From cd45a163ef044fd061c5fc64cd652abab0fb3532 Mon Sep 17 00:00:00 2001 From: shreyaskommuri Date: Mon, 27 Jul 2026 11:14:15 -0700 Subject: [PATCH 2/3] Fix pyright override incompatibility in sleep standalone strategy CI's pyright check failed: store_test_run(self, full_cmd: str) added a required positional parameter not present on the base CommandGenStrategy.store_test_run(self), a Liskov violation (reportIncompatibleMethodOverride). Compute full_cmd inside store_test_run via gen_exec_command(store=False) instead of taking it as a parameter, mirroring FioStandaloneCommandGenStrategy's existing store/gen_exec_command split exactly. Restores store_test_run(self) to match the base signature. Tested: uv run pytest -q (1767 passed, 18 skipped); uv run pyright src/cloudai/workloads/sleep/standalone_command_gen_strategy.py src/cloudai/workloads/sleep/sleep.py (0 errors, 1 pre-existing toml-stub warning also present on fio's equivalent file); ruff check; ruff format --check; manually re-ran the issue #980 repro against this version - still correctly reports FAILED. Issue: NVIDIA/cloudai#980 Signed-off-by: shreyaskommuri --- .../sleep/standalone_command_gen_strategy.py | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py b/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py index 5994041bc..121585945 100644 --- a/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py +++ b/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py @@ -32,22 +32,23 @@ def _generate_sleep_command(self) -> str: tdef: SleepTestDefinition = cast(SleepTestDefinition, self.test_run.test) return f"sleep {tdef.cmd_args.seconds}" - def store_test_run(self, full_cmd: str) -> None: + def store_test_run(self) -> None: test_cmd = self._generate_sleep_command() + full_cmd = self.gen_exec_command(store=False) self.test_run.output_path.mkdir(parents=True, exist_ok=True) with (self.test_run.output_path / self.TEST_RUN_DUMP_FILE_NAME).open("w", encoding="utf-8") as f: trd = TestRunDetails.from_test_run(self.test_run, test_cmd=test_cmd, full_cmd=full_cmd) toml.dump(trd.model_dump(), f) - def gen_exec_command(self) -> str: + def gen_exec_command(self, store: bool = True) -> str: + if store: + self.store_test_run() stdout_path = self.test_run.output_path / "stdout.txt" stderr_path = self.test_run.output_path / "stderr.txt" exit_code_path = self.test_run.output_path / EXIT_CODE_FILE_NAME - full_cmd = ( + return ( f"{self._generate_sleep_command()} " f"> {shlex.quote(str(stdout_path))} 2> {shlex.quote(str(stderr_path))}; " f"echo $? > {shlex.quote(str(exit_code_path))}" ) - self.store_test_run(full_cmd) - return full_cmd From 8ef67036e597ca93816e6ed26a763afe7297c06d Mon Sep 17 00:00:00 2001 From: shreyaskommuri Date: Mon, 27 Jul 2026 11:21:13 -0700 Subject: [PATCH 3/3] Address CodeRabbit feedback on sleep exit-code PR - was_run_successful() now catches OSError/UnicodeDecodeError from exit_code_path.read_text() and returns a clean FAILED JobStatusResult instead of letting the exception propagate out of status evaluation. - Make gen_exec_command's store parameter keyword-only (ruff FBT001/FBT002: boolean positional argument), matching the only call site (self.gen_exec_command(store=False)) which already used keyword form. - Added test coverage for both new error paths (mocked PermissionError for the OSError case, real invalid UTF-8 bytes for the decode-error case). Tested: uv run pytest -q (1769 passed, 18 skipped); uv run pyright src/cloudai/workloads/sleep/sleep.py src/cloudai/workloads/sleep/standalone_command_gen_strategy.py tests/workloads/sleep/test_sleep.py (0 errors); ruff check; ruff format --check Issue: NVIDIA/cloudai#980 Signed-off-by: shreyaskommuri --- src/cloudai/workloads/sleep/sleep.py | 9 ++++++- .../sleep/standalone_command_gen_strategy.py | 2 +- tests/workloads/sleep/test_sleep.py | 24 +++++++++++++++++++ 3 files changed, 33 insertions(+), 2 deletions(-) diff --git a/src/cloudai/workloads/sleep/sleep.py b/src/cloudai/workloads/sleep/sleep.py index ba92fb755..69c46882d 100644 --- a/src/cloudai/workloads/sleep/sleep.py +++ b/src/cloudai/workloads/sleep/sleep.py @@ -46,7 +46,14 @@ def was_run_successful(self, tr: TestRun) -> JobStatusResult: # this file. return JobStatusResult(is_successful=True) - exit_code_text = exit_code_path.read_text().strip() + try: + exit_code_text = exit_code_path.read_text().strip() + except (OSError, UnicodeDecodeError) as e: + return JobStatusResult( + is_successful=False, + error_message=f"Could not read exit code file {exit_code_path}: {e}.", + ) + try: exit_code = int(exit_code_text) except ValueError: diff --git a/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py b/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py index 121585945..9f5b05762 100644 --- a/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py +++ b/src/cloudai/workloads/sleep/standalone_command_gen_strategy.py @@ -40,7 +40,7 @@ def store_test_run(self) -> None: trd = TestRunDetails.from_test_run(self.test_run, test_cmd=test_cmd, full_cmd=full_cmd) toml.dump(trd.model_dump(), f) - def gen_exec_command(self, store: bool = True) -> str: + def gen_exec_command(self, *, store: bool = True) -> str: if store: self.store_test_run() stdout_path = self.test_run.output_path / "stdout.txt" diff --git a/tests/workloads/sleep/test_sleep.py b/tests/workloads/sleep/test_sleep.py index cb602d210..0382d2c79 100644 --- a/tests/workloads/sleep/test_sleep.py +++ b/tests/workloads/sleep/test_sleep.py @@ -15,6 +15,7 @@ # limitations under the License. from pathlib import Path +from unittest.mock import patch from cloudai.core import TestRun from cloudai.workloads.sleep import SleepCmdArgs, SleepTestDefinition @@ -70,3 +71,26 @@ def test_was_run_successful_with_unparseable_exit_code(tmp_path: Path) -> None: assert result.is_successful is False assert "Could not parse exit code" in result.error_message + + +def test_was_run_successful_with_unreadable_exit_code_file(tmp_path: Path) -> None: + tr = _sleep_test_run(tmp_path) + tr.output_path.mkdir(parents=True) + (tr.output_path / EXIT_CODE_FILE_NAME).write_text("0\n") + + with patch("pathlib.Path.read_text", side_effect=PermissionError("permission denied")): + result = tr.test.was_run_successful(tr) + + assert result.is_successful is False + assert "Could not read exit code file" in result.error_message + + +def test_was_run_successful_with_undecodable_exit_code_file(tmp_path: Path) -> None: + tr = _sleep_test_run(tmp_path) + tr.output_path.mkdir(parents=True) + (tr.output_path / EXIT_CODE_FILE_NAME).write_bytes(b"\xff\xfe\x00") + + result = tr.test.was_run_successful(tr) + + assert result.is_successful is False + assert "Could not read exit code file" in result.error_message