Conversation
# Conflicts: # main.go
Contributor
Summary of ChangesHello @ilopezluna, 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 robust and streamlined distribution method for the Diffusers backend, mirroring the successful approach used for vllm-metal. The core idea is to provide a self-contained Python environment, bundled with all required dependencies, that can be easily installed and managed. This significantly simplifies the setup process for users and expands platform compatibility, particularly for macOS ARM64, by abstracting away system-level Python complexities and ensuring consistent environments. 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
The pull request introduces support for distributing Diffusers, similar to the existing vllm-metal mechanism. This includes new Makefile targets for building, installing, developing, and cleaning Diffusers, as well as modifications to the Go backend to integrate the Diffusers service. The changes also involve updating the diffusers.go file to handle installation, downloading, and extraction of a self-contained Python environment for Diffusers, and updating the platform.go file to correctly reflect supported platforms for Diffusers. Overall, the changes are well-structured and align with the existing patterns for backend integration.
# Conflicts: # pkg/inference/backends/diffusers/diffusers.go
ilopezluna
force-pushed
the
diffusers-distribution
branch
from
46af0b5 to
a6c8cab
Compare
ericcurtin
approved these changes
Feb 20, 2026
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The diffusers version is hard-coded both as
diffusersVersioninpkg/inference/backends/diffusers/diffusers.goand asDIFFUSERS_RELEASEin the Makefile; consider deriving one from the other (or a single source of truth) to avoid version drift between the backend and the build/install tooling. - In
diffusers.downloadAndExtract, thehttpClientparameter is unused; if the backend install path will never use a custom client (e.g., for proxies or custom transports), consider removing it from the interface call chain for this backend or wiring it through to the Docker Hub helper for consistency with other backends. - The
scripts/build-diffusers-tarball.shscript assumes a Python 3.12 layout and strips macOS-specific Tcl/Tk.dylibfiles; it may be worth guarding those removals or documenting that the script is only intended for specific platforms/uv Python builds to avoid surprises on other environments.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The diffusers version is hard-coded both as `diffusersVersion` in `pkg/inference/backends/diffusers/diffusers.go` and as `DIFFUSERS_RELEASE` in the Makefile; consider deriving one from the other (or a single source of truth) to avoid version drift between the backend and the build/install tooling.
- In `diffusers.downloadAndExtract`, the `httpClient` parameter is unused; if the backend install path will never use a custom client (e.g., for proxies or custom transports), consider removing it from the interface call chain for this backend or wiring it through to the Docker Hub helper for consistency with other backends.
- The `scripts/build-diffusers-tarball.sh` script assumes a Python 3.12 layout and strips macOS-specific Tcl/Tk `.dylib` files; it may be worth guarding those removals or documenting that the script is only intended for specific platforms/uv Python builds to avoid surprises on other environments.
## Individual Comments
### Comment 1
<location> `pkg/inference/backends/diffusers/diffusers.go:159` </location>
<code_context>
+
+ // Extract the image
+ extractDir := filepath.Join(downloadDir, "extracted")
+ if err := dockerhub.Extract(filepath.Join(downloadDir, "image.tar"), runtime.GOARCH, runtime.GOOS, extractDir); err != nil {
+ return fmt.Errorf("failed to extract image: %w", err)
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The dockerhub.Extract argument order looks swapped compared to PullPlatform and may select the wrong image variant.
PullPlatform is called as `dockerhub.PullPlatform(ctx, image, path, runtime.GOOS, runtime.GOARCH)`, implying the parameter order is `(os, arch)`. Here Extract is invoked with `(tarPath, runtime.GOARCH, runtime.GOOS, extractDir)`, i.e. `(arch, os)`. If Extract expects `(os, arch)` this will look for `<arch>-<os>` instead of `<os>-<arch>` and fail or use the wrong image. Please confirm the Extract signature and align the argument order if needed (e.g. `..., runtime.GOOS, runtime.GOARCH, extractDir`).
</issue_to_address>
### Comment 2
<location> `Makefile:269-284` </location>
<code_context>
+ echo "Tarball created: $(DIFFUSERS_TARBALL)"; \
+ fi
+
+diffusers-install:
+ @VERSION_FILE="$(DIFFUSERS_INSTALL_DIR)/.diffusers-version"; \
+ if [ -f "$$VERSION_FILE" ] && [ "$$(cat "$$VERSION_FILE")" = "$(DIFFUSERS_RELEASE)" ]; then \
+ echo "diffusers $(DIFFUSERS_RELEASE) already installed"; \
+ exit 0; \
+ fi; \
+ if [ ! -f "$(DIFFUSERS_TARBALL)" ]; then \
+ echo "Error: $(DIFFUSERS_TARBALL) not found. Run 'make diffusers-build' first."; \
+ exit 1; \
+ fi; \
+ echo "Installing diffusers to $(DIFFUSERS_INSTALL_DIR)..."; \
+ rm -rf "$(DIFFUSERS_INSTALL_DIR)"; \
+ mkdir -p "$(DIFFUSERS_INSTALL_DIR)"; \
+ tar -xzf "$(DIFFUSERS_TARBALL)" -C "$(DIFFUSERS_INSTALL_DIR)"; \
+ echo "$(DIFFUSERS_RELEASE)" > "$$VERSION_FILE"; \
+ echo "diffusers $(DIFFUSERS_RELEASE) installed successfully!"
+
+diffusers-dev:
</code_context>
<issue_to_address>
**suggestion (bug_risk):** diffusers-install unconditionally wipes the install dir before verifying that extraction succeeds, which can leave users without a working install on failure.
Because the target does `rm -rf "$(DIFFUSERS_INSTALL_DIR)"` before `tar -xzf`, any extraction failure (e.g., corrupt tarball, out of disk space) will remove the last working install. Consider extracting to a temporary directory under the same parent, and only deleting the old directory and moving the new one into place after extraction and verification succeed.
```suggestion
diffusers-install:
@VERSION_FILE="$(DIFFUSERS_INSTALL_DIR)/.diffusers-version"; \
if [ -f "$$VERSION_FILE" ] && [ "$$(cat "$$VERSION_FILE")" = "$(DIFFUSERS_RELEASE)" ]; then \
echo "diffusers $(DIFFUSERS_RELEASE) already installed"; \
exit 0; \
fi; \
if [ ! -f "$(DIFFUSERS_TARBALL)" ]; then \
echo "Error: $(DIFFUSERS_TARBALL) not found. Run 'make diffusers-build' first."; \
exit 1; \
fi; \
PARENT_DIR="$$(dirname "$(DIFFUSERS_INSTALL_DIR)")"; \
TMP_DIR="$$(mktemp -d "$$PARENT_DIR/diffusers.XXXXXX")"; \
if [ -z "$$TMP_DIR" ] || [ ! -d "$$TMP_DIR" ]; then \
echo "Error: failed to create temporary install directory"; \
exit 1; \
fi; \
echo "Installing diffusers to $(DIFFUSERS_INSTALL_DIR) via $$TMP_DIR..."; \
if ! tar -xzf "$(DIFFUSERS_TARBALL)" -C "$$TMP_DIR"; then \
echo "Error: failed to extract $(DIFFUSERS_TARBALL)"; \
rm -rf "$$TMP_DIR"; \
exit 1; \
fi; \
if [ -z "$$(ls -A "$$TMP_DIR" 2>/dev/null)" ]; then \
echo "Error: extracted diffusers directory is empty"; \
rm -rf "$$TMP_DIR"; \
exit 1; \
fi; \
rm -rf "$(DIFFUSERS_INSTALL_DIR)"; \
mv "$$TMP_DIR" "$(DIFFUSERS_INSTALL_DIR)"; \
echo "$(DIFFUSERS_RELEASE)" > "$$VERSION_FILE"; \
echo "diffusers $(DIFFUSERS_RELEASE) installed successfully!"
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
|
|
||
| // Extract the image | ||
| extractDir := filepath.Join(downloadDir, "extracted") | ||
| if err := dockerhub.Extract(filepath.Join(downloadDir, "image.tar"), runtime.GOARCH, runtime.GOOS, extractDir); err != nil { |
Contributor
There was a problem hiding this comment.
issue (bug_risk): The dockerhub.Extract argument order looks swapped compared to PullPlatform and may select the wrong image variant.
PullPlatform is called as dockerhub.PullPlatform(ctx, image, path, runtime.GOOS, runtime.GOARCH), implying the parameter order is (os, arch). Here Extract is invoked with (tarPath, runtime.GOARCH, runtime.GOOS, extractDir), i.e. (arch, os). If Extract expects (os, arch) this will look for <arch>-<os> instead of <os>-<arch> and fail or use the wrong image. Please confirm the Extract signature and align the argument order if needed (e.g. ..., runtime.GOOS, runtime.GOARCH, extractDir).
Comment on lines
+269
to
+284
| diffusers-install: | ||
| @VERSION_FILE="$(DIFFUSERS_INSTALL_DIR)/.diffusers-version"; \ | ||
| if [ -f "$$VERSION_FILE" ] && [ "$$(cat "$$VERSION_FILE")" = "$(DIFFUSERS_RELEASE)" ]; then \ | ||
| echo "diffusers $(DIFFUSERS_RELEASE) already installed"; \ | ||
| exit 0; \ | ||
| fi; \ | ||
| if [ ! -f "$(DIFFUSERS_TARBALL)" ]; then \ | ||
| echo "Error: $(DIFFUSERS_TARBALL) not found. Run 'make diffusers-build' first."; \ | ||
| exit 1; \ | ||
| fi; \ | ||
| echo "Installing diffusers to $(DIFFUSERS_INSTALL_DIR)..."; \ | ||
| rm -rf "$(DIFFUSERS_INSTALL_DIR)"; \ | ||
| mkdir -p "$(DIFFUSERS_INSTALL_DIR)"; \ | ||
| tar -xzf "$(DIFFUSERS_TARBALL)" -C "$(DIFFUSERS_INSTALL_DIR)"; \ | ||
| echo "$(DIFFUSERS_RELEASE)" > "$$VERSION_FILE"; \ | ||
| echo "diffusers $(DIFFUSERS_RELEASE) installed successfully!" |
Contributor
There was a problem hiding this comment.
suggestion (bug_risk): diffusers-install unconditionally wipes the install dir before verifying that extraction succeeds, which can leave users without a working install on failure.
Because the target does rm -rf "$(DIFFUSERS_INSTALL_DIR)" before tar -xzf, any extraction failure (e.g., corrupt tarball, out of disk space) will remove the last working install. Consider extracting to a temporary directory under the same parent, and only deleting the old directory and moving the new one into place after extraction and verification succeed.
Suggested change
| diffusers-install: | |
| @VERSION_FILE="$(DIFFUSERS_INSTALL_DIR)/.diffusers-version"; \ | |
| if [ -f "$$VERSION_FILE" ] && [ "$$(cat "$$VERSION_FILE")" = "$(DIFFUSERS_RELEASE)" ]; then \ | |
| echo "diffusers $(DIFFUSERS_RELEASE) already installed"; \ | |
| exit 0; \ | |
| fi; \ | |
| if [ ! -f "$(DIFFUSERS_TARBALL)" ]; then \ | |
| echo "Error: $(DIFFUSERS_TARBALL) not found. Run 'make diffusers-build' first."; \ | |
| exit 1; \ | |
| fi; \ | |
| echo "Installing diffusers to $(DIFFUSERS_INSTALL_DIR)..."; \ | |
| rm -rf "$(DIFFUSERS_INSTALL_DIR)"; \ | |
| mkdir -p "$(DIFFUSERS_INSTALL_DIR)"; \ | |
| tar -xzf "$(DIFFUSERS_TARBALL)" -C "$(DIFFUSERS_INSTALL_DIR)"; \ | |
| echo "$(DIFFUSERS_RELEASE)" > "$$VERSION_FILE"; \ | |
| echo "diffusers $(DIFFUSERS_RELEASE) installed successfully!" | |
| diffusers-install: | |
| @VERSION_FILE="$(DIFFUSERS_INSTALL_DIR)/.diffusers-version"; \ | |
| if [ -f "$$VERSION_FILE" ] && [ "$$(cat "$$VERSION_FILE")" = "$(DIFFUSERS_RELEASE)" ]; then \ | |
| echo "diffusers $(DIFFUSERS_RELEASE) already installed"; \ | |
| exit 0; \ | |
| fi; \ | |
| if [ ! -f "$(DIFFUSERS_TARBALL)" ]; then \ | |
| echo "Error: $(DIFFUSERS_TARBALL) not found. Run 'make diffusers-build' first."; \ | |
| exit 1; \ | |
| fi; \ | |
| PARENT_DIR="$$(dirname "$(DIFFUSERS_INSTALL_DIR)")"; \ | |
| TMP_DIR="$$(mktemp -d "$$PARENT_DIR/diffusers.XXXXXX")"; \ | |
| if [ -z "$$TMP_DIR" ] || [ ! -d "$$TMP_DIR" ]; then \ | |
| echo "Error: failed to create temporary install directory"; \ | |
| exit 1; \ | |
| fi; \ | |
| echo "Installing diffusers to $(DIFFUSERS_INSTALL_DIR) via $$TMP_DIR..."; \ | |
| if ! tar -xzf "$(DIFFUSERS_TARBALL)" -C "$$TMP_DIR"; then \ | |
| echo "Error: failed to extract $(DIFFUSERS_TARBALL)"; \ | |
| rm -rf "$$TMP_DIR"; \ | |
| exit 1; \ | |
| fi; \ | |
| if [ -z "$$(ls -A "$$TMP_DIR" 2>/dev/null)" ]; then \ | |
| echo "Error: extracted diffusers directory is empty"; \ | |
| rm -rf "$$TMP_DIR"; \ | |
| exit 1; \ | |
| fi; \ | |
| rm -rf "$(DIFFUSERS_INSTALL_DIR)"; \ | |
| mv "$$TMP_DIR" "$(DIFFUSERS_INSTALL_DIR)"; \ | |
| echo "$(DIFFUSERS_RELEASE)" > "$$VERSION_FILE"; \ | |
| echo "diffusers $(DIFFUSERS_RELEASE) installed successfully!" |
ilopezluna
force-pushed
the
diffusers-distribution
branch
from
63b1dc5 to
bdc2101
Compare
Contributor
Author
|
@sourcery-ai review |
Contributor
There was a problem hiding this comment.
Hey - I've found 2 issues, and left some high level feedback:
- The diffusers version/tag is hardcoded in multiple places (Go
diffusersVersionandDIFFUSERS_RELEASEin the Makefile); consider centralizing or deriving one from the other to avoid them drifting out of sync during future releases. - In
diffusers.Install, whenplatform.SupportsDiffusers()is false you now returnErrPlatformNotSupportedwithout updatingd.statusas before; it may be helpful to keep setting a structured status string so the rest of the system can still report a meaningful state. - The new
utils.CopyDiris used for both vllm-metal and diffusers; since it preserves symlinks and modes, you might want to document or guard behavior when the destination already contains files/symlinks (e.g., overwriting existing symlinks or files) to avoid subtle issues if installations are partially present.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The diffusers version/tag is hardcoded in multiple places (Go `diffusersVersion` and `DIFFUSERS_RELEASE` in the Makefile); consider centralizing or deriving one from the other to avoid them drifting out of sync during future releases.
- In `diffusers.Install`, when `platform.SupportsDiffusers()` is false you now return `ErrPlatformNotSupported` without updating `d.status` as before; it may be helpful to keep setting a structured status string so the rest of the system can still report a meaningful state.
- The new `utils.CopyDir` is used for both vllm-metal and diffusers; since it preserves symlinks and modes, you might want to document or guard behavior when the destination already contains files/symlinks (e.g., overwriting existing symlinks or files) to avoid subtle issues if installations are partially present.
## Individual Comments
### Comment 1
<location> `pkg/inference/backends/diffusers/diffusers.go:272-281` </location>
<code_context>
+func (d *diffusers) GetDiskUsage() (int64, error) {
</code_context>
<issue_to_address>
**suggestion:** GetDiskUsage re-implements directory size traversal instead of reusing shared utilities.
This walks `d.installDir` and sums file sizes manually. We already use `diskusage.Size` elsewhere for the same purpose; if it’s still suitable here, please reuse it to keep behavior (symlinks, error handling, etc.) consistent and avoid duplicated traversal logic.
Suggested implementation:
```golang
BackendName: "Diffusers",
Socket: socket,
BinaryPath: d.pythonPath,
SandboxPath: "",
SandboxConfig: "",
Args: args,
Logger: d.log,
```
```golang
import "github.com/ollama/ollama/pkg/diskusage"
// GetDiskUsage implements inference.Backend.GetDiskUsage.
func (d *diffusers) GetDiskUsage() (int64, error) {
// Return 0 if not installed
info, err := os.Stat(d.installDir)
if os.IsNotExist(err) {
return 0, nil
}
if err != nil {
return 0, err
}
if !info.IsDir() {
// If installDir is a file, just return its size for consistency
return info.Size(), nil
}
size, err := diskusage.Size(d.installDir)
if err != nil {
return 0, err
}
return int64(size), nil
```
1. Ensure the `diskusage` import is added to the main `import` block, not as a standalone `import` if your file already has an `import (...)` section; adjust the snippet accordingly.
2. If `diskusage.Size` already returns `int64`, you can remove the `int64` cast and return `size` directly.
3. If your project uses a different import path for the disk usage helper, replace `"github.com/ollama/ollama/pkg/diskusage"` with the correct path used elsewhere in the codebase.
</issue_to_address>
### Comment 2
<location> `scripts/build-diffusers-tarball.sh:92-94` </location>
<code_context>
+rm -rf "$PYLIB/idlelib" "$PYLIB/idle_test"
+rm -rf "$PYLIB/tkinter" "$PYLIB/turtledemo"
+rm -rf "$PYLIB/ensurepip"
+# Remove Tcl/Tk native libraries (we don't need tkinter at runtime)
+rm -f "$PYTHON_DIR"/lib/libtcl*.dylib "$PYTHON_DIR"/lib/libtk*.dylib
+rm -rf "$PYTHON_DIR"/lib/tcl* "$PYTHON_DIR"/lib/tk*
+# Remove dev tools not needed at runtime
+rm -f "$PYTHON_DIR"/bin/*-config "$PYTHON_DIR"/bin/idle*
</code_context>
<issue_to_address>
**suggestion (performance):** Tcl/Tk cleanup is macOS-centric; on Linux the .so libraries will remain and increase tarball size.
This line only deletes macOS `.dylib` files; on Linux the Tcl/Tk shared libraries are `.so` and will be left in place even though tkinter is removed. If you also want to minimize Linux tarball size, consider matching the `.so` variants as well, potentially conditional on `uname` to avoid removing needed libs on other platforms.
```suggestion
# Remove Tcl/Tk native libraries (we don't need tkinter at runtime)
case "$(uname -s)" in
Darwin)
# macOS Tcl/Tk shared libraries
rm -f "$PYTHON_DIR"/lib/libtcl*.dylib "$PYTHON_DIR"/lib/libtk*.dylib
;;
Linux)
# Linux Tcl/Tk shared libraries
rm -f "$PYTHON_DIR"/lib/libtcl*.so "$PYTHON_DIR"/lib/libtk*.so
;;
esac
rm -rf "$PYTHON_DIR"/lib/tcl* "$PYTHON_DIR"/lib/tk*
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
272
to
+281
| func (d *diffusers) GetDiskUsage() (int64, error) { | ||
| // Check if Docker installation exists | ||
| if _, err := os.Stat(diffusersDir); err == nil { | ||
| size, err := diskusage.Size(diffusersDir) | ||
| if err != nil { | ||
| return 0, fmt.Errorf("error while getting diffusers dir size: %w", err) | ||
| } | ||
| return size, nil | ||
| // Return 0 if not installed | ||
| if _, err := os.Stat(d.installDir); os.IsNotExist(err) { | ||
| return 0, nil | ||
| } | ||
| // Python installation doesn't have a dedicated installation directory | ||
| // It's installed via pip in the system Python environment | ||
| return 0, nil | ||
| } | ||
|
|
||
| // GetRequiredMemoryForModel returns the estimated memory requirements for a model. | ||
| func (d *diffusers) GetRequiredMemoryForModel(_ context.Context, _ string, _ *inference.BackendConfiguration) (inference.RequiredMemory, error) { | ||
| if !platform.SupportsDiffusers() { | ||
| return inference.RequiredMemory{}, ErrNotImplemented | ||
| } | ||
|
|
||
| // Stable Diffusion models typically require significant VRAM | ||
| // SD 1.5: ~4GB VRAM, SD 2.1: ~5GB VRAM, SDXL: ~8GB VRAM | ||
| return inference.RequiredMemory{ | ||
| RAM: 4 * 1024 * 1024 * 1024, // 4GB RAM | ||
| VRAM: 6 * 1024 * 1024 * 1024, // 6GB VRAM (average estimate) | ||
| }, nil | ||
| } | ||
|
|
||
| // pythonCmd creates an exec.Cmd that runs python with the given arguments. | ||
| // It uses the configured pythonPath if available, otherwise falls back to "python3". | ||
| func (d *diffusers) pythonCmd(args ...string) *exec.Cmd { | ||
| pythonBinary := "python3" | ||
| if d.pythonPath != "" { | ||
| pythonBinary = d.pythonPath | ||
| var size int64 | ||
| err := filepath.Walk(d.installDir, func(_ string, info os.FileInfo, err error) error { | ||
| if err != nil { | ||
| return err |
Contributor
There was a problem hiding this comment.
suggestion: GetDiskUsage re-implements directory size traversal instead of reusing shared utilities.
This walks d.installDir and sums file sizes manually. We already use diskusage.Size elsewhere for the same purpose; if it’s still suitable here, please reuse it to keep behavior (symlinks, error handling, etc.) consistent and avoid duplicated traversal logic.
Suggested implementation:
BackendName: "Diffusers",
Socket: socket,
BinaryPath: d.pythonPath,
SandboxPath: "",
SandboxConfig: "",
Args: args,
Logger: d.log,import "github.com/ollama/ollama/pkg/diskusage"
// GetDiskUsage implements inference.Backend.GetDiskUsage.
func (d *diffusers) GetDiskUsage() (int64, error) {
// Return 0 if not installed
info, err := os.Stat(d.installDir)
if os.IsNotExist(err) {
return 0, nil
}
if err != nil {
return 0, err
}
if !info.IsDir() {
// If installDir is a file, just return its size for consistency
return info.Size(), nil
}
size, err := diskusage.Size(d.installDir)
if err != nil {
return 0, err
}
return int64(size), nil- Ensure the
diskusageimport is added to the mainimportblock, not as a standaloneimportif your file already has animport (...)section; adjust the snippet accordingly. - If
diskusage.Sizealready returnsint64, you can remove theint64cast and returnsizedirectly. - If your project uses a different import path for the disk usage helper, replace
"github.com/ollama/ollama/pkg/diskusage"with the correct path used elsewhere in the codebase.
Comment on lines
+92
to
+94
| # Remove Tcl/Tk native libraries (we don't need tkinter at runtime) | ||
| rm -f "$PYTHON_DIR"/lib/libtcl*.dylib "$PYTHON_DIR"/lib/libtk*.dylib | ||
| rm -rf "$PYTHON_DIR"/lib/tcl* "$PYTHON_DIR"/lib/tk* |
Contributor
There was a problem hiding this comment.
suggestion (performance): Tcl/Tk cleanup is macOS-centric; on Linux the .so libraries will remain and increase tarball size.
This line only deletes macOS .dylib files; on Linux the Tcl/Tk shared libraries are .so and will be left in place even though tkinter is removed. If you also want to minimize Linux tarball size, consider matching the .so variants as well, potentially conditional on uname to avoid removing needed libs on other platforms.
Suggested change
| # Remove Tcl/Tk native libraries (we don't need tkinter at runtime) | |
| rm -f "$PYTHON_DIR"/lib/libtcl*.dylib "$PYTHON_DIR"/lib/libtk*.dylib | |
| rm -rf "$PYTHON_DIR"/lib/tcl* "$PYTHON_DIR"/lib/tk* | |
| # Remove Tcl/Tk native libraries (we don't need tkinter at runtime) | |
| case "$(uname -s)" in | |
| Darwin) | |
| # macOS Tcl/Tk shared libraries | |
| rm -f "$PYTHON_DIR"/lib/libtcl*.dylib "$PYTHON_DIR"/lib/libtk*.dylib | |
| ;; | |
| Linux) | |
| # Linux Tcl/Tk shared libraries | |
| rm -f "$PYTHON_DIR"/lib/libtcl*.so "$PYTHON_DIR"/lib/libtk*.so | |
| ;; | |
| esac | |
| rm -rf "$PYTHON_DIR"/lib/tcl* "$PYTHON_DIR"/lib/tk* |
# Conflicts: # main.go # pkg/inference/backends/diffusers/diffusers.go
ilopezluna
force-pushed
the
diffusers-distribution
branch
from
bdc2101 to
13f5872
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related to https://github.com/docker/inference-engine-llama.cpp/pull/126
it uses same vllm-metal mechanism to distribute diffusers
You can try it by:
And then from another terminal: