-
-
Notifications
You must be signed in to change notification settings - Fork 9.8k
Fix GLM-4 PP Missing Layer When using with PP. #21531
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.
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
Conversation
Signed-off-by: zRzRzRzRzRzRzR <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
The pull request addresses an issue with pipeline parallelism by handling PPMissingLayer
when iterating through model layers. However, the changes introduce a potential RuntimeError
on pipeline parallel ranks that do not contain any MoE layers. The review includes suggestions to improve the logic and prevent the error from being raised unnecessarily.
if example_moe is None: | ||
raise RuntimeError("No Glm4MoE layer found in model.layers.") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
example_moe = None | ||
for layer in self.model.layers: | ||
if isinstance(layer, PPMissingLayer): | ||
continue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider skipping the loop entirely if self.model.layers
is empty to avoid unnecessary iterations and potential errors. This can happen in pipeline parallel settings where some ranks might not have any layers.
example_moe = None | |
for layer in self.model.layers: | |
if isinstance(layer, PPMissingLayer): | |
continue | |
if self.model.layers: | |
for layer in self.model.layers: | |
if isinstance(layer, PPMissingLayer): | |
continue |
👋 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 🚀 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thank you for this fix
Signed-off-by: zRzRzRzRzRzRzR <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]> Signed-off-by: shuw <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]> Signed-off-by: x22x22 <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]>
Cherry-pick: vllm-project@2ce90e5 Signed-off-by: zRzRzRzRzRzRzR <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]> Signed-off-by: Jinzhen Lin <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]> Signed-off-by: Paul Pak <[email protected]>
Cherry-pick: vllm-project@2ce90e5 Signed-off-by: zRzRzRzRzRzRzR <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]> Signed-off-by: Boyuan Feng <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]> Signed-off-by: Diego-Castan <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]>
Signed-off-by: zRzRzRzRzRzRzR <[email protected]>
No description provided.