Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 3 additions & 3 deletions .github/workflows/_build-engine-image.yml
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@ on:
workflow_call:
inputs:
engine:
description: 'Engine name: vllm, sglang, trtllm, tgl'
description: 'Engine name: vllm, sglang, trtllm, tgl, tokenspeed'
required: true
type: string
backend:
Expand Down Expand Up @@ -102,8 +102,8 @@ jobs:
SOURCE_BUILD_REPO: ${{ inputs.source_build_repo }}
run: |
case "${ENGINE}" in
vllm|sglang|trtllm|tgl) ;;
*) echo "ERROR: Unknown engine '${ENGINE}'. Must be one of: vllm, sglang, trtllm, tgl." >&2; exit 1 ;;
vllm|sglang|trtllm|tgl|tokenspeed) ;;

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Add TokenSpeed serve support before enabling images

Accepting tokenspeed here lets the new release/nightly callers build images with ENGINE=tokenspeed; with no backend override, docker/engine.Dockerfile:53 exports SMG_DEFAULT_BACKEND=tokenspeed, but smg serve still builds its choices only from BACKEND_ARG_ADDERS (sglang, vllm, trtllm) in bindings/python/src/smg/serve.py:438-459. As a result the produced TokenSpeed all-in-one image fails argument parsing before it can start a worker, and there is no valid --backend tokenspeed path; please add a TokenSpeed launcher/parser entry or avoid setting an unsupported default for these images.

Useful? React with 👍 / 👎.

*) echo "ERROR: Unknown engine '${ENGINE}'. Must be one of: vllm, sglang, trtllm, tgl, tokenspeed." >&2; exit 1 ;;
esac
if [ "${ENGINE}" = "trtllm" ] && [ -z "${BASE_IMAGE_REF}" ] && [ -z "${SOURCE_BUILD_REPO}" ]; then
echo "ERROR: For trtllm, either base_image_ref or source_build_repo must be set." >&2
Expand Down
3 changes: 3 additions & 0 deletions .github/workflows/nightly-engine-docker.yml
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,9 @@ jobs:
- engine: trtllm
base_image_ref: nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc18
engine_ver: 1.3.0rc18
- engine: tokenspeed
base_image_ref: lightseekorg/tokenspeed:tml
engine_ver: tml
uses: ./.github/workflows/_build-engine-image.yml
with:
engine: ${{ matrix.engine }}
Expand Down
74 changes: 74 additions & 0 deletions .github/workflows/release-tokenspeed-docker.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
name: Release SMG+TokenSpeed Docker Image

run-name: >-
SMG+TokenSpeed |
${{ github.event_name == 'push' && 'base=lightseekorg/tokenspeed:tml' || format('base={0}', inputs.base_image_ref || 'lightseekorg/tokenspeed:tml') }} |
engine=${{ inputs.tokenspeed_commit || 'latest' }} |
smg=${{ github.event_name == 'pull_request' && 'main' || inputs.smg_commit || 'v1.7.0' }} |

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Build the triggering revision, not main or v1.7.0. The reusable workflow currently checks out main for PR dry runs and v1.7.0 for pushes, so automatic runs never validate or publish the revision that triggered them. Pass the event SHA/ref through smg_commit and keep the manual input override; for fork PRs, pass the head repository/ref pair as well.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/release-tokenspeed-docker.yml at line 7, Update the
workflow’s smg assignment to default to the triggering event’s SHA/ref instead
of hardcoded main or v1.7.0, while preserving the manual inputs.smg_commit
override. For pull requests from forks, also pass the head repository and head
ref through the reusable workflow inputs so checkout targets the PR’s actual
revision.

${{ github.event_name == 'pull_request' && 'dry-run |' || '' }}
by @${{ github.actor }}

on:
push:
branches: [main]
paths:
- bindings/python/pyproject.toml
Comment on lines +12 to +15

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf '\n== workflow file ==\n'
wc -l .github/workflows/release-tokenspeed-docker.yml
cat -n .github/workflows/release-tokenspeed-docker.yml

