FEAT: Adding exclude modules param(#2044) #2102

JINO-ROHIT · 2024-09-26T13:03:10Z

This draft PR addresses the exclude modules parameter to keep specific layers out of target modules

Addresses #2044

Changes made:

Changed LoraConfig class to take in exclude_modules param.
Added the exclusion logic within the check_target_module_exists method.

@BenjaminBossan Can you have a look if this is the right direction? This is my first time contributing at peft , would love to have your guidance here :)

BenjaminBossan

Thanks for taking on this feature. I haven't done an in-depth review yet, but from a quick look, I saw this:

Right now, you only added the option to the LoraConfig. However, since the logic is implemented on the check_target_module_exists function, actually all methods can benefit from this feature (not the prompt learning ones, as they don't have target_modules, but all the rest like LoKr, BOFT, IA³, etc.). Let's add the argument to all relevant configs, i.e. the config of each method that has target_modules.
We need to have tests for this new feature. Please check https://github.com/huggingface/peft/blob/main/tests/test_tuners_utils.py, which already contains a bunch of tests to check that the correct modules are targeted. Similar tests would need to be added for exclude_modules. Could you please take a look? Don't hesitate to ask if you have questions.

JINO-ROHIT · 2024-09-26T16:10:47Z

@BenjaminBossan thanks for the headsup, ill work on the points

JINO-ROHIT · 2024-09-26T17:59:44Z

@BenjaminBossan ive added the test cases too, lmk what you think

BenjaminBossan

Thanks a lot for the latest changes. The tests are already shaping up quite nicely, but let's improve the test coverage a bit more (see my suggestion).

Moreover, you missed a few configs still. These are all I could find, but please double-check:

AdaLoraConfig
BoftConfig
VBLoRAConfig
FourierFTConfig
LNTuningConfig
PolyConfig

BenjaminBossan · 2024-09-27T11:01:15Z

tests/test_tuners_utils.py

@@ -415,6 +415,35 @@ def test_realistic_example(self):
        ]
        assert model.targeted_module_names == expected

+class TestExcludedModuleNames(unittest.TestCase):


Can we get additional tests:

Let's add tests for exclude_modules being a list of str.

What happens if all targeted modules are excluded? I think a nice error message would be the right call in this situation.

What happens if we have excluded modules that don't exist? I think it's fine if some of the entries from exclude_modules don't match, as we also allow that for target_modules. But what if none of the entries in exclude_modules matches? I think this should also result in an error. WDYT?

yeap makes sense, let me work through the tests and edge cases, also the missed out modules

Hey @BenjaminBossan i managed to add 1 and 2 cases. I wanted some help on how 3 should be done. As in, I see currently _check_target_module_exists is a private abstract method that every peft methods override and check for target modules existence. Now for exclude_modules, should we do the same? Or do i open a for loop within inject_adapter and check for keys there?

I don't think we need to go that far. If we check closer, we can see e.g. for LoRA:

peft/src/peft/tuners/lora/model.py

Lines 158 to 160 in aa3bd8f

@staticmethod

def _check_target_module_exists(lora_config, key):

return check_target_module_exists(lora_config, key)

And if we check where that's used, it all comes back to this line:

peft/src/peft/tuners/tuners_utils.py

Line 486 in aa3bd8f

if not self._check_target_module_exists(peft_config, key):

So what would be an elegant way to implement some sanity checks here? I have one proposal, but maybe you have a better idea: Right now, check_target_module_exists simply returns a bool, i.e. True or False if the module matched. We could extend that and, say, return a special object if a module was excluded due to exclude_modules.

