[Bugfix] fix fuse_allreduce_rms when tp =1#30178
[Bugfix] fix fuse_allreduce_rms when tp =1#30178ProExpertProg merged 5 commits intovllm-project:mainfrom
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces a bugfix in AllReduceFusionPass to prevent a potential AttributeError in is_applicable_for_range. The change adds checks to ensure the pass is not applied if it has been disabled during initialization or if a required attribute (max_token_num) is missing. This correctly handles cases where the pass initialization exits early. While the fix is effective, I've noted a small redundancy in the added checks that could be simplified for better code clarity and maintainability.
| if not hasattr(self, "max_token_num"): | ||
| logger.warning_once( | ||
| "AllReduce fusion pass missing max token bound; skipping", | ||
| ) | ||
| return False |
There was a problem hiding this comment.
This check for max_token_num appears to be redundant. A review of the __init__ method for AllReduceFusionPass shows that self.disabled is only False if the initialization completes successfully, which includes setting self.max_token_num. In all early-exit scenarios, self.disabled remains True, and self.max_token_num is not set. Therefore, the initial check for self.disabled on line 1191 is sufficient to guard against the AttributeError. Removing this redundant block will make the code cleaner and easier to reason about.
|
Thanks, merging! |
Signed-off-by: zjy0516 <riverclouds.zhu@qq.com> Signed-off-by: dsuhinin <suhinin.dmitriy@gmail.com>
Purpose
Fix #24252 (comment)
cc @ProExpertProg @hjjq
Test Plan
Test Result
Essential Elements of an Effective PR Description Checklist
supported_models.mdandexamplesfor a new model.