Skip to content

Support period in lora name #76

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

Merged
merged 7 commits into from
May 8, 2025
Merged

Conversation

arledesma
Copy link

Fixes #75

@colinurbs
Copy link
Collaborator

Thank you this looks solid. Just going to try it real quick and then merge it into main.

@colinurbs
Copy link
Collaborator

Actually ended up having an error during the actual lora loading. I wonder if peft can't handle the "." and we need to at least rename it internally.

Loading pixar_7.epochs was unsucessful with the following error:
'module name can\'t contain ".", got: pixar_7.epochs'
Traceback (most recent call last):
  File "/home/colin/FramePack/studio.py", line 571, in worker

@arledesma
Copy link
Author

@colinurbs I'll test some more. I had a dirty branch with a ton of local changes and I ended up partial staging this patch.

https://github.com/pytorch/pytorch/pull/6639/files#diff-4be56271f7bfe650e3521c81fd363da58f109cd23ee80d243156d2d6ccda6263R133-R134

```
Loading pixar_7.epochs was unsucessful with the following error:
'module name can\'t contain ".", got: pixar_7.epochs'
Traceback (most recent call last):
  File "/home/colin/FramePack/studio.py", line 571, in worker
```
@arledesma
Copy link
Author

@colinurbs Seems to work with this last change.

I'm not sure if this is a great way to handle it with the type of audience that is currently using the codebase. The suggested method is to just remove every ., as seen in pytorch/vision#474. I don't know if that would be a good idea though as people have multiple versions of these lora's that may simply have a minor version change. Long term it may be a good idea to replace this hack with some simple metadata that permits the user to safely name the module.

@arledesma
Copy link
Author

Will need further testing. I'll set up a clean install and run through a few scenarios in the next day or so, unless you have time to go through the tests.

  1. Single lora, no period, must continue to work.

  2. Multiple lora, no period, must continue to work.

  3. Cancellation for 1 and 2 must continue to work.

  4. Single lora, period in name, should work

  5. Multiple lora, period in name, should work.

  6. Multiple lora, mixture of period and no period, should work.

  7. Cancellation for 4, 5, 6 should work.

  8. Any additional jobs queued must continue to work.

  9. Any point that lora's are unloaded must continue to unload the lora, regardless of period in the name

@arledesma
Copy link
Author

@colinurbs manual tests passed.

The only time that the tests failed was when a cancellation was sent while sample_hunyuan was performing work prior to invoking the callback, leading to the KeyboardInterrupt in the callback that just leaves the application in an unusable state - gradio still functions but the queue system was basically dead. Let me know if you need a separate issue to track that.

@colinurbs
Copy link
Collaborator

Yeah please open a ticket for that. Thanks for your testing!

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.

3 participants