Repository navigation
ci(docker): move image builds to cpu-e5 runner and parallelize make - #1372
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
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 (4)
📝 WalkthroughWalkthroughGitHub Actions Docker build workflows are retargeted from GPU runners ( Changes
Sequence Diagram(s)(none) Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/installation/install-smg.sh`:
- Line 8: The assignment to MAKEFLAGS uses command substitution inline which can
mask nproc failures under set -e; change to first capture nproc into a variable
(e.g. cpu_count=$(nproc) and check that it succeeded), then set and export
MAKEFLAGS using that variable (MAKEFLAGS="-j${cpu_count}" and export MAKEFLAGS)
so a failing nproc causes an error/exit instead of leaving MAKEFLAGS as "-j".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 15511f53-1256-4808-9e2a-2f2e183feb76
📒 Files selected for processing (4)
.github/workflows/_build-engine-image.yml.github/workflows/nightly-docker.yml.github/workflows/release-docker.ymlscripts/installation/install-smg.sh
585ee68 to
5d5c2dd
Compare
Docker image builds don't touch CUDA — the GPU runner was wasted on a CPU/IO-bound workload (protoc, rustup, maturin, pip). Switch the three image-build workflows to the cpu-e5 runner label. Also export MAKEFLAGS=-j$(nproc) in install-smg.sh so the vendored-openssl compile triggered by `maturin build --features vendored-openssl` fans out across all cores instead of running single-threaded. Signed-off-by: key4ng <rukeyang@gmail.com>
5d5c2dd to
b6a7fed
Compare
Description
Problem
The three Docker-image-build workflows (
_build-engine-image.yml,release-docker.yml,nightly-docker.yml) all run onruns-on: ["8-gpu-h200"]. Nothing in the build path actually uses the GPU:docker/engine.Dockerfileonly clones sources and runs shell install scripts inside the base image (nonvcc, no CUDA-arch compile).scripts/installation/install-smg.shdoes apt → protoc → rustup →maturin build --release --features vendored-openssl→ pip install — all CPU/IO-bound.install-*.shscripts are 10–18 lines each and contain no GPU work.Meanwhile the
vendored-opensslfeature compiles OpenSSL from C source inside abuild.rs, but the innermakeruns single-threaded by default — it doesn't inherit cargo's parallelism. On a 64-vCPU host this step wastes ~90 s of wall-clock on one core.Solution
["8-gpu-h200"]to["cpu-e5"](a new OCIVM.Standard.E5.Flexself-hosted runner).nightly-benchmark.ymlis intentionally left alone — those jobs do need GPU.MAKEFLAGS="-j$(nproc)"at the top ofinstall-smg.shso every downstreammakeinvocation (notablyopenssl-src's build.rs) fans out across all cores.Changes
.github/workflows/_build-engine-image.yml—runs-onlabel →cpu-e5.github/workflows/release-docker.yml—runs-onlabel →cpu-e5.github/workflows/nightly-docker.yml—runs-onlabel →cpu-e5scripts/installation/install-smg.sh— exportMAKEFLAGS="-j$(nproc)"before any build stepTest Plan
_build-engine-imageviaworkflow_dispatchwithdry_run: trueforengine: sglangand confirm it lands on the newcpu-e5runner and completes successfully.release-docker(workflow_dispatch, no version bump → push=false path) and confirm the build step succeeds on the new runner.cpu-e5self-hosted runner is registered on the repo and showsIdlebefore kicking off any workflow.Checklist
cargo +nightly fmtpasses (N/A — no Rust changes)cargo clippy --all-targets --all-features -- -D warningspasses (N/A — no Rust changes)Summary by CodeRabbit