InvokeAI icon indicating copy to clipboard operation
InvokeAI copied to clipboard

feat(nodes): skip on duplicate loras instead of erroring

Open psychedelicious opened this issue 1 year ago • 4 comments

Summary

The LoRA and SDXL LoRA nodes would error if it duplicated LoRAs. To make the nodes more resilient, they now skip dupes and log a warning to the console instead.

Also added a warning for the LoRA Collection Loader nodes. These already skipped but didn't log a warning.

Related Issues / Discussions

https://discord.com/channels/1020123559063990373/1130288930319761428/1266235074647691335

QA Instructions

This workflow has 4 test flows, each with 3 LoRA loaders:

  • SDXL LoRA chain
  • SDXL LoRA collection
  • SD1.5 LoRA chain
  • SD1.5 LoRA collection

For each flow, set two of the loaders to the same LoRA and the 3rd to a different. The output for each flow should have only two LoRAs, and you should have warnings in the console for each.

If you test this on main, we expect errors for both the chain flows.

lora test(1).json

Merge Plan

n/a

Checklist

  • [x] The PR has a short but descriptive title, suitable for a changelog
  • [ ] Tests added / updated (if applicable)
  • [ ] Documentation added / updated (if applicable)

psychedelicious avatar Jul 26 '24 04:07 psychedelicious

Discussed with @StAlKeR7779 - in some cases, skipping duplicates may not be correct - for example, we may want to let the last instance of a LoRA be used, or sum the weights.

Maybe we add an enum field - on_conflict or duplicate_handling, something like that - to describe how to handle duplicate LoRAs. Valid options could be 'skip' | 'replace' | 'error' | 'sum_weights'... 'max' | 'min' | 'average'... Probably overkill.

Also, 'skip' and 'replace' are ambiguous for the collection loader, because we don't know the order in which the collect node does its collecting. It's not clear which would be taken of duplicates provided to the collect.

psychedelicious avatar Jul 26 '24 12:07 psychedelicious

Of all the possible duplicate handling methods, error still makes the most sense to me as the default.

Maybe we start with min, max, error? That would handle most cases I can think of.

RyanJDick avatar Jul 26 '24 15:07 RyanJDick

Some context from the problem report. They were using this node:

image

It outputs unet and clip fields with the loras baked in.

psychedelicious avatar Jul 26 '24 22:07 psychedelicious

I feel that most cases will be closed by:

  • error - error if lora applied before
  • skip - use already applied weight
  • replace/override - use new lora weight
  • sum/merge - sum old and new weight

For example case with metadata node should be closed by replace/override option, as it allow to select weight of details lora independent of what used for original image generation. For default logic I feel that should be sum/merge as it do exactly what applying lora means - patch what provided, or error to notify user about double use of lora. About skip - I not sure in which cases it should fit.

StAlKeR7779 avatar Jul 28 '24 17:07 StAlKeR7779