Debug error handling #577
Debug error handling #577
Conversation
WalkthroughThe changes enhance error handling in the executor framework. In Changes
Sequence Diagram(s)sequenceDiagram
participant TExec as execute_tasks_with_dependencies
participant Futures as Future List
participant ExHandler as _get_exception_lst
participant WTask as _submit_waiting_task
TExec->>Futures: Check states of futures
TExec->>ExHandler: Retrieve exceptions from futures
ExHandler->>Futures: Inspect each future for exceptions
ExHandler-->>TExec: Return list of exceptions
TExec->>TExec: Propagate exception if any found
Note over WTask, ExHandler: Similar exception handling flow is applied in _submit_waiting_task
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 (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #577 +/- ##
==========================================
- Coverage 95.91% 95.89% -0.03%
==========================================
Files 26 26
Lines 1176 1193 +17
==========================================
+ Hits 1128 1144 +16
- Misses 48 49 +1 ☔ View full report in Codecov by Sentry. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/test_dependencies_executor.py (2)
45-46: Document the unused parameter.The
parameterargument is not used in the function. Consider adding a docstring to explain its purpose or remove it if it's not needed.
276-322: Fix unused loop variables and LGTM!The tests thoroughly verify error handling in different block allocation modes and worker counts. However, there are unused loop variables.
Apply this diff to fix the unused loop variables:
- for i in range(1, 4): + for _i in range(1, 4):This change should be applied to all four test methods.
🧰 Tools
🪛 Ruff (0.8.2)
277-278: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
281-281: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
289-290: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
293-293: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
301-302: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
305-305: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
313-314: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
317-317: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
executorlib/interactive/shared.py(5 hunks)tests/test_dependencies_executor.py(3 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
tests/test_dependencies_executor.py
259-260: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
265-266: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
271-272: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
277-278: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
281-281: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
289-290: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
293-293: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
301-302: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
305-305: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
313-314: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
317-317: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
⏰ Context from checks skipped due to timeout of 90000ms (17)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.10)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.11)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.10)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.12)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.11)
- GitHub Check: unittest_openmpi (ubuntu-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.12)
- GitHub Check: unittest_win
- GitHub Check: unittest_openmpi (macos-latest, 3.13)
- GitHub Check: unittest_mpich (ubuntu-latest, 3.13)
- GitHub Check: unittest_old
- GitHub Check: unittest_mpich (macos-latest, 3.13)
- GitHub Check: unittest_flux_mpich
- GitHub Check: unittest_flux_openmpi
- GitHub Check: notebooks
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-mpich.yml)
- GitHub Check: benchmark (ubuntu-latest, 3.13, .ci_support/environment-openmpi.yml)
🔇 Additional comments (3)
tests/test_dependencies_executor.py (2)
109-160: LGTM! Good test coverage for error propagation.The test thoroughly verifies error handling in dependency steps by:
- Checking that both futures have exceptions.
- Verifying that the dependent task (fs2) raises the expected RuntimeError.
162-212: LGTM! Good test coverage for pre-set errors.The test thoroughly verifies error handling when a future is pre-set with an exception by:
- Setting an exception on fs1 before execution.
- Checking that both futures have exceptions.
- Verifying that the dependent task (fs2) raises the expected RuntimeError.
executorlib/interactive/shared.py (1)
311-311: LGTM! Good optimization for process joining.The change ensures that only active processes are joined, which is more efficient than trying to join inactive processes.
executorlib/interactive/shared.py
Outdated
| def _get_exception_lst(future_lst: list) -> list: | ||
| def get_exception(future_obj: Future) -> bool: | ||
| try: | ||
| excp = future_obj.exception(timeout=10 ^ -10) | ||
| return excp is not None and not isinstance(excp, CancelledError) | ||
| except TimeoutError: | ||
| return False | ||
|
|
||
| return [f.exception() for f in future_lst if get_exception(future_obj=f)] |
There was a problem hiding this comment.
Fix exponentiation operator and LGTM!
The function effectively extracts exceptions from futures while filtering out CancelledError. However, there's a bug in the timeout value calculation.
Apply this diff to fix the exponentiation operator:
- excp = future_obj.exception(timeout=10^-10)
+ excp = future_obj.exception(timeout=10**-10)The ^ operator performs bitwise XOR in Python, not exponentiation. Use ** for exponentiation to get the intended timeout value of 10⁻¹⁰ seconds.
📝 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 _get_exception_lst(future_lst: list) -> list: | |
| def get_exception(future_obj: Future) -> bool: | |
| try: | |
| excp = future_obj.exception(timeout=10 ^ -10) | |
| return excp is not None and not isinstance(excp, CancelledError) | |
| except TimeoutError: | |
| return False | |
| return [f.exception() for f in future_lst if get_exception(future_obj=f)] | |
| def _get_exception_lst(future_lst: list) -> list: | |
| def get_exception(future_obj: Future) -> bool: | |
| try: | |
| excp = future_obj.exception(timeout=10**-10) | |
| return excp is not None and not isinstance(excp, CancelledError) | |
| except TimeoutError: | |
| return False | |
| return [f.exception() for f in future_lst if get_exception(future_obj=f)] |
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (1)
executorlib/interactive/shared.py (1)
675-687: 🛠️ Refactor suggestionAdd test coverage for Python versions below 3.11.
The function effectively extracts exceptions from futures while filtering out
CancelledError. However, there are a few improvements to consider:
- The code path for Python versions below 3.11 (line 686) is not covered by tests.
- The function silently returns an empty list for Python versions below 3.11, which might hide errors.
Consider these improvements:
- Add test coverage for Python versions below 3.11:
@unittest.skipIf( condition=sys.version_info[0] >= 3 and sys.version_info[1] >= 11, reason="requires Python version below 3.11", ) def test_get_exception_lst_below_python_3_11(self): fs1 = Future() fs1.set_exception(RuntimeError()) self.assertEqual(_get_exception_lst([fs1]), [])
- Consider logging a warning when running on Python versions below 3.11:
if sys.version_info[0] >= 3 and sys.version_info[1] >= 11: return [f.exception() for f in future_lst if get_exception(future_obj=f)] else: + import warnings + warnings.warn("Exception propagation is disabled for Python versions below 3.11") return []🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 686-686: executorlib/interactive/shared.py#L686
Added line #L686 was not covered by tests
🧹 Nitpick comments (2)
tests/test_dependencies_executor.py (2)
46-47: Consider using the parameter in the error message.The
parameterargument is not used in the function. Consider including it in the error message to provide more context about the failure.Apply this diff to improve the error message:
-def raise_error(parameter): - raise RuntimeError +def raise_error(parameter): + raise RuntimeError(f"Error occurred with parameter: {parameter}")
289-347: Improve loop variable usage and test coverage.The tests effectively verify error handling in different block allocation modes and worker configurations. However, there are a few improvements to consider:
- The loop variable
iis not used in any of the test functions.- The test coverage could be improved by verifying intermediate states.
Apply this diff to improve the loop variable usage and test coverage:
- for i in range(1, 4): + for _i in range(1, 4): # Rename unused variable lst = exe.submit( raise_error, parameter=lst, ) + # Verify intermediate states + self.assertTrue(lst.done()) + self.assertIsNotNone(lst.exception()) lst.result()🧰 Tools
🪛 Ruff (0.8.2)
290-291: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
294-294: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
306-307: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
310-310: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
322-323: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
326-326: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
338-339: Use a single
withstatement with multiple contexts instead of nestedwithstatements(SIM117)
342-342: Loop control variable
inot used within loop bodyRename unused
ito_i(B007)
📜 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(4 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
executorlib/interactive/shared.py
[warning] 686-686: executorlib/interactive/shared.py#L686
Added line #L686 was not covered by tests
🪛 Ruff (0.8.2)
tests/test_dependencies_executor.py
268-269: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
274-275: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
280-281: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
290-291: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
294-294: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
306-307: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
310-310: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
322-323: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
326-326: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
338-339: Use a single with statement with multiple contexts instead of nested with statements
(SIM117)
342-342: Loop control variable i not used within loop body
Rename unused i to _i
(B007)
🔇 Additional comments (2)
tests/test_dependencies_executor.py (2)
110-165: LGTM! Test coverage for error propagation is thorough.The test effectively verifies that:
- Exceptions are propagated through the dependency chain.
- Both futures (
fs1andfs2) have exceptions.- Calling
result()raises the expectedRuntimeError.The Python version check aligns with the implementation in
_get_exception_lst.
167-221: LGTM! Test coverage for pre-set exceptions is thorough.The test effectively verifies that:
- Exceptions are propagated when a future is pre-set with an exception.
- Both futures (
fs1andfs2) have exceptions.- Calling
result()raises the expectedRuntimeError.The Python version check aligns with the implementation in
_get_exception_lst.
Summary by CodeRabbit
Bug Fixes
Tests