Make Sleep's was_run_successful check the real exit code - #981
Open
shreyaskommuri wants to merge 3 commits into
Open
Make Sleep's was_run_successful check the real exit code#981shreyaskommuri wants to merge 3 commits into
shreyaskommuri wants to merge 3 commits into
Conversation
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#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 <shreyaskommuri@gmail.com>
shreyaskommuri
requested review from
jeffnvidia,
podkidyshev and
srivatsankrishnan
as code owners
July 27, 2026 18:08
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesSleep result tracking
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related issues
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR fixes the Sleep workload always reporting success by capturing the standalone command’s real exit code and having SleepTestDefinition.was_run_successful() evaluate that exit code when available.
Changes:
- Add
SleepTestDefinition.was_run_successful()implementation that reads anexit_code.txtartifact (fallbacks to prior behavior if missing). - Update
SleepStandaloneCommandGenStrategy.gen_exec_command()to redirect stdout/stderr to files and persist the command’s exit code. - Add unit tests covering exit-code parsing and standalone command generation.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| tests/workloads/sleep/test_sleep.py | Adds tests for SleepTestDefinition.was_run_successful() behavior with missing/valid/invalid exit code artifacts. |
| tests/workloads/sleep/test_command_gen_strategy_standalone.py | Updates standalone command-gen expectations to include stdout/stderr redirection and exit-code capture. |
| src/cloudai/workloads/sleep/standalone_command_gen_strategy.py | Implements standalone stdout/stderr redirection and writes exit_code.txt for later status evaluation. |
| src/cloudai/workloads/sleep/sleep.py | Introduces exit-code artifact name constant and implements SleepTestDefinition.was_run_successful(). |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+35
to
+53
| 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cloudai/workloads/sleep/sleep.py`:
- Around line 49-56: Update the exit-code reading flow in the sleep status
evaluation to catch OSError and UnicodeDecodeError from
exit_code_path.read_text(). Return an unsuccessful JobStatusResult with an
appropriate error_message for either unreadable-file case, while preserving the
existing ValueError handling for invalid numeric contents.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 120a2561-f4a3-4661-aecf-322e14216ae7
📒 Files selected for processing (4)
src/cloudai/workloads/sleep/sleep.pysrc/cloudai/workloads/sleep/standalone_command_gen_strategy.pytests/workloads/sleep/test_command_gen_strategy_standalone.pytests/workloads/sleep/test_sleep.py
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 NVIDIA#980 repro against this version - still correctly reports FAILED. Issue: NVIDIA#980 Signed-off-by: shreyaskommuri <shreyaskommuri@gmail.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/cloudai/workloads/sleep/standalone_command_gen_strategy.py`:
- Line 43: Update StandaloneCommandGenStrategy.gen_exec_command so the store
boolean parameter is keyword-only, preserving its default value and existing
command-generation behavior while preventing positional arguments.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 3bdb3d0e-cd11-4f89-ad4a-64abef6f3ba0
📒 Files selected for processing (1)
src/cloudai/workloads/sleep/standalone_command_gen_strategy.py
Comment on lines
+40
to
+43
| 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}" | ||
| ) |
| # this file. | ||
| return JobStatusResult(is_successful=True) | ||
|
|
||
| exit_code_text = exit_code_path.read_text().strip() |
Comment on lines
36
to
38
| 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) |
- 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#980 Signed-off-by: shreyaskommuri <shreyaskommuri@gmail.com>
Contributor
Author
|
Addressed both current CodeRabbit comments:
(The earlier Copilot comment about Re-ran |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/cloudai/workloads/sleep/sleep.py:50
Path.read_text()defaults to the process locale encoding, so the behavior (and thetest_was_run_successful_with_undecodable_exit_code_fileexpectation) can vary across environments. Reading the exit-code file with an explicit UTF-8 encoding makes this deterministic and matches the pattern used elsewhere in the codebase when parsing workload artifacts.
exit_code_text = exit_code_path.read_text().strip()
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
SleepTestDefinitionnever overrodewas_run_successful(), so it fell back to the baseTestDefinitiondefault, which unconditionally returnsis_successful=True— no check of exit code, output, or anything elsefio.py,vllm.py,nccl.py,nixl_ep.py,dynamo_mocker.py,nemo_run.py,sglang.py) implements a realwas_run_successful(). Sleep did notSleepStandaloneCommandGenStrategy.gen_exec_command()now redirects stdout/stderr to files and captures the real exit code, following the same patternFioStandaloneCommandGenStrategyalready uses for stdout/stderr redirectionSleepTestDefinition.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 — additive, not a behavior change for those systemsSeparately noticed, out of scope for this PR: even with
was_run_successfulnow correctly returningFAILED,handle_dry_run_and_run's non-DSE path (src/cloudai/cli/handlers.py) unconditionally returns0regardless of job outcome, so the overallcloudai runprocess 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 addressed here.How this was found
Found while validating CloudAI through a small wrapper project, CloudAI Autotune. I was running a real (non-dry-run)
cloudai runthrough Autotune to check whether a genuinely-failing command would be surfaced correctly — it wasn't. Reproduced directly againstcloudai runtoo (not just through the wrapper). Filed as #980.Issue: #980
Test Plan
uv run pytest -q— 1767 passed, 18 skippeduv run ruff check/uv run ruff format --checkon the changed files — cleanseconds = -1(which fails withsleep: illegal option -- 1, exit code 1). Before this fix: results table showedPASSED. After: results table correctly showsFAILEDwithsleep exited with code 1. Check .../stderr.txt for details.AI was used for context and guidance while investigating, implementing, and testing this fix.