[minor] restructure create method #551
Conversation
for more information, see https://pre-commit.ci
|
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 10 minutes and 26 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)
WalkthroughThe pull request refactors the executor creation logic in Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant ExecutorFactory as create_executor
Client->>ExecutorFactory: call create_executor(backend, ...)
alt Backend is "flux", "slurm" or "local"
ExecutorFactory->>ExecutorFactory: Execute backend-specific checks<br/>& assign cache_directory, hostname_localhost, and other resources
ExecutorFactory-->>Client: Return appropriate executor instance
else Unsupported backend
ExecutorFactory-->>Client: Raise ValueError
end
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: 2
🔭 Outside diff range comments (1)
executorlib/interactive/create.py (1)
Line range hint
102-102: Fix type error: Remove unexpected keyword argument.The
create_flux_allocation_executorfunction is called withcores_per_workerbut this parameter is not defined in its signature, causing a type error.Remove the
cores_per_workerargument from the function call:return create_flux_allocation_executor( max_workers=max_workers, max_cores=max_cores, - cores_per_worker=cores_per_worker, resource_dict=resource_dict, flux_executor=flux_executor, flux_executor_pmi_mode=flux_executor_pmi_mode, flux_executor_nesting=flux_executor_nesting, flux_log_files=flux_log_files, block_allocation=block_allocation, init_function=init_function, )
🧹 Nitpick comments (1)
executorlib/interactive/create.py (1)
164-165: Add safety checks when updating resource dictionary.The code directly overwrites
cache_directoryandhostname_localhostinresource_dictwithout checking if they already exist.Add safety checks to prevent unintended overwrites:
- resource_dict["cache_directory"] = cache_directory - resource_dict["hostname_localhost"] = hostname_localhost + if cache_directory is not None: + resource_dict["cache_directory"] = cache_directory + if hostname_localhost is not None: + resource_dict["hostname_localhost"] = hostname_localhost
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
executorlib/interactive/create.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/interactive/create.py
152-152: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
209-209: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
249-249: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
🪛 GitHub Actions: MyPy
executorlib/interactive/create.py
[error] 102-102: Unexpected keyword argument 'cores_per_worker' for 'create_flux_allocation_executor'.
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: build (windows-latest, 3.13)
🔇 Additional comments (2)
executorlib/interactive/create.py (2)
214-217: LGTM: Clean implementation of resource handling in SLURM executor.The implementation correctly handles initialization of resource parameters.
254-262: LGTM: Appropriate validation in local executor.The implementation includes necessary checks for GPU resources and command line arguments, which are essential for local execution.
| max_workers: Optional[int] = None, | ||
| max_cores: Optional[int] = None, | ||
| cores_per_worker: int = 1, | ||
| cache_directory: Optional[str] = None, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix mutable default arguments in function signatures.
Using mutable default arguments (resource_dict = {}) can lead to unexpected behavior if the dictionary is modified between function calls, as the same dictionary instance is shared across all calls.
Replace the mutable defaults with None and initialize within the functions:
def create_flux_allocation_executor(
max_workers: Optional[int] = None,
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 = {}
def create_slurm_allocation_executor(
max_workers: Optional[int] = None,
max_cores: Optional[int] = None,
cache_directory: Optional[str] = None,
- resource_dict: dict = {},
+ resource_dict: Optional[dict] = None,
...
):
+ if resource_dict is None:
+ resource_dict = {}
def create_local_executor(
max_workers: Optional[int] = None,
max_cores: Optional[int] = None,
cache_directory: Optional[str] = None,
- resource_dict: dict = {},
+ resource_dict: Optional[dict] = None,
...
):
+ if resource_dict is None:
+ resource_dict = {}Also applies to: 208-208, 248-248
executorlib/interactive/create.py
Outdated
| cores_per_worker = resource_dict.get("cores", 1) | ||
| resource_dict["cache_directory"] = cache_directory | ||
| resource_dict["hostname_localhost"] = hostname_localhost |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Standardize resource dictionary initialization across backends.
The handling of cache_directory and hostname_localhost is inconsistent. In the flux backend, they're added to resource_dict, but in slurm and local backends, they're passed as separate parameters.
Standardize by moving these parameters into resource_dict for all backends:
elif backend == "slurm_allocation":
check_pmi(backend=backend, pmi=flux_executor_pmi_mode)
check_executor(executor=flux_executor)
check_nested_flux_executor(nested_flux_executor=flux_executor_nesting)
check_flux_log_files(flux_log_files=flux_log_files)
+ resource_dict["cache_directory"] = cache_directory
+ resource_dict["hostname_localhost"] = hostname_localhost
return create_slurm_allocation_executor(
max_workers=max_workers,
max_cores=max_cores,
- cache_directory=cache_directory,
resource_dict=resource_dict,
- hostname_localhost=hostname_localhost,
block_allocation=block_allocation,
init_function=init_function,
)Apply similar changes to the local backend block.
Also applies to: 122-124, 136-138
Summary by CodeRabbit