[major] Refactor the Executor interface #548
Conversation
Replace the backend parameter from the initialization of the Executor class with individual executor classes for the individual backends: * `Executor(backend="local")` becomes `from executorlib import LocalExecutor§ * `Executor(backend="flux_allocation")` becomes `from executorlib import FluxAllocationExecutor§ * `Executor(backend="flux_submission")` becomes `from executorlib import FluxSubmissionExecutor§ * `Executor(backend="slurm_allocation")` becomes "SlurmAllocationExecutor" * `Executor(backend="slurm_submission")` becomes `from executorlib import SlurmSubmissionExecutor` This has two advantages: On the one hand it is less error prone to mistyping the backend name, as the user can use auto completion to import the right module. On the other hand it is more consistent with the standard library which defines the `ProcessPoolExecutor` and the `ThreadPoolExecutor`, rather than a `backend` parameter.
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
for more information, see https://pre-commit.ci
WalkthroughThis pull request removes the generic Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ExecutorFactory
participant Local as SingleNodeExecutor
participant Flux as FluxExecutor
participant Slurm as SlurmExecutor
User->>ExecutorFactory: Request new executor (specify backend)
alt Backend is local
ExecutorFactory->>Local: Instantiate SingleNodeExecutor
Local-->>User: Return local executor instance
else Backend is flux_submission/allocation
ExecutorFactory->>Flux: Instantiate FluxSubmission/AllocationExecutor
Flux-->>User: Return Flux executor instance
else Backend is slurm_submission/allocation
ExecutorFactory->>Slurm: Instantiate SlurmSubmission/AllocationExecutor
Slurm-->>User: Return Slurm executor instance
end
Possibly related PRs
Poem
✨ Finishing Touches
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: 7
🧹 Nitpick comments (11)
executorlib/__init__.py (1)
2-7: Add newly introduced executor classes to__all__or remove them if they are not intended to be publicly accessible.The static analysis tool flags these imports as unused. If these executor classes are meant to be part of the public interface, consider adding them to
__all__. Otherwise, remove the imports to avoid misleading consumers and to pass lint checks.Example fix:
__version__ = _get_versions()["version"] -__all__: list = [] +__all__: list = [ + "FluxAllocationExecutor", + "FluxSubmissionExecutor", + "LocalExecutor", + "SlurmAllocationExecutor", + "SlurmSubmissionExecutor", +]🧰 Tools
🪛 Ruff (0.8.2)
3-3:
executorlib.interfaces.FluxAllocationExecutorimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
4-4:
executorlib.interfaces.FluxSubmissionExecutorimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
5-5:
executorlib.interfaces.LocalExecutorimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
6-6:
executorlib.interfaces.SlurmAllocationExecutorimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
7-7:
executorlib.interfaces.SlurmSubmissionExecutorimported but unused; consider removing, adding to__all__, or using a redundant alias(F401)
executorlib/interfaces.py (6)
24-28: Revise docstring references to the removedexecutorlib.Executor.These lines mention the old
Executorclass. SinceLocalExecutoris now distinct, replace references toexecutorlib.ExecutorwithLocalExecutoror more general wording.- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or - preferable the flux framework for distributing python functions ... - ... it is even possible to execute the executorlib.Executor directly in an interactive Jupyter notebook. + The LocalExecutor manages the parallel distribution of Python functions using local resources + while preserving compatibility with Jupyter notebooks and interactive workflows.
206-214: Remove or reword general references to the oldexecutorlib.Executor.The docstring for
SlurmSubmissionExecutorstill describes the olderExecutorapproach in its overview. Update it to reflect this specialized class.- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or - preferable the flux framework ... + The SlurmSubmissionExecutor leverages the SLURM workload manager ...
416-422: Eliminate outdatedexecutorlib.Executorreferences in the docstring.Use a more accurate, class-specific description for
SlurmAllocationExecutor, avoiding confusion with the deprecated genericExecutorclass.- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or - preferable the flux framework ... + The SlurmAllocationExecutor is specialized for HPC usage with SLURM allocations ...
600-606: Update docstring to reflect the newFluxAllocationExecutorclass name.Avoid referencing
executorlib.Executorgenerally; highlight how this new class specifically leverages the Flux framework.- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or - preferable the flux framework ... + The FluxAllocationExecutor leverages the Flux framework to distribute Python functions ...
800-806: Remove legacy references toexecutorlib.Executorin theFluxSubmissionExecutordocstring.Document how this specialized executor interacts with the Flux job submission mechanism.
- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or - preferable the flux framework ... + The FluxSubmissionExecutor leverages the Flux framework to submit Python functions as Flux jobs ...
657-661: Fix usage example to reflectFluxAllocationExecutor.Replace the generic
Executorusage withFluxAllocationExecutorfor consistency.->>> with Executor(cores=2, init_function=init_k) as p: +>>> with FluxAllocationExecutor(cores=2, init_function=init_k) as p: fs = p.submit(calc, 2, j=4) print(fs.result())tests/test_executor_backend_mpi_noblock.py (1)
59-63: Remove unused variable in error test.The variable
fs_1is assigned but never used in the test case.- LocalExecutor( + _ = LocalExecutor( max_cores=1, resource_dict={ "cores": 1, "gpus_per_core": 1, }, )tests/test_executor_backend_mpi.py (1)
58-61: Remove unused variable in oversubscribe test.The variable
fs_1is assigned but never used in the test case.with LocalExecutor(max_cores=1, block_allocation=True) as exe: cloudpickle_register(ind=1) - fs_1 = exe.submit(calc, 1, resource_dict={"cores": 2}) + _ = exe.submit(calc, 1, resource_dict={"cores": 2})🧰 Tools
🪛 Ruff (0.8.2)
60-60: Local variable
fs_1is assigned to but never usedRemove assignment to unused variable
fs_1(F841)
tests/test_dependencies_executor.py (1)
236-243: Simplify nestedwithstatements.The static analysis tool suggests using a single
withstatement with multiple contexts instead of nestedwithstatements.Apply this diff to simplify the code:
- with LocalExecutor(max_cores=1, block_allocation=False) as exe: - cloudpickle_register(ind=1) + with LocalExecutor(max_cores=1, block_allocation=False) as exe, \ + cloudpickle_register(ind=1): _ = exe.submit(raise_error) - with LocalExecutor(max_cores=1, block_allocation=True) as exe: - cloudpickle_register(ind=1) + with LocalExecutor(max_cores=1, block_allocation=True) as exe, \ + cloudpickle_register(ind=1): _ = exe.submit(raise_error) - with LocalExecutor(max_cores=2, block_allocation=False) as exe: - cloudpickle_register(ind=1) + with LocalExecutor(max_cores=2, block_allocation=False) as exe, \ + cloudpickle_register(ind=1): _ = exe.submit(raise_error) - with LocalExecutor(max_cores=2, block_allocation=True) as exe: - cloudpickle_register(ind=1) + with LocalExecutor(max_cores=2, block_allocation=True) as exe, \ + cloudpickle_register(ind=1): _ = exe.submit(raise_error)Also applies to: 247-249, 253-255
🧰 Tools
🪛 Ruff (0.8.2)
241-242: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
notebooks/3-hpc-allocation.ipynb (1)
380-391: Improve ANSI color code usageThe ANSI color codes are using lowercase escape sequences which could be standardized to uppercase for better readability.
Replace lowercase
\u001bwith uppercase\u001B:- \u001b[01;32m ƒDqBpVYK jan python CD 1 1 0.695s fedora\n + \u001B[01;32m ƒDqBpVYK jan python CD 1 1 0.695s fedora\n ...
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
executorlib/__init__.py(1 hunks)executorlib/interfaces.py(1 hunks)notebooks/1-local.ipynb(15 hunks)notebooks/2-hpc-submission.ipynb(6 hunks)notebooks/3-hpc-allocation.ipynb(8 hunks)notebooks/4-developer.ipynb(3 hunks)tests/test_cache_executor_interactive.py(2 hunks)tests/test_cache_executor_pysqa_flux.py(2 hunks)tests/test_dependencies_executor.py(7 hunks)tests/test_executor_backend_flux.py(9 hunks)tests/test_executor_backend_mpi.py(8 hunks)tests/test_executor_backend_mpi_noblock.py(5 hunks)tests/test_integration_pyiron_workflow.py(7 hunks)tests/test_shell_executor.py(5 hunks)tests/test_shell_interactive.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/__init__.py
3-3: executorlib.interfaces.FluxAllocationExecutor imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
4-4: executorlib.interfaces.FluxSubmissionExecutor imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
5-5: executorlib.interfaces.LocalExecutor imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
6-6: executorlib.interfaces.SlurmAllocationExecutor imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
7-7: executorlib.interfaces.SlurmSubmissionExecutor imported but unused; consider removing, adding to __all__, or using a redundant alias
(F401)
tests/test_executor_backend_mpi.py
60-60: Local variable fs_1 is assigned to but never used
Remove assignment to unused variable fs_1
(F841)
tests/test_executor_backend_mpi_noblock.py
66-70: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
tests/test_dependencies_executor.py
241-242: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
247-248: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
253-254: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms (4)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: mypy
🔇 Additional comments (10)
tests/test_cache_executor_interactive.py (2)
5-5: Adoption ofLocalExecutorlooks good!Importing
LocalExecutordirectly replaces the oldExecutor(backend="local"). This aligns with the new design.
21-21: Validate concurrency if needed.You are using
LocalExecutorwith concurrent submissions. If your test suite also covers concurrency stress tests or varied CPU usage scenarios, that helps confirm correctness under parallel loads. Otherwise, no concerns here.Would you like a script to scan for concurrency test coverage in other test files?
tests/test_cache_executor_pysqa_flux.py (1)
6-6: LGTM! The changes align with the PR objectives.The transition from
ExecutortoFluxSubmissionExecutoris correctly implemented, maintaining the same functionality while providing better type safety and auto-completion support.Also applies to: 35-38
tests/test_executor_backend_mpi_noblock.py (1)
3-3: LGTM! The changes align with the PR objectives.The transition from
ExecutortoLocalExecutoris correctly implemented across all test methods, maintaining the same functionality while providing better type safety and auto-completion support.Also applies to: 17-20, 31-34, 45-48, 59-63, 67-70, 73-76
tests/test_shell_interactive.py (1)
7-7: LGTM! The changes align with the PR objectives.The transition from
ExecutortoLocalExecutoris correctly implemented, maintaining the same functionality while providing better type safety and auto-completion support.Also applies to: 107-110
tests/test_executor_backend_mpi.py (1)
7-7: LGTM! The changes align with the PR objectives.The transition from
ExecutortoLocalExecutoris correctly implemented across all test methods, maintaining the same functionality while providing better type safety and auto-completion support.Also applies to: 37-37, 47-47, 58-58, 66-70, 78-81, 92-97, 117-121, 134-138
tests/test_executor_backend_flux.py (1)
6-6: LGTM! The changes align with the PR objective.The transition from
ExecutortoFluxAllocationExecutoris consistent across all test cases, maintaining the original test functionality while using the specialized executor for Flux operations.Also applies to: 47-51, 60-65, 74-81, 86-92, 104-109, 124-129, 142-147, 156-160
tests/test_shell_executor.py (1)
6-6: LGTM! The changes align with the PR objective.The transition from
ExecutortoLocalExecutoris consistent across all test cases, maintaining the original test functionality while using the specialized executor for local operations.Also applies to: 86-87, 99-100, 112-113, 122-123
tests/test_integration_pyiron_workflow.py (1)
15-15: LGTM! The changes align with the PR objective.The transition from
ExecutortoLocalExecutoris consistent across all test cases, maintaining the original test functionality while using the specialized executor for local operations.Also applies to: 78-79, 108-109, 139-140, 163-164, 193-194, 222-223
tests/test_dependencies_executor.py (1)
7-7: LGTM! The changes align with the PR objective.The transition from
ExecutortoLocalExecutoris consistent across all test cases, maintaining the original test functionality while using the specialized executor for local operations.Also applies to: 49-51, 60-64, 86-90, 159-160, 191-195
executorlib/interfaces.py
Outdated
| >>> import numpy as np | ||
| >>> from executorlib import Executor | ||
| >>> | ||
| >>> def calc(i, j, k): | ||
| >>> from mpi4py import MPI | ||
| >>> size = MPI.COMM_WORLD.Get_size() | ||
| >>> rank = MPI.COMM_WORLD.Get_rank() | ||
| >>> return np.array([i, j, k]), size, rank | ||
| >>> | ||
| >>> def init_k(): | ||
| >>> return {"k": 3} | ||
| >>> | ||
| >>> with Executor(cores=2, init_function=init_k) as p: | ||
| >>> fs = p.submit(calc, 2, j=4) | ||
| >>> print(fs.result()) | ||
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] | ||
| ``` | ||
| """ |
There was a problem hiding this comment.
Correct the old example to use SlurmAllocationExecutor.
This snippet erroneously shows from executorlib import Executor. Update it to match the specialized class.
->>> import numpy as np
->>> from executorlib import Executor
+>>> import numpy as np
+>>> from executorlib.interfaces import SlurmAllocationExecutor
...
->>> with Executor(cores=2, init_function=init_k) as p:
+>>> with SlurmAllocationExecutor(cores=2, init_function=init_k) as p:
fs = p.submit(calc, 2, j=4)
print(fs.result())📝 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.
| >>> import numpy as np | |
| >>> from executorlib import Executor | |
| >>> | |
| >>> def calc(i, j, k): | |
| >>> from mpi4py import MPI | |
| >>> size = MPI.COMM_WORLD.Get_size() | |
| >>> rank = MPI.COMM_WORLD.Get_rank() | |
| >>> return np.array([i, j, k]), size, rank | |
| >>> | |
| >>> def init_k(): | |
| >>> return {"k": 3} | |
| >>> | |
| >>> with Executor(cores=2, init_function=init_k) as p: | |
| >>> fs = p.submit(calc, 2, j=4) | |
| >>> print(fs.result()) | |
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] | |
| ``` | |
| """ | |
| >>> import numpy as np | |
| >>> from executorlib.interfaces import SlurmAllocationExecutor | |
| >>> | |
| >>> def calc(i, j, k): | |
| >>> from mpi4py import MPI | |
| >>> size = MPI.COMM_WORLD.Get_size() | |
| >>> rank = MPI.COMM_WORLD.Get_rank() | |
| >>> return np.array([i, j, k]), size, rank | |
| >>> | |
| >>> def init_k(): | |
| >>> return {"k": 3} | |
| >>> | |
| >>> with SlurmAllocationExecutor(cores=2, init_function=init_k) as p: | |
| >>> fs = p.submit(calc, 2, j=4) | |
| >>> print(fs.result()) | |
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] |
executorlib/interfaces.py
Outdated
| >>> with Executor(cores=2, init_function=init_k) as p: | ||
| >>> fs = p.submit(calc, 2, j=4) | ||
| >>> print(fs.result()) | ||
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] | ||
| ``` |
There was a problem hiding this comment.
Use FluxSubmissionExecutor in the example instead of Executor.
This example should highlight the new specialized class for Flux-based submissions.
->>> with Executor(cores=2, init_function=init_k) as p:
+>>> with FluxSubmissionExecutor(cores=2, init_function=init_k) as p:
fs = p.submit(calc, 2, j=4)
print(fs.result())📝 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.
| >>> with Executor(cores=2, init_function=init_k) as p: | |
| >>> fs = p.submit(calc, 2, j=4) | |
| >>> print(fs.result()) | |
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] | |
| ``` | |
| >>> with FluxSubmissionExecutor(cores=2, init_function=init_k) as p: | |
| >>> fs = p.submit(calc, 2, j=4) | |
| >>> print(fs.result()) | |
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] |
| @@ -46,7 +46,7 @@ | |||
| "metadata": {}, | |||
| "source": [ | |||
| "```python\n", | |||
| "with Executor(backend=\"slurm_submission\", cache_directory=\"./cache\") as exe:\n", | |||
| "with SlurmSubmissionExecutor(cache_directory=\"./cache\") as exe:\n", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update code example documentation
The markdown text above still refers to specifying backend="slurm_submission", but this parameter is no longer needed with the new executor classes.
Update the markdown text to reflect the new usage:
-In comparison to the [Local Mode](https://executorlib.readthedocs.io/en/latest/1-local.html), the only two parameters which are changed are the specification of the backend as `backend="slurm_submission"` and the requirement to specify the cache directory using the `cache_directory="./cache"`. The rest of the syntax remains exactly the same, to simplify the up-scaling of simulation workflows.
+In comparison to the [Local Mode](https://executorlib.readthedocs.io/en/latest/1-local.html), the only parameter required is the cache directory using `cache_directory="./cache"`. The rest of the syntax remains exactly the same, to simplify the up-scaling of simulation workflows.📝 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.
| "with SlurmSubmissionExecutor(cache_directory=\"./cache\") as exe:\n", | |
| -In comparison to the [Local Mode](https://executorlib.readthedocs.io/en/latest/1-local.html), the only two parameters which are changed are the specification of the backend as `backend="slurm_submission"` and the requirement to specify the cache directory using the `cache_directory="./cache"`. The rest of the syntax remains exactly the same, to simplify the up-scaling of simulation workflows. | |
| +In comparison to the [Local Mode](https://executorlib.readthedocs.io/en/latest/1-local.html), the only parameter required is the cache directory using `cache_directory="./cache"`. The rest of the syntax remains exactly the same, to simplify the up-scaling of simulation workflows. |
| @@ -28,7 +28,7 @@ | |||
| "metadata": {}, | |||
| "source": [ | |||
| "```python\n", | |||
| "from executorlib import Executor\n", | |||
| "from executorlib import SlurmSubmissionExecutor\n", | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update import statement documentation
The markdown text above still refers to importing the Executor class, but the code shows importing SlurmSubmissionExecutor.
Update the markdown text to reflect the new class name:
-The [Simple Linux Utility for Resource Management (SLURM)](https://slurm.schedmd.com) job scheduler is currently the most commonly used job scheduler for HPC clusters. In the HPC submission mode executorlib internally uses the [sbatch](https://slurm.schedmd.com/sbatch.html) command this is in contrast to the [HPC allocatiom mode] which internally uses the [srun](https://slurm.schedmd.com/srun.html) command.
+The [Simple Linux Utility for Resource Management (SLURM)](https://slurm.schedmd.com) job scheduler is currently the most commonly used job scheduler for HPC clusters. The `SlurmSubmissionExecutor` internally uses the [sbatch](https://slurm.schedmd.com/sbatch.html) command this is in contrast to the [HPC allocation mode] which internally uses the [srun](https://slurm.schedmd.com/srun.html) command.📝 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.
| "from executorlib import SlurmSubmissionExecutor\n", | |
| The [Simple Linux Utility for Resource Management (SLURM)](https://slurm.schedmd.com) job scheduler is currently the most commonly used job scheduler for HPC clusters. The `SlurmSubmissionExecutor` internally uses the [sbatch](https://slurm.schedmd.com/sbatch.html) command this is in contrast to the [HPC allocation mode] which internally uses the [srun](https://slurm.schedmd.com/srun.html) command. |
notebooks/4-developer.ipynb
Outdated
| "source": [ | ||
| "from executorlib import Executor" | ||
| ] | ||
| "source": "from executorlib import LocalExecutor" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update import statement documentation
The markdown text in the notebook still refers to importing the Executor class, but the code shows importing LocalExecutor.
Update the markdown text to reflect the new class name:
-The general functionality of executorlib follows the [Executor interface](https://docs.python.org/3/library/concurrent.futures.html#concurrent.futures.Executor) of the Python standard library. You can import the `Executor` class directly from executorlib and then just replace the [ProcessPoolExecutor](https://docs.python.org/3/library/concurrent.futures.html#processpoolexecutor) or [ThreadPoolExecutor](https://docs.python.org/3/library/concurrent.futures.html#threadpoolexecutor) with the `Executor` class to start using executorlib.
+The general functionality of executorlib follows the [Executor interface](https://docs.python.org/3/library/concurrent.futures.html#concurrent.futures.Executor) of the Python standard library. You can import specialized executor classes like `LocalExecutor` directly from executorlib and then just replace the [ProcessPoolExecutor](https://docs.python.org/3/library/concurrent.futures.html#processpoolexecutor) or [ThreadPoolExecutor](https://docs.python.org/3/library/concurrent.futures.html#threadpoolexecutor) with the appropriate executor class to start using executorlib.Committable suggestion skipped: line range outside the PR's diff.
notebooks/3-hpc-allocation.ipynb
Outdated
| "source": [ | ||
| "from executorlib import Executor" | ||
| ] | ||
| "source": "from executorlib import SlurmAllocationExecutor" |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update import statement documentation
The markdown text above still refers to setting the backend parameter, but this is no longer needed with the new executor classes.
Update the markdown text to reflect the new usage:
-The only parameter the user has to change is the `backend` parameter.
+The user simply needs to import and use the appropriate executor class for their needs (e.g., `SlurmAllocationExecutor` or `FluxAllocationExecutor`).Committable suggestion skipped: line range outside the PR's diff.
notebooks/1-local.ipynb
Outdated
| @@ -27,7 +27,7 @@ | |||
| "metadata": {}, | |||
| "outputs": [], | |||
| "source": [ | |||
| "from executorlib import Executor" | |||
| "from executorlib import LocalExecutor" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update import statement documentation
The markdown text above still refers to setting the backend parameter to "local", but this is no longer needed with LocalExecutor.
Update the markdown text to reflect the new usage:
-The local mode in executorlib, which is selected by setting the `backend` parameter to `"local"`, is primarily used to enable rapid prototyping on a workstation computer to test your parallel Python program with executorlib before transferring it to an high performance computer (HPC). When the `backend` parameter is not set, it defaults to `"local"`.
+The `LocalExecutor` in executorlib is primarily used to enable rapid prototyping on a workstation computer to test your parallel Python program before transferring it to a high performance computer (HPC).Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_executor_backend_mpi.py (1)
58-60: Remove unused variable assignment.The
fs_1variable is assigned but never used in the test.Apply this diff to fix the issue:
- fs_1 = exe.submit(calc, 1, resource_dict={"cores": 2}) + exe.submit(calc, 1, resource_dict={"cores": 2})🧰 Tools
🪛 Ruff (0.8.2)
60-60: Local variable
fs_1is assigned to but never usedRemove assignment to unused variable
fs_1(F841)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_executor_backend_mpi.py(8 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_executor_backend_mpi.py
60-60: Local variable fs_1 is assigned to but never used
Remove assignment to unused variable fs_1
(F841)
⏰ Context from checks skipped due to timeout of 90000ms (13)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
🔇 Additional comments (5)
tests/test_executor_backend_mpi.py (5)
7-7: LGTM! Import changes align with PR objectives.The direct import of specialized executor classes improves IDE auto-completion support and reduces the likelihood of mistyping backend names.
37-37: LGTM! Consistent replacement of generic Executor with LocalExecutor.The changes align with the PR objectives by replacing backend parameters with specialized executor classes. Each test method now uses
LocalExecutorwith appropriate configuration parameters.Also applies to: 47-47, 58-58, 66-69, 78-81
92-97: LGTM! Cache configuration with LocalExecutor.The changes maintain the test's focus on caching behavior while using the specialized
LocalExecutorclass.
117-121: LGTM! Working directory configuration with LocalExecutor.The changes maintain the test's focus on working directory behavior while using the specialized
LocalExecutorclass.
134-138: LGTM! SLURM-specific configuration with SlurmAllocationExecutor.The changes appropriately use the specialized
SlurmAllocationExecutorclass for SLURM-specific resource validation tests.
|
Cheers, thanks a lot @jan-janssen. For the reasons stated in your PR and the issue I think this is a significant improvement to the API. I didn't yet have time to look into the code of the PR and the interface. I can try to do this later today (can get rather late ;-)). What I would then be looking into is:
|
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (8)
executorlib/interfaces/local.py (2)
23-26: Clarify overlapping usage ofmax_workersandmax_cores.Both parameters appear to handle parallel resource limits. It might be clearer to rely on a single parameter or explain precisely how each interacts with the other to avoid confusion.
149-161: Consider extracting repeated default resource dictionary logic.You define a
default_resource_dictand update user input in multiple specialized executors across the codebase. Extracting this into a reusable helper function or constant could reduce duplication and improve maintainability.executorlib/interfaces/slurm.py (4)
80-96: Suggestion: Provide default values consistently in the docstring.
cache_directoryandmax_coresare labeled “optional,” but the docstring mentions default behaviors (e.g., “Defaults to ‘cache’”). Make sure these defaults are explicitly enforced or documented.
169-188: Add concurrency or HPC usage notes.While the code references HPC submissions (SLURM), the docstrings don't fully explain concurrency constraints or potential HPC pitfalls (e.g., job scheduling limits, cluster environment variables, etc.). Briefly clarifying these could help users avoid resource conflicts.
227-234: Align class description and naming with the specialized usage.“SlurmAllocationExecutor” is described similarly to
Executorin the docstring. Update the text to emphasize that this class specifically handles HPC resource allocations on SLURM.
361-368: Consider DRYing the resource dictionary usage across multiple executors.As with the local executor, you define the same default dictionary repeatedly. Consolidating it can reduce code duplication and maintenance overhead.
executorlib/interfaces/flux.py (1)
168-180: Unify duplication ofdefault_resource_dictdefinitions across executors.Similar to the local and SLURM executors, consider creating a shared mechanism for default resource configuration.
tests/benchmark/llh.py (1)
45-76: Consider aligning parameter names with standard library executors.The specialized executors use
max_coreswhile the standard library executors (ProcessPoolExecutor,ThreadPoolExecutor,MPIPoolExecutor) usemax_workers. Consider aligning these parameter names for consistency with the standard library, as mentioned in the PR discussion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
executorlib/__init__.py(1 hunks)executorlib/interfaces/flux.py(1 hunks)executorlib/interfaces/local.py(1 hunks)executorlib/interfaces/slurm.py(1 hunks)tests/benchmark/llh.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (11)
- 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.13)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
🔇 Additional comments (7)
executorlib/interfaces/local.py (1)
69-79:⚠️ Potential issueAlign the example with the new class and parameters.
Your example snippet still demonstrates “Executor(cores=2, init_function=init_k)” instead of “LocalExecutor”. If you want to highlight usage with multiple cores, rely on
max_coresormax_workersfor consistency.->>> with Executor(cores=2, init_function=init_k) as p: +>>> with LocalExecutor(max_cores=2, init_function=init_k) as p:Likely invalid or redundant comment.
executorlib/interfaces/flux.py (1)
83-103: Confirm the fallback logic forcache_directorydefaults.Even though lines mention “Defaults to ‘cache’,” there’s no explicit fallback here to that string. Ensure the final executor sets the correct directory if
Noneis passed.executorlib/__init__.py (2)
2-9: LGTM! The imports align with the PR objectives.The specialized executor classes are imported from their respective modules, which aligns with the goal of replacing the generic
Executorclass with distinct executor classes for each backend.
13-19: LGTM! The__all__list is properly updated.The
__all__list correctly exports all the specialized executor classes, making them available for import from theexecutorlibpackage.tests/benchmark/llh.py (3)
45-54: LGTM! The "block_allocation" mode correctly uses LocalExecutor.The changes align with the PR objectives by replacing the generic
ExecutorwithLocalExecutorand using the appropriate parameters.
56-65: LGTM! The "executorlib" mode correctly uses LocalExecutor.The changes align with the PR objectives by replacing the generic
ExecutorwithLocalExecutorand using the appropriate parameters.
67-76: LGTM! The "flux" mode correctly uses FluxAllocationExecutor.The changes align with the PR objectives by replacing the generic
ExecutorwithFluxAllocationExecutorand using the appropriate parameters.
executorlib/interfaces/local.py
Outdated
| """ | ||
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | ||
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | ||
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | ||
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | ||
| in an interactive Jupyter notebook. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update references to the deprecated executorlib.Executor in the docstring.
The docstring still mentions "executorlib.Executor" instead of LocalExecutor, which may confuse users. Consider updating these references to reflect the new class name and usage.
- The executorlib.Executor leverages either the message passing interface (MPI), ...
- ... the executorlib.Executor can be executed in a serial python process ...
+ The LocalExecutor leverages either the message passing interface (MPI), ...
+ ... the LocalExecutor can be executed in a serial python process ...📝 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.
| """ | |
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | |
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | |
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | |
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | |
| in an interactive Jupyter notebook. | |
| """ | |
| The LocalExecutor leverages either the message passing interface (MPI), the SLURM workload manager or | |
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | |
| the mpi4py.futures.MPIPoolExecutor the LocalExecutor can be executed in a serial python process and does not | |
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | |
| in an interactive Jupyter notebook. |
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | ||
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | ||
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | ||
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | ||
| in an interactive Jupyter notebook. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace references to executorlib.Executor in the docstring.
Use the new SlurmSubmissionExecutor or SlurmAllocationExecutor references to accurately describe the functionality and reduce confusion.
- The executorlib.Executor leverages either the message passing interface (MPI), ...
+ The SlurmSubmissionExecutor leverages either the message passing interface (MPI), ...📝 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.
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | |
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | |
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | |
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | |
| in an interactive Jupyter notebook. | |
| The SlurmSubmissionExecutor leverages either the message passing interface (MPI), the SLURM workload manager or | |
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | |
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | |
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | |
| in an interactive Jupyter notebook. |
executorlib/interfaces/flux.py
Outdated
| class FluxSubmissionExecutor: | ||
| """ | ||
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | ||
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | ||
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | ||
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | ||
| in an interactive Jupyter notebook. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refine class docstring to remove references to the removed generic Executor.
In the new architecture, FluxSubmissionExecutor is separate from any single, all-purpose Executor class. Mentioning executorlib.Executor can be misleading.
executorlib/interfaces/flux.py
Outdated
| >>> with Executor(cores=2, init_function=init_k) as p: | ||
| >>> fs = p.submit(calc, 2, j=4) | ||
| >>> print(fs.result()) | ||
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] |
There was a problem hiding this comment.
Adjust the example snippet to use the new executor class.
The example references “with Executor(cores=2, ...)” but the code is now FluxAllocationExecutor for local flux HPC usage.
->>> with Executor(cores=2, init_function=init_k) as p:
+>>> with FluxAllocationExecutor(max_cores=2, init_function=init_k) as p:📝 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.
| >>> with Executor(cores=2, init_function=init_k) as p: | |
| >>> fs = p.submit(calc, 2, j=4) | |
| >>> print(fs.result()) | |
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] | |
| >>> with FluxAllocationExecutor(max_cores=2, init_function=init_k) as p: | |
| >>> fs = p.submit(calc, 2, j=4) | |
| >>> print(fs.result()) | |
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] |
# Conflicts: # executorlib/__init__.py # tests/test_dependencies_executor.py
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/test_dependencies_executor.py (1)
Line range hint
236-257: Consider combining nestedwithstatements.The error handling tests correctly use
LocalExecutor. However, there's a minor opportunity for improvement in the test structure.You can simplify the nested
withstatements in these test methods. For example:- with LocalExecutor(max_cores=1, block_allocation=False) as exe: - cloudpickle_register(ind=1) - _ = exe.submit(raise_error) + with cloudpickle_register(ind=1), LocalExecutor(max_cores=1, block_allocation=False) as exe: + _ = exe.submit(raise_error)This pattern can be applied to all similar test methods in this class.
🧰 Tools
🪛 Ruff (0.8.2)
235-236: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
241-242: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
247-248: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
253-254: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_dependencies_executor.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_dependencies_executor.py
241-242: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
247-248: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
253-254: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build
- GitHub Check: build (windows-latest, 3.13)
🔇 Additional comments (5)
tests/test_dependencies_executor.py (5)
7-7: LGTM! Import change aligns with PR objectives.The change from importing
ExecutortoLocalExecutoraligns with the PR's goal of using specialized executor classes instead of a generic backend parameter.
Line range hint
49-54: LGTM! Basic executor test updated correctly.The test case correctly uses
LocalExecutorwhile maintaining the same functionality and assertions.
Line range hint
60-75: LGTM! Dependency plot test updated correctly.The test case properly uses
LocalExecutorwhile preserving the dependency graph plotting functionality and assertions.
Line range hint
86-95: LGTM! Dependency plot filename test updated correctly.The test case correctly uses
LocalExecutorwhile maintaining the file output functionality.
Line range hint
159-180: LGTM! Many-to-one test updated correctly.The test case properly uses
LocalExecutorwhile preserving the complex task dependency chain and resource allocation.
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
executorlib/interfaces/flux.py(1 hunks)executorlib/interfaces/local.py(1 hunks)executorlib/interfaces/slurm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- executorlib/interfaces/local.py
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build
- GitHub Check: mypy
- GitHub Check: build
- GitHub Check: build
🔇 Additional comments (11)
executorlib/interfaces/slurm.py (7)
20-24: Replace references toexecutorlib.Executorand fix grammar in docstring.
The docstring lines still referenceexecutorlib.Executor, which can be confusing now that we have specialized classes. Also, “preferable the flux framework” could be changed to “preferably the flux framework” for better clarity.
[duplicate_comment, nitpick]
80-96:__init__used only for auto-completion looks fine.
Defining__init__solely for IDE auto-completion is an acceptable pattern here, given that the actual construction logic resides in__new__.
156-168: Good usage of default dictionary merging.
Mergingresource_dictwith thedefault_resource_dictis a clean way to ensure missing keys have default values.
232-236: Replace references toexecutorlib.Executorand fix grammar in docstring.
As with the previous docstring, changing “preferable the flux framework” to “preferably the flux framework” and ditching mentions of the genericexecutorlib.Executorwill keep it consistent.
[duplicate_comment, nitpick]
292-306:__init__placeholder approach looks good.
Keeping an empty__init__for improved auto-completion is consistent with the submission executor’s design.
172-188: Confirm the continued usage ofbackend="slurm_submission".
Given that specialized classes were introduced to replace thebackendparameter, verify that this reference to"slurm_submission"is necessary or if it can be removed to align with the new approach.✅ Verification successful
Continued usage of
backend="slurm_submission"confirmed.
The search results indicate that the"slurm_submission"backend is still referenced in multiple parts of the repository (e.g., inexecutorlib/interfaces/slurm.py,executorlib/interactive/create.py,README.md, and the documentation notebooks). This confirms that it is still necessary and active within the current design.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching for references to "slurm_submission" across the repo (beyond this file) rg -A 3 "slurm_submission"Length of output: 2230
380-414: Verify necessity ofbackend="slurm_allocation".
With the specialized class in place, please confirm whether continuing to pass"slurm_allocation"is required, or if deeper refactoring can fully remove backend references.✅ Verification successful
Action: Continue passing "slurm_allocation" as the backend.
The repository search confirms that the string "slurm_allocation" is still actively used in multiple places (e.g., within
executorlib/interfaces/slurm.pyandexecutorlib/interactive/create.py, as well as noted in the README). This indicates that the specialized executor branch for Slurm allocation still relies on this backend reference, so its removal would require a deeper refactoring of the core executor creation logic.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Searching references to "slurm_allocation" across the repo rg -A 3 "slurm_allocation"Length of output: 2051
executorlib/interfaces/flux.py (4)
391-392: Good validation of pysqa_config_directory.The code properly validates the pysqa_config_directory parameter before using it, which helps prevent runtime errors.
Also applies to: 414-415
76-79:⚠️ Potential issueUpdate example code to use
FluxAllocationExecutor.The example code still uses the old
Executorclass which has been replaced.Apply this diff to update the example:
->>> with Executor(cores=2, init_function=init_k) as p: +>>> with FluxAllocationExecutor(max_cores=2, init_function=init_k) as p:Likely invalid or redundant comment.
223-227:⚠️ Potential issueUpdate docstring to reflect the new class name.
The docstring still references the old
executorlib.Executorclass which has been replaced.Apply this diff to update the docstring:
- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or + The FluxSubmissionExecutor leverages either the message passing interface (MPI), the SLURM workload manager or - preferable the flux framework for distributing python functions within a given resource allocation. In contrast to - the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not - require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly + preferable the flux framework for distributing python functions within a given resource allocation. In contrast to + the mpi4py.futures.MPIPoolExecutor this executor can be executed in a serial python process and does not + require the python script to be executed with MPI. It is even possible to execute it directlyLikely invalid or redundant comment.
20-24:⚠️ Potential issueUpdate docstring to reflect the new class name.
The docstring still references the old
executorlib.Executorclass which has been replaced.Apply this diff to update the docstring:
- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or + The FluxAllocationExecutor leverages either the message passing interface (MPI), the SLURM workload manager or - preferable the flux framework for distributing python functions within a given resource allocation. In contrast to - the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not - require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly + preferable the flux framework for distributing python functions within a given resource allocation. In contrast to + the mpi4py.futures.MPIPoolExecutor this executor can be executed in a serial python process and does not + require the python script to be executed with MPI. It is even possible to execute it directlyLikely invalid or redundant comment.
executorlib/interfaces/flux.py
Outdated
| >>> with Executor(cores=2, init_function=init_k) as p: | ||
| >>> fs = p.submit(calc, 2, j=4) | ||
| >>> print(fs.result()) | ||
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] |
There was a problem hiding this comment.
Update example code to use FluxSubmissionExecutor.
The example code still uses the old Executor class which has been replaced.
Apply this diff to update the example:
->>> with Executor(cores=2, init_function=init_k) as p:
+>>> with FluxSubmissionExecutor(max_cores=2, init_function=init_k) as p:📝 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.
| >>> with Executor(cores=2, init_function=init_k) as p: | |
| >>> fs = p.submit(calc, 2, j=4) | |
| >>> print(fs.result()) | |
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] | |
| >>> with FluxSubmissionExecutor(max_cores=2, init_function=init_k) as p: | |
| >>> fs = p.submit(calc, 2, j=4) | |
| >>> print(fs.result()) | |
| [(array([2, 4, 3]), 2, 0), (array([2, 4, 3]), 2, 1)] |
# Conflicts: # executorlib/interfaces/flux.py # executorlib/interfaces/local.py # executorlib/interfaces/slurm.py
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (6)
tests/test_dependencies_executor.py (1)
Line range hint
122-133: Consider extracting resource configuration to a helper function.The resource configuration for
create_local_executoris quite verbose and could be reused across tests. Consider extracting it to a helper function to improve readability and maintainability.+def create_default_resource_dict(): + return { + "cores": 1, + "threads_per_core": 1, + "gpus_per_core": 0, + "cwd": None, + "openmpi_oversubscribe": False, + "slurm_cmd_args": [], + } -executor = create_local_executor( - max_workers=1, - max_cores=2, - resource_dict={ - "cores": 1, - "threads_per_core": 1, - "gpus_per_core": 0, - "cwd": None, - "openmpi_oversubscribe": False, - "slurm_cmd_args": [], - }, -) +executor = create_local_executor( + max_workers=1, + max_cores=2, + resource_dict=create_default_resource_dict(), +)executorlib/interfaces/local.py (1)
214-219: Simplify dictionary key checks and cleanup.The code can be simplified by:
- Using
in dictinstead ofin dict.keys()- Using a more concise approach to remove multiple keys
Apply this diff to improve the code:
- if "threads_per_core" in resource_dict.keys(): - del resource_dict["threads_per_core"] - if "gpus_per_core" in resource_dict.keys(): - del resource_dict["gpus_per_core"] - if "slurm_cmd_args" in resource_dict.keys(): - del resource_dict["slurm_cmd_args"] + # Remove keys that are no longer needed after validation + for key in ["threads_per_core", "gpus_per_core", "slurm_cmd_args"]: + resource_dict.pop(key, None)🧰 Tools
🪛 Ruff (0.8.2)
214-214: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
216-216: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
218-218: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
executorlib/interfaces/flux.py (1)
461-464: Simplify dictionary key checks.Use
key in dictinstead ofkey in dict.keys()for better readability and performance.- 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"]🧰 Tools
🪛 Ruff (0.8.2)
461-461: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
463-463: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
executorlib/interfaces/slurm.py (3)
83-97: Clarify usage ofmax_workersandmax_cores.
Having both parameters may introduce confusion or redundancy, since standard executors typically usemax_workersfor parallelization limits. Consider unifying or documenting more explicitly howmax_workersandmax_coresdiffer and interact.
98-99: Note on using__new__instead of__init__.
Relying on__new__purely for auto-completion can be surprising for developers. If maintainability is a priority, consider an alternative approach or thoroughly document why__new__is required.
284-297: Clarify usage ofmax_workersandmax_cores.
Similar toSlurmSubmissionExecutor, consider unifying or explaining the difference between these two parameters more explicitly to align with standard executor naming and reduce confusion.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
executorlib/interactive/create.py(0 hunks)executorlib/interfaces/flux.py(1 hunks)executorlib/interfaces/local.py(1 hunks)executorlib/interfaces/slurm.py(1 hunks)tests/test_dependencies_executor.py(8 hunks)
💤 Files with no reviewable changes (1)
- executorlib/interactive/create.py
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/interfaces/flux.py
443-443: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
461-461: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
463-463: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
tests/test_dependencies_executor.py
236-237: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
242-243: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
248-249: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
executorlib/interfaces/slurm.py
404-404: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
executorlib/interfaces/local.py
200-200: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
214-214: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
216-216: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
218-218: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (10)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build
- GitHub Check: build
- 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 (macos-latest, 3.13)
- GitHub Check: build (windows-latest, 3.13)
🔇 Additional comments (14)
tests/test_dependencies_executor.py (3)
7-8: LGTM! Import changes align with the refactoring objectives.The imports correctly reflect the transition from generic
Executorto specializedLocalExecutorclass.
49-49: LGTM! Test methods successfully updated to use LocalExecutor.The test methods have been correctly updated to use
LocalExecutorwhile maintaining their original test logic and assertions.Also applies to: 60-63, 86-90, 154-154, 186-189
231-231: LGTM! Error test cases successfully updated to use LocalExecutor.The error test cases have been correctly updated to use
LocalExecutorwhile maintaining their original error testing logic.Also applies to: 237-237, 243-243, 249-249
🧰 Tools
🪛 Ruff (0.8.2)
230-231: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
executorlib/interfaces/local.py (2)
20-25: Update references to the deprecatedexecutorlib.Executorin the docstring.The docstring still mentions "
executorlib.Executor" instead ofLocalExecutor, which may confuse users.
153-160: 🛠️ Refactor suggestionUse immutable default for resource dictionary.
Using mutable defaults in Python can lead to unexpected behavior if the default value is modified.
Apply this diff to fix the issue:
- default_resource_dict: dict = { + default_resource_dict: dict = dict( "cores": 1, "threads_per_core": 1, "gpus_per_core": 0, "cwd": None, "openmpi_oversubscribe": False, "slurm_cmd_args": [], - } + )Likely invalid or redundant comment.
executorlib/interfaces/flux.py (7)
1-27: LGTM! Well-organized imports with proper error handling.The imports are cleanly structured with proper type hints, and the conditional import of Flux-specific modules is a good practice for handling optional dependencies.
30-34: Update docstring references fromexecutorlib.ExecutortoFluxAllocationExecutor.Help users understand that this class serves a more specialized purpose for HPC or local flux frameworks.
93-226: LGTM! Clean implementation with proper parameter handling.The class implementation is well-structured:
- Uses
__new__for instantiation which is appropriate for this factory pattern- Properly handles default resource dictionary
- Has comprehensive parameter validation
229-235: Refine class docstring to remove references to the removed genericExecutor.In the new architecture,
FluxSubmissionExecutoris separate from any single, all-purposeExecutorclass. Mentioningexecutorlib.Executorcan be misleading.
284-284: Update example code to useFluxSubmissionExecutor.The example code still uses the old
Executorclass which has been replaced.->>> with Executor(cores=2, init_function=init_k) as p: +>>> with FluxSubmissionExecutor(max_cores=2, init_function=init_k) as p:
291-436: LGTM! Well-structured implementation with proper error handling.The class implementation:
- Properly handles dependencies and plotting options
- Has comprehensive parameter validation
- Uses appropriate factory pattern with
__new__
439-493: LGTM! Well-structured helper function with comprehensive validation.The function:
- Properly validates all input parameters
- Handles resource dictionary updates correctly
- Returns appropriate executor types based on conditions
🧰 Tools
🪛 Ruff (0.8.2)
443-443: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
461-461: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
463-463: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
executorlib/interfaces/slurm.py (2)
23-27: Replace references toexecutorlib.Executorwith updated class names.
The docstring still refers toexecutorlib.Executor. Consider replacing it withSlurmSubmissionExecutorto avoid confusion.
223-229: Replace references toexecutorlib.Executorwith updated class names.
Like inSlurmSubmissionExecutor, this docstring references the removedexecutorlib.Executorclass. Rename to prevent confusion about which class to use.
executorlib/interfaces/local.py
Outdated
| """ | ||
| Instead of returning a executorlib.Executor object this function returns either a executorlib.mpi.PyMPIExecutor, | ||
| executorlib.slurm.PySlurmExecutor or executorlib.flux.PyFluxExecutor depending on which backend is available. The | ||
| executorlib.flux.PyFluxExecutor is the preferred choice while the executorlib.mpi.PyMPIExecutor is primarily used | ||
| for development and testing. The executorlib.flux.PyFluxExecutor requires flux-core from the flux-framework to be | ||
| installed and in addition flux-sched to enable GPU scheduling. Finally, the executorlib.slurm.PySlurmExecutor | ||
| requires the SLURM workload manager to be installed on the system. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update backend references in the docstring.
The docstring still references old executor classes (PyMPIExecutor, PySlurmExecutor, PyFluxExecutor) instead of the new ones mentioned in the PR objectives (LocalExecutor, FluxAllocationExecutor, etc.).
executorlib/interfaces/local.py
Outdated
| def create_local_executor( | ||
| max_workers: Optional[int] = None, | ||
| max_cores: Optional[int] = None, | ||
| cache_directory: Optional[str] = None, | ||
| resource_dict: dict = {}, | ||
| hostname_localhost: Optional[bool] = None, | ||
| block_allocation: bool = False, | ||
| init_function: Optional[Callable] = None, | ||
| ) -> Union[InteractiveStepExecutor, InteractiveExecutor]: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix mutable default argument.
Using mutable defaults in Python can lead to unexpected behavior if the default value is modified.
Apply this diff to fix the issue:
- resource_dict: dict = {},
+ resource_dict: Optional[dict] = None,Then initialize it at the beginning of the function:
def create_local_executor(...):
+ if resource_dict is None:
+ resource_dict = {}
check_init_function(block_allocation=block_allocation, init_function=init_function)📝 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_local_executor( | |
| max_workers: Optional[int] = None, | |
| max_cores: Optional[int] = None, | |
| cache_directory: Optional[str] = None, | |
| resource_dict: dict = {}, | |
| hostname_localhost: Optional[bool] = None, | |
| block_allocation: bool = False, | |
| init_function: Optional[Callable] = None, | |
| ) -> Union[InteractiveStepExecutor, InteractiveExecutor]: | |
| def create_local_executor( | |
| max_workers: Optional[int] = None, | |
| max_cores: Optional[int] = None, | |
| cache_directory: Optional[str] = None, | |
| resource_dict: Optional[dict] = None, | |
| hostname_localhost: Optional[bool] = None, | |
| block_allocation: bool = False, | |
| init_function: Optional[Callable] = None, | |
| ) -> Union[InteractiveStepExecutor, InteractiveExecutor]: | |
| if resource_dict is None: | |
| resource_dict = {} | |
| check_init_function(block_allocation=block_allocation, init_function=init_function) | |
| # ... rest of the function implementation |
🧰 Tools
🪛 Ruff (0.8.2)
200-200: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
executorlib/interfaces/flux.py
Outdated
| >>> def init_k(): | ||
| >>> return {"k": 3} | ||
| >>> | ||
| >>> with FluxAllocationExecutor(cores=2, init_function=init_k) as p: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update example code to use correct class name.
The example code uses cores parameter but should use max_cores for consistency with the class parameters.
->>> with FluxAllocationExecutor(cores=2, init_function=init_k) as p:
+>>> with FluxAllocationExecutor(max_cores=2, init_function=init_k) as p:📝 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.
| >>> with FluxAllocationExecutor(cores=2, init_function=init_k) as p: | |
| >>> with FluxAllocationExecutor(max_cores=2, init_function=init_k) as p: |
executorlib/interfaces/flux.py
Outdated
| max_workers: Optional[int] = None, | ||
| max_cores: Optional[int] = None, | ||
| cache_directory: Optional[str] = None, | ||
| resource_dict: dict = {}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace mutable default argument with None.
Using mutable default arguments can lead to unexpected behavior.
-def create_flux_executor(
- max_workers: Optional[int] = None,
- max_cores: Optional[int] = None,
- cache_directory: Optional[str] = None,
- resource_dict: dict = {},
+def create_flux_executor(
+ max_workers: Optional[int] = None,
+ max_cores: Optional[int] = None,
+ cache_directory: Optional[str] = None,
+ resource_dict: Optional[dict] = None,📝 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.
| resource_dict: dict = {}, | |
| def create_flux_executor( | |
| max_workers: Optional[int] = None, | |
| max_cores: Optional[int] = None, | |
| cache_directory: Optional[str] = None, | |
| resource_dict: Optional[dict] = None, | |
| ): | |
| # Initialize resource_dict if not provided | |
| if resource_dict is None: | |
| resource_dict = {} | |
| # ... rest of the create_flux_executor implementation ... |
🧰 Tools
🪛 Ruff (0.8.2)
443-443: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
executorlib/interfaces/slurm.py
Outdated
| max_workers: Optional[int] = None, | ||
| max_cores: Optional[int] = None, | ||
| cache_directory: Optional[str] = None, | ||
| resource_dict: dict = {}, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid using a mutable default argument.
Using a mutable data structure ({}) as the default value for resource_dict can lead to unintended side effects.
Apply this diff to fix:
- def create_slurm_executor(
- max_workers: Optional[int] = None,
- max_cores: Optional[int] = None,
- cache_directory: Optional[str] = None,
- resource_dict: dict = {},
- hostname_localhost: Optional[bool] = None,
- block_allocation: bool = False,
- init_function: Optional[Callable] = None,
- ) -> Union[InteractiveStepExecutor, InteractiveExecutor]:
+ def create_slurm_executor(
+ max_workers: Optional[int] = None,
+ max_cores: Optional[int] = None,
+ cache_directory: Optional[str] = None,
+ resource_dict: Optional[dict] = None,
+ hostname_localhost: Optional[bool] = None,
+ block_allocation: bool = False,
+ init_function: Optional[Callable] = None,
+ ) -> Union[InteractiveStepExecutor, InteractiveExecutor]:
+ 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.
| resource_dict: dict = {}, | |
| def create_slurm_executor( | |
| max_workers: Optional[int] = None, | |
| max_cores: Optional[int] = None, | |
| cache_directory: Optional[str] = None, | |
| resource_dict: Optional[dict] = None, | |
| hostname_localhost: Optional[bool] = None, | |
| block_allocation: bool = False, | |
| init_function: Optional[Callable] = None, | |
| ) -> Union[InteractiveStepExecutor, InteractiveExecutor]: | |
| if resource_dict is None: | |
| resource_dict = {} | |
| # Remainder of the function implementation |
🧰 Tools
🪛 Ruff (0.8.2)
404-404: 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: 0
🧹 Nitpick comments (2)
executorlib/interfaces/slurm.py (1)
208-382: Consider extracting common code into a base class.Both
SlurmSubmissionExecutorandSlurmAllocationExecutorshare significant code, including:
- Resource dictionary initialization
- Parameter handling
- Docstrings
__init__and__new__structureThis duplication could be reduced by creating a base class.
Example base class structure:
class BaseSlurmExecutor: def __init__(self, **kwargs): pass @staticmethod def _init_resource_dict(resource_dict: Optional[dict] = None) -> dict: default_resource_dict = { "cores": 1, "threads_per_core": 1, "gpus_per_core": 0, "cwd": None, "openmpi_oversubscribe": False, "slurm_cmd_args": [], } if resource_dict is None: resource_dict = {} resource_dict.update( {k: v for k, v in default_resource_dict.items() if k not in resource_dict} ) return resource_dictexecutorlib/interfaces/flux.py (1)
442-445: Simplify dictionary key checks.Use direct key membership test instead of calling
.keys().- 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"]🧰 Tools
🪛 Ruff (0.8.2)
442-442: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
444-444: 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 (2)
executorlib/interfaces/flux.py(1 hunks)executorlib/interfaces/slurm.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/interfaces/flux.py
424-424: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
442-442: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
444-444: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
executorlib/interfaces/slurm.py
389-389: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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.13)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: build
- GitHub Check: build (windows-latest, 3.13)
🔇 Additional comments (10)
executorlib/interfaces/slurm.py (5)
22-26: Update references toexecutorlib.Executorin the docstring.The docstring still references the old
executorlib.Executorclass which is being replaced.
82-98: LGTM! Well-structured implementation using__new__.The implementation pattern using
__new__for instantiation while keeping__init__for auto-completion is a good approach. The resource dictionary handling with proper defaults is also robust.Also applies to: 100-206
210-214: Update references toexecutorlib.Executorin the docstring.The docstring still references the old
executorlib.Executorclass which is being replaced.
389-389: Fix mutable default argument.Using a mutable default argument (
dict = {}) can lead to unexpected behavior.🧰 Tools
🪛 Ruff (0.8.2)
389-389: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
385-422: LGTM! Well-structured executor creation logic.The function properly handles:
- Parameter validation
- Resource configuration
- Block vs non-block allocation cases
- Worker validation for SLURM
🧰 Tools
🪛 Ruff (0.8.2)
389-389: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
executorlib/interfaces/flux.py (5)
29-33: Update docstring references fromexecutorlib.ExecutortoFluxAllocationExecutor.The docstring still refers to the old
executorlib.Executorclass which has been removed.
85-85: Update example code to use correct parameter name.The example code uses
coresparameter but should usemax_coresfor consistency with the class parameters.
230-234: Refine class docstring to remove references to the removed genericExecutor.The docstring still refers to the old
executorlib.Executorclass which has been removed.
283-283: Update example code to useFluxSubmissionExecutor.The example code still uses the old
Executorclass which has been replaced.
424-424:⚠️ Potential issueReplace mutable default argument with None.
Using mutable default arguments can lead to unexpected behavior.
-def create_flux_executor( - max_workers: Optional[int] = None, - max_cores: Optional[int] = None, - cache_directory: Optional[str] = None, - resource_dict: dict = {}, +def create_flux_executor( + max_workers: Optional[int] = None, + max_cores: Optional[int] = None, + cache_directory: Optional[str] = None, + resource_dict: Optional[dict] = None,Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
424-424: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
It took a couple of separate pull requests, but I managed to separate the interface
The parameter name the classes share with
It is using processes, so I agree
Yes, the naming is inspired by the SLURM language as to my knowledge it is currently the most widely used job scheduler. |
|
While the coverage slightly decreased by 0.5%, this is primarily as we cannot test SLURM in our test environment. Still the overall coverage remains over 95% so I would suggest to merge this pull request. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (6)
tests/test_dependencies_executor.py (1)
137-138: Combine nestedwithstatements.The test methods contain nested
withstatements that can be combined for better readability.Apply this pattern to all test methods:
- with self.assertRaises(RuntimeError): - with LocalExecutor(max_cores=1, block_allocation=False) as exe: + with self.assertRaises(RuntimeError), LocalExecutor(max_cores=1, block_allocation=False) as exe:Also applies to: 143-144, 149-150, 155-156
tests/test_executor_backend_flux.py (1)
59-72: LGTM!The new test method
test_flux_executor_serial_no_depenciescorrectly verifies the behavior when dependencies are disabled.Fix the typo in the method name:
- def test_flux_executor_serial_no_depencies(self): + def test_flux_executor_serial_no_dependencies(self):tests/test_plot_dependency_flux.py (2)
11-13: Remove unused imports.The imports
pygraphviz,flux.job, andFluxPythonSpawnerare only used for availability testing. Consider usingimportlib.util.find_spec.Apply this diff:
-try: - import pygraphviz - import flux.job - from executorlib.interactive.flux import FluxPythonSpawner +from importlib.util import find_spec +try: + if not all(find_spec(pkg) for pkg in ['pygraphviz', 'flux']): + raise ImportError🧰 Tools
🪛 Ruff (0.8.2)
11-11:
pygraphvizimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
12-12:
flux.jobimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
13-13:
executorlib.interactive.flux.FluxPythonSpawnerimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
99-100: Fix ambiguous variable names.The variable name
lis ambiguous. Use a more descriptive name.Apply this pattern:
- for l in lst: - self.assertTrue(l.done()) + for future in lst: + self.assertTrue(future.done())Also applies to: 168-169
🧰 Tools
🪛 Ruff (0.8.2)
99-99: Ambiguous variable name:
l(E741)
tests/test_plot_dependency.py (2)
11-11: Remove unused import.The import
pygraphvizis only used for availability testing. Consider usingimportlib.util.find_spec.Apply this diff:
-try: - import pygraphviz +from importlib.util import find_spec +try: + if not find_spec('pygraphviz'): + raise ImportError🧰 Tools
🪛 Ruff (0.8.2)
11-11:
pygraphvizimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
109-110: Fix ambiguous variable names.The variable name
lis ambiguous. Use a more descriptive name.Apply this pattern in all test methods:
- for l in lst: - self.assertTrue(l.done()) + for future in lst: + self.assertTrue(future.done())Also applies to: 180-181, 249-250
🧰 Tools
🪛 Ruff (0.8.2)
109-109: Ambiguous variable name:
l(E741)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
tests/test_dependencies_executor.py(5 hunks)tests/test_executor_backend_flux.py(9 hunks)tests/test_plot_dependency.py(1 hunks)tests/test_plot_dependency_flux.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_dependencies_executor.py
142-143: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
148-149: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
154-155: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
tests/test_plot_dependency_flux.py
11-11: pygraphviz imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
12-12: flux.job imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
13-13: executorlib.interactive.flux.FluxPythonSpawner imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
99-99: Ambiguous variable name: l
(E741)
168-168: Ambiguous variable name: l
(E741)
tests/test_plot_dependency.py
11-11: pygraphviz imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
109-109: Ambiguous variable name: l
(E741)
180-180: Ambiguous variable name: l
(E741)
249-249: Ambiguous variable name: l
(E741)
🔇 Additional comments (3)
tests/test_dependencies_executor.py (2)
47-47: LGTM!The LocalExecutor instantiation with max_cores=1 is correct and aligns with the PR's objective of using specialized executor classes.
Line range hint
76-97: Verify the resource dictionary structure.The resource dictionary contains several parameters that might need validation:
- threads_per_core
- gpus_per_core
- openmpi_oversubscribe
- slurm_cmd_args
Run the following script to verify the resource dictionary structure across the codebase:
✅ Verification successful
Resource Dictionary Structure Validated
The resource dictionary is used consistently throughout the codebase. All keys under review—including
threads_per_core,gpus_per_core,openmpi_oversubscribe, andslurm_cmd_args—are present and handled appropriately in the executor implementations and tests.
- The test file
tests/test_dependencies_executor.pyprovides a complete resource dictionary that matches the expected structure.- Various modules (such as those under
executorlib/) update or extend this dictionary consistently.- Documentation and additional test cases confirm the expected keys and their data types.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the resource dictionary structure in executor classes. # Search for resource dictionary definitions ast-grep --pattern 'resource_dict = { $$$ }' # Search for resource dictionary usage in tests rg -A 5 'resource_dict ?='Length of output: 33506
tests/test_executor_backend_flux.py (1)
Line range hint
88-97: Verify PMI mode configuration.The test uses
flux_executor_pmi_mode=pmiwhich is obtained from an environment variable. Ensure this configuration is documented.Run the following script to check PMI mode usage:
✅ Verification successful
PMI Mode Configuration is Verified Documented
- The configuration for PMI mode, obtained via the
PYMPIPOOL_PMIXenvironment variable, is documented in the codebase (e.g., indocs/installation.mdand in the docstrings withinexecutorlib/interfaces/flux.pyandexecutorlib/interactive/flux.py).- Test files (like
tests/test_executor_backend_flux.py) use this configuration, verifying that the parameter is retrieved and applied consistently.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify PMI mode configuration documentation. # Search for PMI mode documentation rg -A 5 'PYMPIPOOL_PMIX' # Search for PMI mode usage in tests rg -A 5 'flux_executor_pmi_mode'Length of output: 17454
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
notebooks/4-developer.ipynb (1)
Line range hint
1-24: Update markdown documentation to reflect new executor classes.The markdown cells still reference the generic
Executorclass, but the code now uses specialized executor classes. This inconsistency could confuse users.Apply these changes to the markdown cells:
- In the first cell:
-Most users should only import the `Executor` class from executorlib +Most users should import the appropriate specialized executor class (like `SingleNodeExecutor`, `FluxAllocationExecutor`, etc.) from executorlib
- In the subprocess section:
-which itself is submitted to the `Executor` class. +which itself is submitted to the appropriate executor class.
- In the interactive section:
-functions submitted to the `Executor` class +functions submitted to the executor class
- In the modules section:
-but rather only the `Executor` class should be used as central interface +but rather only the specialized executor classes should be used as central interfacesAlso applies to: 75-77, 111-114, 171-174, 306-309
🧹 Nitpick comments (7)
tests/test_plot_dependency.py (2)
22-25: Consider reducing sleep for faster test runs
These functions (add_function,generate_tasks,calc_from_lst,merge) each includesleep(0.2), which may slow down test execution, especially when many tasks are running. If performance becomes a concern, consider lowering the sleep time or using alternative synchronization/wait strategies.Also applies to: 27-30, 32-35, 37-40
113-113: Avoid using the single-letter variablel
The variablelcan be mistaken for the number 1 or the letterI. Rename it for better clarity and maintainability.Example fix (for one such loop at line 113):
- for l in lst: + for future_item in lst: self.assertTrue(future_item.done())Also applies to: 184-184, 253-253
🧰 Tools
🪛 Ruff (0.8.2)
113-113: Ambiguous variable name:
l(E741)
executorlib/interfaces/single.py (1)
214-214: Use direct membership checks for dictionary keys
Instead ofkey in dict.keys(), preferkey in dict. This is more idiomatic and slightly faster.- if "threads_per_core" in resource_dict.keys(): + if "threads_per_core" in resource_dict: del resource_dict["threads_per_core"] - if "gpus_per_core" in resource_dict.keys(): + if "gpus_per_core" in resource_dict: del resource_dict["gpus_per_core"] - if "slurm_cmd_args" in resource_dict.keys(): + if "slurm_cmd_args" in resource_dict: del resource_dict["slurm_cmd_args"]Also applies to: 216-216, 218-218
🧰 Tools
🪛 Ruff (0.8.2)
214-214: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
tests/test_executor_backend_mpi_noblock.py (1)
3-3: Consider adding test coverage for other executor types.While the transition to
SingleNodeExecutoris correct, consider adding test cases for other specialized executors (e.g.,FluxAllocationExecutor,FluxSubmissionExecutor) to ensure comprehensive coverage of the new executor hierarchy.tests/test_executor_backend_mpi.py (1)
58-61: Remove unused variable assignment.The variable
fs_1is assigned but never used in the test. Consider removing the assignment or using the variable in an assertion.with SingleNodeExecutor(max_cores=1, block_allocation=True) as exe: cloudpickle_register(ind=1) - fs_1 = exe.submit(calc, 1, resource_dict={"cores": 2}) + exe.submit(calc, 1, resource_dict={"cores": 2})🧰 Tools
🪛 Ruff (0.8.2)
60-60: Local variable
fs_1is assigned to but never usedRemove assignment to unused variable
fs_1(F841)
tests/test_dependencies_executor.py (1)
136-145: Simplify nestedwithstatements in test methods.The test methods use nested
withstatements which can be simplified for better readability.- def test_block_allocation_false_one_worker(self): - with self.assertRaises(RuntimeError): - with SingleNodeExecutor(max_cores=1, block_allocation=False) as exe: - cloudpickle_register(ind=1) - _ = exe.submit(raise_error) - - def test_block_allocation_true_one_worker(self): - with self.assertRaises(RuntimeError): - with SingleNodeExecutor(max_cores=1, block_allocation=True) as exe: - cloudpickle_register(ind=1) - _ = exe.submit(raise_error) + def test_block_allocation_false_one_worker(self): + with ( + self.assertRaises(RuntimeError), + SingleNodeExecutor(max_cores=1, block_allocation=False) as exe + ): + cloudpickle_register(ind=1) + _ = exe.submit(raise_error) + + def test_block_allocation_true_one_worker(self): + with ( + self.assertRaises(RuntimeError), + SingleNodeExecutor(max_cores=1, block_allocation=True) as exe + ): + cloudpickle_register(ind=1) + _ = exe.submit(raise_error)🧰 Tools
🪛 Ruff (0.8.2)
136-137: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
142-143: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
tests/test_integration_pyiron_workflow.py (1)
78-78: Add docstring to clarify SingleNodeExecutor usage.Consider adding a docstring to explain why
SingleNodeExecutoris used here and how it differs from other executor types in the context of dynamically defined objects.- executor = SingleNodeExecutor(block_allocation=True, max_workers=1) + # Use SingleNodeExecutor for testing dynamically defined objects in a local context + # Other executor types (e.g., FluxAllocationExecutor) are not suitable for this test + # as they require additional infrastructure + executor = SingleNodeExecutor(block_allocation=True, max_workers=1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
executorlib/__init__.py(1 hunks)executorlib/interfaces/single.py(1 hunks)notebooks/1-local.ipynb(15 hunks)notebooks/4-developer.ipynb(3 hunks)tests/benchmark/llh.py(1 hunks)tests/test_cache_executor_interactive.py(2 hunks)tests/test_dependencies_executor.py(5 hunks)tests/test_executor_backend_mpi.py(8 hunks)tests/test_executor_backend_mpi_noblock.py(5 hunks)tests/test_integration_pyiron_workflow.py(7 hunks)tests/test_plot_dependency.py(1 hunks)tests/test_shell_executor.py(5 hunks)tests/test_shell_interactive.py(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (5)
- tests/benchmark/llh.py
- tests/test_cache_executor_interactive.py
- tests/test_shell_executor.py
- notebooks/1-local.ipynb
- tests/test_shell_interactive.py
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_executor_backend_mpi.py
60-60: Local variable fs_1 is assigned to but never used
Remove assignment to unused variable fs_1
(F841)
tests/test_plot_dependency.py
15-15: pygraphviz imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
113-113: Ambiguous variable name: l
(E741)
184-184: Ambiguous variable name: l
(E741)
253-253: Ambiguous variable name: l
(E741)
executorlib/interfaces/single.py
200-200: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
214-214: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
216-216: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
218-218: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
tests/test_dependencies_executor.py
142-143: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
148-149: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
154-155: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
tests/test_executor_backend_mpi_noblock.py
66-70: Use a single with statement with multiple contexts instead of nested with statements
Combine with statements
(SIM117)
⏰ 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, .ci_support/environment-openmpi.yml)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build
🔇 Additional comments (5)
tests/test_plot_dependency.py (1)
14-19:pygraphvizimport is valid for skip logic checks
The static analysis tool warns thatpygraphvizis unused, but here it's deliberately imported as part of atry-exceptblock to conditionally skip tests ifgraphvizis unavailable. This code is correct as is.🧰 Tools
🪛 Ruff (0.8.2)
15-15:
pygraphvizimported but unused; consider usingimportlib.util.find_specto test for availability(F401)
executorlib/__init__.py (1)
2-9: New specialized executors look good
The dedicated imports for each specialized executor and updated__all__declaration improve clarity and autocomplete. This refactoring aligns with the PR’s goal of removing the genericExecutorin favor of specialized classes.Also applies to: 13-19
notebooks/4-developer.ipynb (3)
59-59: LGTM! Import statement updated correctly.The import statement has been updated to use
SingleNodeExecutorinstead ofExecutor, aligning with the PR's objective of using specialized executor classes.
Line range hint
94-101: LGTM! Executor instantiation updated correctly.The code now uses
SingleNodeExecutor()without a backend parameter, which is consistent with the PR's goal of using specialized executor classes.
Line range hint
251-261: LGTM! Complex executor configuration updated correctly.The code correctly uses
SingleNodeExecutorwith advanced configuration options likemax_workers,init_function, andblock_allocation.
executorlib/interfaces/single.py
Outdated
| max_workers: Optional[int] = None, | ||
| max_cores: Optional[int] = None, | ||
| cache_directory: Optional[str] = None, | ||
| resource_dict: dict = {}, |
There was a problem hiding this comment.
Avoid defaulting to a mutable data structure
Using a mutable default argument (resource_dict: dict = {}) can lead to unexpected behavior if this default object is modified elsewhere.
Proposed fix:
- def create_local_executor(
- max_workers: Optional[int] = None,
- max_cores: Optional[int] = None,
- cache_directory: Optional[str] = None,
- resource_dict: dict = {},
- ...
- ):
+ def create_local_executor(
+ max_workers: Optional[int] = None,
+ max_cores: Optional[int] = None,
+ cache_directory: Optional[str] = None,
+ resource_dict: Optional[dict] = 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.
| resource_dict: dict = {}, | |
| def create_local_executor( | |
| max_workers: Optional[int] = None, | |
| max_cores: Optional[int] = None, | |
| cache_directory: Optional[str] = None, | |
| resource_dict: Optional[dict] = None, | |
| ... | |
| ): | |
| if resource_dict is None: | |
| resource_dict = {} | |
| ... |
🧰 Tools
🪛 Ruff (0.8.2)
200-200: 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: 0
🧹 Nitpick comments (2)
tests/test_plot_dependency_flux.py (1)
98-98: Improve variable naming for better readability.The variable name
lis ambiguous and could be confused with the number one. Consider using a more descriptive name liketaskorfuture.Apply this diff to improve readability:
- for l in lst: - self.assertTrue(l.done()) + for task in lst: + self.assertTrue(task.done())Also applies to: 167-167
🧰 Tools
🪛 Ruff (0.8.2)
98-98: Ambiguous variable name:
l(E741)
tests/test_plot_dependency.py (1)
116-117: Improve variable naming for better readability.The variable name
lis ambiguous and could be confused with the number one. Consider using a more descriptive name liketaskorfuture.Apply this diff to improve readability:
- for l in lst: - self.assertTrue(l.done()) + for task in lst: + self.assertTrue(task.done())Also applies to: 189-190, 258-259
🧰 Tools
🪛 Ruff (0.8.2)
116-116: Ambiguous variable name:
l(E741)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.github/workflows/unittest-flux-mpich.yml(1 hunks).github/workflows/unittest-flux-openmpi.yml(1 hunks)tests/test_plot_dependency.py(1 hunks)tests/test_plot_dependency_flux.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_plot_dependency.py
15-15: pygraphviz imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
116-116: Ambiguous variable name: l
(E741)
189-189: Ambiguous variable name: l
(E741)
258-258: Ambiguous variable name: l
(E741)
tests/test_plot_dependency_flux.py
11-11: pygraphviz imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
12-12: flux.job imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
13-13: executorlib.interactive.flux.FluxPythonSpawner imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
98-98: Ambiguous variable name: l
(E741)
167-167: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms (5)
- GitHub Check: build (windows-latest, 3.13)
- 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 (macos-latest, 3.13)
🔇 Additional comments (2)
.github/workflows/unittest-flux-mpich.yml (1)
37-37: LGTM!The test file
test_plot_dependency_flux.pyis correctly added to the test command..github/workflows/unittest-flux-openmpi.yml (1)
37-37: LGTM!The test file
test_plot_dependency_flux.pyis correctly added to the test command.
I agree you don't want to use the same name as the standard library to avoid confusion.
It also does not yet clarify that it uses processes under the hood.
Having looked through the example notebooks, I understand that (correct me if I'm wrong)
The name can't capture everything, but I think it makes sense if it would capture this grouping in some way. If you want to capture the local/remote differentiation, one could think of something like
If you want to capture the blocking/non-blocking differentiation, one could think of something like
How intuitive these names are is of course subjective; maybe good to get other opinions on these. This is also something that JOSS reviewers could comment on. |
|
P.S. For me, the In any case, even if you keep the current names, I think this PR is great and should be merged. |
|
I have the feeling there is still some confusing. While technically it is possible to submit functions via SSH, this is not recommend as it requires both systems to be binary compatible, as discussed in https://executorlib.readthedocs.io/en/latest/trouble_shooting.html#ssh-connection The difference between submission and allocation is primarily the size of your tasks. If you have a number of rather large tasks like DFT MD calculation, the the submission mode is reasonable, as it submits each python function from the head node using sbatch and each python function is scheduled individually by the queuing system. For a number of small tasks it makes more sense to submit the whole python script via sbatch and then request the resources for the individual python function from the allocation you received via sbatch with the srun command. This is the functionality implemented in the allocation mode. I try to explain this difference at the first page of the documentation but I understand that it is still too complex https://executorlib.readthedocs.io/en/latest/README.html#examples |
|
I thought a bit more about the naming for SLURM. The major difference is one python function call per SLURM job versus multiple python function calls bundled in one SLURM job. How about naming the first EDIT: This might also be a natural way to explain the nesting of Executors, a SlurmJobExecutor can be nested into a function submitted using the SlurmQueueExecutor. |
|
If you can follow the standard library in naming the executor by the resources it has access to, I think that's great.
P.S. In the edit above you also wrote |
|
Yes, that was a mistake in the edit, I tried a couple of different names but you are right SlurmClusterExecutor sounds good. In terms of flux, the application of FluxClusterExecutor is currently limited as El Capitan in Livermore is the only cluster I am aware of using flux as primary job scheduler. But the FluxJobExecutor can be executed in a SLURM allocation or hierarchically as part of a SlurmClusterExecutor function. |
for more information, see https://pre-commit.ci
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
tests/test_plot_dependency_flux.py (1)
167-169: Improve variable naming for better readability.Replace the ambiguous variable name
lwith a more descriptive name likefutureortask.Apply this diff:
- for l in lst: - self.assertTrue(l.done()) + for future in lst: + self.assertTrue(future.done())Also applies to: 258-260
🧰 Tools
🪛 Ruff (0.8.2)
167-167: Ambiguous variable name:
l(E741)
tests/test_plot_dependency.py (1)
116-118: Improve variable naming for better readability.Replace the ambiguous variable name
lwith a more descriptive name likefutureortask.Apply this diff:
- for l in lst: - self.assertTrue(l.done()) + for future in lst: + self.assertTrue(future.done())Also applies to: 189-191, 258-260
🧰 Tools
🪛 Ruff (0.8.2)
116-116: Ambiguous variable name:
l(E741)
executorlib/interfaces/flux.py (1)
442-445: Simplify dictionary key checks.Use
key in dictinstead ofkey in dict.keys()for better readability.Apply this diff to fix:
- 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"]🧰 Tools
🪛 Ruff (0.8.2)
442-442: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
444-444: 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 (10)
executorlib/__init__.py(1 hunks)executorlib/interfaces/flux.py(1 hunks)executorlib/interfaces/slurm.py(1 hunks)notebooks/3-hpc-allocation.ipynb(8 hunks)tests/benchmark/llh.py(1 hunks)tests/test_cache_executor_pysqa_flux.py(2 hunks)tests/test_executor_backend_flux.py(9 hunks)tests/test_executor_backend_mpi.py(8 hunks)tests/test_plot_dependency.py(1 hunks)tests/test_plot_dependency_flux.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- tests/test_cache_executor_pysqa_flux.py
- tests/benchmark/llh.py
- tests/test_executor_backend_flux.py
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/interfaces/flux.py
424-424: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
442-442: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
444-444: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
tests/test_plot_dependency.py
15-15: pygraphviz imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
116-116: Ambiguous variable name: l
(E741)
189-189: Ambiguous variable name: l
(E741)
258-258: Ambiguous variable name: l
(E741)
executorlib/interfaces/slurm.py
389-389: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
tests/test_executor_backend_mpi.py
60-60: Local variable fs_1 is assigned to but never used
Remove assignment to unused variable fs_1
(F841)
tests/test_plot_dependency_flux.py
11-11: pygraphviz imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
12-12: flux.job imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
13-13: executorlib.interactive.flux.FluxPythonSpawner imported but unused; consider using importlib.util.find_spec to test for availability
(F401)
98-98: Ambiguous variable name: l
(E741)
167-167: Ambiguous variable name: l
(E741)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build
🔇 Additional comments (14)
executorlib/__init__.py (1)
1-19: LGTM! The changes align with the PR objectives.The file is well-structured and exports the specialized executor classes as intended.
tests/test_executor_backend_mpi.py (1)
56-61: LGTM! The test validates resource oversubscription.The test correctly raises a
ValueErrorwhen attempting to allocate more cores than available.🧰 Tools
🪛 Ruff (0.8.2)
57-58: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
60-60: Local variable
fs_1is assigned to but never usedRemove assignment to unused variable
fs_1(F841)
tests/test_plot_dependency.py (1)
69-84: LGTM! Good practice in cleaning up test artifacts.The test properly cleans up the generated graph file after the test completes.
notebooks/3-hpc-allocation.ipynb (4)
16-16: Update documentation to reflect the new API.The markdown text still refers to the
backendparameter, but this has been replaced with specialized executor classes.
35-35: LGTM!The import statement correctly uses the new specialized executor class.
114-114: LGTM!The import statement correctly uses the new specialized executor class.
324-324: LGTM!The executor instantiation correctly uses the new specialized executor class with appropriate parameters.
executorlib/interfaces/slurm.py (3)
22-26: Update docstring to reflect the new API.The docstring still refers to
executorlib.Executorwhich has been removed.
210-214: Update docstring to reflect the new API.The docstring still refers to
executorlib.Executorwhich has been removed.
389-389: Replace mutable default argument with None.Using a mutable default argument can lead to unexpected behavior.
🧰 Tools
🪛 Ruff (0.8.2)
389-389: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
executorlib/interfaces/flux.py (4)
29-33: Update docstring to reflect the new API.The docstring still refers to
executorlib.Executorwhich has been removed.
230-234: Update docstring to reflect the new API.The docstring still refers to
executorlib.Executorwhich has been removed.
283-283: Update example code to use the new executor class.The example still uses the old
Executorclass which has been removed.
424-424: Replace mutable default argument with None.Using a mutable default argument can lead to unexpected behavior.
🧰 Tools
🪛 Ruff (0.8.2)
424-424: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
executorlib/interfaces/slurm.py
Outdated
| >>> def init_k(): | ||
| >>> return {"k": 3} | ||
| >>> | ||
| >>> with SlurmClusterExecutor(cores=2, init_function=init_k) as p: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update example code to use correct parameter name.
The example uses cores=2 but should use max_cores=2 to match the class parameters.
Apply this diff to fix:
->>> with SlurmClusterExecutor(cores=2, init_function=init_k) as p:
+>>> with SlurmClusterExecutor(max_cores=2, init_function=init_k) as p:📝 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.
| >>> with SlurmClusterExecutor(cores=2, init_function=init_k) as p: | |
| >>> with SlurmClusterExecutor(max_cores=2, init_function=init_k) as p: |
| def __init__( | ||
| self, | ||
| max_workers: Optional[int] = None, | ||
| cache_directory: Optional[str] = None, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update example code to use correct parameter name.
The example uses cores=2 but should use max_cores=2 to match the class parameters.
Apply this diff to fix:
->>> with FluxJobExecutor(cores=2, init_function=init_k) as p:
+>>> with FluxJobExecutor(max_cores=2, init_function=init_k) as p:Committable suggestion skipped: line range outside the PR's diff.
executorlib/interfaces/flux.py
Outdated
| >>> def init_k(): | ||
| >>> return {"k": 3} | ||
| >>> | ||
| >>> with FluxJobExecutor(cores=2, init_function=init_k) as p: |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update example code to use correct parameter name.
The example uses cores=2 but should use max_cores=2 to match the class parameters.
Apply this diff to fix:
->>> with FluxJobExecutor(cores=2, init_function=init_k) as p:
+>>> with FluxJobExecutor(max_cores=2, init_function=init_k) as p:📝 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.
| >>> with FluxJobExecutor(cores=2, init_function=init_k) as p: | |
| >>> with FluxJobExecutor(max_cores=2, init_function=init_k) as p: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
executorlib/interfaces/slurm.py (1)
389-389:⚠️ Potential issueFix mutable default argument.
Using a mutable default argument can lead to unexpected behavior in Python.
Apply this diff to fix:
-def create_slurm_executor( - max_workers: Optional[int] = None, - max_cores: Optional[int] = None, - cache_directory: Optional[str] = None, - resource_dict: dict = {}, - hostname_localhost: Optional[bool] = None, - block_allocation: bool = False, - init_function: Optional[Callable] = None, -) -> Union[InteractiveStepExecutor, InteractiveExecutor]: +def create_slurm_executor( + max_workers: Optional[int] = None, + max_cores: Optional[int] = None, + cache_directory: Optional[str] = None, + resource_dict: Optional[dict] = None, + hostname_localhost: Optional[bool] = None, + block_allocation: bool = False, + init_function: Optional[Callable] = None, +) -> Union[InteractiveStepExecutor, InteractiveExecutor]: + if resource_dict is None: + resource_dict = {}🧰 Tools
🪛 Ruff (0.8.2)
389-389: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
executorlib/interfaces/single.py (1)
200-200: 🛠️ Refactor suggestionAvoid Using a Mutable Default Argument
Using
dict = {}as a default function parameter can lead to unexpected behavior if that object is modified elsewhere. Opt forNoneand initialize within the function body to prevent bugs.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, hostname_localhost: Optional[bool] = None, block_allocation: bool = False, init_function: Optional[Callable] = None, ) -> Union[InteractiveStepExecutor, InteractiveExecutor]: + if resource_dict is None: + resource_dict = {}🧰 Tools
🪛 Ruff (0.8.2)
200-200: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
🧹 Nitpick comments (7)
executorlib/interfaces/slurm.py (3)
75-75: Usemax_coresparameter in the example code.The example should use
max_coresinstead ofmax_workersto align with the recommended parameter usage.->>> with SlurmClusterExecutor(max_workers=2, init_function=init_k) as p: +>>> with SlurmClusterExecutor(max_cores=2, init_function=init_k) as p:
210-214: Update references in the docstring.Replace references to
executorlib.ExecutorwithSlurmJobExecutorto reflect the new class structure.
262-262: Usemax_coresparameter in the example code.The example should use
max_coresinstead ofmax_workersto align with the recommended parameter usage.->>> with SlurmJobExecutor(max_workers=2, init_function=init_k) as p: +>>> with SlurmJobExecutor(max_cores=2, init_function=init_k) as p:executorlib/interfaces/single.py (1)
214-214: Use Direct Membership Check Instead of.keys()In Python, checking membership via
key in dictionaryis both more concise and idiomatic thankey in dictionary.keys().- if "threads_per_core" in resource_dict.keys(): + if "threads_per_core" in resource_dict: del resource_dict["threads_per_core"] - if "gpus_per_core" in resource_dict.keys(): + if "gpus_per_core" in resource_dict: del resource_dict["gpus_per_core"] - if "slurm_cmd_args" in resource_dict.keys(): + if "slurm_cmd_args" in resource_dict: del resource_dict["slurm_cmd_args"]Also applies to: 216-216, 218-218
🧰 Tools
🪛 Ruff (0.8.2)
214-214: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
executorlib/interfaces/flux.py (3)
85-85: Use max_cores instead of max_workers in example code.The docstring recommends using
max_coresovermax_workers, but the example usesmax_workers.->>> with FluxJobExecutor(max_workers=2, init_function=init_k) as p: +>>> with FluxJobExecutor(max_cores=2, init_function=init_k) as p:
283-283: Use max_cores instead of max_workers in example code.The docstring recommends using
max_coresovermax_workers, but the example usesmax_workers.->>> with FluxClusterExecutor(max_workers=2, init_function=init_k) as p: +>>> with FluxClusterExecutor(max_cores=2, init_function=init_k) as p:
442-445: Simplify dictionary key checks.Use
key in dictinstead ofkey in dict.keys()for better readability and performance.- 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"]🧰 Tools
🪛 Ruff (0.8.2)
442-442: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
444-444: 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 (3)
executorlib/interfaces/flux.py(1 hunks)executorlib/interfaces/single.py(1 hunks)executorlib/interfaces/slurm.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/interfaces/single.py
200-200: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
214-214: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
216-216: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
218-218: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
executorlib/interfaces/slurm.py
389-389: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
executorlib/interfaces/flux.py
424-424: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
442-442: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
444-444: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: build (ubuntu-latest, 3.10)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (ubuntu-latest, 3.11)
- GitHub Check: build
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: build (ubuntu-latest, 3.12)
- GitHub Check: build
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13)
- GitHub Check: build
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build (macos-latest, 3.13)
🔇 Additional comments (4)
executorlib/interfaces/slurm.py (1)
22-26: Update references in the docstring.Replace references to
executorlib.ExecutorwithSlurmClusterExecutorto reflect the new class structure.executorlib/interfaces/single.py (1)
19-78: Impressive and Thorough Class DocumentationYour docstring clearly outlines the purpose and usage of
SingleNodeExecutorand gracefully describes each parameter, providing users with excellent guidance. Great job!executorlib/interfaces/flux.py (2)
424-424: 🛠️ Refactor suggestionReplace mutable default argument with None.
Using mutable default arguments can lead to unexpected behavior.
-def create_flux_executor( - max_workers: Optional[int] = None, - max_cores: Optional[int] = None, - cache_directory: Optional[str] = None, - resource_dict: dict = {}, +def create_flux_executor( + max_workers: Optional[int] = None, + max_cores: Optional[int] = None, + cache_directory: Optional[str] = None, + resource_dict: Optional[dict] = None,Likely invalid or redundant comment.
🧰 Tools
🪛 Ruff (0.8.2)
424-424: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
29-33: 🛠️ Refactor suggestionUpdate docstring to reflect the new specialized class architecture.
The docstring still references the old
executorlib.Executor. Update it to describeFluxJobExecutorspecifically.- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or - preferable the flux framework for distributing python functions within a given resource allocation. In contrast to - the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not - require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly - in an interactive Jupyter notebook. + The FluxJobExecutor leverages the Flux framework for distributing Python functions within a given resource allocation. + In contrast to the mpi4py.futures.MPIPoolExecutor, it can be executed in a serial Python process and does not + require the script to be executed with MPI. It is even possible to execute it directly in an interactive Jupyter notebook.Likely invalid or redundant comment.
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | ||
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | ||
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | ||
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | ||
| in an interactive Jupyter notebook. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update docstring to reflect the new specialized class architecture.
The docstring still references the old executorlib.Executor. Update it to describe FluxClusterExecutor specifically.
- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or
- preferable the flux framework for distributing python functions within a given resource allocation. In contrast to
- the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not
- require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly
- in an interactive Jupyter notebook.
+ The FluxClusterExecutor leverages the Flux framework for distributing Python functions across a cluster. In contrast to
+ the mpi4py.futures.MPIPoolExecutor, it can be executed in a serial Python process and does not require the script to be
+ executed with MPI. It is even possible to execute it directly in an interactive Jupyter notebook.📝 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.
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | |
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | |
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | |
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | |
| in an interactive Jupyter notebook. | |
| The FluxClusterExecutor leverages the Flux framework for distributing Python functions across a cluster. In contrast to | |
| the mpi4py.futures.MPIPoolExecutor, it can be executed in a serial Python process and does not require the script to be | |
| executed with MPI. It is even possible to execute it directly in an interactive Jupyter notebook. |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
executorlib/interfaces/single.py (2)
19-26: Update docstring to reflect the new specializedSingleNodeExecutorclass.References to
executorlib.Executorcan cause confusion since it has been replaced by specialized executors such asSingleNodeExecutor.- The executorlib.Executor leverages either the message passing interface (MPI), ... + The SingleNodeExecutor leverages either the message passing interface (MPI), ...
248-248: Favorkey in dictoverkey in dict.keys().This is a minor optimization and more idiomatic in Python.
-if "threads_per_core" in resource_dict.keys(): +if "threads_per_core" in resource_dict: -if "gpus_per_core" in resource_dict.keys(): +if "gpus_per_core" in resource_dict: -if "slurm_cmd_args" in resource_dict.keys(): +if "slurm_cmd_args" in resource_dict:Also applies to: 250-250, 252-252
🧰 Tools
🪛 Ruff (0.8.2)
248-248: Use
key in dictinstead ofkey in dict.keys()Remove
.keys()(SIM118)
executorlib/interfaces/flux.py (1)
480-480: Usekey in dictinstead ofkey in dict.keys().Simplifies the code and is more idiomatic in Python.
-if "openmpi_oversubscribe" in resource_dict.keys(): +if "openmpi_oversubscribe" in resource_dict: -if "slurm_cmd_args" in resource_dict.keys(): +if "slurm_cmd_args" in resource_dict:Also applies to: 482-482
🧰 Tools
🪛 Ruff (0.8.2)
480-480: 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 (3)
executorlib/interfaces/flux.py(1 hunks)executorlib/interfaces/single.py(1 hunks)executorlib/interfaces/slurm.py(1 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
executorlib/interfaces/slurm.py
389-389: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
executorlib/interfaces/single.py
200-200: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
248-248: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
250-250: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
252-252: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
executorlib/interfaces/flux.py
424-424: Do not use mutable data structures for argument defaults
Replace with None; initialize within function
(B006)
480-480: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
482-482: Use key in dict instead of key in dict.keys()
Remove .keys()
(SIM118)
⏰ 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, .ci_support/environment-openmpi.yml)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build
🔇 Additional comments (6)
executorlib/interfaces/slurm.py (2)
22-26: Remove references to the oldexecutorlib.Executor.This docstring still references the removed generic
Executorclass. Please update it to mentionSlurmClusterExecutororSlurmJobExecutorinstead for consistency with the new architecture.
389-389: Avoid using a mutable default argument forresource_dict.Same as a previously suggested fix: use a
Nonedefault and initialize an empty dictionary within the function body. This prevents inadvertent sharing of state between function calls.🧰 Tools
🪛 Ruff (0.8.2)
389-389: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
executorlib/interfaces/single.py (1)
200-200: UseNoneinstead of{}for default parameter.This avoids potential bugs from inadvertently shared state.
🧰 Tools
🪛 Ruff (0.8.2)
200-200: Do not use mutable data structures for argument defaults
Replace with
None; initialize within function(B006)
executorlib/interfaces/flux.py (3)
29-33: Update docstring references fromexecutorlib.Executorto the new specialized class.Retain clarity by referencing
FluxJobExecutoror other specialized executors directly.
230-234: Update docstring references fromexecutorlib.ExecutortoFluxClusterExecutor.The legacy
executorlib.Executormentions should be replaced for clarity.
424-424: Avoid mutable defaults forresource_dict.Pass
Noneas the default and assign{}inside the function body to prevent unexpected state sharing.🧰 Tools
🪛 Ruff (0.8.2)
424-424: 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: 0
🧹 Nitpick comments (2)
tests/test_dependencies_executor.py (2)
76-87: Consider simplifying the executor initialization.The initialization of
SingleNodeExecutorhas many default parameters that could be simplified using a helper function or default values.Consider creating a helper function for test initialization:
def create_test_executor(max_workers=1, max_cores=2): return create_single_node_executor( max_workers=max_workers, max_cores=max_cores, resource_dict={ "cores": 1, "threads_per_core": 1, "gpus_per_core": 0, "cwd": None, "openmpi_oversubscribe": False, "slurm_cmd_args": [], }, )
134-158: Refactor similar test methods using parameterized tests.The test methods in
TestExecutorErrorshave similar structure and could be refactored using parameterized tests to reduce code duplication.Consider using
@pytest.mark.parametrizeorunittest.TestCase.subTest:def test_block_allocation(self): test_cases = [ {"max_cores": 1, "block_allocation": False}, {"max_cores": 1, "block_allocation": True}, {"max_cores": 2, "block_allocation": False}, {"max_cores": 2, "block_allocation": True}, ] for case in test_cases: with self.subTest(**case): with self.assertRaises(RuntimeError): with SingleNodeExecutor(**case) as exe: cloudpickle_register(ind=1) _ = exe.submit(raise_error)🧰 Tools
🪛 Ruff (0.8.2)
136-137: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
142-143: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
148-149: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
154-155: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/test_dependencies_executor.py(5 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_dependencies_executor.py
142-143: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
148-149: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
154-155: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
🔇 Additional comments (1)
tests/test_dependencies_executor.py (1)
6-7: LGTM! Import changes align with PR objectives.The import changes correctly reflect the transition from the generic
Executorto the specializedSingleNodeExecutorclass, which aligns with the PR's goal of providing distinct executor classes for each backend.
# Conflicts: # executorlib/interactive/create.py
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
executorlib/interfaces/single.py (1)
250-256: Optimize resource dictionary cleanup.Consider using a more efficient dictionary comprehension to remove multiple keys at once.
Apply this diff:
- if "threads_per_core" in resource_dict: - del resource_dict["threads_per_core"] - if "gpus_per_core" in resource_dict: - del resource_dict["gpus_per_core"] - if "slurm_cmd_args" in resource_dict: - del resource_dict["slurm_cmd_args"] + keys_to_remove = ["threads_per_core", "gpus_per_core", "slurm_cmd_args"] + resource_dict = {k: v for k, v in resource_dict.items() if k not in keys_to_remove}executorlib/interfaces/flux.py (2)
85-85: Use recommended parameter in example code.The docstring recommends using
max_coresovermax_workers, but the example usesmax_workers.->>> with FluxJobExecutor(max_workers=2, init_function=init_k) as p: +>>> with FluxJobExecutor(max_cores=2, init_function=init_k) as p:
283-283: Use recommended parameter in example code.The docstring recommends using
max_coresovermax_workers, but the example usesmax_workers.->>> with FluxClusterExecutor(max_workers=2, init_function=init_k) as p: +>>> with FluxClusterExecutor(max_cores=2, init_function=init_k) as p:
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
executorlib/interfaces/flux.py(1 hunks)executorlib/interfaces/single.py(1 hunks)executorlib/interfaces/slurm.py(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- executorlib/interfaces/slurm.py
⏰ Context from checks skipped due to timeout of 90000ms (12)
- 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.13)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: build (macos-latest, 3.13)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: build (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
- GitHub Check: build (windows-latest, 3.13)
- GitHub Check: build
🔇 Additional comments (4)
executorlib/interfaces/single.py (2)
153-193: LGTM! Well-structured implementation with proper error handling.The implementation:
- Safely handles resource dictionary defaults
- Properly manages dependencies
- Includes appropriate validation checks
257-274: LGTM! Clean implementation of executor creation logic.The implementation properly:
- Handles block allocation configuration
- Validates core counts
- Sets up appropriate executor types
executorlib/interfaces/flux.py (2)
420-515: LGTM! Well-implemented factory function.The function has comprehensive input validation, proper error handling, and correct resource dictionary management. The conditional return of different executor types based on block_allocation is a clean implementation.
230-234: 🛠️ Refactor suggestionUpdate docstring to reflect the specialized cluster executor.
The docstring still references the old
executorlib.Executor. Update it to describeFluxClusterExecutorspecifically.- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or - preferable the flux framework for distributing python functions within a given resource allocation. In contrast to - the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not - require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly - in an interactive Jupyter notebook. + The FluxClusterExecutor leverages the Flux framework for distributing Python functions across a cluster. In contrast to + the mpi4py.futures.MPIPoolExecutor, it can be executed in a serial Python process and does not require the script to be + executed with MPI. It is even possible to execute it directly in an interactive Jupyter notebook.Likely invalid or redundant comment.
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | ||
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | ||
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | ||
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | ||
| in an interactive Jupyter notebook. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update docstring to reflect the new class structure.
The docstring still refers to executorlib.Executor which is being replaced. Update it to reference SingleNodeExecutor instead.
Apply this diff:
- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or
- preferable the flux framework for distributing python functions within a given resource allocation. In contrast to
- the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not
- require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly
- in an interactive Jupyter notebook.
+ The SingleNodeExecutor leverages either the message passing interface (MPI), the SLURM workload manager or
+ preferable the flux framework for distributing python functions within a given resource allocation. In contrast to
+ the mpi4py.futures.MPIPoolExecutor the SingleNodeExecutor can be executed in a serial python process and does not
+ require the python script to be executed with MPI. It is even possible to execute the SingleNodeExecutor directly
+ in an interactive Jupyter notebook.📝 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.
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | |
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | |
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | |
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | |
| in an interactive Jupyter notebook. | |
| The SingleNodeExecutor leverages either the message passing interface (MPI), the SLURM workload manager or | |
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | |
| the mpi4py.futures.MPIPoolExecutor the SingleNodeExecutor can be executed in a serial python process and does not | |
| require the python script to be executed with MPI. It is even possible to execute the SingleNodeExecutor directly | |
| in an interactive Jupyter notebook. |
| Instead of returning a executorlib.Executor object this function returns either a executorlib.mpi.PyMPIExecutor, | ||
| executorlib.slurm.PySlurmExecutor or executorlib.flux.PyFluxExecutor depending on which backend is available. The | ||
| executorlib.flux.PyFluxExecutor is the preferred choice while the executorlib.mpi.PyMPIExecutor is primarily used | ||
| for development and testing. The executorlib.flux.PyFluxExecutor requires flux-core from the flux-framework to be | ||
| installed and in addition flux-sched to enable GPU scheduling. Finally, the executorlib.slurm.PySlurmExecutor | ||
| requires the SLURM workload manager to be installed on the system. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update new docstring to reflect the new class structure.
The docstring still refers to the old executor classes. Update it to reflect the new specialized executor structure.
Apply this diff:
- Instead of returning a executorlib.Executor object this function returns either a executorlib.mpi.PyMPIExecutor,
- executorlib.slurm.PySlurmExecutor or executorlib.flux.PyFluxExecutor depending on which backend is available. The
- executorlib.flux.PyFluxExecutor is the preferred choice while the executorlib.mpi.PyMPIExecutor is primarily used
- for development and testing. The executorlib.flux.PyFluxExecutor requires flux-core from the flux-framework to be
- installed and in addition flux-sched to enable GPU scheduling. Finally, the executorlib.slurm.PySlurmExecutor
- requires the SLURM workload manager to be installed on the system.
+ Returns either an InteractiveStepExecutor or InteractiveExecutor with dependencies handling based on the
+ configuration. The implementation leverages MPI for development and testing, with Flux being the preferred
+ choice for production use. Using Flux requires flux-core from the flux-framework to be installed, and
+ flux-sched for GPU scheduling. SLURM support requires the SLURM workload manager to be installed.📝 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.
| Instead of returning a executorlib.Executor object this function returns either a executorlib.mpi.PyMPIExecutor, | |
| executorlib.slurm.PySlurmExecutor or executorlib.flux.PyFluxExecutor depending on which backend is available. The | |
| executorlib.flux.PyFluxExecutor is the preferred choice while the executorlib.mpi.PyMPIExecutor is primarily used | |
| for development and testing. The executorlib.flux.PyFluxExecutor requires flux-core from the flux-framework to be | |
| installed and in addition flux-sched to enable GPU scheduling. Finally, the executorlib.slurm.PySlurmExecutor | |
| requires the SLURM workload manager to be installed on the system. | |
| Returns either an InteractiveStepExecutor or InteractiveExecutor with dependencies handling based on the | |
| configuration. The implementation leverages MPI for development and testing, with Flux being the preferred | |
| choice for production use. Using Flux requires flux-core from the flux-framework to be installed, and | |
| flux-sched for GPU scheduling. SLURM support requires the SLURM workload manager to be installed. |
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | ||
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | ||
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | ||
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | ||
| in an interactive Jupyter notebook. |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Update docstring to reflect the specialized executor class.
The docstring still references the old executorlib.Executor. Update it to describe FluxJobExecutor specifically.
- The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or
- preferable the flux framework for distributing python functions within a given resource allocation. In contrast to
- the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not
- require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly
- in an interactive Jupyter notebook.
+ The FluxJobExecutor leverages the Flux framework for distributing Python functions within a given resource allocation.
+ In contrast to the mpi4py.futures.MPIPoolExecutor, it can be executed in a serial Python process and does not require
+ the script to be executed with MPI. It is even possible to execute it directly in an interactive Jupyter notebook.📝 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.
| The executorlib.Executor leverages either the message passing interface (MPI), the SLURM workload manager or | |
| preferable the flux framework for distributing python functions within a given resource allocation. In contrast to | |
| the mpi4py.futures.MPIPoolExecutor the executorlib.Executor can be executed in a serial python process and does not | |
| require the python script to be executed with MPI. It is even possible to execute the executorlib.Executor directly | |
| in an interactive Jupyter notebook. | |
| The FluxJobExecutor leverages the Flux framework for distributing Python functions within a given resource allocation. | |
| In contrast to the mpi4py.futures.MPIPoolExecutor, it can be executed in a serial Python process and does not require | |
| the script to be executed with MPI. It is even possible to execute it directly in an interactive Jupyter notebook. |
Replace the backend parameter from the initialization of the Executor class with individual executor classes for the individual backends:
Executor(backend="local")becomesfrom executorlib import SingleNodeExecutorExecutor(backend="flux_allocation")becomesfrom executorlib import FluxJobExecutorExecutor(backend="flux_submission")becomesfrom executorlib import FluxClusterExecutorExecutor(backend="slurm_allocation")becomesfrom executorlib import SlurmJobExecutorExecutor(backend="slurm_submission")becomesfrom executorlib import SlurmClusterExecutorThis has two advantages: On the one hand it is less error prone to mistyping the backend name, as the user can use auto completion to import the right module. On the other hand it is more consistent with the standard library which defines the
ProcessPoolExecutorand theThreadPoolExecutor, rather than abackendparameter.Summary by CodeRabbit
Summary by CodeRabbit
New Features
SingleNodeExecutor,SlurmJobExecutor,SlurmClusterExecutor,FluxJobExecutor, andFluxClusterExecutorfor improved task execution management.Refactor
Executorclass with dedicated implementations for clarity and streamlined configuration.Tests
FluxJobExecutorandFluxClusterExecutorfocusing on dependency management.SingleNodeExecutorto ensure robust functionality.