De-duplicate code #387
De-duplicate code #387
Conversation
Summary of ChangesHello @ericcurtin, 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 refactors the codebase by extracting several utility functions that were duplicated across different backend implementations (llama.cpp and vLLM) and the main application logic. These functions, which include argument parsing, socket file management, sanitized logging, and robust process exit handling, have been consolidated into a new 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
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- Double-check that common.HandleSocketCleanup retains the previous behavior of ignoring ErrNotExist and doesn’t introduce new error paths.
- Add unit tests for common.SplitArgs to ensure edge cases like mismatched quotes and escaped spaces match the original functionality.
- Review common.ProcessExitHandler to verify it preserves the original exit error formatting and tail buffer handling across both backends.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Double-check that common.HandleSocketCleanup retains the previous behavior of ignoring ErrNotExist and doesn’t introduce new error paths.
- Add unit tests for common.SplitArgs to ensure edge cases like mismatched quotes and escaped spaces match the original functionality.
- Review common.ProcessExitHandler to verify it preserves the original exit error formatting and tail buffer handling across both backends.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
In various places Signed-off-by: Eric Curtin <[email protected]>
![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 effectively de-duplicates code by moving common functionalities like argument parsing, socket cleanup, argument sanitization for logging, and process exit handling into a new common package. This is a great refactoring that improves maintainability and reduces code repetition across the llamacpp and vllm backends.
I've found a potential issue with redundant socket cleanup calls in both backends after refactoring the process exit handling. My review comments provide details and suggestions for fixing this. Overall, this is a valuable change.
| go func() { | ||
| llamaCppErr := llamaCppSandbox.Command().Wait() | ||
| serverLogStream.Close() | ||
|
|
||
| errOutput := new(strings.Builder) | ||
| if _, err := io.Copy(errOutput, tailBuf); err != nil { | ||
| l.log.Warnf("failed to read server output tail: %w", err) | ||
| } | ||
|
|
||
| if len(errOutput.String()) != 0 { | ||
| llamaCppErr = fmt.Errorf("llama.cpp exit status: %w\nwith output: %s", llamaCppErr, errOutput.String()) | ||
| } else { | ||
| llamaCppErr = fmt.Errorf("llama.cpp exit status: %w", llamaCppErr) | ||
| } | ||
|
|
||
| llamaCppErr := common.ProcessExitHandler(l.log, llamaCppSandbox, tailBuf, serverLogStream, socket) | ||
| llamaCppErrors <- llamaCppErr | ||
| close(llamaCppErrors) | ||
| if err := os.Remove(socket); err != nil && !errors.Is(err, fs.ErrNotExist) { | ||
| l.log.Warnf("failed to remove socket file %s on exit: %w\n", socket, err) | ||
| if err := common.HandleSocketCleanup(socket); err != nil { | ||
| l.log.Warnf("failed to remove socket file %s on exit: %v\n", socket, err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
The call to common.HandleSocketCleanup(socket) on lines 210-212 appears to be redundant, as the socket variable is already passed to common.ProcessExitHandler on line 207. This implies that ProcessExitHandler is responsible for the socket cleanup.
To avoid duplicate operations and improve clarity, the explicit cleanup call should be removed. If ProcessExitHandler does not handle cleanup, its signature should be changed to not accept a socket parameter.
go func() {
llamaCppErr := common.ProcessExitHandler(l.log, llamaCppSandbox, tailBuf, serverLogStream, socket)
llamaCppErrors <- llamaCppErr
close(llamaCppErrors)
}()| go func() { | ||
| vllmErr := vllmSandbox.Command().Wait() | ||
| serverLogStream.Close() | ||
|
|
||
| errOutput := new(strings.Builder) | ||
| if _, err := io.Copy(errOutput, tailBuf); err != nil { | ||
| v.log.Warnf("failed to read server output tail: %v", err) | ||
| } | ||
|
|
||
| if len(errOutput.String()) != 0 { | ||
| vllmErr = fmt.Errorf("vLLM exit status: %w\nwith output: %s", vllmErr, errOutput.String()) | ||
| } else { | ||
| vllmErr = fmt.Errorf("vLLM exit status: %w", vllmErr) | ||
| } | ||
|
|
||
| vllmErr := common.ProcessExitHandler(v.log, vllmSandbox, tailBuf, serverLogStream, socket) | ||
| vllmErrors <- vllmErr | ||
| close(vllmErrors) | ||
| if err := os.Remove(socket); err != nil && !errors.Is(err, fs.ErrNotExist) { | ||
| if err := common.HandleSocketCleanup(socket); err != nil { | ||
| v.log.Warnf("failed to remove socket file %s on exit: %v\n", socket, err) | ||
| } | ||
| }() |
There was a problem hiding this comment.
The call to common.HandleSocketCleanup(socket) on lines 185-187 appears to be redundant, as the socket variable is already passed to common.ProcessExitHandler on line 182. This implies that ProcessExitHandler is responsible for the socket cleanup.
To avoid duplicate operations and improve clarity, the explicit cleanup call should be removed. If ProcessExitHandler does not handle cleanup, its signature should be changed to not accept a socket parameter.
go func() {
vllmErr := common.ProcessExitHandler(v.log, vllmSandbox, tailBuf, serverLogStream, socket)
vllmErrors <- vllmErr
close(vllmErrors)
}()
In various places