Merge release-v171 into main by podkidyshev · Pull Request #975 · NVIDIA/cloudai

podkidyshev · 2026-07-21T12:20:20Z

Summary

Upstream release v1.7.1 changes into main

Test Plan

Automated CI

Additional Notes

[Bug] Single-sbatch termination handling

coderabbitai · 2026-07-21T12:20:33Z

📝 Walkthrough

Walkthrough

Changes

Slurm job lifecycle

Layer / File(s)	Summary
Runner tracking and shutdown cleanup `src/cloudai/systems/slurm/single_sbatch_runner.py`	`SingleSbatchRunner.run()` tracks submitted jobs, kills jobs detected during shutdown, and always removes jobs from tracking.
Shutdown and completion coverage `tests/test_single_sbatch_runner.py`	Tests cover shutdown during monitoring or submission, plus completed jobs that should not be killed.

Estimated code review effort: 2 (Simple) | ~10 minutes

Suggested reviewers: jeffnvidia

🚥 Pre-merge checks | ✅ 4

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Title check	✅ Passed	The title accurately states that release-v171 is being merged into main.
Description check	✅ Passed	The description is clearly related to the merge and release v1.7.1 changes.

✨ Finishing Touches

🧪 Generate unit tests (beta)

Create PR with unit tests
Commit unit tests in branch release-v171

_{Comment @coderabbitai help to get the list of available commands.}

coderabbitai

Actionable comments posted: 3

🤖 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/systems/slurm/single_sbatch_runner.py`:
- Around line 190-193: Update the job submission flow around _submit_test so
placeholder jobs returned in dry-run mode are neither appended to tracking nor
passed to SlurmSystem.kill(); gate both operations on self.mode == "run". Add a
regression test covering shutdown during dry-run and verify no cancellation is
issued for the placeholder job.
- Around line 192-203: Move the self.shutting_down check and
self.system.kill(job) call inside the try/finally scope in the job monitoring
flow, preserving the existing loop behavior. Ensure self.jobs.remove(job) always
executes, including when killing the job raises.

In `@tests/test_single_sbatch_runner.py`:
- Around line 534-555: Extend the shutdown regression coverage around
test_run_tracks_and_cancels_job_on_shutdown with a cancellation-failure case
where SlurmSystem.kill raises, asserting runner.jobs is still cleared, and a
dry-run case asserting shutdown does not invoke Slurm cancellation. Reuse the
existing runner/job setup and monitoring shutdown flow, varying only the mode
and kill behavior.

🪄 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: d3a4d7e5-0f13-432d-8a55-7ac8810ba561

📥 Commits

Reviewing files that changed from the base of the PR and between 09f728c and f597e38.

📒 Files selected for processing (2)

src/cloudai/systems/slurm/single_sbatch_runner.py
tests/test_single_sbatch_runner.py

coderabbitai · 2026-07-21T12:27:22Z

+        self.jobs.append(job)
+
+        if self.shutting_down:
+            self.system.kill(job)


🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Do not cancel placeholder jobs in dry-run mode.

Dry-run _submit_test returns a SlurmJob with ID 0; this unconditional call reaches SlurmSystem.kill() and invokes scancel(0). Gate tracking and cancellation on self.mode == "run", and add a dry-run shutdown regression test.

🤖 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/systems/slurm/single_sbatch_runner.py` around lines 190 - 193, Update the job submission flow around _submit_test so placeholder jobs returned in dry-run mode are neither appended to tracking nor passed to SlurmSystem.kill(); gate both operations on self.mode == "run". Add a regression test covering shutdown during dry-run and verify no cancellation is issued for the placeholder job.

coderabbitai · 2026-07-21T12:27:22Z

+        if self.shutting_down:
+            self.system.kill(job)

        is_completed = False
-        while not is_completed:
-            if self.shutting_down:
-                break
-            is_completed = True if self.mode == "dry-run" else self.system.is_job_completed(job)
-            time.sleep(self.system.monitor_interval)
+        try:
+            while not is_completed:
+                if self.shutting_down:
+                    break
+                is_completed = True if self.mode == "dry-run" else self.system.is_job_completed(job)
+                time.sleep(self.system.monitor_interval)
+        finally:
+            self.jobs.remove(job)


🩺 Stability & Availability | 🟠 Major | ⚡ Quick win

Keep cancellation inside the cleanup-protected scope.

