Interfaces: Use Base Interface rather than factory pattern #634
Conversation
|
Warning Rate limit exceeded@jan-janssen has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 4 minutes and 14 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (14)
## Walkthrough
A new `ExecutorInterface` class was introduced as a standardized wrapper around executor implementations, delegating all operations to an internal executor instance. Classes for different executor types—such as Flux, Slurm, and Single Node—were refactored to inherit from `ExecutorInterface`. Their initialization logic was unified: instead of returning executor instances from `__new__`, each class now uses `super().__init__()` with the appropriate executor, optionally wrapped in a dependency manager. This change consolidates and standardizes executor interface design, with no modifications to the underlying executor logic.
## Changes
| File(s) | Change Summary |
|-----------------------------------------------------|-------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------|
| `executorlib/base/executor.py` | Added `ExecutorInterface` class, inheriting from `FutureExecutor`, acting as a proxy to an internal `ExecutorBase`. Implements property and method delegation, including resource cleanup in `__del__`. Updated `ExecutorBase.submit` method to enforce positional-only `fn` argument. |
| `executorlib/interfaces/flux.py` | Refactored `FluxJobExecutor` and `FluxClusterExecutor` to inherit from `ExecutorInterface`. Removed `__new__` methods; initialization now uses `super().__init__()` with the appropriate executor instance, handling dependency wrapping as needed. |
| `executorlib/interfaces/single.py` | Refactored `SingleNodeExecutor` to inherit from `ExecutorInterface`. Removed `__new__` method; initialization logic moved to `__init__`, calling `super().__init__()` with the correct executor instance, handling dependency wrapping and validation as required. |
| `executorlib/interfaces/slurm.py` | Refactored `SlurmClusterExecutor` and `SlurmJobExecutor` to inherit from `ExecutorInterface`. Removed `__new__` methods and empty `__init__` placeholders; initialization now uses `super().__init__()` with constructed executor, handling dependency wrapping and validation as appropriate. |
| `tests/test_singlenodeexecutor_plot_dependency.py` | Updated test attribute access to retrieve `_future_hash_dict` and `_task_hash_dict` via the internal `_task_scheduler` attribute instead of directly on the executor instance. |
| `tests/test_singlenodeexecutor_resize.py` | Updated test assertions to access the `_process` attribute through the internal `_task_scheduler` attribute rather than directly on the executor instance. |
| `tests/test_fluxjobexecutor_plot.py` | Updated test attribute access to retrieve `_future_hash_dict` and `_task_hash_dict` via the internal `_task_scheduler` attribute instead of directly on the executor instance. |
## Sequence Diagram(s)
```mermaid
sequenceDiagram
participant User
participant ExecutorInterface
participant InternalExecutor
User->>ExecutorInterface: create instance (with parameters)
ExecutorInterface->>InternalExecutor: initialize wrapped executor
User->>ExecutorInterface: submit(fn, *args, **kwargs)
ExecutorInterface->>InternalExecutor: submit(fn, *args, **kwargs)
InternalExecutor-->>ExecutorInterface: Future
ExecutorInterface-->>User: Future
User->>ExecutorInterface: shutdown()
ExecutorInterface->>InternalExecutor: shutdown()Poem
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
executorlib/base/executor.py (1)
191-193:ExecutorInterface.__init__should callsuper().__init__for forward-compatibility
concurrent.futures.Executorcurrently provides a no-op__init__, so skipping it works today.
However, if upstream ever adds bookkeeping logic (e.g. for thread tracking or weak-ref registration), this subclass will silently miss it.- def __init__(self, executor: ExecutorBase): - self._executor = executor + def __init__(self, executor: ExecutorBase): + super().__init__() + self._executor = executorexecutorlib/interfaces/flux.py (1)
168-189: Minor: duplicate resource-dict merge mutates caller’s input
resource_dict.update(...)mutates the user-supplied dictionary. Consider
working on a shallow copy to prevent surprising side-effects:- if resource_dict is None: - resource_dict = {} - resource_dict.update( + if resource_dict is None: + resource_dict = {} + resource_dict = resource_dict.copy() + resource_dict.update(executorlib/interfaces/slurm.py (3)
152-158: Avoid mutating caller-providedresource_dictin-place
resource_dict.update(…)modifies the dictionary object that the caller passed in.
If the same dict is reused elsewhere, the defaults inserted here may leak into unrelated code paths.- resource_dict.update({…}) + resource_dict = {**default_resource_dict, **resource_dict}This creates an internal copy and leaves the caller’s data untouched.
330-346: Duplicate dependency-wrapper logic – consider a helperBoth
SlurmClusterExecutorandSlurmJobExecutorreplicate almost identicalDependencyExecutor( create_slurm_executor(…))scaffolding. Extracting this into a private factory function would reduce duplication and keep the interface classes concise.Example sketch:
def _wrap_for_dependencies(exec_ctor, *, enable_deps, **kwargs): inner = exec_ctor(**kwargs) return DependencyExecutor(inner, **{k: kwargs[k] for k in ("max_cores", "refresh_rate", "plot_dependency_graph", "plot_dependency_graph_filename")}) \ if enable_deps else innerUsing such a helper will simplify future maintenance when the dependency-handling API changes.
347-349: Minor: consolidate validation helpers
check_plot_dependency_graphandcheck_refresh_rateare only needed in the branch where dependencies are disabled. Placing them inside theelsebranch is correct but makes the flow less obvious.A small guard clause at the top of the method could make intent clearer:
if disable_dependencies: check_plot_dependency_graph(plot_dependency_graph) check_refresh_rate(refresh_rate)No functional change, but improves readability.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
executorlib/base/executor.py(1 hunks)executorlib/interfaces/flux.py(5 hunks)executorlib/interfaces/single.py(4 hunks)executorlib/interfaces/slurm.py(5 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
executorlib/interfaces/flux.py (4)
executorlib/base/executor.py (5)
ExecutorInterface(183-287)max_workers(36-37)max_workers(40-41)max_workers(195-196)max_workers(199-200)executorlib/interactive/dependency.py (3)
DependencyExecutor(21-183)max_workers(86-94)max_workers(97-110)executorlib/standalone/inputcheck.py (2)
check_plot_dependency_graph(93-100)check_refresh_rate(83-90)executorlib/cache/executor.py (1)
create_file_executor(84-122)
executorlib/base/executor.py (5)
executorlib/interactive/blockallocation.py (4)
max_workers(74-75)max_workers(78-100)submit(102-138)shutdown(140-165)executorlib/interactive/dependency.py (4)
max_workers(86-94)max_workers(97-110)info(69-83)submit(112-154)tests/test_singlenodeexecutor_noblock.py (1)
resource_dict(11-12)executorlib/standalone/interactive/communication.py (1)
shutdown(90-110)executorlib/standalone/interactive/spawner.py (2)
shutdown(41-48)shutdown(114-126)
🪛 Ruff (0.8.2)
executorlib/base/executor.py
254-254: Star-arg unpacking after a keyword argument is strongly discouraged
(B026)
🔇 Additional comments (2)
executorlib/interfaces/single.py (1)
148-165: Good use ofExecutorInterface— branch handles dependencies cleanlyThe refactor cleanly wraps the single-node executor in
DependencyExecutor
while maintaining the original constructor signature. Nice work!executorlib/interfaces/slurm.py (1)
152-169:❌ Incorrect review comment
Validate
create_file_executorsignature – extra kwargs may break at runtime
create_file_executor(…)is called with several Flux-specific kwargs (flux_executor,flux_executor_pmi_mode,flux_executor_nesting,flux_log_files).
These parameters look unrelated to a SLURM file-submission backend and are not listed in the public signature in earlier releases. If the function does not accept them, aTypeErrorwill be raised the first time the constructor is executed.- flux_executor=None, - flux_executor_pmi_mode=None, - flux_executor_nesting=False, - flux_log_files=False,Please double-check the current signature of
create_file_executorand remove or gate these arguments to avoid accidental breakage.
🏁 Script executed:
#!/bin/bash # Locate the definition of create_file_executor and display context rg "def create_file_executor" -n -C5Length of output: 697
🏁 Script executed:
#!/bin/bash # Display the full signature of create_file_executor and its parameters sed -n '84,200p' executorlib/cache/executor.pyLength of output: 1704
No action needed:
create_file_executoralready accepts Flux-specific kwargsThe signature of
create_file_executor(executorlib/cache/executor.py, lines 84–99) defines and uses all of these parameters:
flux_executorflux_executor_pmi_modeflux_executor_nestingflux_log_filesThey’re validated internally before returning the
FileExecutor. Passing them for the SLURM backend won’t raise aTypeError, so no removal or gating is required.Likely an incorrect or invalid review comment.
executorlib/base/executor.py
Outdated
| def __del__(self): | ||
| """ | ||
| Clean-up the resources associated with the Executor. | ||
| """ | ||
| del self._executor |
There was a problem hiding this comment.
🛠️ Refactor suggestion
__del__ should call shutdown() to avoid dangling threads
Deleting the wrapped executor reference does not guarantee that its destructor
executes promptly; therefore worker threads or processes may survive the
interface object. Be explicit:
- def __del__(self):
- """
- Clean-up the resources associated with the Executor.
- """
- del self._executor
+ def __del__(self):
+ """
+ Ensure the wrapped executor is shut down before disposal.
+ """
+ with contextlib.suppress(Exception):
+ self._executor.shutdown(wait=False, cancel_futures=True)
+ del self._executor📝 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 __del__(self): | |
| """ | |
| Clean-up the resources associated with the Executor. | |
| """ | |
| del self._executor | |
| def __del__(self): | |
| """ | |
| Ensure the wrapped executor is shut down before disposal. | |
| """ | |
| with contextlib.suppress(Exception): | |
| self._executor.shutdown(wait=False, cancel_futures=True) | |
| del self._executor |
| else: | ||
| super().__init__( | ||
| executor=DependencyExecutor( | ||
| executor=create_slurm_executor( | ||
| max_workers=max_workers, | ||
| cache_directory=cache_directory, | ||
| max_cores=max_cores, | ||
| resource_dict=resource_dict, | ||
| hostname_localhost=hostname_localhost, | ||
| block_allocation=block_allocation, | ||
| init_function=init_function, | ||
| ), | ||
| max_cores=max_cores, | ||
| refresh_rate=refresh_rate, | ||
| plot_dependency_graph=plot_dependency_graph, | ||
| plot_dependency_graph_filename=plot_dependency_graph_filename, | ||
| ) | ||
| ) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
disable_dependencies is silently ignored when plot_dependency_graph=True
In the dependency-graph branch the inner create_slurm_executor is always wrapped in a DependencyExecutor, regardless of the user-supplied disable_dependencies flag.
A caller that explicitly sets disable_dependencies=True while requesting a graph will be surprised that dependencies are still resolved.
Consider honouring the flag, e.g.
- executor=DependencyExecutor(
+ executor=DependencyExecutor(
executor=create_slurm_executor(
@@
- )
+ ) if not disable_dependencies else create_slurm_executor(
+ …
+ ),
+ ) if not disable_dependencies else create_slurm_executor(
+ …
+ )or explicitly raise a ValueError when both options are used together.
🏁 Script executed:
#!/bin/bash
# Search for usages of the disable_dependencies flag across the repo
rg --color=never -n "disable_dependencies" -n .Length of output: 4974
🏁 Script executed:
#!/bin/bash
set -e
echo "Plot dependency graph occurrences in slurm.py:"
rg --color=never -n "plot_dependency_graph" executorlib/interfaces/slurm.py || true
echo ""
echo "Context around SLURM executor __init__ (lines 120-200):"
sed -n '120,200p' executorlib/interfaces/slurm.pyLength of output: 6432
🏁 Script executed:
#!/bin/bash
# Locate the definition of create_slurm_executor in the slurm interface module
rg --color=never -n "create_slurm_executor" -n executorlib/interfaces/slurm.pyLength of output: 273
🏁 Script executed:
#!/bin/bash
# Display the definition of create_slurm_executor to inspect its parameters
sed -n '350,420p' executorlib/interfaces/slurm.pyLength of output: 4590
Honor disable_dependencies when plot_dependency_graph=True
Currently, in SlurmExecutor.__init__ (executorlib/interfaces/slurm.py, lines 170–187), the disable_dependencies flag is ignored whenever plot_dependency_graph=True, because we unconditionally wrap the SLURM executor in a DependencyExecutor. This means a caller who sets
disable_dependencies=True, plot_dependency_graph=Truewill still have their futures resolved, violating the intent of disable_dependencies.
Please update the else branch to either:
-
Bypass
DependencyExecutorwhendisable_dependencies=True, e.g.:else: - super().__init__( - executor=DependencyExecutor( - executor=create_slurm_executor(...), - max_cores=max_cores, - ... - ) - ) + if disable_dependencies: + super().__init__( + executor=create_slurm_executor( + max_workers=max_workers, + cache_directory=cache_directory, + max_cores=max_cores, + resource_dict=resource_dict, + hostname_localhost=hostname_localhost, + block_allocation=block_allocation, + init_function=init_function, + ) + ) + else: + super().__init__( + executor=DependencyExecutor( + executor=create_slurm_executor( + max_workers=max_workers, + cache_directory=cache_directory, + max_cores=max_cores, + resource_dict=resource_dict, + hostname_localhost=hostname_localhost, + block_allocation=block_allocation, + init_function=init_function, + ), + max_cores=max_cores, + refresh_rate=refresh_rate, + plot_dependency_graph=plot_dependency_graph, + plot_dependency_graph_filename=plot_dependency_graph_filename, + ) + )
-
Or explicitly forbid the combination:
if plot_dependency_graph and disable_dependencies: raise ValueError( "`disable_dependencies=True` cannot be used with `plot_dependency_graph=True`" )
This will prevent the silent mismatch between user intent and actual behavior.
jan-janssen
force-pushed
the
executorbaseinterface
branch
from
6f7ad18 to
5f66419
Compare
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #634 +/- ##
==========================================
+ Coverage 96.30% 96.65% +0.35%
==========================================
Files 28 28
Lines 1271 1287 +16
==========================================
+ Hits 1224 1244 +20
+ Misses 47 43 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Check out this pull request on See visual diffs & provide feedback on Jupyter Notebooks. Powered by ReviewNB |
Summary by CodeRabbit
New Features
Refactor