[ROCm][Perf][Bugfix] Multi-stream perf improvements; rocprofiler fixes - #55099
Conversation
|
✅ @mawong-amd, CI is now available for this PR.
|
|
/ci run |
|
✅ Triggered Buildkite CI #87032 for commit |
|
/ci run |
|
✅ Triggered Buildkite CI #87091 for commit |
| raise | ||
| finally: | ||
| torch.distributed.destroy_process_group() | ||
| if current_platform.is_rocm(): |
There was a problem hiding this comment.
oh is this fixed by your changes/the ROCr+CLR bump? awesome!
| low_latency_mode = False | ||
| use_fp8_dispatch = False | ||
|
|
||
| if current_platform.is_rocm(): |
| && git config --global user.email "you@example.com" && git config --global user.name "Your Name" \ | ||
| && if [ ! -f setup.py ]; then cd python; fi \ | ||
| && python3 setup.py bdist_wheel --dist-dir=dist \ | ||
| && MAX_JOBS=16 python3 setup.py bdist_wheel --dist-dir=dist \ |
There was a problem hiding this comment.
There's a race condition in Triton build which we hit somewhat often when we use the default MAX_JOBS (when unset it's defaulted to $(nproc) in Triton setup.py). It's related to the issue mentioned in triton-lang/triton#6718 but is not necessarily the same manifestation. They haven't bothered to fix it and honestly it's not a blocker for us -- Triton builds quickly by itself and if it's a cold build, the build time of the Triton stage is hidden by the long build time of the other stages it builds in parallel with.
There was a problem hiding this comment.
good point; I'd forgotten about this occasional error, LGTM
| RUN --mount=type=bind,from=build_mori,src=/app/install/,target=/install \ | ||
| cp /install/*.whl /app/debs | ||
|
|
||
| ### |
There was a problem hiding this comment.
sorry just to clarify, why'd we move this down here instead of up top before the other build stages? It's a minor nit I'd just prefer ROCm stuff to be before component libs
There was a problem hiding this comment.
Some Docker build backends (not BuildKit though) invalidate all stages below when a change is made above it, even in a stage it doesn't depend on. Since the ROCR/CLR runtime builds are quick, it's pretty cheap if it's invalidated. And conversely, we don't want to invalidate expensive stages if we bump the runtime, because with the proper caching it should just be a <5 min build of the new ROCm base.
There was a problem hiding this comment.
I'm fine with this for now, I think we use buildkit by default across both CI and release pipelines and it'd be a little more organized to put it up top to track+remove it later once the fixes land in ROCm
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review. WalkthroughThe ROCm image now uses a shared ROCm systems repository for runtime and profiler builds. Runtime installation moves to the final image stage. DeepEP workers use common exception teardown, and the ROCm profiler environment workaround is removed. ChangesROCm runtime and DeepEP integration
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The ROCm image now uses the updated shared runtime build, installs it in the final image, and removes the DeepEP teardown workarounds. No concrete merge-blocking regression is identified. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
/ci run |
|
✅ Triggered Buildkite CI #87141 for commit |
torch 2.12 attaches rocprofiler-sdk implicitly and its queue interposition hangs at HSA teardown. Set ROCPROFILER_QUEUE_INTERPOSITION=0 in the final image, matching the mitigation in vllm-project#55099 for Dockerfile.rocm_base. The ROCr/CLR and rocprofiler-sdk source rebuilds from rocm_base are not ported: they target ROCm 7.2.x and ROCm 10.0 already carries those fixes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Rohan Potdar <rohan.potdar@amd.com>
torch 2.12 attaches rocprofiler-sdk implicitly and its queue interposition hangs at HSA teardown. Set ROCPROFILER_QUEUE_INTERPOSITION=0 in the final image, matching the mitigation in vllm-project#55099 for Dockerfile.rocm_base. The ROCr/CLR and rocprofiler-sdk source rebuilds from rocm_base are not ported: they target ROCm 7.2.x and ROCm 10.0 already carries those fixes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Signed-off-by: Rohan Potdar <rohan.potdar@amd.com>
|
/ci run |
|
✅ Triggered Buildkite CI #87339 for commit |
Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
vllm-project#51174)" This reverts commit c2d8009. Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
…kers (vllm-project#51173)" This reverts commit 65addac. Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
27fc2d3 to
f00cd34
Compare
|
/ci run |
|
✅ Triggered Buildkite CI #87617 for commit |
Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
|
/ci run |
|
✅ Triggered Buildkite CI #87629 for commit |
AndreasKaratzas
left a comment
There was a problem hiding this comment.
Stamping for clarity -- all AMD failures here are pre-existing.
vllm-project#55099) Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com> Signed-off-by: Jyotirmoy Roy <jyotirmoyroy649@gmail.com>
…e baseline Remove the ROCr + CLR source rebuild (build_rocm_runtime stage, final-stage COPY/relink, and the ROCM_SYSTEMS_REPO/ROCM_RUNTIME_COMMIT ARGs) from the rock base, runs the stock TheRock ROCr/CLR runtimes. Baseline for comparing ROCm 10 with and without the multi-stream runtime patches. Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
…e baseline Remove the ROCr + CLR source rebuild (build_rocm_runtime stage, final-stage COPY/relink, and the ROCM_SYSTEMS_REPO/ROCM_RUNTIME_COMMIT ARGs) from the rock base so the image runs the stock TheRock ROCr/CLR runtimes. Baseline for comparing ROCm 10 with and without the multi-stream runtime patches. Co-Authored-By: Claude Code <noreply@anthropic.com> Signed-off-by: Matthew Wong <Matthew.Wong2@amd.com>
Purpose
This PR does the following:
Relocates the ROCR+CLR build to its separate stage, instead of being a prerequisite of the
basestage. This allows ROCR+CLR updates to not invalidate + force rebuilds of all build stages. Downstream libraries currently only ever link against them dynamically, so this relocation is safe.Brings a backport of changes in rocr: Add device-resident ordering edges ROCm/rocm-systems#11212 to ROCm 7.2.4's ROCR+CLR, which resolves slow cross-stream dependencies in various workloads (primarily observed in multi-stream use cases, e.g. [Perf][ROCm] Dual-stream decode with hipgraphs #52033, [ROCm][Perf] Enable CSA multi-stream overlap for DeepSeek-V4 #51794, [ROCm] Enable shared-expert multi-stream overlap on ROCm #51117) and completes the fix series started in [Hardware][AMD][Perf][Bugfix] Update ROCr and clr in base image #53712 which only worked on certain stream events.
Brings in the aforementioned multi-stream improvements into the ROCm 10.0 image as well.
Update ROCR to contain a 7.2.4-specific backport of fix(rocr): release GWS queue in ReleaseResources to fix shutdown UAF ROCm/rocm-systems#6942. This resolves the issues previously reported in [ROCm] Work around DeepEP teardown SIGSEGV in MoE test harness #51174 and [ROCm][CI] Keep rocprofiler-sdk out of DeepEP HT MoE test workers #51173 (also occasionally seen during test teardown in some AMD CI jobs e.g. here), so those PRs are reverted as part of this change. Fixes [CI Failure]: tests/kernels/moe/test_deepep_moe.py SIGSEGV on ROCm, drop the two teardown workarounds once the image ships rocm-systems#6942 #51644.
Sets the env var
ROCPROFILER_QUEUE_INTERPOSITION=0to avoid the hangs mentioned in [Bugfix][ROCm][Build] fix profiler hang due to queue interposition bug #54171. While most of them were resolved by said PR, a few remain in some profiling workloads.Test Plan
Full AMD CI will run on the image built from this PR.
Test Result
There are no new regressions.
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.