-
Notifications
You must be signed in to change notification settings - Fork 45
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
Set default value of target_modules to be None in LoraConfig #269
Conversation
Signed-off-by: Will Johnson <mwjohnson728@gmail.com>
Example run of TinyLLAMA-v0 with Lora (using llama model_type):
Results in the following adapter_config.json file:
This matches with what we expect of a llama type model. I also tried to run with gpt-2-medium:
I get the following error:
Which didn't get resolved until I added
|
Signed-off-by: Will Johnson <mwjohnson728@gmail.com>
With target_modules defaulting to None, if they are not explicitly specified, they will be automatically updated by the peft library based on the model architecture. TinyLlama-v0 test:
Result:
gpt2-medium test:
Result:
opt-2-125m test:
Result:
|
@@ -46,7 +46,7 @@ class LoraConfig: | |||
r: int = 8 | |||
lora_alpha: int = 32 | |||
target_modules: List[str] = field( | |||
default_factory=lambda: ["q_proj", "v_proj"], | |||
default=None, |
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.
It would be good to have a unit test that uses the llama model, this LoRA config, does LoRA tuning and gets apater config and ensures adapter config still have q_proj , v_proj .
@anhuong ^
by default if we set None, HF should use the q_proj etc default values per model architecture. we should test that is indeed the case and it is an optional field
excellent testing above #269 (comment) would be good to capture it via some unit test too, maybe it is already captured :) lets just double check and add if not |
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.
This is great! Thanks @willmj. Excellent testing, and I agree with Sukriti about running the logical test via the code if possible!
Description of the change
Original issue (from Slack):
In short, changed
default_factory=lambda: ["q_proj", "v_proj"],
todefault=None,
in the LoraConfig class.Related issue number
N/A
How to verify the PR
Run unit tests.
Additionally, you can run tuning on a model, and on the output model check target_modules in adapter_config.json matches the intended target_modules of the model used.
Was the PR tested
If you have any questions or concerns, please respond below. Thanks!