Fix standalone sleep dry-run command generation#921
Conversation
Implement the sleep standalone command generator's store_test_run contract so dry-run can instantiate it and persist test-run.toml like the other standalone generators. Constraint: CloudAI CommandGenStrategy requires store_test_run on concrete strategies Rejected: Leaving sleep standalone without persisted test-run details | it keeps the documented dry-run smoke path broken Confidence: high Scope-risk: narrow Directive: Preserve test-run.toml persistence when changing standalone command generator contracts Tested: uv run pytest tests/workloads/sleep/test_command_gen_strategy_standalone.py -q Tested: uv run pytest tests/workloads/sleep -q Tested: uv run pytest tests/test_init.py tests/workloads/sleep/test_command_gen_strategy_standalone.py tests/workloads/sleep/test_command_gen_strategy_slurm.py -q Tested: uv run ruff check src/cloudai/workloads/sleep/standalone_command_gen_strategy.py tests/workloads/sleep/test_command_gen_strategy_standalone.py Not-tested: Full sleep dry-run completion; local harness interrupted the CLI smoke run after it passed the previous abstract-class crash and wrote test-run.toml files Signed-off-by: shreyaskommuri <shreyaskommuri@gmail.com>
|
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 (1)
📝 WalkthroughWalkthroughImplements TOML persistence for sleep test runs: SleepStandaloneCommandGenStrategy now builds the ChangesSleep Command Generator Test Run Persistence
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cloudai/workloads/sleep/standalone_command_gen_strategy.py (1)
30-44: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winExtract command generation to eliminate duplication.
The command generation logic (casting test definition, extracting seconds, formatting command) is duplicated between
store_test_run()(lines 31-33) andgen_exec_command()(lines 41-44). This violates DRY and could lead to inconsistencies if one is updated without the other.♻️ Proposed refactor to extract command generation
+ def _generate_sleep_command(self) -> str: + """Generate the sleep command string from test definition.""" + tdef: SleepTestDefinition = cast(SleepTestDefinition, self.test_run.test) + return f"sleep {tdef.cmd_args.seconds}" + def store_test_run(self) -> None: - tdef: SleepTestDefinition = cast(SleepTestDefinition, self.test_run.test) - sec = tdef.cmd_args.seconds - test_cmd = f"sleep {sec}" + 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) toml.dump(trd.model_dump(), f) def gen_exec_command(self) -> str: self.store_test_run() - tdef: SleepTestDefinition = cast(SleepTestDefinition, self.test_run.test) - tdef_cmd_args: SleepCmdArgs = tdef.cmd_args - sec = tdef_cmd_args.seconds - return f"sleep {sec}" + return self._generate_sleep_command()🤖 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/sleep/standalone_command_gen_strategy.py` around lines 30 - 44, The command construction is duplicated in store_test_run() and gen_exec_command() leading to DRY violations; extract a helper (e.g., _build_sleep_command or similar) that accepts no args, performs the cast to SleepTestDefinition, reads cmd_args.seconds, and returns the formatted command string ("sleep {sec}"); update store_test_run() to call this helper and pass its result into TestRunDetails.from_test_run(..., test_cmd=..., full_cmd=...), and update gen_exec_command() to simply return the helper's value so both sites share the same logic.
🤖 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.
Outside diff comments:
In `@src/cloudai/workloads/sleep/standalone_command_gen_strategy.py`:
- Around line 30-44: The command construction is duplicated in store_test_run()
and gen_exec_command() leading to DRY violations; extract a helper (e.g.,
_build_sleep_command or similar) that accepts no args, performs the cast to
SleepTestDefinition, reads cmd_args.seconds, and returns the formatted command
string ("sleep {sec}"); update store_test_run() to call this helper and pass its
result into TestRunDetails.from_test_run(..., test_cmd=..., full_cmd=...), and
update gen_exec_command() to simply return the helper's value so both sites
share the same logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: a9ba7142-e800-49f5-a92f-3bf51d7d03aa
📒 Files selected for processing (2)
src/cloudai/workloads/sleep/standalone_command_gen_strategy.pytests/workloads/sleep/test_command_gen_strategy_standalone.py
The CodeRabbit review found the persisted test command and returned exec command were assembled independently. This keeps both paths sourced from one helper while preserving the existing output. Constraint: Address PR NVIDIA#921 review without changing sleep command behavior Rejected: Keep duplicated command formatting | preserves the reviewed drift risk Confidence: high Scope-risk: narrow Directive: Keep persisted test_cmd and returned exec command sourced from the same helper Tested: UV_CACHE_DIR=.uv-cache uv run --extra dev pytest tests/workloads/sleep/test_command_gen_strategy_standalone.py Not-tested: Full test suite
|
@shreyaskommuri please fix the tests (looks like just copyright) |
Update the modified standalone sleep command generator header so the CI-only copyright check reflects the 2026 edit year. Constraint: CI derives expected copyright years from git history and dirty status. Rejected: Changing the copyright checker | the checker correctly identified the modified source file. Confidence: high Scope-risk: narrow Directive: Preserve generated copyright year ranges when modifying existing files. Tested: uv run python -m pytest tests/test_check_copyright_headers.py -q -m ci_only; uv run pytest tests/test_init.py tests/workloads/sleep/test_command_gen_strategy_standalone.py tests/workloads/sleep/test_command_gen_strategy_slurm.py -q; git diff --check Not-tested: Full repository pytest suite after this one-line metadata fix.
|
Fixed the CI copyright failure and pushed the update in commit 325fa78. Validation rerun on the rebased PR branch:
|
Summary
store_test_run()forSleepStandaloneCommandGenStrategyso the standalone sleep dry-run command generator is concrete and can be instantiated.test-run.tomlwithTestRunDetails, matching the command generator contract used by other standalone workloads.Test Plan
issue/sleep-standalone-store-test-run.uv run pytest tests/workloads/sleep/test_command_gen_strategy_standalone.py -q1 passed in 0.01suv run pytest tests/workloads/sleep -q4 passed in 0.19suv run pytest tests/test_init.py tests/workloads/sleep/test_command_gen_strategy_standalone.py tests/workloads/sleep/test_command_gen_strategy_slurm.py -q12 passed in 0.02suv run ruff check src/cloudai/workloads/sleep/standalone_command_gen_strategy.py tests/workloads/sleep/test_command_gen_strategy_standalone.pyAll checks passed!uv run pre-commit run --all-filespyright,ruff check,ruff format,vulture,import-linter, andtaplo.I also ran a standalone sleep dry-run smoke check with
--output-dir /tmp/.... It got past the previous abstract-class crash, executed the sleep commands, and wrotetest-run.tomlfiles for the sleep test runs. The local harness interrupted before full CLI completion, so I am not claiming full dry-run completion from that smoke run.Additional Notes