Skip to content
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.

Sign up for GitHub

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

[Misc][LoRA] Support Rank Stabilized LoRA (RSLoRA) #6909

Merged
merged 10 commits into from
Dec 31, 2024
+30 −22
Merged

[Misc][LoRA] Support Rank Stabilized LoRA (RSLoRA) #6909

Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
e308f37
feat: support rslora
JohnGiorgi Jul 29, 2024
6690e8b
fix: lint the codebase
JohnGiorgi Jul 29, 2024
e2cf420
fix: default to not using rslora
JohnGiorgi Jul 29, 2024
b782763
Merge branch 'main' into support-rslora
JohnGiorgi Dec 23, 2024
a3206bb
fix: move rslora scaling to peft_helper
JohnGiorgi Dec 23, 2024
42e04aa
fix: set scaling factor directly, instead of modifying alpha
JohnGiorgi Dec 23, 2024
f73a3db
fix: remove error message about RSLoRA not being supported
JohnGiorgi Dec 23, 2024
419a6fb
docs: add comments with arxiv links for rsLoRA and DoRA
JohnGiorgi Dec 23, 2024
7ace76c
Done
jeejeelee Dec 24, 2024
20c9f14
Merge branch 'main' into support-rslora
JohnGiorgi Dec 30, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions vllm/lora/peft_helper.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,8 @@ def _validate_features(self):

def __post_init__(self):
self._validate_features()
if self.use_rslora:
self.vllm_scaling_factor = self.lora_alpha / math.sqrt(self.r)
if self.context_length:
if self.vllm_max_position_embeddings is None:
self.vllm_max_position_embeddings = self.context_length
Copy link
Contributor Author

@JohnGiorgi JohnGiorgi Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jeejeelee I think this is the cleanest way to support the rsLoRA scaling logic using the new PEFTHelper! My main outstanding worry is I am not sure how the scaling applied for rsLoRA and the scaling applied when self.context_length is provided should interact.

The way this is written, if both self.use_rslora and self.context_length, the custom scaling factor logic for self.context_length will take precedence.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My main outstanding worry is I am not sure how the scaling applied for rsLoRA and the scaling applied when self.context_length is provided should interact.

IMHO, These two scaling are distinct and operate independently of each other.

We shouldn't use vllm_scaling_factor - instead, we can add a variable called scaling

Copy link
Contributor Author

@JohnGiorgi JohnGiorgi Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fine by me, but its gets a little confusing as LoRAModel already defines scaling_factor and uses it for long context support:

class LoRAModel(AdapterModel):
    """A LoRA fine-tuned model."""

    def __init__(
        self,
        lora_model_id: int,
        rank: int,
        loras: Dict[str, LoRALayerWeights],
        scaling_factor: Optional[float] = None,
    ) -> None:
        """
        Args:
            lora_model_id: The integer id for the lora model.
            rank: lora rank.
            loras: module name -> weights for lora-replaced layers.
            scaling_factor: Scaling factor to support long context lora model.
                None if the lora is not tuned for long context support.
         """

which is populated by PEFTHelper.vllm_scaling_factor,

return cls(lora_model_id,
                   peft_helper.r,
                   loras,
                   scaling_factor=peft_helper.vllm_scaling_factor)

Is there an argument to sync LoRAModel and PEFTHelper so each has a scaling_factor and vllm_scaling_factor argument? (Also, if vllm_scaling_factor is strictly used for long context support, maybe a more explicit name is in order, e.g. long_context_scaling_factor or something)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

long_context_scaling_factor looks reasonable. Let me verify this tomorrow and confirm

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good! I'll hang tight until then

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have implemented the related logic, please check if it's reasonable

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good to me @jeejeelee! Thanks for adding tests

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you please sync with the main branch?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. It is just the DCO check that is failing but I am unsure how to fix, it suggests a rebase but I don't want to do that as there's multiple commit authors on this branch

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is a known issue in the current lora test. Let's check if other lora tests can pass first. If they do, we can consider force merging

Expand Down
Loading