Delete SubprocessExecutor and ShellExecutor #440
Conversation
The aim is to unify the interface, the unit tests were updated to demonstrate how both can be realized with the existing general Executor class
for more information, see https://pre-commit.ci
WalkthroughThe changes in this pull request involve the removal of the Changes
Possibly related issues
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: 3
🧹 Outside diff range and nitpick comments (3)
executorlib/__init__.py (2)
Line range hint
24-24: Add return type hints to class methods.The
__new__method should specify its return type for better type safety and IDE support.def __new__( cls, max_workers: int = 1, - backend: str = "local", + backend: str = "local", ) -> "ExecutorWithDependencies | Any":Also applies to: 142-142
Line range hint
142-207: Consider optimizing parameter validation.The
__new__method performs parameter validation only whendisable_dependencies=True. Consider moving common parameter validations (likemax_workers,cores_per_worker, etc.) before the conditional branch to ensure consistent validation regardless of the dependency setting.Example refactor:
def __new__(...): + # Common parameter validation + _check_max_workers(max_workers) + _check_cores(max_cores, cores_per_worker) + if not disable_dependencies: return ExecutorWithDependencies(...) else: _check_plot_dependency_graph(plot_dependency_graph=plot_dependency_graph) _check_refresh_rate(refresh_rate=refresh_rate) return create_executor(...)tests/test_shell_interactive.py (1)
21-21: Consider usingtext=Trueinstead ofuniversal_newlines=Trueinsubprocess.PopenIn Python 3.7 and above,
text=Trueis preferred overuniversal_newlines=Truefor improved clarity.Apply this diff:
stdout=subprocess.PIPE, - universal_newlines=True, + text=True, shell=False,
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (6)
- executorlib/init.py (1 hunks)
- executorlib/shell/init.py (0 hunks)
- executorlib/shell/executor.py (0 hunks)
- executorlib/shell/interactive.py (0 hunks)
- tests/test_shell_executor.py (2 hunks)
- tests/test_shell_interactive.py (1 hunks)
💤 Files with no reviewable changes (3)
- executorlib/shell/init.py
- executorlib/shell/executor.py
- executorlib/shell/interactive.py
🧰 Additional context used
🪛 Ruff
tests/test_shell_interactive.py
36-39: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
🔇 Additional comments (8)
tests/test_shell_executor.py (8)
6-8: Import statements updated correctlyThe imports have been updated to reflect the removal of
SubprocessExecutorandShellExecutor. The necessary classes and functions are now imported fromexecutorliband its submodules.
25-31: Test updated to use the genericExecutorThe
test_execute_single_taskfunction has been updated to use the generalExecutorclass and the newsubmit_shell_commandfunction. This aligns with the PR objective to unify executor interfaces.
46-54: Correct exception handling for invalid keyword argumentsIn
test_wrong_error, the test now expects aTypeErrorwhen incorrect keyword arguments are provided. This is appropriate, as passing unexpected keyword arguments should raise aTypeError.
68-71: Proper testing of non-existent executablesThe
test_broken_executablefunction correctly tests the scenario where an invalid executable path is provided, and it expects aFileNotFoundError. This ensures that the code handles such errors gracefully.
83-89: Consistent use ofExecutorwith shell command submissionIn
test_shell_static_executor_args, the test appropriately uses theExecutorclass with thesubmit_shell_commandfunction. The parameters passed are consistent with the function definition.
95-101: Verification of binary output whenuniversal_newlines=FalseThe
test_shell_static_executor_binarytest checks that whenuniversal_newlinesis set toFalse, the output is in bytes. This correctly verifies the behavior of thesubmit_shell_commandfunction under different settings.
107-110: Inconsistent type usage withcommandparameterIn
test_shell_static_executor_shell, a string is passed tosubmit_shell_commandwithshell=True, but the function expectscommandto be alist. This inconsistency can be resolved by updating the type annotation as previously suggested.
116-128: Efficient parallel task execution usingExecutorThe
test_shell_executorfunction demonstrates the execution of multiple shell commands in parallel using theExecutorclass. The implementation is correct and effectively tests the executor's ability to handle concurrent tasks.
| SubprocessExecutor, | ||
| ShellExecutor, | ||
| ] | ||
| __all__ = [] |
There was a problem hiding this comment.
💡 Codebase verification
Based on the search results, I can now generate the final response:
Breaking change in public API requires documentation updates
The empty __all__ list removes SubprocessExecutor and ShellExecutor from the public API. The search results show these classes are extensively documented in docs/development.md with examples and usage patterns, but this documentation is now outdated.
- Update
docs/development.mdto reflect the removal ofSubprocessExecutorandShellExecutor - Add migration guide explaining how to transition from the removed executors to the new unified
Executorclass - Consider adding a deprecation notice in the documentation
🔗 Analysis chain
Document breaking changes in public API.
The empty __all__ list removes previously exported classes (SubprocessExecutor and ShellExecutor) from the public API. This is a breaking change that could affect existing code.
Let's verify the impact:
Consider:
- Adding migration guide in documentation
- Providing deprecation warnings in previous version
- Updating version number to reflect breaking change (major version bump)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for usage of removed executors in the codebase
echo "Searching for SubprocessExecutor usage..."
rg "SubprocessExecutor"
echo "Searching for ShellExecutor usage..."
rg "ShellExecutor"
Length of output: 2282
| if stop_read_pattern is not None and stop_read_pattern in output_current: | ||
| break | ||
| elif lines_to_read is not None and lines_to_read == lines_count: | ||
| break |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Simplify conditional statements by combining 'if' branches using logical or operator
Combining the if and elif conditions into a single if statement enhances readability and reduces code complexity.
Apply this diff to combine the conditions:
while True:
output_current = process.stdout.readline()
output += output_current
lines_count += 1
- if stop_read_pattern is not None and stop_read_pattern in output_current:
- break
- elif lines_to_read is not None and lines_to_read == lines_count:
+ if (stop_read_pattern is not None and stop_read_pattern in output_current) or \
+ (lines_to_read is not None and lines_to_read == lines_count):
break📝 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.
| if stop_read_pattern is not None and stop_read_pattern in output_current: | |
| break | |
| elif lines_to_read is not None and lines_to_read == lines_count: | |
| break | |
| if (stop_read_pattern is not None and stop_read_pattern in output_current) or \ | |
| (lines_to_read is not None and lines_to_read == lines_count): | |
| break |
🧰 Tools
🪛 Ruff
36-39: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
| def submit_shell_command( | ||
| command: list, universal_newlines: bool = True, shell: bool = False | ||
| ): | ||
| return subprocess.check_output( | ||
| command, universal_newlines=universal_newlines, shell=shell | ||
| ) |
There was a problem hiding this comment.
Update type annotation for command parameter
The submit_shell_command function currently annotates the command parameter as a list, but in some test cases, a string is passed when shell=True. To accommodate both strings and lists as valid input types for command, consider updating the type annotation.
Apply this diff to modify the type annotation:
+from typing import Union, List
-def submit_shell_command(
- command: list, universal_newlines: bool = True, shell: bool = False
+def submit_shell_command(
+ command: Union[List[str], str], universal_newlines: bool = True, shell: bool = False
):
return subprocess.check_output(
command, universal_newlines=universal_newlines, shell=shell
)This change ensures that the function accepts both list and str types for the command parameter, aligning with how subprocess.check_output handles input when shell=True.
📝 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 submit_shell_command( | |
| command: list, universal_newlines: bool = True, shell: bool = False | |
| ): | |
| return subprocess.check_output( | |
| command, universal_newlines=universal_newlines, shell=shell | |
| ) | |
| from typing import Union, List | |
| def submit_shell_command( | |
| command: Union[List[str], str], universal_newlines: bool = True, shell: bool = False | |
| ): | |
| return subprocess.check_output( | |
| command, universal_newlines=universal_newlines, shell=shell | |
| ) |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
tests/test_shell_executor.py (2)
11-16: Consider adding error handling to the helper function.The
submit_shell_commandhelper function could benefit from basic error handling to provide more meaningful error messages when command execution fails.def submit_shell_command( command: list, universal_newlines: bool = True, shell: bool = False ): + try: return subprocess.check_output( command, universal_newlines=universal_newlines, shell=shell ) + except subprocess.CalledProcessError as e: + raise RuntimeError(f"Command failed with exit code {e.returncode}: {e.output}")
Line range hint
1-146: Well-structured transition to unified executor interface.The test suite successfully demonstrates how the functionality previously provided by
SubprocessExecutorcan be achieved using the genericExecutorclass. The changes maintain test coverage while simplifying the overall architecture.A few suggestions to consider:
- Add docstrings to document the new execution patterns
- Consider creating shared test utilities for common executor setup code
- Add test cases for concurrent execution with different worker counts
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/test_shell_executor.py (2 hunks)
🔇 Additional comments (3)
tests/test_shell_executor.py (3)
6-8: LGTM: Import changes align with PR objectives.The transition from specific executor imports to the generic
Executorand related utilities aligns well with the PR's goal of unifying the executor interface.
55-61: LGTM: Improved error handling.The change from
KeyErrortoTypeErrorfor invalid kwargs is more appropriate as it better reflects the nature of the error (type mismatch rather than missing key).
34-39: Verify parallel execution configuration.The transition to
execute_parallel_tasksintroduces new parameters. Ensure thatcores=1withopenmpi_oversubscribe=Falseprovides equivalent behavior to the previous sequential execution.✅ Verification successful
The parallel execution configuration is correctly implemented.
The code review concern can be resolved. The
execute_parallel_tasksfunction is consistently used across test files withcores=1andopenmpi_oversubscribe=Falsefor sequential execution testing. This configuration ensures:
- Single core execution (
cores=1) which maintains sequential behavior- No oversubscription, preventing resource conflicts
- Consistent usage pattern across all test files (test_shell_executor.py, test_shell_interactive.py, test_local_executor.py)
The implementation in the executor.py shows this is the intended way to run sequential tasks through the parallel execution framework.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for other test files using similar configuration to verify consistency rg -l 'execute_parallel_tasks.*cores.*openmpi_oversubscribe'Length of output: 62
Script:
#!/bin/bash # Let's search for all occurrences of execute_parallel_tasks to understand its usage rg -A 5 "execute_parallel_tasks" # Also search for the function definition to understand expected parameters ast-grep --pattern 'def execute_parallel_tasks($$$)'Length of output: 7108
Script:
#!/bin/bash # Let's look at the function definition and its parameters in more detail cat executorlib/shared/executor.pyLength of output: 28333
| with Executor(max_workers=2) as exe: | ||
| cloudpickle_register(ind=1) | ||
| f_1 = exe.submit( | ||
| submit_shell_command, ["echo", "test_1"], universal_newlines=True | ||
| ) | ||
| f_2 = exe.submit( | ||
| submit_shell_command, ["echo", "test_2"], universal_newlines=True | ||
| ) | ||
| f_3 = exe.submit( | ||
| submit_shell_command, ["echo", "test_3"], universal_newlines=True | ||
| ) | ||
| f_4 = exe.submit( | ||
| submit_shell_command, ["echo", "test_4"], universal_newlines=True | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider parameterizing the parallel execution test.
The parallel execution test could be made more maintainable using parameterized testing. This would make it easier to add more test cases and reduce code duplication.
@pytest.mark.parametrize("test_input,expected", [
("test_1", "test_1\n"),
("test_2", "test_2\n"),
("test_3", "test_3\n"),
("test_4", "test_4\n"),
])
def test_shell_executor_parameterized(test_input, expected):
with Executor(max_workers=2) as exe:
cloudpickle_register(ind=1)
future = exe.submit(
submit_shell_command,
["echo", test_input],
universal_newlines=True
)
assert not future.done()
assert future.result() == expected
assert future.done()
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/test_shell_interactive.py (1)
48-102: Consider adding error case tests.The test_execute_single_task method effectively tests the happy path, but consider adding tests for error scenarios such as:
- Invalid input
- Process termination before shutdown
- Pattern not found
- Zero or negative line count
Would you like me to help generate additional test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/test_shell_interactive.py (1 hunks)
🧰 Additional context used
🪛 Ruff
tests/test_shell_interactive.py
36-39: Combine
ifbranches using logicaloroperatorCombine
ifbranches(SIM114)
🔇 Additional comments (4)
tests/test_shell_interactive.py (4)
1-13: LGTM! Import changes align with PR objectives.The imports have been correctly updated to reflect the transition from specialized executors to the generic Executor class.
15-24: LGTM! Well-structured process initialization.The init_process function correctly configures the subprocess with appropriate parameters and returns the expected dictionary format.
43-46: LGTM! Clean shutdown implementation.The shutdown function properly handles process termination by writing and flushing the shutdown command.
104-127: LGTM! Well-structured executor context usage.The test demonstrates proper usage of the Executor context manager and appropriate verification of task completion. The shutdown handling ensures clean test cleanup.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
tests/test_shell_executor.py (1)
85-92: LGTM! Comprehensive coverage of execution modes.The tests effectively demonstrate how the general Executor class replaces ShellExecutor functionality across different execution modes (args, binary, shell).
Consider adding docstrings to explain the purpose of each test case and what specific ShellExecutor functionality it replaces.
Also applies to: 98-105, 111-115
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- tests/test_shell_executor.py (2 hunks)
🔇 Additional comments (5)
tests/test_shell_executor.py (5)
6-8: LGTM! Import changes align with PR objective.The changes correctly replace SubprocessExecutor imports with the general Executor class and related utilities, which aligns with the PR's goal of unifying the executor interface.
77-82: LGTM! Error handling test properly adapted.The test maintains its purpose of verifying FileNotFoundError while properly adapting to the new execution framework.
121-134: LGTM! Effective demonstration of parallel execution.The test successfully demonstrates how the general Executor handles parallel task execution, properly replacing the parallel execution capabilities of the removed executors.
56-62: Verify error type change alignment with Executor implementation.The test now expects TypeError instead of KeyError for invalid arguments. Let's verify this aligns with the new Executor implementation.
#!/bin/bash # Search for error handling in Executor implementation rg -A 5 "raise TypeError" rg -A 5 "raise KeyError"
34-39: Consider using a simpler execution method for single task test.The test is using
execute_parallel_taskswith MPI configuration for a single task execution, which might be overengineered. Consider if a simpler execution method would be more appropriate for this test case.
The aim is to unify the interface, the unit tests were updated to demonstrate how both can be realized with the existing general Executor class
Summary by CodeRabbit
Release Notes
New Features
Executorclass for task execution.Bug Fixes
Documentation
Chores