added checks for layers to transforms and layer pattern in lora #2159

JINO-ROHIT · 2024-10-17T16:05:22Z

This PR is aimed to address point 3 in #2155 to adjust the error message when layers transforms and layer patterns are specified.

Tbh, I dont quite understand the functionality of layern_patterns but assuming that we need layers_pattern when layers_transform is applied, ive added this simple check.

BenjaminBossan · 2024-10-18T11:19:03Z

Thanks for working on this PR so quickly. Could you please propagate this check to the other config classes that support this too? Also, it would be great to add a test to tests/test_config.py for this. LMK if you have questions about this.

Apart from this fix, I think we can also improve the error reporting when users pass layers_pattern and layers_to_transform. Right now, we only check target_modules. In particular, I'm thinking about this message:

peft/src/peft/tuners/tuners_utils.py

Lines 512 to 515 in 57a452a

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

As the user reported, they got the error

*** ValueError: Target modules {'v_proj', 'q_proj'} not found in the base model. Please check the target modules and try again.

But the real reason was how they defined layers_pattern and layers_to_transform. Therefore, I think it would be nice if we check if those arguments were set and if yes, extend the error message accordingly. This could be in a separate PR though.

JINO-ROHIT · 2024-10-18T16:53:11Z

Sure, i will work through this one

…, oft, vblora, vera

JINO-ROHIT · 2024-10-19T10:31:45Z

ive added a test case and extended to other classes.

I noticed that one unrelated testcase was failing:

FAILED tests/test_config.py::TestPeftConfig::test_regex_with_layer_indexing_lora - AssertionError: Regex pattern did not match.

Should we try and fix this?

JINO-ROHIT · 2024-10-19T10:33:44Z

As the user reported, they got the error

*** ValueError: Target modules {'v_proj', 'q_proj'} not found in the base model. Please check the target modules and try again.

But the real reason was how they defined layers_pattern and layers_to_transform. Therefore, I think it would be nice if we check if those arguments were set and if yes, extend the error message accordingly. This could be in a separate PR thoug

ouuu, il raise another PR to handle this second part

HuggingFaceDocBuilderDev · 2024-10-21T09:45:50Z

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-21T10:14:11Z

Could you please run make style?

JINO-ROHIT · 2024-10-21T11:26:43Z

Hi @BenjaminBossan done!

BenjaminBossan · 2024-10-21T12:38:36Z

@JINO-ROHIT Oh I see there is an error there. It is actually possible to have layers_pattern being None, as we use a different regex in that case:

peft/src/peft/tuners/tuners_utils.py

Lines 981 to 982 in e8259ff

    
           if layers_pattern is None or len(layers_pattern) == 0: 
        
               layer_index = re.match(r".*\.[^.]*\.(\d+)\.", key)

This is why a bunch of tests are failing. I didn't know that (or forgot), so this is my bad. I think we can still do the reverse though, i.e. if layers_to_transform is None, we should not have layers_pattern. WDYT?

…issue-2155

JINO-ROHIT · 2024-10-21T13:45:52Z

yeah missed this too, did you mean to do something like this?

BenjaminBossan · 2024-10-22T12:50:47Z

Ah no, I don't think that's quite right. You added if layer_indexes is not None but above that, we already check if is_using_layer_indexes, so that's redundant.

What I mean is that the check

        if self.layers_to_transform is not None and self.layers_pattern is None:
            raise ValueError("When `layers_to_transform` is specified, `layers_pattern` must also be specified. ")

should be changed to

        if self.layers_pattern is not None and self.layers_to_transform is None:
            raise ValueError("When `layers_pattern` is specified, `layers_to_transform` must also be specified. ")

I also like to add some parentheses in these cases for readability:

if (self.layers_pattern is not None) and (self.layers_to_transform is None):

JINO-ROHIT · 2024-10-22T16:44:31Z

yeah, now i get why the testcase werent passing after seeing the other test config files, ive made the reverse check and moved the check down so now i think all the testcases should pass

BenjaminBossan

As you can see, some tests in the CI are failing. This is actually expected, as some of them just don't make sense anymore, like this one:

peft/tests/test_tuners_utils.py

Line 97 in 0d58942

("foo.bar.7.baz", ["baz"], None, ["bar"], True),

While checking this, I also noticed some room for improvement in the checking logic, please take a look at my comment.

I think this needs to be changed and the tests adjusted a bit, like I mentioned. You can call pytest tests/test_tuners_utils.py locally to ensure that all relevant tests pass.

BenjaminBossan · 2024-10-23T11:09:00Z

src/peft/tuners/adalora/config.py

-        if isinstance(self.target_modules, str) and self.layers_pattern is not None:
-            raise ValueError("`layers_pattern` cannot be used when `target_modules` is a str.")
+        # check for layers_to_transform and layers_pattern
+        if (self.layers_pattern is not None) and (self.layers_to_transform is None):


I think these checks should be adjusted like so:

Suggested change

if (self.layers_pattern is not None) and (self.layers_to_transform is None):

if self.layers_pattern and not self.layers_to_transform:

Why?

If users pass layers_pattern=[], layers_to_transform=None, we should not raise.