printf '\n== search related references ==\n'
rg -n "release-tokenspeed-docker|pyproject.toml|docker|publish|dry run|pull_request|push:" .github/workflows bindings -S

Repository: lightseekorg/smg

Length of output: 26489


🏁 Script executed:

#!/bin/bash
set -euo pipefail
sed -n '1,220p' .github/workflows/release-tokenspeed-docker.yml | cat -n

Repository: lightseekorg/smg

Length of output: 3288


Broaden the push trigger for TokenSpeed releases. push only watches bindings/python/pyproject.toml, so Dockerfile, installation-script, or reusable-workflow changes merged to main won’t publish a new image unless the version file also changes. Add the release-relevant paths here, or require a version bump before merge.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/release-tokenspeed-docker.yml around lines 12 - 15,
Broaden the push path filters in the release workflow’s push trigger to include
the TokenSpeed Dockerfile, installation script, and reusable workflow paths
alongside bindings/python/pyproject.toml, so relevant changes merged to main
publish a new image.

pull_request:
branches: [main]
paths:
- 'docker/engine.Dockerfile'
- '.github/workflows/_build-engine-image.yml'
- '.github/workflows/release-*-docker.yml'
- 'scripts/installation/**'
workflow_dispatch:
inputs:
base_image_ref:
description: 'Base image (e.g. lightseekorg/tokenspeed:tml). Engine is baked in; leave engine repo empty.'
default: 'lightseekorg/tokenspeed:tml'
required: true
type: string
tokenspeed_repo:
description: 'TokenSpeed repo URL (empty = use engine from base image)'
required: false
type: string
tokenspeed_commit:
description: 'TokenSpeed commit/ref ("latest" for HEAD)'
required: false
default: 'latest'
type: string
smg_repo:
description: 'SMG repo URL'
required: false
default: 'https://github.com/lightseekorg/smg'
type: string
smg_commit:
description: 'SMG commit/ref ("latest" for HEAD)'
required: false
default: 'v1.7.0'

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Register TokenSpeed workflow in release version sync

This new workflow has the same smg_commit default as the other engine release workflows, but scripts/check_release_versions.sh:96-104 only tracks sglang/vllm/trtllm in SMG_VERSION_SYNC, and the release check/update loop uses only that list to validate and rewrite workflow defaults. On the next SMG version bump, make check-release-versions will not flag or update this TokenSpeed default, so manual TokenSpeed releases will keep building v1.7.0 unless the operator remembers to override the input.

Useful? React with 👍 / 👎.

type: string
tag:
description: 'Override image tag (e.g. v1.7.0-tokenspeed-tml)'
required: false
type: string

permissions:
contents: read
packages: write
Comment on lines +54 to +56

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🔵 Trivial | 💤 Low value

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

file=".github/workflows/release-tokenspeed-docker.yml"

echo "== file outline =="
ast-grep outline "$file" --view expanded || true

echo
echo "== numbered file excerpt =="
cat -n "$file"

Repository: lightseekorg/smg

Length of output: 3398


Scope packages: write to jobs.build. Workflow-level permissions apply to the whole workflow, so any later job would inherit registry-write access.

🧰 Tools
🪛 zizmor (1.26.1)

[error] 56-56: overly broad permissions (excessive-permissions): packages: write is overly broad at the workflow level

(excessive-permissions)


[warning] 56-56: permissions without explanatory comments (undocumented-permissions): needs an explanatory comment

(undocumented-permissions)

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/release-tokenspeed-docker.yml around lines 54 - 56, Move
packages: write from the workflow-level permissions block into the jobs.build
permissions block, while retaining contents: read at the appropriate scope.
Ensure only build receives registry-write access and other jobs do not inherit
it.

Source: Linters/SAST tools


