Skip to content

Extend base Trainer with LoRATrainer using PEFT#51

Open
SaviNimz wants to merge 1 commit intopyfenn:mainfrom
SaviNimz:implement_lora_trainer
Open

Extend base Trainer with LoRATrainer using PEFT#51
SaviNimz wants to merge 1 commit intopyfenn:mainfrom
SaviNimz:implement_lora_trainer

Conversation

@SaviNimz
Copy link

This PR introduces a LoRATrainer class that extends the base Trainer to support Parameter-Efficient Fine-Tuning (PEFT) using LoRA via the peft library. If a LoraConfig is provided, the model is wrapped using get_peft_model; otherwise, it behaves exactly like the standard Trainer.

Copy link
Collaborator

@blkdmr blkdmr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi, the code is well written, but there are a few issues that make this class unusable as it is. For example, at least a base LLM model, a tokenizer and a PEFT config object should be passed to the trainer. Also, the training procedure needs to be reworked, since we have attention masks, tokenizers, and other components that aren’t supported by the base Trainer class. Do you think you could address these issues?

@SaviNimz
Copy link
Author

Hi I've been looking in to the possible implementations and there are few issues I've came across. It would be helpful to have some insights:

  • Batch/input formats vary a lot between datasets and model types (dict vs tuple, different required keys).
  • Model outputs aren’t consistent (some expose outputs.loss, others return tuples or need labels to compute loss).
  • LoRA target_modules are architecture-specific, so the same config won’t work reliably across different models.
  • PEFT checkpoint save/load can be fragile across versions, and resuming with optimizer state often mismatches after wrapping.

I'm also not much familiar with LORA. But from what I see even if I override the existing fit function it won't be generalizable right?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants