Uh oh!
There was an error while loading. Please reload this page.
Move SLURM to separate module - #528
Conversation
WalkthroughThis pull request focuses on restructuring the SLURM spawner implementation by moving the Changes
Sequence DiagramsequenceDiagram
participant Executor as create_executor()
participant Spawner as SrunSpawner
participant Backend as SLURM Backend
Executor->>Spawner: Initialize with parameters
Spawner->>Backend: Generate SLURM command
Backend-->>Spawner: Return command list
Spawner-->>Executor: Return configured spawner
Possibly related PRs
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File ( |
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
executorlib/interactive/executor.py(2 hunks)executorlib/interactive/slurm.py(1 hunks)executorlib/standalone/__init__.py(1 hunks)executorlib/standalone/interactive/spawner.py(0 hunks)tests/test_shared_backend.py(1 hunks)
💤 Files with no reviewable changes (1)
- executorlib/standalone/interactive/spawner.py
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/interactive/slurm.py
16-16: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
67-67: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🔇 Additional comments (4)
executorlib/interactive/slurm.py (1)
1-94: Overall module structure and functionality look good.
The code successfully introduces the separate SLURM module and the SrunSpawner class with a clear interface. Aside from the mutable default arguments and the typo in the SLURM argument, everything else looks well-documented and consistent with the objective of moving SLURM functionality into its own module.
🧰 Tools
🪛 Ruff (0.8.2)
16-16: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
67-67: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
executorlib/standalone/__init__.py (1)
9-9: Removal of SrunSpawner from standalone exports is appropriate.
No issues found. This change aligns with moving SLURM logic to its dedicated module.
tests/test_shared_backend.py (1)
6-7: Updated import for SrunSpawner looks good.
All tests exercise the new SLURM spawner’s functionality properly, which is important for ensuring feature parity after the relocation.
executorlib/interactive/executor.py (1)
Line range hint 10-21: Integration with the new SrunSpawner is coherent.
The changes correctly reflect the separate SLURM module. The code structure for choosing spawners based on the backend remains clear and maintainable.
| if cwd is not None: | ||
| command_prepend_lst += ["-D", cwd] | ||
| if threads_per_core > 1: | ||
| command_prepend_lst += ["--cpus-per-task" + str(threads_per_core)] |
There was a problem hiding this comment.
Potential typo: missing equals sign in '--cpus-per-task'.
SLURM typically expects an equals sign for this argument to parse the integer value properly.
Proposed fix:
- command_prepend_lst += ["--cpus-per-task" + str(threads_per_core)]+ command_prepend_lst += ["--cpus-per-task=" + str(threads_per_core)]📝 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.
| command_prepend_lst+= ["--cpus-per-task"+str(threads_per_core)] | |
| command_prepend_lst+= ["--cpus-per-task="+str(threads_per_core)] |
| threads_per_core: int = 1, | ||
| gpus_per_core: int = 0, | ||
| openmpi_oversubscribe: bool = False, | ||
| slurm_cmd_args: list[str] = [], |
There was a problem hiding this comment.
Avoid using mutable default arguments.
Using mutable lists as default argument values can lead to unexpected behavior when multiple instances share the same list reference.
You can fix this by setting the default to None and initializing it in the method body:
- slurm_cmd_args: list[str] = [],+ slurm_cmd_args: Optional[list[str]] = None,And inside the constructor:
if slurm_cmd_args is None:
slurm_cmd_args = []Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
16-16: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
| threads_per_core: int = 1, | ||
| gpus_per_core: int = 0, | ||
| openmpi_oversubscribe: bool = False, | ||
| slurm_cmd_args: list[str] = [], |
There was a problem hiding this comment.
Same mutable default argument concern.
This function parameter also defines a mutable list as the default value, raising the same risk of shared state across multiple calls.
Use the same approach as above to ensure each invocation gets its own list instance:
- slurm_cmd_args: list[str] = [],+ slurm_cmd_args: Optional[list[str]] = None,and initialize within the function body:
if slurm_cmd_args is None:
slurm_cmd_args = []Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Ruff (0.8.2)
67-67: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
Summary by CodeRabbit
New Features
SrunSpawnerclass for managing SLURM job submissions with enhanced command generation capabilities.Bug Fixes
Documentation
Tests
SrunSpawnerclass and its parameters.