TEMP changes for matching #22856
TEMP changes for matching #22856
Conversation
Signed-off-by: Luka Govedic <[email protected]>
|
👋 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 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 🚀 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors the FP8 quantization logic to use a QuantFP8 module, which is a good abstraction. The fusion passes are updated accordingly to match this new pattern. While the changes are generally moving in the right direction, I've identified a few critical issues, including commented-out code that disables functionality or tests, and leftover debugging statements. These should be addressed before this pull request can be merged.
| # if current_platform.has_device_capability(100): | ||
| # AllReduceFusedRMSNormStaticQuantNVFP4Pattern( | ||
| # epsilon, | ||
| # self.model_dtype, | ||
| # self.device, | ||
| # self.allreduce_params, | ||
| # ).register(self.patterns) | ||
| # AllReduceFusedAddRMSNormStaticQuantNVFP4Pattern( | ||
| # epsilon, | ||
| # self.model_dtype, | ||
| # self.device, | ||
| # self.allreduce_params, | ||
| # ).register(self.patterns) | ||
| # AllReduceRMSNormPattern( | ||
| # epsilon, | ||
| # self.model_dtype, | ||
| # self.device, | ||
| # self.allreduce_params, | ||
| # ).register(self.patterns) | ||
| # AllReduceFusedAddRMSNormPattern( | ||
| # epsilon, | ||
| # self.model_dtype, | ||
| # self.device, | ||
| # self.allreduce_params, | ||
| # ).register(self.patterns) |
There was a problem hiding this comment.
This large block of commented-out code disables several fusion patterns, including those for FP4 and non-quantized RMSNorm. This appears to be a significant functional regression. If this is a temporary change for development, it should be reverted before merging. If this is intentional, it needs a clear justification.
| torch.testing.assert_close(result, result2, atol=ATOL, rtol=RTOL) | ||
|
|
||
| # In pre-nodes, fp8 quant should be there and fused kernels should not | ||
| # backend.check_before_ops(model.ops_in_model_before()) |
There was a problem hiding this comment.
This assertion is commented out. In test files, assertions should not be commented out as it can hide regressions or bugs. If this check is failing, it should be investigated and fixed. Please either re-enable this check or provide a justification for its removal.
| print(backend.graph_pre_pass) | ||
| print(backend.graph_post_pass) | ||
| for node in find_op_nodes( | ||
| torch.ops.vllm.flashinfer_trtllm_fused_allreduce_norm.default, | ||
| backend.graph_post_pass): | ||
| print(f"{node.args=}") | ||
| print(f"{node.kwargs=}") | ||
|
|
There was a problem hiding this comment.
These print statements and the following loop appear to be debugging code. They should be removed before merging to keep the test output clean.
Signed-off-by: Luka Govedic <[email protected]>
…d fused_add_rms_norm Signed-off-by: Luka Govedic <[email protected]>
ProExpertProg
force-pushed
the
luka/torch-custom-op-pattern
branch
from
6374e53 to
5eeb376
Compare
Signed-off-by: Luka Govedič <[email protected]>
|
This pull request has merge conflicts that must be resolved before it can be |
|
Superseeded by #24604 |
github-project-automation
bot
moved this from In progress
to Done
in torch.compile integration
1 participant
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.Purpose
Test Plan
Test Result
(Optional) Documentation Update