filter by num_hidden_layers #7056
Conversation
There was a problem hiding this comment.
Summary of Changes
Hello @pansicheng, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request implements a feature to filter the number of layers loaded for the DeepSeek V2 model based on the num_hidden_layers configuration. This is particularly useful for testing or running the model on hardware with memory constraints, preventing out-of-memory errors by loading only the necessary layers instead of the full model.
Highlights
- Model Loading: Modified the weight loading logic for the DeepSeek V2 model to respect the
num_hidden_layersconfiguration parameter. - Memory Optimization: This change allows users to load only a specified subset of model layers, which is crucial for running models in environments with limited GPU memory.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments to provide feedback.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configureGemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request aims to allow loading a user-specified number of hidden layers, which is useful for environments with constrained GPU memory. The change in python/sglang/srt/models/deepseek_v2.py modifies a condition in the post_load_weights method. The new condition layer_id < self.config.num_hidden_layers correctly selects layers up to the specified limit, which is crucial if self.config.num_hidden_layers now represents the count of layers to be loaded, as this prevents potential IndexError issues when accessing self.model.layers.
The primary feedback is to clarify the code comment # filter the nextn layer. on line 1789, as it seems inconsistent with the new filtering logic which selects layers up to a limit rather than filtering out a specific layer. Updating this comment will improve code maintainability and clarity.
| @@ -1787,7 +1787,7 @@ def post_load_weights(self, is_nextn=False, weight_names=None): | |||
| if "kv_b_proj" in name: | |||
| layer_id = int(name.split(".")[2]) | |||
| # filter the nextn layer. | |||
| if layer_id != self.config.num_hidden_layers: | |||
| if layer_id < self.config.num_hidden_layers: | |||
There was a problem hiding this comment.
The code change from layer_id != self.config.num_hidden_layers to layer_id < self.config.num_hidden_layers seems correct and necessary, especially if self.config.num_hidden_layers is updated to the user-specified count of layers to load. This change ensures that layer_id remains a valid index for self.model.layers.
However, the preceding comment on line 1789, # filter the nextn layer., is now potentially misleading.
- If
self.config.num_hidden_layersrepresents the count of layers to load (e.g., 10), this condition selects layers0through9for the fusion process. This aligns with the PR's goal of limiting the number of loaded layers. - The phrase "filter the nextn layer" usually implies excluding a specific layer. If there's a particular layer that should be excluded from this fusion logic (even if its
layer_idis less thanself.config.num_hidden_layers), this condition doesn't explicitly do that.
To improve clarity and prevent future misunderstandings, consider updating the comment on line 1789 to better reflect the new logic. For example:
# Select layers for nextn-related fusion, up to the specified num_hidden_layers.Or, if there's a specific "nextn layer" that should indeed be filtered out from this fusion process, the condition might need to be adjusted further (e.g., if layer_id < self.config.num_hidden_layers and layer_id != ID_OF_THE_NEXTN_LAYER_TO_EXCLUDE:). Assuming the current code change is complete for the intended feature, updating the comment for clarity would be beneficial.
| @@ -1787,7 +1787,7 @@ def post_load_weights(self, is_nextn=False, weight_names=None): | |||
| if "kv_b_proj" in name: | |||
| layer_id = int(name.split(".")[2]) | |||
| # filter the nextn layer. | |||
| if layer_id != self.config.num_hidden_layers: | |||
| if layer_id < self.config.num_hidden_layers: | |||
There was a problem hiding this comment.
Could you please update the comment in the above line accordingly?
There was a problem hiding this comment.
@ch-wan Fixed, please recheck 🙏
3e5a62f to
10f67cf
Compare
10f67cf to
8281069
Compare
Motivation
In constrained GPU memory environments (e.g., low-end GPUs), we may need to reduce the number of layers to avoid out-of-memory errors while still validating model functionality. This PR allows us to load only user-specified num_hidden_layers (instead of the full model's layer count).
Modifications
Checklist