Report run status from Slurm exit code in SlurmContainer#964
Report run status from Slurm exit code in SlurmContainer#964malagows-nvd wants to merge 1 commit into
Conversation
malagows-nvd
requested review from
jeffnvidia,
podkidyshev and
srivatsankrishnan
as code owners
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughAdds a ChangesSlurm container success grading
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 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}
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
| with slurm_job_path.open("r", encoding="utf-8") as file: | ||
| metadata = SlurmJobMetadata.model_validate(toml.load(file)) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm whether callers of was_run_successful catch exceptions.
rg -n -B3 -A6 'def is_successful' src/cloudai/reporter.py
rg -n -B3 -A10 'was_run_successful' src/cloudai/reporter.py src/cloudai/_coreRepository: NVIDIA/cloudai
Length of output: 3665
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the workload loader and the metadata model.
sed -n '1,180p' src/cloudai/workloads/slurm_container/slurm_container.py
printf '\n---\n'
rg -n -A6 -B6 'class SlurmJobMetadata|model_config|extra\s*=' src/cloudai/workloads/slurm_container src/cloudai/_core
# Inspect the runner/status plumbing around the call sites.
printf '\n=== reporter.py ===\n'
sed -n '100,240p' src/cloudai/reporter.py
printf '\n=== base_runner.py ===\n'
sed -n '260,320p' src/cloudai/_core/base_runner.pyRepository: NVIDIA/cloudai
Length of output: 12906
Catch Slurm metadata parse errors in was_run_successful (src/cloudai/workloads/slurm_container/slurm_container.py:71-72) toml.load() and SlurmJobMetadata.model_validate() can raise on malformed or partially written slurm-job.toml, and the result is consumed directly by Reporter.is_successful() and BaseRunner.get_job_status(). Return a failed JobStatusResult with an error message instead of letting the exception abort grading/reporting for the whole scenario.
🤖 Prompt for 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.
In `@src/cloudai/workloads/slurm_container/slurm_container.py` around lines 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.
| @pytest.mark.parametrize( | ||
| ("exit_code", "is_successful"), | ||
| ( | ||
| ("0:0", True), | ||
| ("0", True), | ||
| ("0:15", True), | ||
| ("1:0", False), | ||
| ("15:0", False), | ||
| ("42:0", False), | ||
| ("137:0", False), | ||
| ), | ||
| ) |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
Use list of tuples for parametrize values.
Static analysis flags PT007: the values collection should be a list, not a tuple, of tuples.
🎨 Proposed fix
`@pytest.mark.parametrize`(
("exit_code", "is_successful"),
- (
+ [
("0:0", True),
("0", True),
("0:15", True),
("1:0", False),
("15:0", False),
("42:0", False),
("137:0", False),
- ),
+ ],
)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| @pytest.mark.parametrize( | |
| ("exit_code", "is_successful"), | |
| ( | |
| ("0:0", True), | |
| ("0", True), | |
| ("0:15", True), | |
| ("1:0", False), | |
| ("15:0", False), | |
| ("42:0", False), | |
| ("137:0", False), | |
| ), | |
| ) | |
| `@pytest.mark.parametrize`( | |
| ("exit_code", "is_successful"), | |
| [ | |
| ("0:0", True), | |
| ("0", True), | |
| ("0:15", True), | |
| ("1:0", False), | |
| ("15:0", False), | |
| ("42:0", False), | |
| ("137:0", False), | |
| ], | |
| ) |
🧰 Tools
🪛 Ruff (0.15.20)
[warning] 64-72: Wrong values type in pytest.mark.parametrize expected list of tuple
Use list of tuple for parameter values
(PT007)
🤖 Prompt for 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.
In `@tests/workloads/slurm_container/test_slurm_container.py` around lines 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.
Source: Linters/SAST tools
Grade the run via was_run_successful() - currently based on the exit_code field in slurm-job.toml (non-zero = FAILED). Signed-off-by: Marcin Malagowski <mmalagowski@nvidia.com>
malagows-nvd
force-pushed
the
mm/slurmcontainer-add-wasrunsuccessful
branch
from
a286eb8 to
3081afb
Compare
1 participant
Grade the run via
was_run_successful()- currently based on the exit_code field in slurm-job.toml (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.tomlTest Plan
Scenario TOML
CloudAI run
PyTest
Additional Notes
None