feat: add launch_server and teardown_server in inference engine api#550
feat: add launch_server and teardown_server in inference engine api#550
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 introduces a more robust and integrated approach to managing local inference server processes within the system. By adding explicit 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
|
e176283 to
6900436
Compare
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable feature for programmatically launching and tearing down inference servers, which enhances the flexibility of the system. The overall implementation is well-structured, centralizing server launch logic into backend-specific classes. However, I've identified a critical race condition in the vLLM server launcher that needs to be addressed. Additionally, there are several opportunities for improvement regarding code quality, such as removing redundant imports, and enhancing robustness in server teardown and logging. My detailed comments are below.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces launch_server and teardown_server APIs to programmatically manage local inference servers, which is a great addition for testing and dynamic environments. The implementation is solid, covering server launch, health checks, and graceful shutdown. My main feedback is to improve the logging for these locally launched servers. Currently, their output is directed to the main process's stdout, which can become messy when multiple servers are launched. I've suggested a refactoring to redirect output to unique log files and manage their lifecycle properly. This involves small, coordinated changes across a few files to pass around and manage the log file handle.
| return self.workflow_executor.resume() | ||
|
|
||
| def launch_server(self, server_args: dict[str, Any]) -> LocalInfServerInfo: | ||
| """Launch a local inference server.""" |
There was a problem hiding this comment.
Maybe we should add doc strings to remind users this function should be carefully used (or just should not be used) in SPMD mode since inference servers and training processes will share GPUs.
nuzant
left a comment
There was a problem hiding this comment.
We should also add a test for launch_serverand teardown_server.
fa5a79a to
fb79c64
Compare
…nclusionAI#550) * add launch_server and teardown_server in inference server api * revert vllm sglang launcher * update doc * update tests * fix tests
…nclusionAI#550) * add launch_server and teardown_server in inference server api * revert vllm sglang launcher * update doc * update tests * fix tests
Description
Fixes #530
Type of Change
work as expected)
Checklist
jb build docs/gemini review)