If self.system.kill(job) raises, it executes before the try/finally, leaving job in self.jobs. Move the shutdown check into the try block so tracking is removed even when scancel fails.

Proposed fix

- if self.shutting_down: - self.system.kill(job) - - is_completed = False try: + if self.shutting_down: + self.system.kill(job) + is_completed = False while not is_completed:

📝 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.

Suggested change

if self.shutting_down:

self.system.kill(job)

is_completed = False

while not is_completed:

if self.shutting_down:

break

is_completed = True if self.mode == "dry-run" else self.system.is_job_completed(job)

time.sleep(self.system.monitor_interval)

try:

while not is_completed:

if self.shutting_down:

break

is_completed = True if self.mode == "dry-run" else self.system.is_job_completed(job)

time.sleep(self.system.monitor_interval)

finally:

self.jobs.remove(job)

try:

if self.shutting_down:

self.system.kill(job)

is_completed = False

while not is_completed:

if self.shutting_down:

break

is_completed = True if self.mode == "dry-run" else self.system.is_job_completed(job)

time.sleep(self.system.monitor_interval)

finally:

self.jobs.remove(job)

🤖 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/systems/slurm/single_sbatch_runner.py` around lines 192 - 203, Move the self.shutting_down check and self.system.kill(job) call inside the try/finally scope in the job monitoring flow, preserving the existing loop behavior. Ensure self.jobs.remove(job) always executes, including when killing the job raises.

coderabbitai · 2026-07-21T12:27:22Z

+def test_run_tracks_and_cancels_job_on_shutdown(sleep_tr: TestRun, slurm_system: SlurmSystem) -> None:
+    tc = TestScenario(name="tc", test_runs=[sleep_tr])
+    runner = SingleSbatchRunner(mode="run", system=slurm_system, test_scenario=tc, output_path=slurm_system.output_path)
+    job = SlurmJob(sleep_tr, id=123)
+    runner._submit_test = Mock(return_value=job)
+    runner.handle_dse = Mock()
+    runner.on_job_completion = Mock()
+
+    def shutdown_during_monitoring(monitored_job: SlurmJob) -> bool:
+        assert monitored_job is job
+        assert runner.jobs == [job]
+        runner.shutdown()
+        return False
+
+    with (
+        patch.object(SlurmSystem, "is_job_completed", side_effect=shutdown_during_monitoring),
+        patch.object(SlurmSystem, "kill") as kill,
+    ):
+        runner.run()
+
+    kill.assert_called_once_with(job)
+    assert runner.jobs == []


🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick win

Add regression coverage for cancellation failures and dry-run shutdown.

These tests cover successful mode="run" cancellation only. Add cases asserting that a raised kill() still clears runner.jobs, and that shutdown in mode="dry-run" does not call Slurm cancellation.

Also applies to: 558-576, 578-594

🤖 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/test_single_sbatch_runner.py` around lines 534 - 555, Extend the shutdown regression coverage around test_run_tracks_and_cancels_job_on_shutdown with a cancellation-failure case where SlurmSystem.kill raises, asserting runner.jobs is still cleared, and a dry-run case asserting shutdown does not invoke Slurm cancellation. Reuse the existing runner/job setup and monitoring shutdown flow, varying only the mode and kill behavior.

podkidyshev added 2 commits July 20, 2026 12:25

fix single sbatch mode termination handling

4061184

Merge pull request #971 from NVIDIA/ipod/ssbatch-sigterm

f597e38

[Bug] Single-sbatch termination handling

podkidyshev self-assigned this Jul 21, 2026

podkidyshev requested review from jeffnvidia and srivatsankrishnan as code owners July 21, 2026 12:20

coderabbitai Bot reviewed Jul 21, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Merge release-v171 into main#975

Merge release-v171 into main#975
podkidyshev wants to merge 2 commits into
mainfrom
release-v171

podkidyshev commented Jul 21, 2026

Uh oh!

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Uh oh!

coderabbitai Bot Jul 21, 2026

Uh oh!

coderabbitai Bot Jul 21, 2026

Uh oh!

coderabbitai Bot Jul 21, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

podkidyshev commented Jul 21, 2026

Summary

Test Plan

Additional Notes

Uh oh!

coderabbitai Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

coderabbitai Bot Jul 21, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

coderabbitai Bot commented Jul 21, 2026 •

edited

Loading