Skip to content

[Misc] Removed force_fp8_e4m3fnuz from FP8LinearOp #23725

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
ProExpertProg merged 8 commits into from
Sep 4, 2025
Merged

[Misc] Removed force_fp8_e4m3fnuz from FP8LinearOp #23725

ProExpertProg merged 8 commits into from
Sep 4, 2025

Conversation

@nvjullin
Copy link
Contributor

@nvjullin nvjullin commented Aug 27, 2025 •
edited by github-actions bot
Loading

Purpose

Follow up on #22895.
Removed force_fp8_e4m3fnuz and monkey-patch to test for torch code path on cutlass_fp8_supported platforms.

Test Plan

Test Result

Essential Elements of an Effective PR Description Checklist
  • The purpose of the PR, such as "Fix some issue (link existing issues this PR will resolve)".
  • The test plan, such as providing test command.
  • The test results, such as pasting the results comparison before and after, or e2e results
  • (Optional) The necessary documentation update, such as updating supported_models.md and examples for a new model.
  • (Optional) Release notes update. If your change is user facing, please update the release notes draft in the Google Doc.

@nvjullin
removed force_fp8_e4m3fnuz
f190ee1 
Signed-off-by: Julien Lin <jullin@nvidia.com>
gemini-code-assist[bot]
gemini-code-assist bot reviewed Aug 27, 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 refactors the Fp8LinearOp to remove the force_fp8_e4m3fnuz parameter, simplifying its interface. The logic to force a specific backend for testing is now handled in the tests themselves using a new override_cutlass_fp8_supported context manager, which is a good improvement. However, I've found a potential issue in the test parametrization for cases where cutlass is not supported, which could leave a code path untested. My review includes suggestions to fix this.

@nvjullin nvjullin mentioned this pull request Aug 27, 2025
Merged
4 tasks
ProExpertProg
ProExpertProg approved these changes Aug 27, 2025
View reviewed changes
Copy link
Collaborator

@ProExpertProg ProExpertProg left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for this follow up. Could you just add quick comments in the tests that we do this in order to test fusion for the non-cutlass path on cutlass platform?

@nvjullin
fixed test parameter and added comment
30c5069 
Signed-off-by: Julien Lin <jullin@nvidia.com>
@mergify
Copy link

mergify bot commented Aug 30, 2025

This pull request has merge conflicts that must be resolved before it can be
merged. Please rebase the PR, @nvjullin.

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/working-with-forks/syncing-a-fork

@mergify mergify bot added the needs-rebase label Aug 30, 2025
@mergify mergify bot removed the needs-rebase label Sep 2, 2025
@ProExpertProg ProExpertProg enabled auto-merge (squash) September 2, 2025 12:49
@github-actions github-actions bot added the ready ONLY add when PR is ready to merge/full CI is needed label Sep 2, 2025
ProExpertProg
ProExpertProg reviewed Sep 2, 2025
View reviewed changes
ProExpertProg
ProExpertProg reviewed Sep 2, 2025
View reviewed changes
@ProExpertProg
Copy link
Collaborator

ProExpertProg commented Sep 2, 2025

@nvjullin it looks like the test failure is related, trying a fix

ProExpertProg and others added 4 commits September 2, 2025 14:46
@ProExpertProg
Update tests/compile/test_silu_mul_quant_fusion.py
c735c6b 
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
@ProExpertProg
relative utils import
a6aaac4 
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
@nvjullin
Merge branch 'main' into flashinfer-fp8-gemms
f223100
@nvjullin
fixed pre-commit
e6e4daf 
Signed-off-by: Julien Lin <jullin@nvidia.com>
auto-merge was automatically disabled September 3, 2025 09:00

Head branch was pushed to by a user without write access

@ProExpertProg ProExpertProg merged commit 3724107 into vllm-project:main Sep 4, 2025
44 checks passed
@elvischenv elvischenv mentioned this pull request Sep 5, 2025
Merged
5 tasks
eicherseiji pushed a commit to eicherseiji/vllm that referenced this pull request Sep 9, 2025
@nvjullin @ProExpertProg @eicherseiji
[Misc] Removed force_fp8_e4m3fnuz from FP8LinearOp (vllm-project#23725)
2318ce5 
Signed-off-by: Julien Lin <jullin@nvidia.com>
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
skyloevil pushed a commit to skyloevil/vllm that referenced this pull request Sep 13, 2025
@nvjullin @ProExpertProg @skyloevil
[Misc] Removed force_fp8_e4m3fnuz from FP8LinearOp (vllm-project#23725)
ba64638 
Signed-off-by: Julien Lin <jullin@nvidia.com>
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
FeiDaLI pushed a commit to FeiDaLI/vllm that referenced this pull request Sep 25, 2025
@nvjullin @ProExpertProg @FeiDaLI
[Misc] Removed force_fp8_e4m3fnuz from FP8LinearOp (vllm-project#23725)
b82bab0 
Signed-off-by: Julien Lin <jullin@nvidia.com>
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 10, 2025
@nvjullin @ProExpertProg @xuebwang-amd
[Misc] Removed force_fp8_e4m3fnuz from FP8LinearOp (vllm-project#23725)
1eb3921 
Signed-off-by: Julien Lin <jullin@nvidia.com>
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
xuebwang-amd pushed a commit to xuebwang-amd/vllm that referenced this pull request Oct 24, 2025
@nvjullin @ProExpertProg @xuebwang-amd
[Misc] Removed force_fp8_e4m3fnuz from FP8LinearOp (vllm-project#23725)
d2d948c 
Signed-off-by: Julien Lin <jullin@nvidia.com>
Signed-off-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Co-authored-by: Luka Govedič <ProExpertProg@users.noreply.github.com>
Signed-off-by: xuebwang-amd <xuebwang@amd.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ProExpertProg ProExpertProg ProExpertProg approved these changes

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

@robertgshaw2-redhat robertgshaw2-redhat Awaiting requested review from robertgshaw2-redhat robertgshaw2-redhat is a code owner

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

@yewentao256 yewentao256 Awaiting requested review from yewentao256 yewentao256 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

ready ONLY add when PR is ready to merge/full CI is needed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@nvjullin @ProExpertProg