jobs:
build:
strategy:
fail-fast: false
matrix:
base_image: ${{ github.event_name == 'push' && fromJSON('["lightseekorg/tokenspeed:tml"]') || fromJSON(format('["{0}"]', inputs.base_image_ref || 'lightseekorg/tokenspeed:tml')) }}
uses: ./.github/workflows/_build-engine-image.yml
with:
engine: tokenspeed
base_image_ref: ${{ matrix.base_image }}
engine_repo: ${{ inputs.tokenspeed_repo }}
engine_commit: ${{ inputs.tokenspeed_commit || 'latest' }}
smg_repo: ${{ inputs.smg_repo || 'https://github.com/lightseekorg/smg' }}
smg_commit: ${{ github.event_name == 'pull_request' && 'main' || inputs.smg_commit || 'v1.7.0' }}
tag: ${{ inputs.tag }}
dry_run: ${{ github.event_name == 'pull_request' }}
secrets: inherit

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick win

Avoid inheriting all caller secrets.

Pass only the named secret(s) required by the reusable workflow, such as GH_SYNC_TOKEN if needed, and declare them explicitly in the called workflow. secrets: inherit unnecessarily broadens the secret surface of this release job.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In @.github/workflows/release-tokenspeed-docker.yml at line 74, Replace secrets:
inherit in the release workflow’s reusable-workflow invocation with an explicit
mapping of only the required secret(s), such as GH_SYNC_TOKEN if used. Declare
each passed secret in the called workflow’s workflow_call configuration and
remove reliance on inherited secrets.

Source: Linters/SAST tools

22 changes: 20 additions & 2 deletions docker/engine.Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@
#
# Build args:
# BASE_IMAGE_REF - full image:tag to start FROM
# ENGINE - engine name: vllm | sglang | trtllm | tgl
# ENGINE - engine name: vllm | sglang | trtllm | tgl | tokenspeed
# BACKEND - SMG_DEFAULT_BACKEND value (defaults to ENGINE; tgl overrides to sglang)
# ENGINE_REPO - if set, engine source is cloned and install-<ENGINE>.sh runs
# ENGINE_COMMIT - commit/ref for ENGINE_REPO ("latest" = HEAD)
Expand Down Expand Up @@ -44,7 +44,7 @@
COPY scripts/installation/ /tmp/scripts/

# ── final stage: install SMG + conditionally install engine ──────────────────
FROM ${BASE_IMAGE_REF}

Check warning on line 47 in docker/engine.Dockerfile

View workflow job for this annotation

GitHub Actions / build (vllm/vllm-openai:v0.22.1) / build-and-push

Default value for global ARG results in an empty or invalid base image name

InvalidDefaultArgInFrom: Default value for ARG ${BASE_IMAGE_REF} results in empty or invalid base image name More info: https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/

Check warning on line 47 in docker/engine.Dockerfile

View workflow job for this annotation

GitHub Actions / build (lmsysorg/sglang:v0.5.12.post1) / build-and-push

Default value for global ARG results in an empty or invalid base image name

InvalidDefaultArgInFrom: Default value for ARG ${BASE_IMAGE_REF} results in empty or invalid base image name More info: https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/

Check warning on line 47 in docker/engine.Dockerfile

View workflow job for this annotation

GitHub Actions / build (lightseekorg/tokenspeed:tml) / build-and-push

Default value for global ARG results in an empty or invalid base image name

InvalidDefaultArgInFrom: Default value for ARG ${BASE_IMAGE_REF} results in empty or invalid base image name More info: https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/

Check warning on line 47 in docker/engine.Dockerfile

View workflow job for this annotation

GitHub Actions / build (nvcr.io/nvidia/tensorrt-llm/release:1.3.0rc18) / build-and-push

Default value for global ARG results in an empty or invalid base image name

InvalidDefaultArgInFrom: Default value for ARG ${BASE_IMAGE_REF} results in empty or invalid base image name More info: https://docs.docker.com/go/dockerfile/rule/invalid-default-arg-in-from/

ARG ENGINE=sglang
ARG BACKEND
Expand All @@ -66,10 +66,28 @@
pip install --no-cache-dir --ignore-installed pyyaml; \
fi

