Skip to content

feat: improve llama.cpp base image tag for cpu#391

Merged
mostlygeek merged 1 commit intomostlygeek:mainfrom
ryan-steed-usa:cpu-build-tag
Nov 8, 2025
Merged

feat: improve llama.cpp base image tag for cpu#391
mostlygeek merged 1 commit intomostlygeek:mainfrom
ryan-steed-usa:cpu-build-tag

Conversation

@ryan-steed-usa
Copy link
Copy Markdown
Contributor

@ryan-steed-usa ryan-steed-usa commented Nov 8, 2025

Refactor the container build script to resolve llama.cpp base image for CPU, also tag these builds accordingly.

  • For CPU containers, now fetch the latest server tagged llama.cpp image instead of using a generic server tag
  • Cleans up the docker build command to use dynamic BASE_TAG variable
  • Maintains existing push functionality for built images

Summary by CodeRabbit

  • Chores
    • Enhanced container build automation to dynamically manage image tags and versions from the GitHub Container Registry, improving build consistency and reducing manual configuration steps.

Refactor the container build script to resolve llama.cpp base image for CPU, also tag these builds accordingly.

- For CPU containers, now fetch the latest 'server' tagged llama.cpp image instead of using a generic 'server' tag
- Cleans up the docker build command to use dynamic BASE_TAG variable
- Maintains existing push functionality for built images
@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 8, 2025

Walkthrough

The Docker build container script is refactored to dynamically fetch llama.cpp versions from the GitHub Container registry instead of using fixed tags. Container tags are derived based on fetched versions and build architecture, while the build and push logic is restructured to use unified tag generation with BASE_TAG and LS_VER as build arguments.

Changes

Cohort / File(s) Summary
Docker build container tag and version management
docker/build-container.sh
Reworks CPU architecture branch to fetch latest server-tagged llama.cpp version from registry and derive BASE_TAG dynamically. For non-CPU architectures, retrieves latest server-${ARCH} tag and derives BASE_TAG accordingly. Replaces per-arch tag logic with unified container tag generation: CONTAINER_TAG as ghcr.io/mostlygeek/llama-swap:v${LS_VER}-${ARCH}-${LCPP_TAG} and CONTAINER_LATEST as ghcr.io/mostlygeek/llama-swap:${ARCH}. Build and push steps adjusted to pass BASE_TAG and LS_VER as build arguments and use new tag variables. Consolidates multiple tag push flow into single build command with both tags. Retains explicit abort check when LCPP_TAG is empty.

Sequence Diagram

sequenceDiagram
    participant Script as build-container.sh
    participant Registry as ghcr.io/mostlygeek
    participant Docker as Docker Build

    Script->>Registry: Fetch latest server-tagged version
    Registry-->>Script: Return LCPP_TAG
    
    alt LCPP_TAG is empty
        Script->>Script: ABORT with error
    else Valid LCPP_TAG
        Script->>Script: Derive BASE_TAG based on arch
        Script->>Script: Generate CONTAINER_TAG and CONTAINER_LATEST
        Note over Script: v${LS_VER}-${ARCH}-${LCPP_TAG}
        Note over Script: ${ARCH} (latest tag)
        
        Script->>Docker: Build with BASE_TAG and LS_VER args
        Docker-->>Script: Build complete
        
        alt PUSH_IMAGES is true
            Script->>Registry: Push CONTAINER_TAG
            Script->>Registry: Push CONTAINER_LATEST
        end
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Requires verification of registry fetch logic and error handling for network/API failures
  • Validate tag derivation logic matches expected format across CPU and non-CPU architecture branches
  • Confirm build argument passing (BASE_TAG, LS_VER) is correctly integrated with Containerfile expectations

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (3 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: improving how llama.cpp base image tags are handled for CPU builds by fetching the latest server tag instead of using a fixed tag.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@ryan-steed-usa
Copy link
Copy Markdown
Contributor Author

Closes #390

@ryan-steed-usa
Copy link
Copy Markdown
Contributor Author

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Nov 8, 2025

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
docker/build-container.sh (1)

29-32: Consider adding error handling for API calls.

The curl and jq commands on lines 29–32 and 35–38 can fail silently or return invalid data. The existing check on line 43 validates that LCPP_TAG is non-empty, but does not account for malformed responses or API failures that might still produce a non-empty but incorrect value.

Consider adding early validation (e.g., checking HTTP status codes with curl -f, or verifying jq output format) to surface API errors earlier.

Also applies to: 35-38

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 6aedbe1 and 312ae5b.

📒 Files selected for processing (1)
  • docker/build-container.sh (1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-10-27T16:04:23.381Z
Learnt from: ryan-steed-usa
Repo: mostlygeek/llama-swap PR: 368
File: docker/llama-swap.Containerfile:14-20
Timestamp: 2025-10-27T16:04:23.381Z
Learning: In docker/llama-swap.Containerfile, the user/group creation logic intentionally supports adding a non-root user (non-zero UID) to the root group (GID=0) as a valid configuration. This allows flexible UID/GID customization where users can specify only a UID or both UID and GID.

Applied to files:

  • docker/build-container.sh

@mostlygeek
Copy link
Copy Markdown
Owner

I tested it and it works as expected so far.

@ryan-steed-usa ryan-steed-usa marked this pull request as ready for review November 8, 2025 17:52
@mostlygeek mostlygeek merged commit eab2efd into mostlygeek:main Nov 8, 2025
3 checks passed
@ryan-steed-usa ryan-steed-usa deleted the cpu-build-tag branch November 11, 2025 05:17
napmany pushed a commit to napmany/llmsnap that referenced this pull request Nov 18, 2025
Refactor the container build script to resolve llama.cpp base image for CPU, also tag these builds accordingly.

- For CPU containers, now fetch the latest 'server' tagged llama.cpp image instead of using a generic 'server' tag
- Cleans up the docker build command to use dynamic BASE_TAG variable
- Maintains existing push functionality for built images
0uep pushed a commit to lynxai-team/llama-swap that referenced this pull request Nov 21, 2025
Refactor the container build script to resolve llama.cpp base image for CPU, also tag these builds accordingly.

- For CPU containers, now fetch the latest 'server' tagged llama.cpp image instead of using a generic 'server' tag
- Cleans up the docker build command to use dynamic BASE_TAG variable
- Maintains existing push functionality for built images
rohitpaul pushed a commit to rohitpaul/llama-swap that referenced this pull request Mar 29, 2026
Refactor the container build script to resolve llama.cpp base image for CPU, also tag these builds accordingly.

- For CPU containers, now fetch the latest 'server' tagged llama.cpp image instead of using a generic 'server' tag
- Cleans up the docker build command to use dynamic BASE_TAG variable
- Maintains existing push functionality for built images
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants