Throw exception on ERROR and terminate TOPP workflow run #210
Conversation
WalkthroughThe change introduces a new mechanism for handling exceptions in the Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant CommandExecutor
Caller->>CommandExecutor: run_multiple_commands(...)
CommandExecutor->>CommandExecutor: Start threads for commands
alt Exception Detected
CommandExecutor->>Queue: Place exception in queue
end
CommandExecutor->>CommandExecutor: Wait for threads to complete
alt Exceptions in Queue
CommandExecutor->>Caller: Raise RuntimeError(exception)
else No Exceptions
CommandExecutor->>Caller: Return results
end
Assessment against linked issues
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (3)
🪧 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 (1)
src/workflow/CommandExecutor.py (1)
63-108: Consider enhancing the error message for improved debuggingWhile the current implementation correctly raises an exception with the stderr content, consider including the command that failed in the error message for easier debugging.
- raise RuntimeError(error_message) + raise RuntimeError(f"Command failed: '{' '.join(command)}'\n{error_message}")🧰 Tools
🪛 Ruff (0.8.2)
77-77: f-string without any placeholders
Remove extraneous
fprefix(F541)
98-98: f-string without any placeholders
Remove extraneous
fprefix(F541)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/workflow/CommandExecutor.py(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build-full-app
- GitHub Check: build-openms
- GitHub Check: build-simple-app
🔇 Additional comments (2)
src/workflow/CommandExecutor.py (2)
108-108: Appropriate exception raising to propagate errorsThis change aligns with the PR objective to terminate workflow runs on error, ensuring issues are not silently handled. The RuntimeError will propagate to the calling code, allowing proper error handling at higher levels.
28-62:Details
❓ Verification inconclusive
Verify how exceptions impact parallel command execution
Since
run_multiple_commandsexecutes commands in separate threads, an exception in one thread won't affect others. Consider if you need additional exception handling to ensure that an error in one command correctly terminates the entire workflow.
🏁 Script executed:
#!/bin/bash # Check if there are any exception handlers in code that calls run_multiple_commands rg -A 3 -B 3 "run_multiple_commands.*\)" --include "*.py" --exclude "CommandExecutor.py" # Check if there are try/except blocks that catch RuntimeError rg -A 3 -B 3 "try:.*except\s+(RuntimeError|Exception)" --include "*.py"Length of output: 1046
Action: Revisit Exception Handling in Parallel Command Execution
The current implementation of
run_multiple_commandslaunches each command in its own thread without a centralized error-handling mechanism. This means that if one thread raises an exception (for example, withinrun_command), it will not affect or interrupt the execution of the other threads. Our initial search for external try/except blocks around calls torun_multiple_commandsand for exception capturing in Python files didn’t yield conclusive results—likely due to the need to adjust search parameters.Please consider the following:
- Thread Exception Handling: If an exception in any thread should halt the overall workflow, you might need to wrap the thread target’s execution (i.e., the
run_commandmethod) in its own try/except block and propagate errors back to the main thread.- Caller Responsibility: Verify if the callers of
run_multiple_commandsare expected to handle such exceptions or if additional safeguards are needed.- Centralized Error Reporting: You may want to implement or integrate a mechanism that collects exceptions from all threads and makes a unified decision on workflow continuation.
Kindly validate whether this behavior meets your design intent and adjust the exception-handling strategy if a failure in one command must impact the entire operation.
| # Exceptions are logged once all threads have completed execution | ||
| # The first exception amongst all is logged and raised | ||
| if not exceptions_queue.empty(): | ||
| raise RuntimeError(exceptions_queue.get()) | ||
|
|
There was a problem hiding this comment.
@t0mdavid-m what I've done here is that as the threads execute .... I catch the threads and then keep array of exceptions and as soon as all threads complete I throw/raise the first occurred exception to terminate the workflow. I've tested it out and it works. Only thing is that the termination happens after all threads complete.
Wouldn't it be safe to not kill other threads and simply let them run but in the minimal logs just show the error that was hit first?
There was a problem hiding this comment.
I think we can kill all threads as soon as an exception was raised. The workflow will not be able to complete in any case after that so there is no reason to keep threads running.
There was a problem hiding this comment.
So should I use Thread.Event to listen to a STOP event across all threads in a while loop within each thread run and terminate/kill the thread as soon as the event is set to true
2 participants
Fixes #154
Description
I've added a new raise exception block that throws an exception to be caught by the outermost try / catch block at the workflow run level to terminate the workflow run
Streamlit.Terminate.Run.mp4
Summary by CodeRabbit