-
Notifications
You must be signed in to change notification settings - Fork 5.3k
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
[PEFT
/ LoRA
] Fix civitai bug when network alpha is an empty dict
#5608
[PEFT
/ LoRA
] Fix civitai bug when network alpha is an empty dict
#5608
Conversation
@@ -129,7 +129,7 @@ def get_peft_kwargs(rank_dict, network_alpha_dict, peft_state_dict, is_unet=True | |||
rank_pattern = dict(filter(lambda x: x[1] != r, rank_dict.items())) | |||
rank_pattern = {k.split(".lora_B.")[0]: v for k, v in rank_pattern.items()} | |||
|
|||
if network_alpha_dict is not None: | |||
if network_alpha_dict is not None and len(network_alpha_dict) > 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if network_alpha_dict is not None and len(network_alpha_dict) > 0: | |
if network_alpha_dict: |
Wouldn't this be sufficient?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah in practice it should be sufficient, but I think that we should avoid python magic and make sure we test explicit checks :D
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wouldn't consider this "magic", even PEP8 recommends it:
For sequences, (strings, lists, tuples), use the fact that empty sequences are false:
# Correct:
if not seq:
if seq:# Wrong:
if len(seq):
if not len(seq):
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see ok ! happy to add that, I know in transformers we usually avoid these things, I will let diffusers maintainers comment on this
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would like to stay as explicit as possible here following the transformers
approach.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks much!
Co-authored-by: Benjamin Bossan <[email protected]>
Nice fix! |
…huggingface#5608) * fix civitai bug * add test * up * fix test * added slow test. * style * Update src/diffusers/utils/peft_utils.py Co-authored-by: Benjamin Bossan <[email protected]> * Update src/diffusers/utils/peft_utils.py --------- Co-authored-by: Benjamin Bossan <[email protected]>
…huggingface#5608) * fix civitai bug * add test * up * fix test * added slow test. * style * Update src/diffusers/utils/peft_utils.py Co-authored-by: Benjamin Bossan <[email protected]> * Update src/diffusers/utils/peft_utils.py --------- Co-authored-by: Benjamin Bossan <[email protected]>
…huggingface#5608) * fix civitai bug * add test * up * fix test * added slow test. * style * Update src/diffusers/utils/peft_utils.py Co-authored-by: Benjamin Bossan <[email protected]> * Update src/diffusers/utils/peft_utils.py --------- Co-authored-by: Benjamin Bossan <[email protected]>
What does this PR do?
Fixes: #5606
The fix is simply to ignore the processing of
network_alpha_dict
in case that dictionary is empty.Also confirmed that I got the same generation with that checkpoint on main vs with this branch, hence added a slow test for it
cc @sayakpaul @patrickvonplaten