Skip to content

[Bugfix][ROCm][Build] Fix build regression on ROCm #21393

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 2 commits into from
Jul 23, 2025
Merged

[Bugfix][ROCm][Build] Fix build regression on ROCm #21393

merged 2 commits into from
Jul 23, 2025

Conversation

@gshtras
Copy link
Collaborator

@gshtras gshtras commented Jul 22, 2025 •
edited by github-actions bot
Loading

Moving CUDA specific file to the ifdef CUDA section
Fix regression from #21083

@gshtras
Fix build on ROCm
a827c74 
Signed-off-by: Gregory Shtrasberg <[email protected]>
@github-actions
Copy link

github-actions bot commented Jul 22, 2025

👋 Hi! Thank you for contributing to the vLLM project.

💬 Join our developer Slack at https://slack.vllm.ai to discuss your PR in #pr-reviews, coordinate on features in #feat- channels, or join special interest groups in #sig- channels.

Just a reminder: PRs would not trigger full CI run by default. Instead, it would only run fastcheck CI which starts running only a small and essential subset of CI tests to quickly catch errors. You can run other CI tests on top of those by going to your fastcheck build on Buildkite UI (linked in the PR checks section) and unblock them. If you do not have permission to unblock, ping simon-mo or khluu to add you in our Buildkite org.

Once the PR is approved and ready to go, your PR reviewer(s) can run CI to test the changes comprehensively before merging.

To run CI, PR reviewers can either: Add ready label to the PR or enable auto-merge.

🚀

@gshtras gshtras added the rocm Related to AMD ROCm label Jul 22, 2025
@mergify mergify bot added the ci/build label Jul 22, 2025
gemini-code-assist[bot]
gemini-code-assist bot reviewed Jul 22, 2025
View reviewed changes
Copy link
Contributor

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

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 fixes a build regression on ROCm by correctly scoping a CUDA-specific quantization kernel and its corresponding PyTorch operator registration to only be included in CUDA builds. I've added one high-severity comment regarding a potential future build issue due to an unconditional function declaration in a header file that should also be made conditional to ensure long-term maintainability.

csrc/torch_bindings.cpp
Comment on lines 617 to +626
"scale_ue8m0) -> ()");
ops.impl("per_token_group_fp8_quant", torch::kCUDA,
&per_token_group_quant_fp8);

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot Jul 22, 2025

Choose a reason for hiding this comment

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

high

This change correctly makes the registration of per_token_group_fp8_quant conditional for CUDA builds. To prevent potential linker errors on ROCm builds, it's highly recommended to also wrap the declaration of per_token_group_fp8_quant in csrc/ops.h with #ifndef USE_ROCM.

Copy link
Member

@yewentao256 yewentao256 Jul 22, 2025

Choose a reason for hiding this comment

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

Nice bot!

@mgoin mgoin mentioned this pull request Jul 22, 2025
Closed
yewentao256
yewentao256 reviewed Jul 22, 2025
View reviewed changes
Copy link
Member

@yewentao256 yewentao256 left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

csrc/torch_bindings.cpp
Comment on lines 617 to +626
"scale_ue8m0) -> ()");
ops.impl("per_token_group_fp8_quant", torch::kCUDA,
&per_token_group_quant_fp8);

Copy link
Member

@yewentao256 yewentao256 Jul 22, 2025

Choose a reason for hiding this comment

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

Nice bot!

mgoin
mgoin approved these changes Jul 22, 2025
View reviewed changes
Copy link
Member

@mgoin mgoin left a comment

Choose a reason for hiding this comment

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

Thank you for fixing this, apologies for the disruption

@mgoin mgoin enabled auto-merge (squash) July 22, 2025 17:15
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Jul 22, 2025
@vllm-bot vllm-bot merged commit 3ec7170 into vllm-project:main Jul 23, 2025
96 of 98 checks passed
@tjtanaa tjtanaa mentioned this pull request Jul 23, 2025
Closed
zixi-qi pushed a commit to zixi-qi/vllm that referenced this pull request Jul 23, 2025
@gshtras @zixi-qi
[Bugfix][ROCm][Build] Fix build regression on ROCm (vllm-project#21393)
701a331 
Signed-off-by: Gregory Shtrasberg <[email protected]>
Signed-off-by: qizixi <[email protected]>
@gshtras gshtras deleted the rocm_build_fix branch August 5, 2025 16:52
x22x22 pushed a commit to x22x22/vllm that referenced this pull request Aug 5, 2025
@gshtras @x22x22
[Bugfix][ROCm][Build] Fix build regression on ROCm (vllm-project#21393)
1cea502 
Signed-off-by: Gregory Shtrasberg <[email protected]>
Signed-off-by: x22x22 <[email protected]>
Pradyun92 pushed a commit to Pradyun92/vllm that referenced this pull request Aug 6, 2025
@gshtras
[Bugfix][ROCm][Build] Fix build regression on ROCm (vllm-project#21393)
aebeb75 
Signed-off-by: Gregory Shtrasberg <[email protected]>
npanpaliya pushed a commit to odh-on-pz/vllm-upstream that referenced this pull request Aug 6, 2025
@gshtras @npanpaliya
[Bugfix][ROCm][Build] Fix build regression on ROCm (vllm-project#21393)
6736335 
Signed-off-by: Gregory Shtrasberg <[email protected]>
jinzhen-lin pushed a commit to jinzhen-lin/vllm that referenced this pull request Aug 9, 2025
@gshtras @jinzhen-lin
[Bugfix][ROCm][Build] Fix build regression on ROCm (vllm-project#21393)
a88a793 
Signed-off-by: Gregory Shtrasberg <[email protected]>
Signed-off-by: Jinzhen Lin <[email protected]>
paulpak58 pushed a commit to paulpak58/vllm that referenced this pull request Aug 13, 2025
@gshtras @paulpak58
[Bugfix][ROCm][Build] Fix build regression on ROCm (vllm-project#21393)
f17fec1 
Signed-off-by: Gregory Shtrasberg <[email protected]>
Signed-off-by: Paul Pak <[email protected]>
diegocastanibm pushed a commit to diegocastanibm/vllm that referenced this pull request Aug 15, 2025
@gshtras @diegocastanibm
[Bugfix][ROCm][Build] Fix build regression on ROCm (vllm-project#21393)
a2ec109 
Signed-off-by: Gregory Shtrasberg <[email protected]>
Signed-off-by: Diego-Castan <[email protected]>
epwalsh pushed a commit to epwalsh/vllm that referenced this pull request Aug 28, 2025
@gshtras @epwalsh
[Bugfix][ROCm][Build] Fix build regression on ROCm (vllm-project#21393)
3296ca5 
Signed-off-by: Gregory Shtrasberg <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@yewentao256 yewentao256 yewentao256 left review comments

@mgoin mgoin mgoin approved these changes

@tlrmchlsmth tlrmchlsmth Awaiting requested review from tlrmchlsmth tlrmchlsmth is a code owner

@LucasWilkinson LucasWilkinson Awaiting requested review from LucasWilkinson LucasWilkinson is a code owner

+1 more reviewer

@gemini-code-assist gemini-code-assist[bot] gemini-code-assist[bot] left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

ci/build ready ONLY add when PR is ready to merge/full CI is needed rocm Related to AMD ROCm

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@gshtras @mgoin @yewentao256 @vllm-bot