added checks for layers to transforms and layer pattern in lora

[JINO-ROHIT]

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]

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]

Sure, i will work through this one

[JINO-ROHIT]

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]

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]

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]

Could you please run make style?

[JINO-ROHIT]

Hi @BenjaminBossan done!

[BenjaminBossan]

@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?

[JINO-ROHIT]

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

[BenjaminBossan]

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]

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]

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?

1. If users pass layers_pattern=[], layers_to_transform=None, we should not raise.
2. 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]

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

[BenjaminBossan]

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

[JINO-ROHIT]

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

[JINO-ROHIT]

Hi @BenjaminBossan any clue why this is failing?

[BenjaminBossan]

Thanks for adding this check to the configs.