Skip to content

Add LoRA support - #873

Closed
cosmicoptima wants to merge 13 commits into
PrimeIntellect-ai:mainfrom
cosmicoptima:main
Closed

cosmicoptima wants to merge 13 commits into
PrimeIntellect-ai:mainfrom
cosmicoptima:main

Conversation

@cosmicoptima

Copy link
Copy Markdown
Contributor

This PR adds support for LoRA. It adds a model.lora config section with the fields enabled rank alpha dropout target_modules (modules adapted by the LoRA) trainable_modules (modules kept fully trainable).

The LoRA is merged into the base model on save for now because vLLM doesn't support loading a LoRA that keeps some modules fully trained. Support for it is being added though: vllm-project/vllm#11714.


GitHub Issue: #796

@cosmicoptima

cosmicoptima commented Sep 6, 2025 •

Copy link
Copy Markdown
Contributor Author

oops one sec im fixing the style check (edit: it's good now)

@cosmicoptima

Copy link
Copy Markdown
Contributor Author

i'm able to fit 14b parameters into 2 trainer h100s with lora enabled, and 32b parameters into 3 trainer h100s with lora + liger kernel

@kalomaze

kalomaze commented Sep 6, 2025

Copy link
Copy Markdown
Contributor

some notes:

  • if we want to standardize naming conventions with PEFT then trainable_modules should be modules_to_save
  • this implementation is less invasive in terms of changing stuff at the expense of not being any faster at saving to disk (entire model is saved, not just the lora adapter), this is perfectly fine tho bc you cannot keep some layers like the output head or embeddings fully trainable otherwise because of how vllm's lora implementation works
  • this implementation also implicitly adds support for freezing arbitrary layers or module types via regex, i.e someone could specify no target modules and leave only attention projs in trainable_modules, which would keep the MLPs frozen. this is slightly less direct than having an explicit configuration for freezing, but IMO is fine

@cosmicoptima

Copy link
Copy Markdown
Contributor Author

if we want to standardize naming conventions with PEFT then trainable_modules should be modules_to_save

good idea

@mikasenghaas mikasenghaas left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

integration looks nice, i like it. will need some experiments and integration tests to confirm correctness

Comment thread src/prime_rl/trainer/config.py Resolved
Comment thread src/prime_rl/trainer/model.py Resolved
Comment thread src/prime_rl/trainer/weights.py Resolved
Comment thread src/prime_rl/trainer/weights.py Resolved
@cosmicoptima

Copy link
Copy Markdown
Contributor Author

the gpu tests pass on my runpod and it looks like there has been the same OOM for unit tests on some other recent checks outside of this PR

@mikasenghaas

Copy link
Copy Markdown
Member

ah yea, looks like a dangling process on our ci machine, cleaned it up and re-ran your tests

@samsja

samsja commented Oct 2, 2025

Copy link
Copy Markdown
Member

btw @cosmicoptima I forget to thank you for the great PR. We were not ready at the time to merge it. We are working on porting it here #1059 and will make sure you get the correct attribution for your work :)

@cosmicoptima

Copy link
Copy Markdown
Contributor Author

cool to know it's being worked on @samsja ! also found a little trick whose implementation i don't think i put in this PR, and that i don't think i mentioned here--which is initializing a new lora every step and merging it into the base model at the end of the step

thereby approximating a higher rank over time while saving the same amount of compute

@mikasenghaas

Copy link
Copy Markdown
Member

tysm for your contribution @cosmicoptima, super clean pr. we updated with latest main, ran some experiments to validate and merged this as an experimental feature now!

@mikasenghaas

Copy link
Copy Markdown
Member

closing this now:)

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.

4 participants