[main] chore: add bias for base layer with lora - #22169
Conversation
There was a problem hiding this comment.
Code Review
This pull request updates the BaseLoRALayer to include the bias attribute from the base layer. The review feedback recommends removing the is not None check during assignment to ensure the wrapper's interface consistently mirrors the base layer, which is important for reflection and weight synchronization logic.
| if hasattr(self.base_layer, "bias") and self.base_layer.bias is not None: | ||
| self.bias = self.base_layer.bias |
There was a problem hiding this comment.
To maintain consistency with how the weight attribute is handled on lines 39-40, it is recommended to remove the is not None check. If the base layer has a bias attribute that is explicitly set to None (which is common in SGLang layers using register_parameter("bias", None)), the wrapper should mirror this attribute. This ensures that hasattr(self, "bias") returns the same result for both the wrapper and the base layer, providing a more consistent interface for reflection and weight synchronization logic.
| if hasattr(self.base_layer, "bias") and self.base_layer.bias is not None: | |
| self.bias = self.base_layer.bias | |
| if hasattr(self.base_layer, "bias"): | |
| self.bias = self.base_layer.bias |
|
/tag-run-ci-label |
There was a problem hiding this comment.
Pull request overview
Fixes incorrect / “garbage” generation when enabling LoRA and performing base-weight sync for models whose weight loaders rely on named_parameters() containing *.bias entries (e.g., Qwen2.* stacked-parameter mapping).
Changes:
- Expose
base_layer.biasonBaseLayerWithLoRA(when present) so the wrapped module surfaces abiasparameter at the expected module path.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if hasattr(self.base_layer, "weight"): | ||
| self.weight = self.base_layer.weight | ||
| if hasattr(self.base_layer, "bias") and self.base_layer.bias is not None: | ||
| self.bias = self.base_layer.bias |
There was a problem hiding this comment.
Consider adding a regression unit test for this behavior: when a base layer with a real bias parameter is wrapped by BaseLayerWithLoRA (and installed as a submodule in a parent module), dict(parent.named_parameters()) should expose the bias under the wrapper path (e.g., wrapped.bias / sub.bias) rather than only under sub.base_layer.bias. This would guard against future regressions in weight-sync loaders (e.g., Qwen2 load_weights) that look up *.bias entries directly in named_parameters().
|
/rerun-failed-ci |
|
/rerun-failed-ci |
3 similar comments
|
/rerun-failed-ci |
|
/rerun-failed-ci |
|
/rerun-failed-ci |
|
/rerun-failed-ci |
3 similar comments
|
/rerun-failed-ci |
|
/rerun-failed-ci |
|
/rerun-failed-ci |
Motivation
Bug fix for LoRA base layer when I m doing miles RL lora training
related PR: #22846
model: qwen2.5-3B
To reproduce it the error we can mimic the way that miles start sglang:
And run the reproduce script
python3 test.py --mimic-colocateresult:
with LoRA adapter it will produce garbage output
The reason is that LoRA layer does not has .bias, when LoRA is enabled, it will not be stored in
named_parameters(), and will be skipped by qwen2 model:In miles, currently we have
enable_weights_cpu_backup=Truefor sglang, which allows us to bypass the issue by never sync the base weights, but at cost of moving base weight between cpu and gpu during onload/offload, which is slow for big models.Modifications
Accuracy Tests
Speed Tests and Profiling
Checklist
Review and Merge Process
/tag-and-rerun-ci,/tag-run-ci-label,/rerun-failed-ci