Skip to content

Conversation

glerzing
Copy link
Contributor

It was simpler to solve both issues in the same PR.

peft is not completely stable, there are still a few bugs, especially on seq2seq (e.g. flan-t5-small does not work). LORA looks pretty stable though. I mostly relied on unit tests, but I will keep testing a few things in the coming days. And I will maybe add in a next PR a use-case example if you think that's useful. I removed "ref_model" because it didn't seem to be actually used.

Feel free to ask questions or to make remarks. For example on the coding style, I don't know if there are some guidelines. Also, if you want the automated tests to be faster, I can reduce the number of models or configs to test.

@glerzing
Copy link
Contributor Author

Some things I could do next, tell me if you agree :

  • See if I can make it possible to use multiple methods (e.g. prefix tuning + Lora)
  • Make it possible to retrain a peft fine-tuned model (currently trlx can create but can't load a peft model)

@glerzing
Copy link
Contributor Author

Is it ok if we save only the peft adapter and not the value head ?

@LouisCastricato
Copy link
Contributor

no there are many situations you would need to save the value head. also yes we should have loading, especially for resuming from checkpoints.

I am going to mark this as draft as it seems to be not ready for review. I do think in the interim you should resolve the loading/saving issues and add more unit tests.

@LouisCastricato LouisCastricato marked this pull request as draft May 14, 2023 13:21
@glerzing
Copy link
Contributor Author

no there are many situations you would need to save the value head.

But then should we save the whole model instead of just the peft adapter ? Or let a boolean option for whether to save the whole model or just the adapter ? Or make some sophisticated code to save just the peft adapter and the value head ? (I'm not sure how important it is to limit the memory size.)

@glerzing glerzing force-pushed the peft_migration branch 7 times, most recently from 984001f to 3430029 Compare May 23, 2023 22:45
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