Do not run CommandShell check during object creation

[amaslenn]

Summary

Do not run CommandShell check during object creation. That addresses issue with building documentation on Windows.

Test Plan

1. CI
2. Docs build on Windows.

Additional Notes

Re-created as a replacement for #841 .

[coderabbitai]

📝 Walkthrough

Walkthrough

Two files were modified: SlurmSystem now uses default_factory for lazy instantiation of cmd_shell instead of eagerly creating a single instance. CommandShell had verbose docstrings removed and executable-path validation deferred from __init__ to execute method.

Changes

Cohort / File(s)	Summary
Pydantic Field Initialization src/cloudai/systems/slurm/slurm_system.py	Changed cmd_shell field from default=CommandShell() to default_factory=CommandShell, enabling new instance creation per model instantiation instead of sharing a single instance.
CommandShell Validation & Documentation src/cloudai/util/command_shell.py	Removed verbose docstring blocks from __init__ and execute methods. Shifted executable-path existence validation from __init__ to execute, which now raises FileNotFoundError when executable is missing.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Poem

🐰 A factory springs forth, no singleton's plight,
Each model gets fresh shells, oh what a delight!
Validation now waits till execution's call,
Docstrings retire—simpler code for all! 🐇

🚥 Pre-merge checks | ✅ 2

✅ Passed checks (2 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title accurately describes the main change: moving the CommandShell executable check from initialization to execution time.
Description check	✅ Passed	The description clearly relates to the changeset, explaining the motivation (documentation build issue on Windows) and the change being made.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch am/no-runtime-shell-check

---

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

[greptile-apps]

Greptile Summary

This PR fixes a Windows compatibility issue for documentation builds by deferring the /bin/bash existence check until a command is actually executed, rather than at object construction time.

Root cause fixed: Field(default=CommandShell()) in slurm_system.py evaluated CommandShell() eagerly at class-definition / module-import time. The old __init__ immediately checked executable.exists(), causing an ImportError-level failure on Windows (where /bin/bash does not exist) even when no commands were ever run.

Two complementary changes:

• slurm_system.py: default=CommandShell() → default_factory=CommandShell — correct Pydantic v2 pattern for per-instance factory defaults; also fixes the shared-singleton mutable-default anti-pattern.
• command_shell.py: existence check moved from __init__ to execute() — on Linux/HPC systems where Slurm actually runs, behaviour is unchanged; on Windows, construction succeeds and an error is only raised if execution is attempted.

Minor concern: The existence check now runs on every execute() call rather than once at construction, adding a redundant stat() syscall per command invocation.

Confidence Score: 4/5

• Safe to merge — the fix is targeted, correct, and non-breaking for all existing Linux/HPC usage.
• Both changes are logically sound: default_factory is the correct Pydantic v2 pattern, and deferring the existence check to execute() resolves the Windows import-time failure without affecting runtime behaviour on Linux. No existing tests relied on the old __init__ guard. The only minor concern is the repeated stat() per execute() call, which is a style/performance issue, not a correctness one.
• No files require special attention — the changes are minimal and well-scoped.

Important Files Changed

Filename	Overview
src/cloudai/util/command_shell.py	Moves the executable existence check from __init__ to execute(), deferring it until a command is actually run. Simplifies docstrings by removing Args/Returns/Raises sections. The logic change is correct for the Windows doc-build use case.
src/cloudai/systems/slurm/slurm_system.py	Switches from default=CommandShell() (eager singleton evaluation at class-definition time) to default_factory=CommandShell (lazy per-instance creation), which is both the correct Pydantic v2 pattern and fixes the root cause of the Windows import-time failure.

Sequence Diagram

sequenceDiagram
    participant DocBuild as Docs Build (Windows)
    participant SlurmSystem
    participant CommandShell

    Note over DocBuild,CommandShell: Before this PR (Windows fails at import)
    DocBuild->>SlurmSystem: import / class definition
    SlurmSystem->>CommandShell: CommandShell() [default= eager eval]
    CommandShell->>CommandShell: __init__: executable.exists()?
    CommandShell-->>SlurmSystem: ❌ FileNotFoundError (/bin/bash not found)

    Note over DocBuild,CommandShell: After this PR (Windows succeeds)
    DocBuild->>SlurmSystem: import / class definition
    Note right of SlurmSystem: default_factory=CommandShell<br/>(not evaluated yet)
    DocBuild->>SlurmSystem: SlurmSystem(...) instance created
    SlurmSystem->>CommandShell: CommandShell() [factory called now]
    CommandShell-->>SlurmSystem: ✅ instance created (no existence check)

    Note over DocBuild,CommandShell: execute() path (Linux HPC, runtime)
    SlurmSystem->>CommandShell: execute(command)
    CommandShell->>CommandShell: executable.exists()? ✅
    CommandShell->>CommandShell: subprocess.Popen(...)
    CommandShell-->>SlurmSystem: Popen process

Loading

_{Last reviewed commit: "Do not run CommandSh..."}

[coderabbitai]

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@src/cloudai/util/command_shell.py`:
- Around line 33-36: The execute method on CommandShell currently checks
self.executable.exists() on every call which causes repeated filesystem hits;
add a cached boolean (e.g., self._executable_valid or similar) initialized to
None and validate once (set True/False) the first time execute() runs (or
validate in __init__), then use that cached value in subsequent execute() calls
and raise FileNotFoundError only if the cached validation fails; update
references to self.executable and the execute method to use the cache and ensure
thread-safety if needed.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: a1e0a542-773d-4e70-b34a-f91d2874f0a0

📥 Commits

Reviewing files that changed from the base of the PR and between 9916d02 and d87f3da.

📒 Files selected for processing (2)

• src/cloudai/systems/slurm/slurm_system.py
• src/cloudai/util/command_shell.py