Skip to content

LORA/MODEL: Use max rank of pattern for add_weighted_adapter #2512

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

Beinsezii
Copy link

Otherwise it's possible that the new merged adapter won't allocate enough space when a patterned rank is larger than config.r.

For example, the cat method uses sum(adapters_ranks) for the empty tensor alloc, so if cat(loras) for a given layer is ever > the sum of config.r the assignment will overflow.

Local test fails but it's all entirely the TestEvaInitialization module which appears completely unrelated.

Otherwise it's possible that the new merged adapter won't allocate
enough space when a patterned rank is larger than config.r
Copy link
Member

@BenjaminBossan BenjaminBossan left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, indeed the rank_pattern. Would you please be so kind to also add a unit test for this? A good starting point could be this test:

def test_multiple_adapters_mixed_modules_to_save_merging_adapters(self):
# See issue 1574
# This test is similar to test_multiple_adapters_mixed_modules_to_save, but it also checks that merging adapter
# weights works when one adapter has a modules_to_save and the other hasn't
config0 = LoraConfig(target_modules=["lin0"], modules_to_save=["lin1"])
config1 = LoraConfig(target_modules=["lin0"])
model = MLP()
model = get_peft_model(model, config0).to(self.torch_device)
model.add_adapter("other", config1)
# check that this does not raise
model.add_weighted_adapter(["default", "other"], weights=[1.0, 1.0], adapter_name="merged")

I think it can be copied and slightly adjusted by adding a config with rank_pattern.

I wonder if we need even more checks for rank_pattern, especially when not using the cat method. But this can be dealt with in another PR.

@bghira
Copy link

bghira commented Apr 24, 2025

thanks Ben, i'd requested this pull request be opened since it seems trivial enough but I'm not sure we'll have time to add unit tests here, especially considering none of us are particularly familiar with the unit test ecosystem for PEFT.

No need to allocate a list for max()

Co-authored-by: Benjamin Bossan <[email protected]>
@BenjaminBossan
Copy link
Member

Thanks for providing more context @bghira. I wrote the code required to test this change:

    @parameterized.expand([IA3Config, LoHaConfig, LoKrConfig, LoraConfig, HRAConfig, BoneConfig])
    def test_add_weighted_adapter_cat_with_rank_pattern(self, config_cls):
        # Fixes a bug described in #2512, which resulted from the rank_pattern not being taken into account
        config0 = LoraConfig(target_modules=["lin0", "lin1"], r=8)
        config1 = LoraConfig(target_modules=["lin0", "lin1"], r=8, rank_pattern={"lin0": 16})
        model = MLP()
        model = get_peft_model(model, config0).to(self.torch_device)
        model.add_adapter("other", config1)
        model.add_weighted_adapter(["default", "other"], weights=[1.0, 1.0], adapter_name="merged", combination_type="cat")

I can best be added in this line. @Beinsezii would you be so kind to add it to your PR?

@Beinsezii
Copy link
Author

Verified it fails on main and passes on beinsezii/weighted_adapter_max_rank_pattern

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants