[minor] Move create_executor() function to separate module #550
Conversation
|
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 14 minutes and 32 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughThis pull request moves the source of the Changes
Sequence Diagram(s)sequenceDiagram
participant U as User/Client
participant E as Executor
participant C as create_executor
participant B as Backend Executor
U->>E: Instantiate Executor
E->>C: Call create_executor(backend, parameters)
C->>B: Validate backend & resources
B-->>C: Return appropriate executor instance
C-->>E: Return executor instance
Poem
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
executorlib/interactive/create.py (5)
100-100: Use membership testing directly on the dict.Use
if "openmpi_oversubscribe" in resource_dict:instead ofif "openmpi_oversubscribe" in resource_dict.keys():.-if "openmpi_oversubscribe" in resource_dict.keys(): +if "openmpi_oversubscribe" in resource_dict:🧰 Tools
🪛 Ruff (0.8.2)
100-100: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
102-102: Use membership testing directly on the dict.Use
if "slurm_cmd_args" in resource_dict:instead ofif "slurm_cmd_args" in resource_dict.keys():.-if "slurm_cmd_args" in resource_dict.keys(): +if "slurm_cmd_args" in resource_dict:🧰 Tools
🪛 Ruff (0.8.2)
102-102: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
170-170: Use membership testing directly on the dict.Use
if "threads_per_core" in resource_dict:instead ofif "threads_per_core" in resource_dict.keys():.-if "threads_per_core" in resource_dict.keys(): +if "threads_per_core" in resource_dict:🧰 Tools
🪛 Ruff (0.8.2)
170-170: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
172-172: Use membership testing directly on the dict.Use
if "gpus_per_core" in resource_dict:instead ofif "gpus_per_core" in resource_dict.keys():.-if "gpus_per_core" in resource_dict.keys(): +if "gpus_per_core" in resource_dict:🧰 Tools
🪛 Ruff (0.8.2)
172-172: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
174-174: Use membership testing directly on the dict.Use
if "slurm_cmd_args" in resource_dict:instead ofif "slurm_cmd_args" in resource_dict.keys():.-if "slurm_cmd_args" in resource_dict.keys(): +if "slurm_cmd_args" in resource_dict:🧰 Tools
🪛 Ruff (0.8.2)
174-174: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
executorlib/__init__.py(1 hunks)executorlib/interactive/create.py(1 hunks)executorlib/interactive/executor.py(1 hunks)tests/test_dependencies_executor.py(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- tests/test_dependencies_executor.py
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/interactive/create.py
38-38: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
100-100: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
102-102: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
170-170: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
172-172: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
174-174: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build
🔇 Additional comments (5)
executorlib/interactive/executor.py (1)
5-6: Congrats on the modular import.The new import from
executorlib.interactive.createproperly references the relocated function. No issues found.executorlib/interactive/create.py (3)
3-31: Imports and optional dependency management look good.The try-except block for Flux ensures graceful handling of optional dependencies. No issues found.
47-85: Comprehensive docstring.The docstring clearly outlines usage, parameters, and resource requirements. Nicely done.
93-99: Well-structured branching logic for different backends.The code effectively handles Flux, SLURM, and local scenarios with consistent validations. The flow is straightforward and readable.
Also applies to: 101-101, 103-169, 171-171, 173-173, 175-195
executorlib/__init__.py (1)
4-4: Correct relocation of the import statement.The import reference to
_create_executorfromexecutorlib.interactive.createnow correctly reflects the new file structure.
executorlib/interactive/create.py
Outdated
| def create_executor( | ||
| max_workers: Optional[int] = None, | ||
| backend: str = "local", | ||
| max_cores: Optional[int] = None, | ||
| cache_directory: Optional[str] = None, | ||
| resource_dict: dict = {}, | ||
| flux_executor=None, | ||
| flux_executor_pmi_mode: Optional[str] = None, | ||
| flux_executor_nesting: bool = False, | ||
| flux_log_files: bool = False, | ||
| hostname_localhost: Optional[bool] = None, | ||
| block_allocation: bool = False, | ||
| init_function: Optional[Callable] = None, | ||
| ): |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutable default argument for resource_dict.
It's safer to use a default value of None instead of {} to avoid unintended side effects from mutable default arguments.
Here's a suggested fix:
def create_executor(
max_workers: Optional[int] = None,
backend: str = "local",
max_cores: Optional[int] = None,
cache_directory: Optional[str] = None,
- resource_dict: dict = {},
+ resource_dict: Optional[dict] = None,
flux_executor=None,
...
):
if resource_dict is None:
resource_dict = {}📝 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.
| def create_executor( | |
| max_workers: Optional[int] = None, | |
| backend: str = "local", | |
| max_cores: Optional[int] = None, | |
| cache_directory: Optional[str] = None, | |
| resource_dict: dict = {}, | |
| flux_executor=None, | |
| flux_executor_pmi_mode: Optional[str] = None, | |
| flux_executor_nesting: bool = False, | |
| flux_log_files: bool = False, | |
| hostname_localhost: Optional[bool] = None, | |
| block_allocation: bool = False, | |
| init_function: Optional[Callable] = None, | |
| ): | |
| def create_executor( | |
| max_workers: Optional[int] = None, | |
| backend: str = "local", | |
| max_cores: Optional[int] = None, | |
| cache_directory: Optional[str] = None, | |
| resource_dict: Optional[dict] = None, | |
| flux_executor=None, | |
| flux_executor_pmi_mode: Optional[str] = None, | |
| flux_executor_nesting: bool = False, | |
| flux_log_files: bool = False, | |
| hostname_localhost: Optional[bool] = None, | |
| block_allocation: bool = False, | |
| init_function: Optional[Callable] = None, | |
| ): | |
| if resource_dict is None: | |
| resource_dict = {} | |
| # ... rest of the function implementation ... |
🧰 Tools
🪛 Ruff (0.8.2)
38-38: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
executorlib/interactive/create.py (1)
158-161: Simplify dictionary key checks.Using
.keys()is unnecessary when checking for key existence.Apply this diff to simplify the checks:
- if "openmpi_oversubscribe" in resource_dict.keys(): + if "openmpi_oversubscribe" in resource_dict: del resource_dict["openmpi_oversubscribe"] - if "slurm_cmd_args" in resource_dict.keys(): + if "slurm_cmd_args" in resource_dict: del resource_dict["slurm_cmd_args"]Apply similar changes to all other
.keys()usage.Also applies to: 236-241
🧰 Tools
🪛 Ruff (0.8.2)
158-158: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
160-160: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executorlib/interactive/create.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/interactive/create.py
38-38: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
150-150: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
158-158: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
160-160: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
197-197: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
232-232: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
236-236: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
238-238: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
240-240: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
🪛 GitHub Actions: MyPy
executorlib/interactive/create.py
[error] 176-176: Argument "cores" to "validate_max_workers" has incompatible type "int | None"; expected "int"
[error] 211-211: Argument "cores" to "validate_max_workers" has incompatible type "int | None"; expected "int"
⏰ Context from checks skipped due to timeout of 90000ms (14)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (1)
executorlib/interactive/create.py (1)
38-38: Fix mutable default argument.Using mutable default arguments can lead to unexpected behavior.
🧰 Tools
🪛 Ruff (0.8.2)
38-38: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
| validate_max_workers_flux( | ||
| max_workers=max_workers, | ||
| cores=cores_per_worker, | ||
| threads_per_core=resource_dict.get("threads_per_core", 1), | ||
| ) |
There was a problem hiding this comment.
Fix type incompatibility in validate_max_workers calls.
The cores parameter in validate_max_workers calls expects a non-optional int, but cores_per_worker is Optional[int].
Add validation before the calls:
+ if cores_per_worker is None:
+ cores_per_worker = 1
validate_max_workers_flux(
max_workers=max_workers,
cores=cores_per_worker,
threads_per_core=resource_dict.get("threads_per_core", 1),
)Apply the same fix for validate_max_workers_slurm.
Also applies to: 209-213
🧰 Tools
🪛 GitHub Actions: MyPy
[error] 176-176: Argument "cores" to "validate_max_workers" has incompatible type "int | None"; expected "int"
| raise ValueError( | ||
| "The supported backends are slurm_allocation, slurm_submission, flux_allocation, flux_submission and local." | ||
| ) |
There was a problem hiding this comment.
Update error message to match supported backends.
The error message lists backends that don't match the actual supported values in the code. The message mentions "slurm_submission" and "flux_submission" which aren't handled in the code.
Apply this diff to fix the error message:
- "The supported backends are slurm_allocation, slurm_submission, flux_allocation, flux_submission and local."
+ "The supported backends are slurm_allocation, flux_allocation and local."📝 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.
| raise ValueError( | |
| "The supported backends are slurm_allocation, slurm_submission, flux_allocation, flux_submission and local." | |
| ) | |
| raise ValueError( | |
| "The supported backends are slurm_allocation, flux_allocation and local." | |
| ) |
Summary by CodeRabbit
These updates enhance executor management while preserving existing workflows and overall behavior.