This special value could be a very simple object. Its __bool__ should return False (so that users who rely on checking if not self._check_target_module_exists(peft_config, key) don't suddenly get different results).

When we call _check_target_module_exists, we can collect keys that did not match because of exclude_modules in one list and keys that did not match for other reasons in another list. When we exit the loop, if no key was matched because they were all matching exclude_modules, we can raise a nice error message to say so. Otherwise, we raise the existing error:

peft/src/peft/tuners/tuners_utils.py

Lines 506 to 510 in aa3bd8f

if not is_target_modules_in_base_model and hasattr(peft_config, "target_modules"):

raise ValueError(

f"Target modules {peft_config.target_modules} not found in the base model. "

f"Please check the target modules and try again."

)

On the other hand, if not a single key was excluded due to exclude_modules, even though exclude_modules was passed by the user, we can give a nice warning saying something like "You passed exclude_modules=[...] but no key was matching this".

…blora

…ules overlap

BenjaminBossan

Thanks for your further work on this PR. You raised a good question about how to implement the checks I mentioned. I made a proposal, but please LMK if you have a better idea.

BenjaminBossan · 2024-09-30T12:30:02Z

src/peft/tuners/tuners_utils.py

@@ -914,6 +914,20 @@ def check_target_module_exists(config, key: str) -> bool | re.Match[str] | None:
        `bool` | `re.Match[str]` | `None`: True of match object if key matches any target modules from config, False or
        None if no match found
    """
+    if config.target_modules and config.exclude_modules:
+        if config.exclude_modules == config.target_modules:


I don't think that this is a sufficient check. Just to give some examples:

target_modules is a list of str and exclude_modules is a str, but both essentially target the same modules

target_modules is ["lin0", "lin1", "lin2"] and exclude_modules is ["lin0", "lin1"]. They look different, but if "lin2" doesn't exist, they're actually the same

We thus need to check what modules were actually targeted, we can't rely on the passed arguments.

BenjaminBossan · 2024-09-30T12:45:01Z

tests/test_tuners_utils.py

@@ -415,6 +415,35 @@ def test_realistic_example(self):
        ]
        assert model.targeted_module_names == expected

+class TestExcludedModuleNames(unittest.TestCase):


I don't think we need to go that far. If we check closer, we can see e.g. for LoRA:

peft/src/peft/tuners/lora/model.py

Lines 158 to 160 in aa3bd8f

@staticmethod

def _check_target_module_exists(lora_config, key):

return check_target_module_exists(lora_config, key)

And if we check where that's used, it all comes back to this line:

peft/src/peft/tuners/tuners_utils.py

Line 486 in aa3bd8f

if not self._check_target_module_exists(peft_config, key):

So what would be an elegant way to implement some sanity checks here? I have one proposal, but maybe you have a better idea: Right now, check_target_module_exists simply returns a bool, i.e. True or False if the module matched. We could extend that and, say, return a special object if a module was excluded due to exclude_modules.

This special value could be a very simple object. Its __bool__ should return False (so that users who rely on checking if not self._check_target_module_exists(peft_config, key) don't suddenly get different results).

When we call _check_target_module_exists, we can collect keys that did not match because of exclude_modules in one list and keys that did not match for other reasons in another list. When we exit the loop, if no key was matched because they were all matching exclude_modules, we can raise a nice error message to say so. Otherwise, we raise the existing error:

peft/src/peft/tuners/tuners_utils.py

Lines 506 to 510 in aa3bd8f

if not is_target_modules_in_base_model and hasattr(peft_config, "target_modules"):

raise ValueError(

f"Target modules {peft_config.target_modules} not found in the base model. "

f"Please check the target modules and try again."

)

On the other hand, if not a single key was excluded due to exclude_modules, even though exclude_modules was passed by the user, we can give a nice warning saying something like "You passed exclude_modules=[...] but no key was matching this".

JINO-ROHIT · 2024-09-30T14:17:19Z

Hey, thanks for the suggestions. I think I fixed both the issues. Happy to work if more fixed needed, lmk

BenjaminBossan

Thanks for making these adjustments, we're getting closer to completing the PR. The general logic looks sound but I still have some comments about how the errors are raised. Please check my comments.

Also, please remember to call make style before pushing.

BenjaminBossan · 2024-09-30T14:46:26Z

src/peft/tuners/tuners_utils.py

@@ -902,6 +912,12 @@ def generate_suffixes(s):
        return set(target_modules)
    return required_suffixes

+class ExcludedModule:


Let's make this a "private" class: _ExcludedModule. Also, let's add a short docstring to describe why it's needed.

BenjaminBossan · 2024-09-30T14:46:36Z

src/peft/tuners/tuners_utils.py

+        return False
+
+    def __repr__(self):
+        return "ExcludedModule()"


Do you think we need this?

BenjaminBossan · 2024-09-30T14:50:22Z

src/peft/tuners/tuners_utils.py

-
+
+        if hasattr(peft_config, "exclude_modules") and peft_config.exclude_modules and not excluded_modules:
+            raise ValueError(


I think raising an error here is a bit too much, let's switch to a warning instead. Also, I'd rephrase the message a bit: "You have passed exclude_modules=... but no modules were excluded, please check that exclude_modules was set correctly.".

BenjaminBossan · 2024-09-30T14:58:51Z

src/peft/tuners/tuners_utils.py

+                    self._create_and_replace(peft_config, adapter_name, target, target_name, parent, current_key=key)
+
+        if len(self.targeted_module_names) == 0:
+            # this means all the targeted modules have been excluded hence both are identical


I don't think this check is quite correct. There could be other reasons why we have not targeted any modules. Instead, I think we need to check that no module was targeted, that all modules were excluded and that there were no unmatched modules.

We should also collect all checks and put them in a single if ... else instead of having them in a few places, to increase readability. So the final code would be something like this:

if not self.targeted_modules and excluded_modules and not unmatched_modules: # this means all targeted modules were excluded raise ... elif not self.targeted_modules and not excluded_modules and unmatched_modules: # this means none of the targeted modules matched raise ... # raise the old error message we already have elif not self.targeted_modules: # this means some modules did not match and some matched but were excluded raise ... # there is no error for this yet, let's raise a nice error here elif hasattr(peft_config, "exclude_modules") and peft_config.exclude_modules and not excluded_modules: # this is the case that exclude_modules was passed but was useless warnings.warn ...

Also, let's ensure that we have a test case for each of these situations on top of the tests that are already there.

JINO-ROHIT · 2024-09-30T17:19:19Z

gotcha, ive made the changes and i think it looks okayish now

BenjaminBossan

Great refactor, thanks a lot, this looks very clean now. I only found a small issue, otherwise this looks good.

BenjaminBossan · 2024-10-01T09:01:52Z

src/peft/tuners/tuners_utils.py

@@ -509,6 +544,11 @@ def inject_adapter(
                f"Please check the target modules and try again."
            )

+        if hasattr(peft_config, "exclude_modules") and peft_config.exclude_modules and not excluded_modules:


This check is redundant now with the one in line 524, right? Let's remove it.

JINO-ROHIT · 2024-10-01T12:15:47Z

yeap, done now :)

HuggingFaceDocBuilderDev · 2024-10-01T12:32:10Z

The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update.

BenjaminBossan · 2024-10-01T12:37:02Z

Please run make style :)

JINO-ROHIT · 2024-10-01T12:39:16Z

hmm weird, i actually ran make style already. is this because of windows? should i switch to a ubuntu and try again?

BenjaminBossan · 2024-10-01T12:42:40Z

I don't think it's a Windows issue. Do you perhaps have another ruff version installed? CI uses v0.6.8.

JINO-ROHIT · 2024-10-01T13:02:46Z

yeah i think its the version, hopefully it works now

BenjaminBossan · 2024-10-01T13:30:57Z

Ah damn, now the doc-builder complains. Could you please run doc-builder style src/peft tests docs/source --max_len 119 --check_only. Normally, this should run as part of make style but I assume you ran ruff separately.

JINO-ROHIT · 2024-10-01T14:12:36Z

my bad , i didnt notice the doc-builder wasnt installed, it should be alright now

JINO-ROHIT · 2024-10-01T14:21:36Z

i see the tests are failed due to a typeerror, this seems like an older python version run right?

BenjaminBossan · 2024-10-01T14:21:53Z

Ah, CI is just relentless :D Okay, this time the issue is with older Python versions, we can't do type annotations like list[str] unless we add the from __future__ import annotations import. At the very least in the LoHa config file, this is missing, but probably also in a few more. Could you please fix that. Sorry that I missed it.

JINO-ROHIT · 2024-10-01T14:23:11Z

sure lemme get on it

… vblora

JINO-ROHIT · 2024-10-01T14:31:05Z

done!

BenjaminBossan · 2024-10-01T14:41:27Z

You really seem to be on the bad side of the CI, now there is an issue with ruff again. It should be easy enough to address, please check.

JINO-ROHIT · 2024-10-01T14:44:02Z

yeah seems like after adding the import, I had to lowercase the List Union[list[int], int]

BenjaminBossan · 2024-10-01T14:53:05Z

Of course that's not the end of it, now there is a merge conflict. It should be easy enough to fix but don't hesitate to ask if you have questions.

Thanks A TON for your patience.

JINO-ROHIT · 2024-10-01T15:39:28Z

haha no worries, hope this is okay now.

JINO-ROHIT · 2024-10-01T18:24:22Z

hey, any clue why these tests are failing?

BenjaminBossan · 2024-10-01T19:58:04Z

Looks like it's unrelated, I'll investigate tomorrow.

JINO-ROHIT · 2024-10-02T05:54:33Z

alrighty

BenjaminBossan · 2024-10-02T09:05:22Z

Okay, I understand now what's going on. The issue is that with X-LoRA, we don't have any targeted modules, which is fine, but we currently raise an error because of it. See this check:

peft/src/peft/tuners/tuners_utils.py

Lines 505 to 510 in 2a80735

    
           # Handle X-LoRA case. 
        
           if not is_target_modules_in_base_model and hasattr(peft_config, "target_modules"): 
        
               raise ValueError( 
        
                   f"Target modules {peft_config.target_modules} not found in the base model. " 
        
                   f"Please check the target modules and try again." 
        
               )

I think the tests should pass if you move it up over if not self.targeted_module_names in line 503, then change that line to elif not self.targeted_module_names.

JINO-ROHIT · 2024-10-02T11:27:00Z

ahh okay, like this?

BenjaminBossan

The logic still doesn't work quite right but the good news is that we can simplify it.

Right now, we check self.targeted_module_names and also is_target_modules_in_base_model, which is a bit redundant. So let's get rid of the latter.

Additionally, to ensure that X-LoRA does not raise an error (since there, it's okay to not have target modules), we need to check that we're not using dummy target modules (a special value used by X-LoRA). I sketched this out and this diff makes the tests pass for me:

modified   src/peft/tuners/tuners_utils.py
@@ -437,13 +437,12 @@ class BaseTuner(nn.Module, ABC):
         peft_config = self._prepare_adapter_config(peft_config, model_config)
 
         self._prepare_model(peft_config, model)
-        is_target_modules_in_base_model = False
         key_list = [key for key, _ in model.named_modules()]
 
-        if getattr(peft_config, "target_modules", None) == DUMMY_TARGET_MODULES:
+        uses_dummy_target_modules = getattr(peft_config, "target_modules", None) == DUMMY_TARGET_MODULES
+        if uses_dummy_target_modules:
             # dummy adapter, we allow not matching any module
             key_list = []
-            is_target_modules_in_base_model = True
 
         # update peft_config.target_modules if required
         peft_config = _maybe_include_all_linear_layers(peft_config, model)
@@ -494,19 +493,12 @@ class BaseTuner(nn.Module, ABC):
                 unmatched_modules.append(key)
             else:
                 self.targeted_module_names.append(key)
-                is_target_modules_in_base_model = True
                 parent, target, target_name = _get_submodules(model, key)
                 ctx = init_empty_weights if low_cpu_mem_usage else nullcontext
                 with ctx():
                     self._create_and_replace(peft_config, adapter_name, target, target_name, parent, current_key=key)
 
-        # Handle X-LoRA case.
-        if not is_target_modules_in_base_model and hasattr(peft_config, "target_modules"):
-            raise ValueError(
-                f"Target modules {peft_config.target_modules} not found in the base model. "
-                f"Please check the target modules and try again."
-            )
-        elif not self.targeted_module_names:
+        if not self.targeted_module_names and not uses_dummy_target_modules:
             if excluded_modules and not unmatched_modules:
                 # All targeted modules were excluded
                 raise ValueError(

JINO-ROHIT · 2024-10-02T13:29:05Z

thanks so much for the detailed suggestion!

lets hope its okay now

JINO-ROHIT · 2024-10-02T14:11:07Z

some unrelated tests failing right? i think with windows some tensor equality is not fulfilled?

BenjaminBossan

Yes, CI is failing for unrelated reasons. I'll just re-run until the flaky test passes :)

So this PR is basically finished, but when I did a final review, I saw a minor oversight, please check my comment. But then we should really be good to go!

BenjaminBossan · 2024-10-02T15:26:35Z

src/peft/tuners/loha/config.py

@@ -119,3 +128,6 @@ def __post_init__(self):
        self.target_modules = (
            set(self.target_modules) if isinstance(self.target_modules, list) else self.target_modules
        )
+        self.exclude_modules = (


This conversion to set is missing in the LoKrConfig.

JINO-ROHIT · 2024-10-02T18:25:36Z

yeap done! thanks for guiding all the way through!

BenjaminBossan

Thanks for this nice PR, good work.

Allows to exclude target modules.

adding exclude modules param

55f3203

BenjaminBossan requested changes Sep 26, 2024

View reviewed changes

adding configs for hra, ia3, loha, lokr, oft

08a74e8

adding test cases in test_tuners_utils.py

79aa7cb

BenjaminBossan requested changes Sep 27, 2024

View reviewed changes

JINO-ROHIT added 4 commits September 27, 2024 22:05

adding param in configs for adalora, boft, fourier, lntuning, poly, v…

2db1e5d

…blora

fixing typos for hra, ia3, loha, lokr, oft

092b46f

adding test cases for exclude modules and handling target/exclude mod…

9ae2457

…ules overlap

remove debug logs

b41cb6d

BenjaminBossan requested changes Sep 30, 2024

View reviewed changes

JINO-ROHIT added 2 commits September 30, 2024 19:33

logic to check if none of the exclude modules exist

e8c6676

check target and excluded modules equality

70011d6

BenjaminBossan requested changes Sep 30, 2024

View reviewed changes

fix styling + refactor + testcases

e4586a3

BenjaminBossan requested changes Oct 1, 2024

View reviewed changes

removed redundant check

02cc534

JINO-ROHIT added 2 commits October 1, 2024 18:30

fix styling with v0.6.8

00a4d08

Merge remote-tracking branch 'origin/main' into fix-2044

3f0df02

doc builder

0ce9123

add type annotation import for boft, hra, ia3, loha, lokr, oft, poly,…

633ea87

… vblora

linting fix

331e786

Merge remote-tracking branch 'origin/main' into fix-2044

e15514a

fix xlora test fail issue

3c260b7

BenjaminBossan requested changes Oct 2, 2024

View reviewed changes

fix xlora failing tests

a714799

BenjaminBossan requested changes Oct 2, 2024

View reviewed changes

set conversion for lokr

8e778fa

BenjaminBossan approved these changes Oct 3, 2024

View reviewed changes

BenjaminBossan merged commit 8d9ecbe into huggingface:main Oct 3, 2024
14 checks passed

This was referenced Oct 17, 2024

LoraConfig conflict when using layers_to_transform in LlamaModel #2155

Closed

exclude_modules to keep specific layers or other quirky components out of a target_modules selection #2044

Closed

BenjaminBossan pushed a commit to BenjaminBossan/peft that referenced this pull request Oct 22, 2024

FEAT: Adding exclude modules param(huggingface#2044) (huggingface#2102)

9ddc9f1

Allows to exclude target modules.

	@staticmethod
	def _check_target_module_exists(lora_config, key):
	return check_target_module_exists(lora_config, key)

	if not is_target_modules_in_base_model and hasattr(peft_config, "target_modules"):
	raise ValueError(
	f"Target modules {peft_config.target_modules} not found in the base model. "
	f"Please check the target modules and try again."
	)



		if hasattr(peft_config, "exclude_modules") and peft_config.exclude_modules and not excluded_modules:
		raise ValueError(

FEAT: Adding exclude modules param(#2044) #2102

FEAT: Adding exclude modules param(#2044) #2102

Conversation

JINO-ROHIT commented Sep 26, 2024

BenjaminBossan left a comment

Choose a reason for hiding this comment

JINO-ROHIT commented Sep 26, 2024

JINO-ROHIT commented Sep 26, 2024

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

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

JINO-ROHIT commented Sep 30, 2024

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

JINO-ROHIT commented Sep 30, 2024

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JINO-ROHIT commented Oct 1, 2024

HuggingFaceDocBuilderDev commented Oct 1, 2024

BenjaminBossan commented Oct 1, 2024

JINO-ROHIT commented Oct 1, 2024

BenjaminBossan commented Oct 1, 2024

JINO-ROHIT commented Oct 1, 2024

BenjaminBossan commented Oct 1, 2024

JINO-ROHIT commented Oct 1, 2024

JINO-ROHIT commented Oct 1, 2024

BenjaminBossan commented Oct 1, 2024

JINO-ROHIT commented Oct 1, 2024

JINO-ROHIT commented Oct 1, 2024

BenjaminBossan commented Oct 1, 2024

JINO-ROHIT commented Oct 1, 2024

BenjaminBossan commented Oct 1, 2024

JINO-ROHIT commented Oct 1, 2024

JINO-ROHIT commented Oct 1, 2024

BenjaminBossan commented Oct 1, 2024

JINO-ROHIT commented Oct 2, 2024

BenjaminBossan commented Oct 2, 2024

JINO-ROHIT commented Oct 2, 2024

BenjaminBossan left a comment

Choose a reason for hiding this comment

JINO-ROHIT commented Oct 2, 2024 • edited Loading

JINO-ROHIT commented Oct 2, 2024

BenjaminBossan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JINO-ROHIT commented Oct 2, 2024

BenjaminBossan left a comment

Choose a reason for hiding this comment

JINO-ROHIT commented Oct 2, 2024 •

edited

Loading