Refactor Interface #400
Refactor Interface #400
Conversation
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
WalkthroughThe changes involve significant updates to the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Executor
participant Spawner
User->>Executor: Create Executor(max_workers, flux_executor)
Executor->>Spawner: Initialize Spawner with parameters
Spawner-->>Executor: Return Spawner instance
Executor-->>User: Return Executor instance
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, codebase verification and nitpick comments (2)
executorlib/__init__.py (2)
Line range hint
43-107: Review ofExecutorclass constructorThe constructor of the
Executorclass has been significantly updated with new parameters to enhance configurability for different execution environments. The addition of parameters likeflux_executor,flux_executor_pmi_mode, andflux_executor_nestingare particularly notable for their role in integrating with the Flux framework.However, there are issues with mutable default arguments (
slurm_cmd_args) which can lead to unexpected behavior if modified. This should be addressed by usingNoneas a default and initializing within the function.Replace mutable default arguments with immutable ones to prevent potential bugs:
- slurm_cmd_args: list[str] = [] + slurm_cmd_args: Optional[list[str]] = NoneAnd then within the constructor:
if slurm_cmd_args is None: slurm_cmd_args = []Additionally, ensure that all new parameters are documented properly and that their integration does not disrupt existing functionality.
Line range hint
123-199: Review ofExecutorclass__new__methodThe
__new__method mirrors the constructor in terms of parameter additions and adjustments. It is crucial to apply the same fixes regarding mutable default arguments here as well.Apply the same fix for mutable default arguments as suggested for the constructor to ensure consistency and prevent bugs:
- slurm_cmd_args: list[str] = [] + slurm_cmd_args: Optional[list[str]] = NoneAnd initialize within the method:
if slurm_cmd_args is None: slurm_cmd_args = []This change ensures that the
__new__method remains robust and free from common pitfalls associated with mutable default arguments.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (15)
- executorlib/init.py (6 hunks)
- executorlib/interactive/init.py (3 hunks)
- executorlib/interactive/executor.py (4 hunks)
- executorlib/interactive/flux.py (4 hunks)
- executorlib/shared/communication.py (4 hunks)
- executorlib/shared/executor.py (9 hunks)
- executorlib/shared/spawner.py (11 hunks)
- notebooks/examples.ipynb (22 hunks)
- tests/test_dependencies_executor.py (1 hunks)
- tests/test_executor_backend_flux.py (5 hunks)
- tests/test_flux_executor.py (8 hunks)
- tests/test_local_executor.py (28 hunks)
- tests/test_local_executor_future.py (4 hunks)
- tests/test_shared_backend.py (3 hunks)
- tests/test_shared_communication.py (2 hunks)
Files skipped from review due to trivial changes (3)
- executorlib/interactive/executor.py
- executorlib/shared/communication.py
- executorlib/shared/executor.py
Additional context used
Ruff
executorlib/interactive/__init__.py
48-48: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
executorlib/shared/spawner.py
174-174: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
247-247: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
executorlib/__init__.py
104-104: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
130-130: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Additional comments not posted (45)
tests/test_executor_backend_flux.py (5)
49-49: Parameter Renaming ApprovedThe renaming of the parameter from
executortoflux_executorenhances clarity and aligns with the PR objectives.
64-64: Parameter Renaming ApprovedThe renaming of the parameter from
executortoflux_executorin this context also enhances clarity and aligns with the PR objectives.
79-82: Parameter Renaming ApprovedThe renaming of the parameters from
executortoflux_executorand frompmitoflux_executor_pmi_modeenhances clarity and aligns with the PR objectives.
92-95: Parameter Renaming ApprovedThe renaming of the parameters from
executortoflux_executorand frompmitoflux_executor_pmi_modein this context also enhances clarity and aligns with the PR objectives.
108-108: Parameter Renaming ApprovedThe renaming of the parameter from
executortoflux_executorenhances clarity and aligns with the PR objectives.tests/test_shared_communication.py (2)
35-35: Parameter Renaming ApprovedThe renaming of the parameters from
interfacetospawnerand fromoversubscribetoopenmpi_oversubscribeenhances clarity and aligns with the PR objectives.
63-63: Parameter Renaming ApprovedThe renaming of the parameters from
interfacetospawnerand fromoversubscribetoopenmpi_oversubscribein this context also enhances clarity and aligns with the PR objectives.tests/test_shared_backend.py (3)
25-25: Parameter Renaming ApprovedThe renaming of the parameter from
oversubscribetoopenmpi_oversubscribeenhances clarity and aligns with the PR objectives.
58-58: Parameter Renaming ApprovedThe renaming of the parameter from
oversubscribetoopenmpi_oversubscribeenhances clarity and aligns with the PR objectives.
101-102: Parameter Renaming and Introduction ApprovedThe renaming of the parameter from
oversubscribetoopenmpi_oversubscribeand the introduction ofslurm_cmd_argsenhance clarity and configurability, aligning with the PR objectives.tests/test_local_executor_future.py (4)
24-24: Parameter name change approved.The change from
interface_classtospawnerin theInteractiveExecutorconstructor is consistent with the PR objectives to improve configurability and usability.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Parameter change verified successfully.
The
InteractiveExecutoris consistently used with thespawnerparameter across the codebase, confirming the successful integration of the change. No instances of the oldinterface_classparameter were found.
- Verified in files like
tests/test_local_executor.py,tests/test_flux_executor.py, andtests/test_local_executor_future.py.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `InteractiveExecutor` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'InteractiveExecutor'Length of output: 74
Script:
#!/bin/bash # Description: Verify all function calls to `InteractiveExecutor` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'InteractiveExecutor'Length of output: 15080
40-40: Parameter name change approved.The change from
interface_classtospawnerin theInteractiveExecutorconstructor is consistent with the PR objectives to improve configurability and usability.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Parameter name change verified successfully.
The
InteractiveExecutoris consistently using thespawnerparameter across the codebase, confirming that the change frominterface_classhas been correctly implemented.
- Instances found in
tests/test_local_executor_future.pyandtests/test_local_executor.pyconfirm the update.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `InteractiveExecutor` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'InteractiveExecutor'Length of output: 74
Script:
#!/bin/bash # Description: Verify all function calls to `InteractiveExecutor` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'InteractiveExecutor'Length of output: 15080
112-112: Parameter name change approved.The change from
interface_classtospawnerin theInteractiveExecutorconstructor is consistent with the PR objectives to improve configurability and usability.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Parameter name change verified.
The change from
interface_classtospawnerin theInteractiveExecutorconstructor is consistently applied across the codebase. All instances reflect the new parameter name correctly.
- Verified in
tests/test_local_executor_future.py- Verified in
tests/test_flux_executor.py- Verified in
tests/test_local_executor.py- Verified in
executorlib/interactive/__init__.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `InteractiveExecutor` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'InteractiveExecutor'Length of output: 74
Script:
#!/bin/bash # Description: Verify all function calls to `InteractiveExecutor` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'InteractiveExecutor'Length of output: 15080
72-72: Parameter name change approved.The change from
interface_classtospawnerin theInteractiveExecutorconstructor is consistent with the PR objectives to improve configurability and usability.The code changes are approved.
Run the following script to verify the function usage:
Verification successful
Parameter name change verified.
The change from
interface_classtospawnerin theInteractiveExecutorconstructor has been consistently applied across the codebase. All instances reflect the new parameter name, confirming the successful implementation of the change.
- Verified in
tests/test_local_executor.py- Verified in
tests/test_local_executor_future.py- Verified in
tests/test_flux_executor.pyScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `InteractiveExecutor` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'InteractiveExecutor'Length of output: 74
Script:
#!/bin/bash # Description: Verify all function calls to `InteractiveExecutor` match the new signature. # Test: Search for the function usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'InteractiveExecutor'Length of output: 15080
tests/test_dependencies_executor.py (1)
106-106: Parameter Change Approved:openmpi_oversubscribeThe replacement of
oversubscribewithopenmpi_oversubscribein thetest_dependency_stepsfunction is consistent with the PR's objectives to enhance configurability for OpenMPI settings. Ensure that this change is reflected in all relevant parts of the test suite and that it does not negatively impact the existing functionality.Verification successful
Parameter Change Verified:
openmpi_oversubscribeThe
openmpi_oversubscribeparameter is consistently used across multiple test files and modules, confirming its integration into the codebase. This change aligns with the PR's objectives and does not negatively impact existing functionality.
- Files with usage:
tests/test_shared_communication.pytests/test_local_executor.pytests/test_flux_executor.pytests/test_dependencies_executor.pytests/test_shared_backend.pyexecutorlibmoduleScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of `openmpi_oversubscribe` in the test suite. # Test: Search for the parameter usage. Expect: Only occurrences of the new parameter. rg --type python -A 5 $'openmpi_oversubscribe'Length of output: 76
Script:
#!/bin/bash # Description: Verify the usage of `openmpi_oversubscribe` in the test suite. # Test: Search for the parameter usage. Expect: Only occurrences of the new parameter. rg --type py -A 5 $'openmpi_oversubscribe'Length of output: 20454
executorlib/interactive/flux.py (3)
18-21: Parameter Renaming inFluxPythonSpawnerConstructorThe renaming of parameters to include
flux_executorprefixes enhances clarity and specificity. This change is consistent with the PR objectives to improve configurability and usability.Also applies to: 30-33
38-44: Internal Variable Assignments inFluxPythonSpawnerThe internal variable assignments correctly reflect the new parameter names, ensuring consistency across the class.
63-73: Logic Adjustment inbootupMethodThe
bootupmethod has been updated to use the new variable names. The logic correctly handles scenarios based on the new parameter settings, such asflux_executor_nestingandflux_executor_pmi_mode.Also applies to: 92-93
tests/test_flux_executor.py (1)
47-47: Variable Renaming in Test MethodsThe renaming of the variable from
executortoflux_executoracross various test methods is consistent and aligns with the changes in the main class. This ensures that the test cases reflect the updated API and maintain their validity.Also applies to: 52-53, 65-69, 81-85, 95-100, 117-118, 133-134, 143-147, 157-164
executorlib/shared/spawner.py (2)
10-21: Parameter Renaming inBaseSpawnerConstructorThe renaming of the parameter to
openmpi_oversubscribeis consistent and enhances clarity regarding its specific use with OpenMPI. This change aligns with the PR objectives to improve semantic clarity.
Line range hint
63-76: Parameter Renaming inSubprocessSpawnerConstructorThe renaming of the parameter to
openmpi_oversubscribeis consistent and enhances clarity regarding its specific use with OpenMPI. This change aligns with the PR objectives to improve semantic clarity.tests/test_local_executor.py (18)
67-67: Parameter Renaming Approved:spawnerintest_pympiexecutor_two_workers.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
81-81: Parameter Renaming Approved:spawnerintest_pympiexecutor_one_worker.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
97-97: Parameter Renaming Approved:spawnerintest_pympiexecutor_two_workersofTestPyMpiExecutorStepSerial.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
111-111: Parameter Renaming Approved:spawnerintest_pympiexecutor_one_workerofTestPyMpiExecutorStepSerial.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
130-130: Parameter Renaming Approved:spawnerintest_pympiexecutor_one_worker_with_mpi.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
141-141: Parameter Renaming Approved:spawnerintest_pympiexecutor_one_worker_with_mpi_multiple_submissions.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
161-161: Parameter Renaming Approved:spawnerintest_pympiexecutor_one_worker_with_mpi_echo.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
176-176: Parameter Renaming Approved:spawnerintest_pympiexecutor_one_worker_with_mpiofTestPyMpiStepExecutorMPI.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
187-187: Parameter Renaming Approved:spawnerintest_pympiexecutor_one_worker_with_mpi_multiple_submissionsofTestPyMpiStepExecutorMPI.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
207-207: Parameter Renaming Approved:spawnerintest_pympiexecutor_one_worker_with_mpi_echoofTestPyMpiStepExecutorMPI.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
223-223: Parameter Renaming Approved:spawnerintest_internal_memory.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
248-248: Parameter Renaming Approved:openmpi_oversubscribeintest_execute_task.The renaming from
oversubscribetoopenmpi_oversubscribeenhances clarity and specificity, aligning with the intended use of the parameter within the context of OpenMPI.
262-262: Parameter Renaming Approved:spawnerintest_pool_serial.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
277-277: Parameter Renaming Approved:spawnerintest_executor_multi_submission.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
290-290: Parameter Renaming Approved:spawnerintest_shutdown.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
306-306: Parameter Renaming Approved:spawnerintest_pool_serial_map.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
316-316: Parameter Renaming Approved:spawnerintest_executor_exception.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.
325-325: Parameter Renaming Approved:spawnerintest_executor_exception_future.The renaming from
interface_classtospawnerenhances clarity and aligns with the intended use of theMpiExecSpawnerwithin the executor's context.notebooks/examples.ipynb (6)
Line range hint
95-123: Consistent Parameter Usage and Clear ExampleThe use of the
flux_executorparameter is consistent with the PR's objectives. The example clearly demonstrates submitting multiple tasks in parallel, which is a practical use case for theExecutorclass.The code changes are approved.
Line range hint
153-170: Effective Demonstration ofmapFunctionThis cell effectively demonstrates the use of the
mapfunction with theExecutorclass, using the renamedflux_executorparameter. The example is clear and practical.The code changes are approved.
Line range hint
196-257: Comprehensive Resource Management ExampleThis cell provides a detailed example of resource management at both the executor and submission levels. The use of the
flux_executorparameter is consistent and enhances clarity. Consider adding comments within the code to explain the resource settings more clearly to users unfamiliar with the configuration details.The code changes are approved.
Line range hint
273-299: Efficient Resource Management with Block AllocationThis cell demonstrates efficient resource management using block allocation, which is crucial for performance optimization in parallel execution environments. The consistent use of the
flux_executorparameter aligns with the PR's objectives.The code changes are approved.
Line range hint
327-354: Advanced Use Case: Handling Dependent TasksThis cell demonstrates an advanced use case of handling dependent tasks using the
Executorclass. The consistent use of theflux_executorparameter enhances clarity and usability in complex scenarios.The code changes are approved.
64-77: Parameter Renaming: Verify Across Codebase and DocumentationThe renaming of the
executorparameter toflux_executorenhances clarity and aligns with the PR objectives. Ensure that this change is consistently applied across the entire codebase and reflected in the documentation.The code changes are approved.
Run the following script to verify the parameter usage:
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- notebooks/examples.ipynb (22 hunks)
Files skipped from review as they are similar to previous changes (1)
- notebooks/examples.ipynb
There was a problem hiding this comment.
Actionable comments posted: 2
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- executorlib/interactive/init.py (2 hunks)
- executorlib/interactive/flux.py (3 hunks)
- executorlib/shared/communication.py (6 hunks)
- executorlib/shared/executor.py (9 hunks)
- executorlib/shared/spawner.py (12 hunks)
- executorlib/shell/executor.py (4 hunks)
- tests/test_flux_executor.py (8 hunks)
Files skipped from review due to trivial changes (3)
- executorlib/shared/communication.py
- executorlib/shared/executor.py
- tests/test_flux_executor.py
Additional context used
Ruff
executorlib/interactive/__init__.py
48-48: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
executorlib/shared/spawner.py
174-174: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
247-247: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
Additional comments not posted (7)
executorlib/shell/executor.py (2)
99-100: Correct usage of renamed parameters inSubprocessExecutor.The updated parameter names are correctly passed to the
execute_single_taskfunction within theSubprocessExecutorclass. This ensures consistency and correct functionality.The code changes are approved.
12-13: Parameter renaming and documentation update approved.The renaming of
prefix_nameandprefix_pathtoconda_environment_nameandconda_environment_pathenhances clarity. The updates in the docstring and the internal logic are consistent with these changes.The code changes are approved.
Run the following script to verify the usage of the new parameter names across the codebase:
Also applies to: 20-21, 33-50
executorlib/interactive/flux.py (1)
18-21: Parameter renaming and documentation update approved inFluxPythonSpawner.The renaming of parameters to include the
flux_executorprefix enhances clarity and explicitly links them to the FluxExecutor's functionality. The updates in the docstring and the internal logic are consistent with these changes.The code changes are approved.
Run the following script to verify the usage of the new parameter names across the codebase:
Also applies to: 30-44, 50-59, 63-67, 71-93
Verification successful
Parameter renaming verified across the codebase.
The new parameter names for
FluxPythonSpawnerhave been consistently applied and are being tested in multiple test files. This confirms that the renaming is integrated throughout the codebase.
- Instances of
FluxPythonSpawnerwith new parameters found intests/test_flux_executor.py.- Usage also confirmed in
executorlib/interactive/__init__.py.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all class instantiations and method calls of `FluxPythonSpawner` match the new signature. # Test: Search for the class usage. Expect: Only occurrences of the new signature. rg --type python -A 5 $'FluxPythonSpawner'Length of output: 72
Script:
#!/bin/bash # Description: Verify all class instantiations and method calls of `FluxPythonSpawner` match the new signature. # Test: Search for the class usage. Expect: Only occurrences of the new signature. rg --type py -A 5 $'FluxPythonSpawner'Length of output: 6222
executorlib/shared/spawner.py (4)
10-21: Parameter Renaming in BaseSpawner ConstructorThe renaming of the parameter from
oversubscribetoopenmpi_oversubscribein theBaseSpawnerconstructor is clear and improves semantic clarity, aligning with the specific use case of OpenMPI oversubscription.The code changes are approved.
26-35: Addition of Conda Environment Parameters in BaseSpawnerThe addition of
conda_environment_nameandconda_environment_pathparameters in thebootupmethod ofBaseSpawneris a good practice for flexibility in specifying environment configurations. However, ensure that these parameters are properly documented and validated.The code changes are approved.
Line range hint
63-76: Parameter Renaming in SubprocessSpawner ConstructorThe renaming of the parameter from
oversubscribetoopenmpi_oversubscribein theSubprocessSpawnerconstructor is consistent with the changes inBaseSpawner. This maintains uniformity across the class hierarchy.The code changes are approved.
Line range hint
83-108: Handling of Conda Environments in SubprocessSpawnerThe implementation of conditional logic to handle conda environments based on the presence of
conda_environment_nameandconda_environment_pathis robust. It ensures that the subprocess is appropriately configured with the specified environment.The code changes are approved.
| backend: str = "auto", | ||
| max_cores: int = 1, | ||
| cores_per_worker: int = 1, | ||
| threads_per_core: int = 1, | ||
| gpus_per_worker: int = 0, | ||
| oversubscribe: bool = False, | ||
| cwd: Optional[str] = None, | ||
| openmpi_oversubscribe: bool = False, | ||
| slurm_cmd_args: list[str] = [], | ||
| flux_executor=None, | ||
| flux_executor_pmi_mode: Optional[str] = None, | ||
| flux_executor_nesting: bool = False, |
There was a problem hiding this comment.
Enhancements and Fixes for create_executor Function
The addition of new parameters (backend, openmpi_oversubscribe, slurm_cmd_args, flux_executor, flux_executor_pmi_mode, flux_executor_nesting) significantly enhances the configurability and flexibility of the create_executor function. The replacement of oversubscribe with openmpi_oversubscribe is well-aligned with the PR's objectives.
However, the use of a mutable default argument (slurm_cmd_args) is problematic and should be fixed to avoid potential bugs.
Apply this diff to fix the mutable default issue:
-def create_executor(max_workers: int = 1, backend: str = "auto", openmpi_oversubscribe: bool = False, slurm_cmd_args: list[str] = [], flux_executor=None, flux_executor_pmi_mode: Optional[str] = None, flux_executor_nesting: bool = False, ...):
+def create_executor(max_workers: int = 1, backend: str = "auto", openmpi_oversubscribe: bool = False, slurm_cmd_args: Optional[list[str]] = None, flux_executor=None, flux_executor_pmi_mode: Optional[str] = None, flux_executor_nesting: bool = False, ...):
+ if slurm_cmd_args is None:
+ slurm_cmd_args = []Also applies to: 71-168
Tools
Ruff
48-48: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
| command_prepend_lst = generate_mpiexec_command( | ||
| cores=self._cores, | ||
| oversubscribe=self._oversubscribe, | ||
| openmpi_oversubscribe=self._openmpi_oversubscribe, |
There was a problem hiding this comment.
Consistent Use of Renamed Parameter and Mutable Default Arguments
The consistent use of the renamed parameter openmpi_oversubscribe across different spawners and command generation functions is good. However, the use of mutable default arguments for slurm_cmd_args should be addressed to avoid potential bugs.
Replace mutable default arguments with None and initialize them within the functions:
-def generate_slurm_command(cwd: str, threads_per_core: int = 1, gpus_per_core: int = 0, openmpi_oversubscribe: bool = False, slurm_cmd_args: list[str] = []):
+def generate_slurm_command(cwd: str, threads_per_core: int = 1, gpus_per_core: int = 0, openmpi_oversubscribe: bool = False, slurm_cmd_args: Optional[list[str]] = None):
+ if slurm_cmd_args is None:
+ slurm_cmd_args = []Also applies to other instances where mutable default arguments are used.
Also applies to: 173-194, 211-212, 219-273
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- notebooks/examples.ipynb (22 hunks)
Files skipped from review as they are similar to previous changes (1)
- notebooks/examples.ipynb
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests