Fix block allocation with two or more workers hanging on failed function #532
Conversation
|
Caution Review failedThe pull request is closed. WalkthroughThis pull request introduces modifications to the executor library, focusing on enhancing error handling and shutdown mechanisms. The changes include adding a new test class to validate executor behavior under error conditions and updating the Changes
Sequence DiagramsequenceDiagram
participant Executor
participant Queue
participant Worker
Executor->>Queue: Submit tasks
Worker->>Queue: Retrieve tasks
alt Error Scenario
Worker-->>Executor: Raise RuntimeError
Executor->>Executor: Handle shutdown
alt queue_join_on_shutdown is True
Executor->>Queue: Join queue
else queue_join_on_shutdown is False
Executor->>Queue: Skip queue join
end
end
Possibly related PRs
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
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: 0
🧹 Nitpick comments (2)
tests/test_dependencies_executor.py (2)
43-45: Consider adding a docstring for clarity.The function serves its purpose well as a test helper, but a brief docstring would help document its intended use in tests.
def raise_error(): + """Helper function that raises a RuntimeError for testing error handling.""" raise RuntimeError
236-254: Improve test structure and reduce duplication.The test cases are well-organized but could benefit from some structural improvements:
- Combine nested
withstatements for better readability- Consider using parameterized tests to reduce code duplication
Here's how you could refactor this:
+ @unittest.expectedFailure + @unittest.parameterize.expand([ + ("one_worker_no_block", 1, False), + ("one_worker_block", 1, True), + ("two_workers_no_block", 2, False), + ]) + def test_executor_error(self, name, max_cores, block_allocation): + with self.assertRaises(RuntimeError), \ + Executor(max_cores=max_cores, + backend="local", + block_allocation=block_allocation) as exe: + cloudpickle_register(ind=1) + _ = exe.submit(raise_error) + - def test_block_allocation_false_one_worker(self): - with self.assertRaises(RuntimeError): - with Executor(max_cores=1, backend="local", 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 Executor(max_cores=1, backend="local", block_allocation=True) as exe: - cloudpickle_register(ind=1) - _ = exe.submit(raise_error) - - def test_block_allocation_false_two_workers(self): - with self.assertRaises(RuntimeError): - with Executor(max_cores=2, backend="local", block_allocation=False) as exe: - cloudpickle_register(ind=1) - _ = exe.submit(raise_error)🧰 Tools
🪛 Ruff (0.8.2)
238-239: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
244-245: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
250-251: 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(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_dependencies_executor.py
238-239: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
244-245: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
250-251: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🔇 Additional comments (1)
tests/test_dependencies_executor.py (1)
255-259: Clarify the status of the commented-out test case.
The test for block_allocation=True with two workers is commented out without explanation. This could indicate:
- An unresolved issue that needs attention
- Missing test coverage for this configuration
- A pending implementation
Please either:
- Implement the test if it's needed
- Remove if it's not applicable
- Add a comment explaining why it's disabled
Let's check if this configuration is tested elsewhere:
tests/test_dependencies_executor.py
Outdated
| # def test_block_allocation_true_two_workers(self): | ||
| # with self.assertRaises(RuntimeError): | ||
| # with Executor(max_cores=2, backend="local", block_allocation=True) as exe: | ||
| # cloudpickle_register(ind=1) | ||
| # _ = exe.submit(raise_error) |
There was a problem hiding this comment.
This tests hangs which makes debugging rather complex.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
executorlib/interactive/shared.py (2)
138-140: Consider makingqueue_join_on_shutdownconfigurable via constructor arguments.
Currently, this parameter is hard-coded toFalsein theInteractiveExecutorinitialization. If the use case requires varying queue shutdown semantics, making this parameter configurable at construction time (similar toexecute_parallel_tasks) would increase flexibility.
211-211: Method signature alignment recommendation.
Document the newly addedqueue_join_on_shutdownparameter in the docstring forexecute_parallel_tasksmore explicitly in terms of how it should align with theInteractiveExecutorusage. This will help maintain clarity if the two differ (i.e., one is forced toFalsewhile the other defaults toTrue).tests/test_dependencies_executor.py (2)
43-45: Add a descriptive docstring or comment forraise_error.
A small docstring that clarifies why we raiseRuntimeErrorwill help future contributors quickly discover this method’s usage in the error-handling tests.
237-242: Simplify nestedwithstatements.
Static analysis (SIM117) suggests combining these lines into a singlewithstatement for improved readability.-with self.assertRaises(RuntimeError): - with Executor(max_cores=1, backend="local", block_allocation=False) as exe: +with self.assertRaises(RuntimeError), \ + Executor(max_cores=1, backend="local", block_allocation=False) as exe: cloudpickle_register(ind=1) _ = exe.submit(raise_error)🧰 Tools
🪛 Ruff (0.8.2)
238-239: 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 (2)
executorlib/interactive/shared.py(4 hunks)tests/test_dependencies_executor.py(2 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_dependencies_executor.py
238-239: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
244-245: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
250-251: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
256-257: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
🔇 Additional comments (4)
executorlib/interactive/shared.py (1)
248-249: Use caution with queue joins on shutdown.
Joining the queue here before breaking might block indefinitely if there are hidden issues. Ensure that no additional tasks are put into the queue in shutdown scenarios or handle potential edge cases (e.g., timeouts or signals) to avoid deadlocks.
tests/test_dependencies_executor.py (3)
243-248: Same nested with statement pattern.
This code block repeats the pattern of nested with statements. Consider applying the same fix as above for consistency.
🧰 Tools
🪛 Ruff (0.8.2)
244-245: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
249-254: Same nested with statement pattern (two workers).
Again, consider merging the context managers for clarity.
🧰 Tools
🪛 Ruff (0.8.2)
250-251: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
Line range hint 255-260: Same nested with statement pattern (two workers, block_allocation=true).
As before, merging the with statements improves readability and consistency.
🧰 Tools
🪛 Ruff (0.8.2)
238-239: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
244-245: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
250-251: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
256-257: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
jan-janssen
force-pushed
the
more_error_tests
branch
from
3e3140b to
7759fb2
Compare
jan-janssen
changed the title
Summary by CodeRabbit
New Features
Bug Fixes