-
Notifications
You must be signed in to change notification settings - Fork 2.3k
FIX BdLoraConfig.nblocks default value #3252
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -65,7 +65,7 @@ | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| ) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from peft.mapping import PEFT_TYPE_TO_PREFIX_MAPPING | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from peft.tuners.lokr.layer import LoKrLayer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from peft.tuners.lora.config import CordaConfig | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from peft.tuners.lora.config import BdLoraConfig, CordaConfig | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from peft.tuners.lora.corda import preprocess_corda | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from peft.tuners.lora.layer import LoraLayer | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| from peft.utils import infer_device | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
@@ -1233,6 +1233,23 @@ def test_bdlora_feature_size_non_divisible_by_blocksize_raises(self): | |||||||||||||||||||||||||||||||||||||||||||||||||||||||
| with pytest.raises(ValueError, match="not divisible by"): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| get_peft_model(model, config) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_bdlora_default_nblocks_is_int(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We don't need dedicated tests for this. Our normal tests should already surface the error but they don't because they all override peft/tests/test_custom_models.py Lines 1014 to 1040 in 1b3d06d
So to trigger the error, we can just remove |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # The default value of BdLoraConfig.nblocks must be the integer 1, not a wrapped field object. A misplaced | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # trailing comma previously turned the default into a 1-tuple, which broke get_peft_model whenever the user | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # did not pass nblocks explicitly. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert BdLoraConfig().nblocks == 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def test_bdlora_get_peft_model_with_default_nblocks(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # Building a BD-LoRA model without specifying nblocks must work and rely on the documented default of 1. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| # This used to raise a TypeError because nblocks defaulted to a tuple instead of an int. | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| model = self.get_model() | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| bdlora_config = {"target_modules_bd_a": ["linear"], "target_modules_bd_b": []} | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| config = LoraConfig(target_modules=["linear"], use_bdlora=bdlora_config) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| model = get_peft_model(model, config) | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| assert model.linear.lora_A["default"].nblocks == 1 | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| @pytest.fixture | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| def mha_cls(self): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
| class ModelMha(nn.Module): | ||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||||||||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ignore this