-
Notifications
You must be signed in to change notification settings - Fork 241
refactor: merge duplicate process termination functions into unified kill_process_tree #648
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Summary of ChangesHello @garrett4wade, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the codebase by unifying disparate process termination mechanisms into a single, robust Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request is a great refactoring that successfully merges duplicated process termination logic into a single, well-documented utility function kill_process_tree. This change significantly improves code maintainability and consistency across the project. The new function is more robust, handling both graceful and aggressive termination, and its adoption simplifies the cleanup logic in multiple modules. The accompanying test updates are also well-executed, making the tests for this functionality cleaner and more direct. I've found one potential issue in the new utility function that could lead to orphaned child processes and have left a comment with a suggested fix. Overall, this is a high-quality contribution.
…kill_process_tree Merged `terminate_process_tree` from vllm_server.py and `kill_process_tree` from utils/proc.py into a single unified function in areal/utils/proc.py. Key changes: - Created unified kill_process_tree with graceful termination as default - Supports both graceful (SIGTERM → wait → SIGKILL) and aggressive (immediate SIGKILL) modes - Preserves all features: configurable timeout, skip_pid, include_parent, SIGCHLD cleanup, SIGQUIT fallback - Updated all call sites across areal/ to use the unified function - Updated tests to mock the centralized implementation Benefits: - Eliminates code duplication (3 implementations → 1) - Consistent process cleanup behavior across the codebase - Better maintainability with single source of truth 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <[email protected]>
32ece11 to
f2f355a
Compare
nuzant
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM.
Description
Merged
terminate_process_treefrom vllm_server.py andkill_process_treefrom utils/proc.py into a single unified function in areal/utils/proc.py.Type of Change
work as expected)
Checklist
jb build docs/gemini review)