[`PEFT`] Fix PEFT multi adapters support #26407

younesbelkada · 2023-09-26T08:03:28Z

What does this PR do?

With huggingface/peft#905 being merged, it added the support for multi-adapter inference (combining multiple adapters at inference).

The PR has introduced some changes that are not compatible anymore with peft integration of transformers, for example active_adapter is now a property method and returns a list. This PR adds the corresponding fixes to make everything compatible with the newest features of PEFT, while preserving backward compatiblity

cc @BenjaminBossan @pacman100

BenjaminBossan

Thanks for the quick fix. We will extend the PEFT CI to catch the integration issues earlier in the future.

Just a small correction, multiple active adapters were enabled in huggingface/peft#873, huggingface/peft#905 was for correctly handling requires_grad.

BenjaminBossan · 2023-09-26T08:11:13Z

src/transformers/integrations/peft.py

@@ -357,6 +373,15 @@ def get_adapter_state_dict(self, adapter_name: Optional[str] = None) -> dict:
        if adapter_name is None:
            adapter_name = self.active_adapter()

+        if isinstance(adapter_name, list):


Is it possible for this method to be called with a list of str? Wouldn't this require the user to explicitly pass that argument? If not, I think this check is not necessary. If it is a valid argument, then the type annotation should also be adjusted.

Good point, I don't think we should allow users to call this method with a list of str - I refactored a bit the logic of def active_adapter to extend it for multi-adapter inference. let me know what do you think

HuggingFaceDocBuilderDev · 2023-09-26T08:21:21Z

The documentation is not available anymore as the PR was closed or merged.

pacman100

Thank you for the quick fix, LGTM!

LysandreJik · 2023-09-26T09:26:30Z

src/transformers/modeling_utils.py

+                active_adapter = self.active_adapter()
+
+                if isinstance(active_adapter, list):
+                    if len(active_adapter) > 1:
+                        logger.warning(
+                            "Multiple active adapters detected, will only consider the first active adapter. In order to save them all, please iteratively call `set_adapter()` on each"
+                            " adapter name and save them one by one manually. "
+                        )
+                    active_adapter = active_adapter[0]
+
+                current_peft_config = self.peft_config[active_adapter]


Should multi-adapter serialization be supported out of the box?

I would advocate to educate users to manually save them one by one as this might require some imporant refactor in save_pretrained

We could also expose a new method in PeftAdapterMixin save_adapters to support that by iteratively calling save_pretrained on all adapters

Ok understood, let's go with what you have right now then

LysandreJik

Thanks for your implementation!

In the case of multi-adapters, is a single adapter usually considered the "main" adapter, or are all adapters having the same impact? If so, I would advocate to switch completely to an API supporting multi-adapter rather than having a mix between both dictated by kwargs.

I would likely set all methods to plural rather than singular, as all methods correctly handle multi adapters.

LysandreJik · 2023-09-26T09:34:49Z

src/transformers/integrations/peft.py

+            return_multi_adapters (`bool`, *optional*, defaults to `True`):
+                Whether to return a list of active adapters or not. If `False`, only the first adapter is returned. If
+                `True`, returns the list of active adapters.


Are we sure about this API? It would seem better to me to have active_adapter changed to active_adapters and to always default to returning the list of active adapters.

BC would be handled by having another method active_adapter that would log a warning + call active_adapters under the hood

LysandreJik · 2023-09-26T09:36:50Z

tests/peft_integration/test_peft_integration.py

+                self.assertTrue(model.active_adapter() == ["default"])
+                self.assertTrue(model.active_adapter(return_multi_adapters=False) == "default")


This is a breaking change; are we ok with that breaking change?

We can set the default return_multi_adapters to True, but there will be still the case of users that used the main branch of PEFT for multiple active adapters that will face a behaviour which is going to be different whether they use 2 different versions of transformers - cc @BenjaminBossan also what do you think

So I think the most common case would be users with only a single active adapter, as multiple active adapters just landed on PEFT main and have not been advertised yet. Therefore, I would suggest to keep BC for those users and accept that maybe a tiny minority who uses multiple active adapters with the current transformers version will get a breaking change when they upgrade transformers.

BenjaminBossan

