Repository navigation
refactor(docker): consolidate engine image build into reusable workflow - #658
Conversation
Replace 3 near-identical workflow files and a 4-target Dockerfile with a
single reusable workflow (_build-engine-image.yml) called by thin
per-engine wrappers.
Dockerfile changes:
- Rename Dockerfile.engine to engine.Dockerfile
- Replace 4 duplicate multi-target stages (custom-vllm, custom-sglang,
custom-trtllm, custom-tgl) with a single parameterized stage using
ENGINE build arg to select the install script at runtime
- BACKEND build arg handles the tgl->sglang mapping
- Single COPY of scripts/installation/ replaces 5 individual COPYs
Workflow changes:
- New _build-engine-image.yml reusable workflow containing all shared
build logic: tag resolution, Docker build, GHCR push, cleanup
- TRT-LLM source build path preserved as conditional steps
(source_build_repo/source_build_ref inputs)
- dry_run input for PR validation (build without push)
- release-sglang-docker.yml now triggers on PRs that touch Docker/
workflow files for CI validation
- All 3 wrappers reduced to ~50 lines each (from 148/144/213)
- Unified tag resolution: {SMG_VERSION}-{ENGINE}-{ENGINE_VERSION}
- All run: blocks use env vars instead of direct ${{ }} interpolation
Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
github-actions
Bot
added
ci
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughExtracts engine-image build logic into a new reusable GitHub Actions workflow, replaces per-engine release workflows to call it, removes Changes
Sequence Diagram(s)sequenceDiagram
participant Trigger as Workflow Trigger
participant Reusable as _build-engine-image.yml
participant Runner as GitHub Runner (8-gpu-h200)
participant Source as Source Repos
participant Buildx as Docker Buildx
participant GHCR as GHCR
Trigger->>Reusable: invoke(inputs: engine, backend, base_image_ref, repos, commits, tag, dry_run)
Reusable->>Runner: start job (print inputs, validate rules)
Runner->>Source: optionally checkout engine/smg/source-build repos
Runner->>Buildx: setup Buildx, resolve tag and base image
alt base image built from source
Runner->>Buildx: build base image from source
end
Runner->>Buildx: build engine image
alt not dry_run
Runner->>GHCR: login & push image
end
Runner->>Reusable: cleanup and final summary
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Summary of ChangesHello, 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 Docker image build system by consolidating disparate, engine-specific build logic into a unified, parameterized Dockerfile and a reusable GitHub Actions workflow. This change aims to reduce code duplication, improve maintainability, and streamline the image release pipeline for various inference engines, while also enabling automated build validation on pull requests. Highlights
Changelog
Ignored Files
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
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/_build-engine-image.yml:
- Around line 136-142: The GHCR login step (the step named "Login to GitHub
Container Registry" that uses docker/login-action@v3) runs unconditionally and
should be skipped when the workflow's dry_run flag is true; modify that step to
include a conditional (for example add an if: ${{ !env.dry_run }} or equivalent
expression that matches how dry_run is defined in your workflow) so the
docker/login-action@v3 step is only executed when dry_run is false.
In @.github/workflows/release-sglang-docker.yml:
- Line 7: The default SMG commit fallback currently uses 'latest' in the smg
variable (smg=${{ inputs.smg_commit || 'latest' }}) causing non-deterministic PR
dry-runs; change the fallback value to the pinned release tag 'v1.1.0' so it
reads smg=${{ inputs.smg_commit || 'v1.1.0' }} (ensure you update the same
expression used for pull_request dry-run paths as well).
In `@docker/engine.Dockerfile`:
- Around line 45-61: The final image runs as root; create and switch to an
unprivileged runtime user after installation steps by adding a user/group (e.g.,
smg or sguser) and setting ownership of runtime directories created by RUN steps
(refer to the COPY/RUN-created paths: /opt/engine-src, /opt/smg-src,
/tmp/scripts) and then add a USER instruction before the image is finished;
ensure the created user has a non-root UID/GID and that any required directory
permissions are chowned to that user so the container runs unprivileged at
runtime.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 39e6d48d-2250-460f-871a-d0de178640e9
📒 Files selected for processing (6)
.github/workflows/_build-engine-image.yml.github/workflows/release-sglang-docker.yml.github/workflows/release-trtllm-docker.yml.github/workflows/release-vllm-docker.ymldocker/Dockerfile.enginedocker/engine.Dockerfile
💤 Files with no reviewable changes (1)
- docker/Dockerfile.engine
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 536468c2a4
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
![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 significantly improves the maintainability of the engine image build process by consolidating duplicated logic from multiple workflow files and a multi-target Dockerfile into a single reusable workflow and a parameterized Dockerfile. While this approach is well-structured and uses build arguments effectively, it introduces a high-severity command injection vulnerability in the new docker/engine.Dockerfile. Specifically, the ENGINE build argument is used in a shell command without validation or quoting, which could allow arbitrary code execution during the build process if controlled by an untrusted source. Implementing an allow-list check for the ENGINE argument is strongly recommended to mitigate this. Additionally, to improve robustness, consider adding a check for the presence of SMG_REPO and SMG_COMMIT build arguments, providing clearer error messages if they are missing.
- Skip GHCR login when dry_run is true (no push needed) - Validate ENGINE arg against allowlist (vllm|sglang|trtllm|tgl) in both the reusable workflow and Dockerfile to prevent injection - Add SMG_REPO/SMG_COMMIT guards in Dockerfile sources stage - Remove pull_request trigger from sglang wrapper to avoid running untrusted fork code on self-hosted GPU runners - Fix SMG commit fallback from 'latest' to pinned 'v1.1.0' in sglang wrapper run-name and job inputs Signed-off-by: Wei Gong <simolin@gmail.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
docker/engine.Dockerfile (1)
47-67:⚠️ Potential issue | 🟠 MajorRun the final container as a non-root user.
The final stage never sets
USER, so runtime remains root (Line 47 onward). Please drop privileges before image completion.Suggested patch
RUN case "${ENGINE}" in \ vllm|sglang|trtllm|tgl) ;; \ *) echo "ERROR: Unknown ENGINE '${ENGINE}'" >&2; exit 1 ;; \ esac \ && if [ -n "${ENGINE_REPO}" ]; then \ bash /tmp/scripts/install-${ENGINE}.sh /opt/engine-src; \ fi + +# Drop root privileges for runtime +RUN groupadd --gid 10001 smg \ + && useradd --uid 10001 --gid 10001 --create-home --shell /usr/sbin/nologin smg \ + && chown -R smg:smg /opt/engine-src /opt/smg-src /tmp/scripts +USER smg🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docker/engine.Dockerfile` around lines 47 - 67, The final image still runs as root; add a non-root runtime user and drop privileges at the end of this Dockerfile final stage: create a dedicated user/group (e.g., addgroup/adduser or groupadd/useradd with a fixed UID/GID), chown the application directories copied earlier (referencing COPY --from=sources /opt/engine-src, /opt/smg-src and /tmp/scripts and any dirs created by RUN bash /tmp/scripts/install-smg.sh and install-${ENGINE}.sh) to that user, set ENV HOME appropriately, and set USER to that non-root account so the container no longer runs as root. Ensure file ownership covers all runtime paths the app needs.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/_build-engine-image.yml:
- Around line 62-63: The workflow step named "Print inputs to summary" is
writing raw repo input values into GITHUB_STEP_SUMMARY which can expose
credentialed URLs; update that step to avoid printing raw inputs by sanitizing
or redacting any repo/URL values before appending to GITHUB_STEP_SUMMARY (e.g.,
strip credentials from URLs or replace with "<redacted>"), or only write
non-sensitive parts (owner/repo name) instead; reference the step name "Print
inputs to summary" and the environment variable GITHUB_STEP_SUMMARY when making
the change so the summary no longer contains unhashed or unmasked secrets.
- Around line 101-104: The conditional that triggers the TRT-LLM source-build
flow must explicitly require ENGINE="trtllm"; update each check that currently
tests only BASE_IMAGE_REF and SOURCE_BUILD_REPO to also verify ENGINE ==
"trtllm" (use the ENGINE, BASE_IMAGE_REF, SOURCE_BUILD_REPO variables), and
adjust the related error messages so they only apply when ENGINE is "trtllm";
apply the same change to the other occurrences of this pattern (the other if
blocks that reference BASE_IMAGE_REF and SOURCE_BUILD_REPO) to prevent
source-build from running for sglang/vllm.
---
Duplicate comments:
In `@docker/engine.Dockerfile`:
- Around line 47-67: The final image still runs as root; add a non-root runtime
user and drop privileges at the end of this Dockerfile final stage: create a
dedicated user/group (e.g., addgroup/adduser or groupadd/useradd with a fixed
UID/GID), chown the application directories copied earlier (referencing COPY
--from=sources /opt/engine-src, /opt/smg-src and /tmp/scripts and any dirs
created by RUN bash /tmp/scripts/install-smg.sh and install-${ENGINE}.sh) to
that user, set ENV HOME appropriately, and set USER to that non-root account so
the container no longer runs as root. Ensure file ownership covers all runtime
paths the app needs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: a932747d-dd5b-4d47-94ca-752237c4d83b
📒 Files selected for processing (3)
.github/workflows/_build-engine-image.yml.github/workflows/release-sglang-docker.ymldocker/engine.Dockerfile
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5a8df73bc3
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| ARG BACKEND | ||
| ARG ENGINE_REPO | ||
|
|
||
| ENV SMG_DEFAULT_BACKEND=${BACKEND:-${ENGINE}} |
There was a problem hiding this comment.
This refactor changes the default backend for ENGINE=tgl from sglang (previous custom-tgl stage behavior) to tgl via SMG_DEFAULT_BACKEND=${BACKEND:-${ENGINE}}. That breaks default runtime behavior for TGL builds, because smg serve only accepts sglang, vllm, or trtllm as backend choices (bindings/python/src/smg/serve.py), so a TGL image started without an explicit --backend/BACKEND override will fail argument validation.
Useful? React with 👍 / 👎.
…trtllm - Sanitize repo URL inputs in step summary to strip credentials before logging to GITHUB_STEP_SUMMARY - Add explicit engine=trtllm guard to all source-build conditional steps (resolve ref, checkout, build) to prevent accidental source builds for other engines - Add validation that source_build_repo is only accepted for trtllm Signed-off-by: Wei Gong <simolin@gmail.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
Re-add pull_request trigger on docker/workflow/install-script paths so PRs get a build validation. dry_run=true skips GHCR login and push. Signed-off-by: Wei Gong <simolin@gmail.com> Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/_build-engine-image.yml:
- Around line 121-130: Normalize SOURCE_BUILD_REPO before emitting it by
removing any credentials, protocol, host prefix and converting SSH host-style to
owner/repo form: trim any leading "https://", "http://", optional "user@...@",
strip "git@" and "ssh://" prefixes, convert "github.com:org/repo" to "org/repo"
(e.g. by removing everything up to and including "github.com" and any separating
":" or "/"), then remove trailing ".git" and any trailing slashes; replace the
existing repo normalization (repo="${SOURCE_BUILD_REPO}" ... repo="${repo%/}")
with this single robust sequence so the emitted echo "repo=${repo}" >>
"$GITHUB_OUTPUT" always contains "owner/repo" suitable for checkouts (apply same
normalization at the other occurrence noted around line 136).
- Around line 172-181: The tag computation can produce a blank ENGINE_VERSION
for TRT-LLM source builds because the script only checks ENGINE_REPO and
BASE_IMAGE_REF; update the logic that sets IMAGE_TAG/ENGINE_VERSION (the block
using TAG_OVERRIDE, ENGINE_REPO, ENGINE_COMMIT, BASE_IMAGE_REF) to also consider
the source-build inputs (e.g., source_build_repo and source_build_ref or their
env equivalents) when ENGINE_REPO is empty so ENGINE_VERSION is derived from
source_build_ref (sanitizing slashes/spaces like ENGINE_COMMIT is) before
falling back to BASE_IMAGE_REF; ensure the same sanitization (replace "/" and
trim spaces) is applied so the final tag never becomes "${SMG_VERSION}-trtllm-"
with a blank suffix.
- Around line 103-110: The validation block that currently only rejects missing
base_image_ref for trtllm and rejects source_build_repo for non-trtllm must also
reject an empty BASE_IMAGE_REF for non-trtllm engines: in the same validation
area containing the two if checks, add a guard that if [ "${ENGINE}" != "trtllm"
] && [ -z "${BASE_IMAGE_REF}" ]; then echo an ERROR stating base_image_ref is
required for non-trtllm engines to stderr and exit 1; ensure this check runs
together with the existing engine/source_build_repo logic so the workflow fails
fast rather than letting a fabricated smg-build/release:${IMAGE_TAG} be used
later.
- Around line 163-170: The step extracts SMG_VERSION from
bindings/python/pyproject.toml in the workflow repo, ignoring the inputs
smg_repo and smg_commit; update the step so it checks out (or switches to) the
requested SMG ref before reading the file: use inputs.smg_repo and
inputs.smg_commit (or perform a git fetch + git checkout of that ref into a
known directory) and then run the SMG_VERSION extraction against that checkout's
bindings/python/pyproject.toml (the line that sets SMG_VERSION should point to
the checked-out repo path rather than the workflow repo).
- Around line 221-228: The workflow currently passes ENGINE_REPO and SMG_REPO as
build-args which the Dockerfile uses in RUN git clone, risking credential
exposure; change the workflow to avoid embedding repo URLs as build-args by
either (A) pre-checking-out the repositories with actions/checkout (use
repository/input variables to fetch ENGINE and SMG into workspace paths) and
pass those local paths to the docker build context or as ARGs, or (B) use
BuildKit secrets (docker build --secret id=git_credentials, and in
docker/engine.Dockerfile switch the RUN git clone steps to use RUN
--mount=type=secret,id=git_credentials ... or a credential helper that reads the
secret) so credentials are mounted at build time instead of being visible in
build logs; update the workflow step that sets build-args (ENGINE_REPO,
SMG_REPO) and the docker/engine.Dockerfile git clone lines to follow one of
these secure approaches.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 687fb118-3d2f-4f75-9350-94b7b0cc016d
📒 Files selected for processing (1)
.github/workflows/_build-engine-image.yml
…ow (#658) Signed-off-by: Simo Lin <linsimo.mark@gmail.com>
1 participant
Summary
Replace 3 near-identical workflow files (500+ lines total) and a 4-target Dockerfile with a single reusable workflow + thin wrappers.
What changed
Dockerfile (
docker/engine.Dockerfile, renamed fromDockerfile.engine):ENGINEbuild arginstall-${ENGINE}.shselected at runtime, no more--targetReusable workflow (
_build-engine-image.yml, new):source_build_repo/source_build_ref)dry_runinput for PR validation (build without push)Thin wrappers (3 files, ~50 lines each):
release-sglang-docker.yml: 148 → 57 linesrelease-vllm-docker.yml: 144 → 53 linesrelease-trtllm-docker.yml: 213 → 54 linesBehavior preserved
{smg_version}-{engine}-{engine_version})Test plan
Summary by CodeRabbit