Skip to content
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

Update pip package RWKV.model and v2/chat.py to support LoRA #82

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

RafaRed
Copy link

@RafaRed RafaRed commented Apr 5, 2023

Update pip package RWKV.model and v2/chat.py to support LoRA based on Blealtan/RWKV-LM-LoRA implementation.

Notice that pip package need to be uploaded again with the new changes to work.

…n Blealtan/RWKV-LM-LoRA implementation. Notice that pip package need to be uploaded again with the new changes to work.
@KerfuffleV2
Copy link
Contributor

Won't this break everything that uses the rwkv PIP package? It's trying to access attributes on lora. Why not have it as a keyword argument that defaults to None, that way existing code can keep working.

Also, what happens if the model is "preconverted"? I'm assuming it won't work correctly if that part has already occurred, so probably would want to add checks for whether it's a preconverted model before doing the lora part.

@RafaRed
Copy link
Author

RafaRed commented Apr 8, 2023

@KerfuffleV2 you are right about attributes, Need to change then to optional.
About the preconverted I don't know, did not perform any tests about it yet.

@KerfuffleV2
Copy link
Contributor

About the preconverted I don't know, did not perform any tests about it yet.

I'm almost certain it couldn't work. Those tensors may be in a different format like u8. They will also have some additional fields like mx, my, rx, ry (maximums, minimums related to quantization).

So it may be possible to support that but I'm pretty positive it would need special handling. Probably the easiest approach at first is to just raise an exception if Lora is specified and the model file has been preconverted.

@Blealtan
Copy link
Contributor

Blealtan commented Apr 9, 2023

IMO the LoRA merging should go before the conversion since there are too many things happening during converting the model (including the xy quantizing, etc.). +1 on disallowing LoRA when pre-converted model is specified.

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.

None yet

3 participants