[Misc] Rework process titles #21780
[Misc] Rework process titles #21780
Conversation
Follow-on from vllm-project#21445 which I hadn't finished reviewing. - Rename method from "bind process name" to "set process title" which is clearer imo. - Choose process names more carefully, decouple from class names. - Correctly include/order DP/PP/TP ranks in worker proc names. - Don't change the title of the top-level processes (which may be confusing to administrators who expect it to match the command line). - [Related] Add APIServer process logging prefix in single-API-server case to match multi-api-server log appearance. Signed-off-by: Nick Hill <[email protected]>
njhill
requested review from
WoosukKwon,
aarnphm,
alexm-redhat,
comaniac,
robertgshaw2-redhat and
ywang96
as code owners
|
👋 Hi! Thank you for contributing to the vLLM project. 💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels. Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging. To run CI, PR reviewers can either: Add 🚀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors process title handling for better clarity and correctness. It renames bind_process_name to set_process_title, improves the naming logic for worker processes to correctly include distributed ranks, and avoids renaming top-level processes. A logging prefix is also added for the single API server case to align with the multi-server case. My review identified a logical bug in the new set_process_title function that causes incorrect process titles when appending. I've provided a suggestion to fix this.
Signed-off-by: Nick Hill <[email protected]>
DarkLight1337
left a comment
DarkLight1337
left a comment
There was a problem hiding this comment.
Thanks for the cleanup!
github-actions
bot
added
the
ready
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: Nick Hill <[email protected]> Signed-off-by: x22x22 <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: Nick Hill <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
Signed-off-by: Nick Hill <[email protected]> Signed-off-by: Noam Gat <[email protected]>
Signed-off-by: Nick Hill <[email protected]> Signed-off-by: Paul Pak <[email protected]>
Signed-off-by: Nick Hill <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
Signed-off-by: Nick Hill <[email protected]>
2 participants
Follow-on from #21445 which I hadn't finished reviewing.
APIServerprocess logging prefix in single-API-server case to match multi-api-server log appearance.