# TokenSpeed prep. The base is a debian-system-Python image, so it needs the
# same debian-shadow treatment as the NGC trtllm base plus a gRPC cleanup:
# 1. pip/pyyaml are debian-owned with no pip RECORD file, so install-smg.sh's
# `pip install --upgrade pip` (and the smg wheel's pyyaml dep) fail with
# "Cannot uninstall … RECORD file not found". Shadow them with pip-managed
# copies (--ignore-installed leaves the distro files alone) so the smg
# install can upgrade them normally.
# 2. The base bakes in `tokenspeed-smg-grpc-proto` / `tokenspeed-smg-grpc-servicer`,
# which claim the same `smg_grpc_proto` / `smg_grpc_servicer` import paths
# that install-smg.sh reinstalls from source. Left in place they can shadow
# the source installs and serve stale proto descriptors ("Method not
# found!"). Drop them first so SMG's own gRPC modules win.
# Scoped to tokenspeed so the other engine bases keep their exact pip behavior.
RUN if [ "${ENGINE}" = "tokenspeed" ]; then \
pip install --no-cache-dir --ignore-installed pip pyyaml; \

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Pin the TokenSpeed bootstrap dependencies.

Line 83 resolves pip and pyyaml to whatever versions are latest at build time, making release images nondeterministic and allowing future incompatible releases to break install-smg.sh. Use validated exact versions or a constraints file for both packages.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@docker/engine.Dockerfile` at line 83, Update the pip install command in the
Dockerfile to pin both pip and pyyaml to validated exact versions, or reference
a constraints file that fixes both versions. Preserve the existing no-cache and
ignore-installed behavior while ensuring TokenSpeed bootstrap dependencies are
deterministic.

Source: Linters/SAST tools

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Set break-system-packages before TokenSpeed pip installs

When building on the Debian system-Python TokenSpeed base described here, this first pip install still runs without PIP_BREAK_SYSTEM_PACKAGES=1 or --break-system-packages; TokenSpeed's own Dockerfile sets that env before installing packages (source). On those bases pip rejects system-environment installs before install-smg.sh runs, so the new TokenSpeed release/nightly image builds can fail before producing an image.

Useful? React with 👍 / 👎.

pip uninstall -y tokenspeed-smg-grpc-proto tokenspeed-smg-grpc-servicer || true; \
fi

RUN bash /tmp/scripts/install-smg.sh /opt/smg-src

RUN case "${ENGINE}" in \
vllm|sglang|trtllm|tgl) ;; \
vllm|sglang|trtllm|tgl|tokenspeed) ;; \
*) echo "ERROR: Unknown ENGINE '${ENGINE}'" >&2; exit 1 ;; \
esac \
&& if [ -n "${ENGINE_REPO}" ]; then \
Expand Down
15 changes: 15 additions & 0 deletions scripts/installation/install-tokenspeed.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
#!/bin/sh
set -e

# Install tokenspeed from source.
# Usage: install-tokenspeed.sh [path-to-tokenspeed-src]
# Default path: /tmp/tokenspeed-src
#
# Only used when the engine image is built with ENGINE_REPO set (build the
# engine from source into a runner base). The published lightseekorg/tokenspeed
# images already bake the engine in, so the default release path does not run
# this script.

TS_SRC="${1:-/tmp/tokenspeed-src}"
cd "${TS_SRC}/python"
pip install --no-deps --force-reinstall --editable .

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

medium

Using --editable (or -e) in a Docker image build is generally discouraged for production-ready images. It creates a direct dependency on the source directory remaining present at the exact same path at runtime. If the source directory is deleted or modified to optimize image size, the installation will break. Consider performing a standard install instead.

Suggested change
pip install --no-deps --force-reinstall --editable .
pip install --no-deps --force-reinstall .

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Install all TokenSpeed source packages

When a caller supplies tokenspeed_repo to use the advertised source-install path, this only reinstalls the top-level ./python package; the upstream install flow installs the kernel and scheduler packages as well before verification (docs). That means source-built images can keep missing or stale tokenspeed-kernel/tokenspeed-scheduler code from the base image even though the workflow labels the image with the selected TokenSpeed commit.

Useful? React with 👍 / 👎.

Loading