LGTM, thanks for working on these fixes so quickly. As I'm not a transformers expert, I won't approve/request changes. Still, I have a few comments, please take a look.

BenjaminBossan · 2023-09-26T09:34:29Z

src/transformers/integrations/peft.py

        """
        check_peft_version(min_version=MIN_PEFT_VERSION)
        if not self._hf_peft_config_loaded:
            raise ValueError("No adapter loaded. Please load an adapter first.")
+        elif isinstance(adapter_name, list):
+            for adapter in adapter_name:


In situations like this, I always prefer to identify all incorrect entries, not just the first. Otherwise, the user fixes the first, then gets an error on 2nd, fixes the error on 2nd, gets third, etc. Much nicer to check all and runtime shouldn't really matter here.

missing = set(adapter_name) - set(self.peft_config) if missing: raise ValueError( f"Following adapter(s) could not be found: {', '.join(missing)}. Please ...")

With a bit of extra work, this could even be merged with the code below which checks for a single adapter not being found.

src/transformers/integrations/peft.py

BenjaminBossan · 2023-09-26T09:42:41Z

tests/peft_integration/test_peft_integration.py

@@ -265,17 +265,31 @@ def test_peft_add_multi_adapter(self):
                _ = model.generate(input_ids=dummy_input)

                model.set_adapter("default")
-                self.assertTrue(model.active_adapter() == "default")
+                self.assertTrue(model.active_adapter() == ["default"])


Hmm, so the change is backwards incompatible for this specific case? Can we live with that?

BenjaminBossan · 2023-09-26T09:43:45Z

tests/peft_integration/test_peft_integration.py


                # Logits comparison
                self.assertFalse(
                    torch.allclose(logits_adapter_1.logits, logits_adapter_2.logits, atol=1e-6, rtol=1e-6)
                )
                self.assertFalse(torch.allclose(logits_original_model, logits_adapter_2.logits, atol=1e-6, rtol=1e-6))

+                model.set_adapter(["adapter-2", "default"])
+                self.assertTrue(model.active_adapter() == ["adapter-2", "default"])


Nice addition to the test. Can we also check model.active_adapter() and model.active_adapter(return_multi_adapters=False) here?

Co-authored-by: Benjamin Bossan <[email protected]>

younesbelkada · 2023-09-26T10:37:11Z

src/transformers/integrations/peft.py

+
+        active_adapters = self.active_adapters()
+
+        if isinstance(active_adapters, list):


Note this check is necessary as for previous PEFT versions active_adapter is an str

younesbelkada · 2023-09-26T10:37:59Z

Thanks for all the reviews, I think I have addressed most of them now, I propose to handle the multi-adapter saving in a follow up PR #26411 to be consistent with PEFT API

BenjaminBossan

Thanks, this LGTM now.

I noticed a small potential issue: If the next transformers version is released in 10 days or so, users will see that they can now have multiple active PEFT adapters -- however, this only works with PEFT main, or v0.6 if it is released by then. Could this be an issue?

younesbelkada · 2023-09-26T11:12:07Z

Thanks ! For that we could maybe sync the next PEFT release before / at the same time than the transformers release

pacman100

Thank you @younesbelkada for quick fixes, LGTM!

patrickvonplaten · 2023-09-27T09:03:07Z

src/transformers/integrations/peft.py

+        # For previous PEFT versions
+        if isinstance(active_adapters, str):
+            active_adapters = [active_adapters]
+
+        return active_adapters


Suggested change

# For previous PEFT versions

if isinstance(active_adapters, str):

active_adapters = [active_adapters]

return active_adapters

return active_adapters

younesbelkada · 2023-09-27T09:05:17Z

src/transformers/integrations/peft.py


-    def active_adapter(self) -> str:
+    def active_adapters(self) -> List[str]:


I think that way it won't be BC --> for users that have previous PEFT version they will get an str whereas for newest PEFT versions they will get a list of str

patrickvonplaten · 2023-09-27T09:05:49Z

src/transformers/integrations/peft.py

+
+        return active_adapters
+
+    def active_adapter(self) -> str:


I do not think it's a good idea to have both active_adapter and active_adapters:

It's easier for the user to just have to remember one method and deal with the output type (we can also deprecate returning a single string in favor of returning a list going forward)

Reduces the API surface - only a single source of truth

patrickvonplaten · 2023-09-27T09:07:58Z

src/transformers/modeling_utils.py

+                active_adapter = self.active_adapters()
+
+                if len(active_adapter) > 1:
+                    logger.warning(


Instead of warning, I would raise here. It's important for the user to understand the difference between multiple active adapters and single. I would limit the functionality for multi active adapters heavily for now

patrickvonplaten · 2023-09-27T09:21:56Z

src/transformers/integrations/peft.py

+
+        return active_adapters
+
+    def active_adapter(self) -> str:


As discussed let's just deprecate active_adapter(...) in favor of active_adapters

Suggested change

def active_adapter(self) -> str:

def active_adapter(self) -> str:

# deprecate ...

return self.active_adapters[0]

src/transformers/integrations/peft.py

patrickvonplaten · 2023-09-27T09:25:40Z

As discussed offline I would propose the following:

1.) We either deprecate or fully remove active_adapter(...) and replace it with active_adapters(...) which always returns a list. active_adapter(...) function has not been in a release yet (right @younesbelkada ? ), so I would deprecate it for two minor versions and then remove it. It's cleaner to always return a list here.
2.) Instead of "magically" only saving the first adapter when having multiple active adapters, let's teach the user the difference between multi and single active adapters here by raising a ValueError when trying to save multi adapters. It's really not yet necessary to be able to save multiple adapters at once

younesbelkada · 2023-09-27T09:32:15Z

I am ok to deprecate the active_adapter method ! The plan sounds great

active_adapter(...) function has not been in a release yet

I meant that active_adapters was not released yet :D In any case I think it is ok here

Agreed also on your second point! let's keep it simple

LysandreJik · 2023-09-27T09:33:39Z

Agree with Patrick!

If we choose to go with

2.) Instead of "magically" only saving the first adapter when having multiple active adapters, let's teach the user the difference between multi and single active adapters here by raising a ValueError when trying to save multi adapters. It's really not yet necessary to be able to save multiple adapters at once

let's maybe put this PR #26411 to the side for now and focus on merging this PR with single-adapter saving first? WDYT @younesbelkada ?

younesbelkada · 2023-09-27T09:34:24Z

Sounds great @LysandreJik !

Co-authored-by: Patrick von Platen <[email protected]>

younesbelkada · 2023-09-27T09:43:45Z

I should have addressed the final comments @patrickvonplaten , @LysandreJik , thanks for your time reviewing the PR !

patrickvonplaten · 2023-09-27T10:38:44Z

src/transformers/modeling_utils.py

+                active_adapter = self.active_adapters()
+
+                if len(active_adapter) > 1:
+                    raise ValueError(


LysandreJik

LGTM if all tests pass!

younesbelkada · 2023-09-27T14:30:15Z

Failing test seems unrelated - merging !

patrickvonplaten · 2023-09-27T15:14:38Z

src/transformers/integrations/peft.py

@@ -245,20 +245,27 @@ def add_adapter(self, adapter_config, adapter_name: Optional[str] = None) -> Non

        self.set_adapter(adapter_name)

-    def set_adapter(self, adapter_name: str) -> None:
+    def set_adapter(self, adapter_name: Union[List[str], str]) -> None:


Let's do the same in diffusers :-) https://github.com/huggingface/diffusers/pull/5151/files#r1338774571

* fix PEFT multi adapters support * refactor a bit * save pretrained + BC + added tests * Update src/transformers/integrations/peft.py Co-authored-by: Benjamin Bossan <[email protected]> * add more tests * add suggestion * final changes * adapt a bit * fixup * Update src/transformers/integrations/peft.py Co-authored-by: Patrick von Platen <[email protected]> * adapt from suggestions --------- Co-authored-by: Benjamin Bossan <[email protected]> Co-authored-by: Patrick von Platen <[email protected]>

fix PEFT multi adapters support

05bbcd7

younesbelkada requested review from BenjaminBossan and pacman100 September 26, 2023 08:05

younesbelkada mentioned this pull request Sep 26, 2023

[PEFT] Fix peft ci huggingface/diffusers#5185

Closed

BenjaminBossan reviewed Sep 26, 2023

View reviewed changes

refactor a bit

b2e32a5

younesbelkada requested a review from BenjaminBossan September 26, 2023 08:23

pacman100 approved these changes Sep 26, 2023

View reviewed changes

save pretrained + BC + added tests

d772435

younesbelkada requested a review from pacman100 September 26, 2023 09:15

LysandreJik reviewed Sep 26, 2023

View reviewed changes

BenjaminBossan reviewed Sep 26, 2023

View reviewed changes

younesbelkada and others added 2 commits September 26, 2023 12:17

Update src/transformers/integrations/peft.py

87e304a

Co-authored-by: Benjamin Bossan <[email protected]>

add more tests

3dc62a3

younesbelkada mentioned this pull request Sep 26, 2023

[WIP] Multi-adapter saving support for PEFT #26411

Closed

younesbelkada added 2 commits September 26, 2023 10:22

add suggestion

9250ccf

final changes

c7ad2b4

younesbelkada commented Sep 26, 2023

View reviewed changes

younesbelkada requested review from LysandreJik and BenjaminBossan September 26, 2023 10:38

younesbelkada added 2 commits September 26, 2023 10:41

adapt a bit

9e93a26

fixup

f8e435f

BenjaminBossan approved these changes Sep 26, 2023

View reviewed changes

pacman100 approved these changes Sep 26, 2023

View reviewed changes

patrickvonplaten reviewed Sep 27, 2023

View reviewed changes

younesbelkada commented Sep 27, 2023

View reviewed changes

patrickvonplaten reviewed Sep 27, 2023

View reviewed changes

src/transformers/integrations/peft.py Outdated Show resolved Hide resolved

younesbelkada and others added 2 commits September 27, 2023 11:35

Update src/transformers/integrations/peft.py

a46e13b

Co-authored-by: Patrick von Platen <[email protected]>

adapt from suggestions

4ebe629

younesbelkada requested a review from patrickvonplaten September 27, 2023 09:43

patrickvonplaten reviewed Sep 27, 2023

View reviewed changes

patrickvonplaten approved these changes Sep 27, 2023

View reviewed changes

LysandreJik approved these changes Sep 27, 2023

View reviewed changes

Merge remote-tracking branch 'upstream/main' into HEAD

9f538ad

younesbelkada merged commit 3ca18d6 into huggingface:main Sep 27, 2023
3 checks passed

younesbelkada deleted the fix-peft-setter branch September 27, 2023 14:45

patrickvonplaten reviewed Sep 27, 2023

View reviewed changes

tomaarsen mentioned this pull request Nov 19, 2024

[peft] Given that self.active_adapter is deprecated, avoid using it #34804

Merged

5 tasks

		self.assertTrue(model.active_adapter() == ["default"])
		self.assertTrue(model.active_adapter(return_multi_adapters=False) == "default")


		active_adapters = self.active_adapters()

		if isinstance(active_adapters, list):


		def active_adapter(self) -> str:
		def active_adapters(self) -> List[str]:

[PEFT] Fix PEFT multi adapters support #26407

[PEFT] Fix PEFT multi adapters support #26407

Conversation

younesbelkada commented Sep 26, 2023

What does this PR do?

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HuggingFaceDocBuilderDev commented Sep 26, 2023 • edited Loading

pacman100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LysandreJik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

younesbelkada commented Sep 26, 2023

BenjaminBossan left a comment

Choose a reason for hiding this comment

younesbelkada commented Sep 26, 2023

pacman100 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrickvonplaten commented Sep 27, 2023

younesbelkada commented Sep 27, 2023 • edited Loading

LysandreJik commented Sep 27, 2023 • edited Loading

younesbelkada commented Sep 27, 2023

younesbelkada commented Sep 27, 2023

Choose a reason for hiding this comment

LysandreJik left a comment

Choose a reason for hiding this comment

younesbelkada commented Sep 27, 2023

Choose a reason for hiding this comment

[`PEFT`] Fix PEFT multi adapters support #26407

[`PEFT`] Fix PEFT multi adapters support #26407

HuggingFaceDocBuilderDev commented Sep 26, 2023 •

edited

Loading

younesbelkada commented Sep 27, 2023 •

edited

Loading

LysandreJik commented Sep 27, 2023 •

edited

Loading