fix: subclass Lora config from upstream peft.LoraConfig#609
Conversation
|
Thanks for making a pull request! 😃 |
Signed-off-by: romit <romit@ibm.com>
…umentParser Signed-off-by: romit <romit@ibm.com>
romitjain
force-pushed
the
fix/lora-config
branch
from
328c0c3 to
de6baa7
Compare
romitjain
requested review from
aluu317,
anhuong,
dushyantbehl,
fabianlim and
kmehant
as code owners
| from typing import List, Optional | ||
|
|
||
| # Third Party | ||
| from peft import LoraConfig as _LoraConfig |
There was a problem hiding this comment.
Can you rename this import as HFLoraConfig
like we have for other imports.
There was a problem hiding this comment.
@dushyantbehl I have fixed this, can you please review the PR again?
Signed-off-by: romit <romit@ibm.com>
romitjain
force-pushed
the
fix/lora-config
branch
from
4907de8 to
5acad56
Compare
Signed-off-by: romit <romit@ibm.com>
| alora_config.task_type = task_type | ||
| hf_peft_config = alora_config | ||
| elif isinstance(tuning_config, peft_config.LoraConfig): | ||
| lora_config = asdict(tuning_config) |
There was a problem hiding this comment.
I have removed this since tuning_config will now already be of type peft.LoraConfig.
Flattening the parameters using asdict recursively flattens all the dataclass parameters. This means that any parameter like: a.b.c is now converted to a[b][c]. When passed using **a, it does not preserve the dataclass structure of b which can cause issues if the underlying library accesses b.c instead of b[c]
Signed-off-by: r0 <11757603+romitjain@users.noreply.github.com>
Signed-off-by: romit <romit@ibm.com>
dushyantbehl
left a comment
dushyantbehl
left a comment
There was a problem hiding this comment.
largely looks good to me...just minor comments
| ) | ||
| bias = "none" | ||
| lora_dropout: float = 0.05 | ||
| init_lora_weights: bool = field( |
There was a problem hiding this comment.
@romitjain per your comment on line 58 above the other arguments are incompatible with arg parser but do we need to have arguments like this too?
There was a problem hiding this comment.
Yes, any field that accepts heterogeneous fields will need to be defined again.
For init_lora_weights, the original type is:
bool | Literal["gaussian", "eva", "olora", "pissa", "pissa_niter_[number of iters]", "corda", "loftq", "orthogonal"]
| exp_metadata: Dict of key value pairs passed to train to be recoreded by the tracker. | ||
| quantized_lora_config: tuning.config.acceleration_configs.QuantizedLoraConfig \ | ||
| Should be used in combination with peft_config.LoraConfig for Lora tuning \ | ||
| Should be used in combination with LoraConfig for Lora tuning \ |
There was a problem hiding this comment.
Can we add a reference to the HuggingFace loraconfig arguments here?
Signed-off-by: romit <romit@ibm.com>
dushyantbehl
changed the title
2 participants
Description of the change
tuning/config/peft_config.pysubclasses frompeft.LoraConfigwhich will now support all the LoraConfig arguments. I have decided to subclass because of 2 reasonslora_alphaandlora_dropout)Related issue number
There is no specific issue number for this yet. However, the issue was that the launch only accepted 5 arguments for LoraConfig, and if any standard parameter (like
modules_to_save) was provided, it was ignored.How to verify the PR
Any run that passes
modules_to_savewill now be respected (instead of being ignored earlier).Was the PR tested
All the config-related tests are passing