Conversation
Particularly python stuff Signed-off-by: Eric Curtin <eric.curtin@docker.com>
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 significantly refactors the handling of Python executable discovery and command execution within the inference backends. By extracting these common functionalities into a new, shared utility file, it eliminates redundant code present in multiple backend implementations. This change streamlines the process of interacting with Python environments, ensuring consistency and simplifying future maintenance for Python-dependent components. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 - I've found 1 issue, and left some high level feedback:
- In
mlx.Install, you now always mapFindPythonPathfailures toErrStatusNotFound; consider propagatingbackends.ErrPythonNotFound(and/or using its message forstatus) so callers can distinguish between a missing Python binary and other MLX-related setup errors. - Since
FindPythonPathandNewPythonCmdare now shared utilities, it may be worth updating any other backends that manually resolve Python and spawn commands to use these helpers for consistent behavior and error handling across backends.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `mlx.Install`, you now always map `FindPythonPath` failures to `ErrStatusNotFound`; consider propagating `backends.ErrPythonNotFound` (and/or using its message for `status`) so callers can distinguish between a missing Python binary and other MLX-related setup errors.
- Since `FindPythonPath` and `NewPythonCmd` are now shared utilities, it may be worth updating any other backends that manually resolve Python and spawn commands to use these helpers for consistent behavior and error handling across backends.
## Individual Comments
### Comment 1
<location> `pkg/inference/backends/mlx/mlx.go:83-88` </location>
<code_context>
- m.status = ErrStatusNotFound.Error()
- return ErrStatusNotFound
- }
+ pythonPath, err := backends.FindPythonPath(m.customPythonPath, "")
+ if err != nil {
+ m.status = ErrStatusNotFound.Error()
+ return ErrStatusNotFound
</code_context>
<issue_to_address>
**suggestion:** Preserve the underlying Python resolution error instead of always mapping to ErrStatusNotFound
Now that `FindPythonPath` can return a more specific `ErrPythonNotFound`, consider either propagating `err` directly (as in the diffusers/sglang backends) or setting `m.status = err.Error()` instead of `ErrStatusNotFound.Error()`. This would preserve the more detailed failure information (e.g., “MLX not available” vs. “python3 not found”), improving diagnosis of misconfigured environments.
```suggestion
pythonPath, err := backends.FindPythonPath(m.customPythonPath, "")
if err != nil {
m.status = err.Error()
return err
}
m.pythonPath = pythonPath
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| pythonPath, err := backends.FindPythonPath(m.customPythonPath, "") | ||
| if err != nil { | ||
| m.status = ErrStatusNotFound.Error() | ||
| return ErrStatusNotFound | ||
| } | ||
|
|
||
| // Store the python path for later use | ||
| m.pythonPath = pythonPath |
There was a problem hiding this comment.
suggestion: Preserve the underlying Python resolution error instead of always mapping to ErrStatusNotFound
Now that FindPythonPath can return a more specific ErrPythonNotFound, consider either propagating err directly (as in the diffusers/sglang backends) or setting m.status = err.Error() instead of ErrStatusNotFound.Error(). This would preserve the more detailed failure information (e.g., “MLX not available” vs. “python3 not found”), improving diagnosis of misconfigured environments.
| pythonPath, err := backends.FindPythonPath(m.customPythonPath, "") | |
| if err != nil { | |
| m.status = ErrStatusNotFound.Error() | |
| return ErrStatusNotFound | |
| } | |
| // Store the python path for later use | |
| m.pythonPath = pythonPath | |
| pythonPath, err := backends.FindPythonPath(m.customPythonPath, "") | |
| if err != nil { | |
| m.status = err.Error() | |
| return err | |
| } | |
| m.pythonPath = pythonPath |
![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
The pull request successfully de-duplicates Python-related logic across the diffusers, mlx, and sglang backends by introducing a centralized FindPythonPath helper and a NewPythonCmd utility in the backends package. This refactoring improves maintainability and ensures consistent Python discovery behavior across different inference engines. The implementation correctly handles custom Python paths, virtual environments, and system-wide fallbacks while preserving existing error contracts.
| import ( | ||
| "errors" | ||
| "os" | ||
| "os/exec" | ||
| "path/filepath" | ||
| ) |
There was a problem hiding this comment.
Add the context package to the imports to support context-aware command creation helpers.
| import ( | |
| "errors" | |
| "os" | |
| "os/exec" | |
| "path/filepath" | |
| ) | |
| import ( | |
| "context" | |
| "errors" | |
| "os" | |
| "os/exec" | |
| "path/filepath" | |
| ) |
| func NewPythonCmd(pythonPath string, args ...string) *exec.Cmd { | ||
| binary := "python3" | ||
| if pythonPath != "" { | ||
| binary = pythonPath | ||
| } | ||
| return exec.Command(binary, args...) | ||
| } |
There was a problem hiding this comment.
Consider adding a context-aware version of this helper (e.g., NewPythonCmdContext) that uses exec.CommandContext. Since the Install methods in the backends already receive a context.Context, leveraging it for these check commands would allow for better process management and cancellation support during the installation phase.
1 participant
Particularly python stuff