Skip to content

[AMD] ci: reinstall MoRI if Different from Dockerfile-pinned commit during install_dependency - #27538

Merged
yctseng0211 merged 1 commit into
mainfrom
bingxche/amd-ci-reinstall-mori
Jun 8, 2026
Merged

yctseng0211 merged 1 commit into
mainfrom
bingxche/amd-ci-reinstall-mori

Conversation

@bingxche

@bingxche bingxche commented Jun 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • The AMD CI image bakes MoRI at the commit pinned in docker/rocm.Dockerfile (MORI_COMMIT). When a PR bumps MORI_COMMIT ahead of the published image, the image is not rebuilt per-PR, so stage-b-test-large-8-gpu-mi35x-disaggregation-amd runs against a stale MoRI (e.g. missing newer IOEngine APIs such as wait_all).
  • Add a step to scripts/ci/amd/amd_ci_install_dependency.sh that reinstalls MoRI inside the running ci_sglang container from the repo/commit pinned in rocm.Dockerfile, mirroring the image's build steps — so the container matches the Dockerfile without rebuilding/republishing the image.
  • Guarded on /sgl-workspace/mori, so non-MoRI images (every other suite) skip it.

Test plan

Made with Cursor


CI States

Latest PR Test (Base): ✅ Run #27122910680
Latest PR Test (Extra): ❌ Run #27122910447

PR #26922 bumps MORI_COMMIT in docker/rocm.Dockerfile to pick up new MoRI
APIs (e.g. IOEngine.wait_all), but the published CI image still bakes the
old MoRI and is not rebuilt per-PR, so the mi35x disaggregation suite runs
against the stale library and fails.

Reinstall MoRI in amd_ci_install_dependency.sh using the repo/commit pinned
in rocm.Dockerfile (same build steps as the image build), so the running
container matches the Dockerfile without rebuilding the image. Guarded on
/sgl-workspace/mori, so non-MoRI images are unaffected.

Co-authored-by: Cursor <cursoragent@cursor.com>
@github-actions github-actions Bot added the amd label Jun 8, 2026

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request updates the AMD CI dependency installation script to dynamically reinstall MORI inside the CI container if the workspace directory exists, ensuring that any updates to the MORI commit in the Dockerfile are reflected in the CI run. The reviewer suggested a safer way to pass host environment variables into the Docker container using docker exec -e and single-quoted strings to prevent shell parsing and escaping issues.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +199 to +210
docker exec ci_sglang bash -c "
set -euo pipefail
export MORI_GPU_ARCHS='${MORI_GPU_ARCHS}'
rm -rf /sgl-workspace/mori
git clone '${MORI_REPO}' /sgl-workspace/mori
cd /sgl-workspace/mori
git checkout '${MORI_COMMIT}'
git submodule update --init --recursive
python3 setup.py develop
python3 -c 'import os, torch; print(os.path.join(os.path.dirname(torch.__file__), \"lib\"))' > /etc/ld.so.conf.d/torch.conf
ldconfig
"

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

Passing host variables directly into a double-quoted docker exec bash -c "..." string is fragile and can lead to shell parsing or escaping issues if the variables contain special characters.

Instead, it is much safer and cleaner to pass these variables into the container using docker exec -e and use a single-quoted string for the bash -c command. This avoids host-side expansion and simplifies nested quoting/escaping.

Suggested change
docker exec ci_sglang bash -c "
set -euo pipefail
export MORI_GPU_ARCHS='${MORI_GPU_ARCHS}'
rm -rf /sgl-workspace/mori
git clone '${MORI_REPO}' /sgl-workspace/mori
cd /sgl-workspace/mori
git checkout '${MORI_COMMIT}'
git submodule update --init --recursive
python3 setup.py develop
python3 -c 'import os, torch; print(os.path.join(os.path.dirname(torch.__file__), \"lib\"))' > /etc/ld.so.conf.d/torch.conf
ldconfig
"
docker exec \
-e MORI_GPU_ARCHS="${MORI_GPU_ARCHS}" \
-e MORI_REPO="${MORI_REPO}" \
-e MORI_COMMIT="${MORI_COMMIT}" \
ci_sglang bash -c '
set -euo pipefail
rm -rf /sgl-workspace/mori
git clone "$MORI_REPO" /sgl-workspace/mori
cd /sgl-workspace/mori
git checkout "$MORI_COMMIT"
git submodule update --init --recursive
python3 setup.py develop
python3 -c "import os, torch; print(os.path.join(os.path.dirname(torch.__file__), \"lib\"))" > /etc/ld.so.conf.d/torch.conf
ldconfig
'

@bingxche
bingxche marked this pull request as ready for review June 8, 2026 08:19
@bingxche bingxche changed the title [AMD] ci: reinstall MoRI from Dockerfile-pinned commit during install_dependency [AMD] ci: reinstall MoRI if Different from Dockerfile-pinned commit during install_dependency Jun 8, 2026
@yctseng0211
yctseng0211 merged commit df6b9c2 into main Jun 8, 2026
94 of 98 checks passed
@yctseng0211
yctseng0211 deleted the bingxche/amd-ci-reinstall-mori branch June 8, 2026 08:29
Chronostasys pushed a commit to MindLab-Research/sglang that referenced this pull request Aug 24, 2026
…uring install_dependency (sgl-project#27538)

Co-authored-by: Cursor <cursoragent@cursor.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants