feat: make llama.cpp a deferred backend on macOS/Windows#737
feat: make llama.cpp a deferred backend on macOS/Windows#737doringeman wants to merge 1 commit intodocker:mainfrom
Conversation
Signed-off-by: Dorin Geman <dorin.geman@docker.com>
There was a problem hiding this comment.
Code Review
This pull request refactors the llama.cpp backend to support deferred installation on macOS and Windows, which is a great improvement. The changes are well-structured, introducing a unified installDir and a BackendUpdater interface for managing binary updates. My main concern is with error handling in main.go where a failure to determine the llama.cpp server path is logged but not treated as a fatal error, which could lead to issues later on. I've added a critical comment with a suggestion to address this.
| if err != nil { | ||
| log.Error("Failed to get llama.cpp server path", "error", err) | ||
| } |
There was a problem hiding this comment.
The program continues to run even if envconfig.LlamaServerPath() returns an error. If this happens, llamaServerPath will be an empty string, which will cause the model runner to attempt to write to the current working directory instead of the intended installation directory. This can lead to unexpected behavior and potential permission issues. This error should be treated as fatal, and the program should exit.
| if err != nil { | |
| log.Error("Failed to get llama.cpp server path", "error", err) | |
| } | |
| if err != nil { | |
| log.Error("Failed to get llama.cpp server path", "error", err) | |
| exitFunc(1) | |
| } |
docker/model-runner:llamacpp-metal-v0.1.0