Repository navigation
Conversation
Signed-off-by: Randall Smith <Randall.Smith@amd.com>
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
…iton wheel Move the TheRock stack from ROCm 10.0 to 10.1, keeping torch 2.13 (2.13.0+rocm10.1.0). Its torch pins triton 3.8.0+git669b31ac.rocm10.1.0, the release/internal/3.8.x commit we were building from source, so install that wheel and drop the triton source build, the torch repack and the METADATA edit. All pins resolve from the stable index, so drop the nightly extra index. The wheel/image variant becomes rocm101 (setup.py derives it from the SDK version); rename the release pipeline's rocm100 steps and the perf-eval image tag to match, and update the install docs. Co-authored-by: Claude Signed-off-by: Rohan Potdar <rohan.potdar@amd.com>
|
Documentation preview: https://vllm--62536.org.readthedocs.build/en/62536/ |
|
✅ @rasmith, CI is now available for this PR.
|
|
/ci run |
|
/amd-ci run nightly |
|
✅ Triggered Buildkite CI #96172 for commit ℹ️ This PR is 1 commit behind upstream |
|
✅ Triggered Buildkite AMD CI #15045 for commit |
Signed-off-by: Randall Smith <Randall.Smith@amd.com>
|
Hi @rasmith, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
Signed-off-by: Randall Smith <Randall.Smith@amd.com>
|
/ci run |
|
✅ Triggered Buildkite CI #100005 for commit ℹ️ This PR is 2 commits behind upstream |
|
Not a blocker, but something to keep in mind if users see RCCL perf differences from ROCm 10.1: sgl-project/sglang#45576 |
|
I suspect this patch will conflict once ROCm/rocm-systems#14510 gets merged? |
Oh nice, will this make it into 10.2? |
| - tests/compile/fusions_e2e/ | ||
| commands: | ||
| - rocm-smi | ||
| - amd-smi |
There was a problem hiding this comment.
Oh btw, this reminded me, do we need to also check ci-infra repo and replace any rocm-smi calls? is it getting fully deprecated in 10.1? or are these changes here just paving the way?
There was a problem hiding this comment.
Oh btw, this reminded me, do we need to also check
ci-infrarepo and replace anyrocm-smicalls? is it getting fully deprecated in 10.1? or are these changes here just paving the way?
I don't know much about ci-infra, but rocm-smi has been completely removed.
There was a problem hiding this comment.
it's back in 10.2 AFAIK, but we are planning to deprecate it yes :)
|
/amd-ci run nightly --allow-stale |
|
✅ Triggered Buildkite AMD CI #15770 for commit
|
Signed-off-by: Randall Smith <Randall.Smith@amd.com>
|
/ci run |
|
/amd-ci run nightly |
|
✅ Triggered Buildkite CI #100298 for commit |
|
✅ Triggered Buildkite AMD CI #15775 for commit |
rasmith
changed the title
|
/ci retry |
|
✅ Queued 2 failed job(s) for retry in Buildkite CI #100298. |
I think it might be too late for 10.2 which sucks! |
|
/ci run |
|
✅ Triggered Buildkite CI #100643 for commit |
|
/amd-ci run nightly |
|
✅ Triggered Buildkite AMD CI #15785 for commit |
|
Hi @rasmith, the pre-commit checks have failed. Please run: uv pip install pre-commit>=4.5.1
pre-commit install
pre-commit run --all-filesThen, commit the changes and push to your branch. For future commits, |
Overview
This PR updates the current ROCm
Dockerfile.rocm_baseto10.1andMORItov1.2.4.Things to note:
rocm-smi_libwas removed from10.1pip install amd-smianymore, its included in10.1MORIversionv1.2.3.post1doesn't have fix: migrate GPU topology from rocm_smi to amd_smi (ROCm 10.1+) ROCm/mori#663 butv1.2.4does, which builds with10.110.0needed to be updated for10.1(applying the patch to the newer version in rocm-systems) users/rasmith/10.1/hsa_gpu_edge_signalamd-smiis the replacement forrocm-smi, but this broke a number of things:rocm-smibeforerocm-smiremoval and made it useamd-smi.pthfile soamd-smican always be found, without having to change the Dockerfile or always have to ensure one doesn't forget to add it toPYTHONPATH. We might need to file an issue so that it gets installed properly through the usual Rock wheel installs.Rayneeded to be updated since the current version depends onrocm-smi, but the newer versions now useamd-smi.v0.1.28.post1for to avoid the perf loss due to 10.1 compiler changeExtra
amd-smihas an option if you want the oldrocm-smioutput:amd-smi --rocm-smiClaims
Validation
Details
ROCR performance check
Performance run to check that the ROCR performance fixes are preserved:
Currently builds:
Result of:
DOCKER_BUILDKIT=1 docker build -f docker/Dockerfile.rocm_base --build-arg -t vllm-base:rocm10_1 .was
Commands used to benchmark were:
Import and Install Ability
Since
amdsmiis now included in 10.1 but is not a complete package, I needed to add anamdsmibuild step in the Dockerfile and make sure that the vLLM wheels would still work when users install them. I wanted to detail the results of that here.Every table below compares the same five vLLM builds. Tables that only concern this PR use the three 10.1 ones.
bd84cb1a6f) — this PR before the.pthwas added. amdsmi viaENV PYTHONPATH.513d206965) — what the PR has carried since. amdsmi via a.pth.bc5e7f00d6) — this change. amdsmi installed as a package.The three 10.1 columns were measured on images built from those commits.
Every measurement is on one host with 8 GPUs (gfx950 / MI355X), so a correct amdsmi reports 8. A 0 means it enumerated none.
How amdsmi reaches the user
ENV PYTHONPATHENV PYTHONPATH.pthfilefind_speclookup.patchfileIn the Docker image
import amdsmiwhen CI sets PYTHONPATHpip show amdsmiThis is why the
.pthwas added. CI'srun-amd-test.shsetsPYTHONPATH=/vllm-workspace, replacing theENV PYTHONPATHthe base image relied on.import amdsmithen raisesModuleNotFoundError, vLLM falls back toUnspecifiedPlatform, and running a model fails withRuntimeError: Device string must not be empty. A.pthlives in site-packages, so nothing a caller puts inPYTHONPATHcan displace it. An installed package has the same property. Without that override every column works, so the override is the whole of the difference.Installing the vLLM wheel on a clean machine
A stock
python:3.12container — no ROCm, no vLLM — with the GPUs passed through, following the install docs:The vLLM wheel contains no amdsmi files and ships no
.pth. It only declaresRequires-Dist: amdsmi==..., and the index serves amdsmi as a separate 87 KB wheel that pip pulls in — the same arrangement as torch and triton.Nothing from the ROCm image exists here, so each column is only what its own wheel brings:
pip show amdsmiimport amdsmion its ownOSErrorModuleNotFoundErrorimport torch, thenimport amdsmi.pthis the regression: no pin, so no amdsmi, soUnspecifiedPlatformandRuntimeError: Device string must not be empty.Two independent problems sit behind those red cells:
.pthinstalls nothing — so amdsmi quietly dropped out of the vLLM wheel's requirements. It was not removed on purpose.import amdsmidoes not.The PR fixes both: an amdsmi wheel built from the SDK's own sources with the loader patched, installed in the image and pinned by the vLLM wheel.
Installing the vLLM wheel: inside the image, three ways
The same wheels, but installed inside that commit's own vLLM image rather than on a clean machine, with the image's own vLLM uninstalled first. The contrast with the table above is the point: here the image's
.pthorENV PYTHONPATHstill exists and can rescue an install that the wheel alone would not. Three ways to make the environment — the image's system Python, a plainpython3 -m venv, and the documenteduv venv --managed-python.python3:import amdsmipython3:pip show amdsmipython3 -m venv:import amdsmiuv venv --managed-python:import amdsmiimport torch(8 is correct)PYTHONPATHA
.pthis the one mechanism a user's own virtual environment throws away. It lives in the image's site-packages, so neither venv recipe sees it, and that state has no pin to install amdsmi instead.ENV PYTHONPATHsurviving a venv is a liability, not a feature. Base's pin does install amdsmi into the venv, butPYTHONPATHthen shadows it with the image's SDK copy, which pairs with the venv's own torch and reports 0 GPUs instead of 8 — a wrong answer rather than an error.Building vLLM from source
Two kinds of editable install are tested:
Both run inside that commit's own image against a bind-mounted checkout, with
PYTORCH_ROCM_ARCH=gfx950, after removing the vLLM the image ships so it cannot shadow the build. They agree on every row below.pip show amdsmiimport amdsmiThe last row is the developer-facing form of the CI failure: build editable on base under that
PYTHONPATHand vLLM cannot run a model.The docs' amdsmi step, which a source builder outside these images still needs:
pip show amdsmiafterwardsThe 10.1 SDK stopped shipping packaging metadata, so the documented
pip installfails there. The PR's step supplies that metadata and the loader patch itself, and shrinks back to one line once a future SDK restores it.AI assistance used and reviewed by the author.
Pull Request Checklist
I used vLLM's
/pr-checklistskill. (Mandatory for agents, optional for humans).AI assistance was used during the creation of this PR.
[X ] Design Fit: Minimizes impact on core components, reuses existing functionality, and justifies added complexity.
[X ] Testing and Validation: Validates the change and ensures any added tests are meaningful and reliable, with CI coverage or documented CI resource constraints and validation performed outside CI.
[ X] Code Quality and Style: Keeps code and comments clear and concise, and updates relevant documentation and examples.
Pull Request Contents: Includes a brief summary and relevant links, supports claims with evidence, explains root causes and implementation trade-offs, and follows the contributing guide.