Report run status from Slurm exit code in SlurmContainer - #964
Open
malagows-nvd wants to merge 2 commits into
Open
Report run status from Slurm exit code in SlurmContainer#964malagows-nvd wants to merge 2 commits into
malagows-nvd wants to merge 2 commits into
Conversation
malagows-nvd
requested review from
jeffnvidia,
podkidyshev and
srivatsankrishnan
as code owners
July 8, 2026 12:24
malagows-nvd
marked this pull request as draft
July 8, 2026 12:24
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)
📝 WalkthroughWalkthroughAdds exit-code recording to generated Slurm container commands and adds ChangesSlurm container exit status handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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: 2
🤖 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/slurm_container/slurm_container.py`:
- Around line 71-72: The `was_run_successful` path in `SlurmContainer` currently
lets `toml.load()` and `SlurmJobMetadata.model_validate()` throw on bad or
partial `slurm-job.toml`, which can abort `Reporter.is_successful()` and
`BaseRunner.get_job_status()`. Wrap the metadata parsing in this method with
error handling, catch parse/validation failures, and return a failed
`JobStatusResult` that includes an error message instead of propagating the
exception. Keep the fix localized to the `was_run_successful` logic and preserve
the existing success path when metadata loads correctly.
In `@tests/workloads/slurm_container/test_slurm_container.py`:
- Around line 62-73: The pytest parametrization in the test module uses a tuple
of tuples for `pytest.mark.parametrize`, which triggers PT007. Update the
`parametrize` values in `test_slurm_container.py` to use a list of tuples
instead of a tuple, keeping the existing `exit_code` and `is_successful` cases
unchanged.
🪄 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: ff3fb4b3-e0f1-470e-8df6-c60fa43d873b
📒 Files selected for processing (2)
src/cloudai/workloads/slurm_container/slurm_container.pytests/workloads/slurm_container/test_slurm_container.py
malagows-nvd
force-pushed
the
mm/slurmcontainer-add-wasrunsuccessful
branch
from
July 8, 2026 13:57
a286eb8 to
3081afb
Compare
malagows-nvd
force-pushed
the
mm/slurmcontainer-add-wasrunsuccessful
branch
2 times, most recently
from
August 4, 2026 12:52
1551373 to
fda7f77
Compare
malagows-nvd
marked this pull request as ready for review
August 4, 2026 13:23
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 `@doc/workloads/slurm_container.rst`:
- Around line 56-58: Update the success-criteria documentation to describe
CloudAI reading per-test exit_code.txt from the test output directory: only
integer 0 succeeds, while non-zero, missing, malformed, unreadable, or
undecodable files fail the run; explicitly remove the claim that 0:0 is
accepted.
🪄 Autofix
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: 5d2f4b8f-7cbc-44f2-8335-f5ddd5934faf
📒 Files selected for processing (6)
doc/workloads/slurm_container.rstsrc/cloudai/workloads/slurm_container/slurm_command_gen_strategy.pysrc/cloudai/workloads/slurm_container/slurm_container.pytests/ref_data/slurm_container.sbatchtests/workloads/slurm_container/test_command_gen_strategy_slurm.pytests/workloads/slurm_container/test_slurm_container.py
Grade the run via was_run_successful() - now based on the per-test exit_code.txt file (non-zero = FAILED). Document the exit-code file and its parsing and failure rules. Signed-off-by: Marcin Malagowski <mmalagowski@nvidia.com>
malagows-nvd
force-pushed
the
mm/slurmcontainer-add-wasrunsuccessful
branch
from
August 4, 2026 13:35
fda7f77 to
d6d97b2
Compare
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/slurm_container/slurm_command_gen_strategy.py`:
- Around line 49-55: Change the wrapper generated by the Slurm command strategy
so one aggregate srun status is recorded outside per-task container execution,
or collect per-task statuses and fail if any task fails; do not let concurrent
tasks overwrite a shared result with the last writer’s status. Update
tests/workloads/slurm_container/test_command_gen_strategy_slurm.py lines 32-38
for the new wrapper, add coverage at lines 114-128 for differing multi-task exit
codes, and regenerate tests/ref_data/slurm_container.sbatch line 17.
🪄 Autofix
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: 090b383f-afa9-4d4f-b63d-176bc858e827
📒 Files selected for processing (6)
doc/workloads/slurm_container.rstsrc/cloudai/workloads/slurm_container/slurm_command_gen_strategy.pysrc/cloudai/workloads/slurm_container/slurm_container.pytests/ref_data/slurm_container.sbatchtests/workloads/slurm_container/test_command_gen_strategy_slurm.pytests/workloads/slurm_container/test_slurm_container.py
Move exit-code recording outside per-task container execution so one host-side writer records the aggregate srun result. Add multi-task coverage and update the sbatch reference. Signed-off-by: Marcin Malagowski <mmalagowski@nvidia.com>
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
1 participant
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.
Grade the run via was_run_successful() - now based on the per-test exit_code.txt file (non-zero = FAILED).
Summary
Every run of SlurmContainer workload was reported PASSED in the scenario summary regardless of what the
container actually did. Grade the run from the container real exit code, which CloudAI already records in
slurm-job.tomlImplement
was_run_successful()to grade each run from the container command's actual exit code. The generated command writes the exit code to a per-testexit_code.txtfile, allowing accurate reporting in both regular and single-sbatch modes without changes to user scripts or container images.Test Plan
Scenario TOML
CloudAI run
CloudAI run --single-sbatch
PyTest (SlurmContainer)
PyTest (all)
Additional Notes
None