If users pass layers_pattern=["foo"], layers_to_transform=[], we should raise

Right now, we don't do that because of the strict None check.

JINO-ROHIT · 2024-10-23T18:58:34Z

Sure, im away for a couple days on a conference, ill be back and fix this :)

BenjaminBossan · 2024-10-24T08:41:18Z

Thanks @JINO-ROHIT. It's not urgent, so take your time, enjoy the conference.

JINO-ROHIT · 2024-10-26T07:11:00Z

Thank you very much @BenjaminBossan , im back . the checks now are making a lot more sense, ive fixed them and run tests.

JINO-ROHIT · 2024-10-29T12:42:08Z

Hi @BenjaminBossan any clue why this is failing?

BenjaminBossan

Thanks for adding this check to the configs.

* base judge * adding mixture of judges * update doc * update doc * formatting * fix small typo in doc * fix randomcontraintjudge * replace arxiv by hf papers Co-authored-by: Quentin Gallouédec <45557362+qgallouedec@users.noreply.github.com> * formatting Co-authored-by: Quentin Gallouédec <45557362+qgallouedec@users.noreply.github.com> * fix naming in __init__ * run precommi * adding gold answers to judges * cgpo llm judges * fix init * output type * adjust booleans in test * adapt moj doc * renaming and removing factuality and safety judges * fix typo in import * fix small typo in naming * formatting * Update trl/trainer/judges.py Co-authored-by: Quentin Gallouédec <45557362+qgallouedec@users.noreply.github.com> * update parameter name * update tests * update doc * Update trl/trainer/judges.py Co-authored-by: Quentin Gallouédec <45557362+qgallouedec@users.noreply.github.com> * Update doc Co-authored-by: Quentin Gallouédec <45557362+qgallouedec@users.noreply.github.com> * fix alltruejudge type * Refactor judge variable names and update test names * Clarify judgment logic * Fix invalid binary judgment check in AllTrueJudge class * Fix invalid binary judgment check in AllTrueJudge class --------- Co-authored-by: Quentin Gallouédec <45557362+qgallouedec@users.noreply.github.com> Co-authored-by: Quentin Gallouédec <quentin.gallouedec@huggingface.co>

added checks for layers to transforms and layer pattern lora

50a1d5a

JINO-ROHIT added 2 commits October 19, 2024 15:30

propagating checks to lora, adalora, boft, fourierft, hra, loha, lokr…

63b526e

…, oft, vblora, vera

adding test case for layers_transform without layers_pattern

28ab006

JINO-ROHIT added 2 commits October 21, 2024 16:42

make style

8075df1

style

ebf688f

BenjaminBossan mentioned this pull request Oct 21, 2024

Improving error message when users pass layers_to_transform and layers_pattern #2164

Closed

JINO-ROHIT added 2 commits October 21, 2024 19:12

handle layers pattern only for layers transform

ada25ce

Merge branch 'issue-2155' of https://github.com/JINO-ROHIT/peft into …

72653b0

…issue-2155

reversing the check, testcase

0c3bca3

make style

406ec3c

BenjaminBossan requested changes Oct 23, 2024

View reviewed changes

revised check and removed obselete testcases

3d58401

BenjaminBossan approved these changes Oct 29, 2024

View reviewed changes

BenjaminBossan merged commit 214345e into huggingface:main Oct 29, 2024
14 checks passed

yaswanth19 pushed a commit to yaswanth19/peft that referenced this pull request Oct 29, 2024

ENH Check layers to transforms and layer pattern (huggingface#2159)

eb6b314

Guy-Bilitski pushed a commit to Guy-Bilitski/peft that referenced this pull request May 13, 2025

ENH Check layers to transforms and layer pattern (huggingface#2159)

ed0254e

	if (self.layers_pattern is not None) and (self.layers_to_transform is None):
	if self.layers_pattern and not self.layers_to_transform:

added checks for layers to transforms and layer pattern in lora #2159

added checks for layers to transforms and layer pattern in lora #2159

Uh oh!

Conversation

JINO-ROHIT commented Oct 17, 2024

Uh oh!

BenjaminBossan commented Oct 18, 2024

Uh oh!

JINO-ROHIT commented Oct 18, 2024

Uh oh!

JINO-ROHIT commented Oct 19, 2024

Uh oh!

JINO-ROHIT commented Oct 19, 2024

Uh oh!

HuggingFaceDocBuilderDev commented Oct 21, 2024

Uh oh!

BenjaminBossan commented Oct 21, 2024

Uh oh!

JINO-ROHIT commented Oct 21, 2024

Uh oh!

BenjaminBossan commented Oct 21, 2024

Uh oh!

JINO-ROHIT commented Oct 21, 2024

Uh oh!

BenjaminBossan commented Oct 22, 2024

Uh oh!

JINO-ROHIT commented Oct 22, 2024

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan Oct 23, 2024

Choose a reason for hiding this comment

Uh oh!

JINO-ROHIT commented Oct 23, 2024

Uh oh!

BenjaminBossan commented Oct 24, 2024

Uh oh!

JINO-ROHIT commented Oct 26, 2024

Uh oh!

JINO-ROHIT commented Oct 29, 2024

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants