154-workflows-are-not-terminated #223
Conversation
when Exrro occur in any thread, of the TOPP execution, then don't start the next execution.
it will terminated all the threads by terminating the it process (as direct thread termination is not possible in python).
WalkthroughThis pull request updates the Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CE as CommandExecutor
participant Thread as Command Thread
User->>CE: run_multiple_commands()
CE->>Thread: Launch command threads
alt Error Occurs in a Thread
Thread-->>CE: Report error (set is_executing_error_occurred)
CE->>CE: Log error & child PID
CE->>Thread: Invoke stop() to terminate processes
end
CE->>CE: Check error state after threads complete
alt Error Detected
CE->>User: Raise exception and log termination
else No Errors
CE->>User: Complete execution normally
end
Assessment against linked issues
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 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 (3)
src/workflow/CommandExecutor.py (3)
117-120: Remove unused variableThe variable
execution_timeis calculated but not used since the log message on line 119 is commented out.- end_time = time.time() - execution_time = end_time - start_time + end_time = time.time()🧰 Tools
🪛 Ruff (0.8.2)
117-117: Local variable
execution_timeis assigned to but never usedRemove assignment to unused variable
execution_time(F841)
218-223: Remove commented-out test codeThese commented-out test commands should be removed as they don't contribute to the functionality and could cause confusion.
- # commands = [ -# ["ping", "127.0.0.1", "-n", "10"], -# ["ping", "127.0.0.1", "-n", "10"], -# ["ping", "127.0.0.1", "-n", "15"], -# ["python", "-c", "import time; time.sleep(3); raise Exception('Simulated Error')"] -# ]
233-234: Remove commented-out codeThese commented-out lines should be removed for code cleanliness.
- # self.logger.log(f"Stopping-{stop_id} all running processes...") - # pids = [Path(f).stem for f in self.pid_dir.iterdir()]
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/workflow/CommandExecutor.py(7 hunks)
🧰 Additional context used
🪛 Ruff (0.8.2)
src/workflow/CommandExecutor.py
117-117: Local variable execution_time is assigned to but never used
Remove assignment to unused variable execution_time
(F841)
🔇 Additional comments (9)
src/workflow/CommandExecutor.py (9)
12-12: LGTM: Import for improved logging in stop methodThe addition of
randintis used to generate a unique identifier for stop operations in logs, which enhances debugging capabilities.
26-31: Well-designed state management additionsThese additions create an effective tracking system for:
- Process IDs via
execution_pidslist- Error states via the
statesdictionaryThis approach aligns perfectly with the PR objective of ensuring workflows terminate properly when errors occur.
51-52: Good practice: Reset error stateResetting the error state before executing commands ensures a clean slate for each execution run, preventing previous errors from affecting the current execution.
67-70: Key improvement: Error detection and workflow terminationThis code directly addresses the core issue by checking for errors after all threads complete and stopping further execution if any errors occurred. The clear error message and exception raising create a proper termination mechanism.
93-96: Enhanced process tracking and loggingGood improvements to:
- Include the child process ID in log messages
- Track process IDs in the execution_pids list
- Format log messages for better readability
These changes make debugging and monitoring workflow execution more effective.
Also applies to: 101-101
108-115: Robust process cleanup and early terminationThis code:
- Safely removes completed process IDs from tracking
- Detects errors in other threads and stops execution early
- Provides clear logging about why execution was stopped
These changes help coordinate termination across threads and prevent orphaned processes.
127-132: Effective error state management and process terminationWhen an error occurs, the code:
- Updates the error state
- Logs detailed error information with process ID
- Calls
stop()to terminate all running processes- Logs success for completed commands
This comprehensive approach ensures workflows terminate properly on errors.
232-236: Improved process termination reliabilityThe updated
stopmethod:
- Generates a unique ID for each stop operation
- Uses the tracked process IDs instead of scanning directories
- Provides detailed logging of which processes are being terminated
This makes process termination more reliable and easier to debug.
239-244: Detailed process termination loggingThe enhanced logging during process termination provides clear visibility into:
- Which processes are being terminated
- Whether termination was successful
- Any errors that occurred during termination
This level of detail is valuable for troubleshooting workflow termination issues.
1 participant
Fix: #154
disclaimer
This is intentional as the primary goal of this PR is to present the approach. Further modifications may be needed during the review process. Once the logic is accepted, we will refine the PR before merging.
here how I self-review the PR
Reproduce the bug by changing the configuration
FeatureFinderMetabo->masstrace_snr_filteringset to True.here are the
alllog.Before
After