huggingface / diffusers

🤗 Diffusers: State-of-the-art diffusion models for image and audio generation in PyTorch and FLAX.
https://huggingface.co/docs/diffusers
Apache License 2.0
25.47k stars 5.27k forks source link

Making custom component <> custom file mapping really optional. #6563

Closed ashawkey closed 7 months ago

ashawkey commented 9 months ago

What API design would you like to have changed or added to the library? Why?

I have a custom pipeline using a custom component class defined locally, for example, a custom UNet in mylib/unet.py. The exported model_index.json will describe it as:

"unet": [
    "mylib.unet",
    "myUNet"
],

However, after uploading the weights to a huggingface repo, it fails to load the custom component class and raises this ValueError since it cannot find unet/mylib.unet.py:

https://github.com/huggingface/diffusers/blob/79df50388df09d9615e3c067695a453bb0a694c0/src/diffusers/pipelines/pipeline_utils.py#L1690-L1706

Despite the comment says this mapping could be optional, the current code strictly requires such a mapping relationship, which makes loading custom component inflexible.

I have checked the examples to load custom pipelines, but they assume no custom components defined in a nested local module.

This also leads to inconsistent behaviour using local or remote weights, i.e.,

myPipeline.save_pretrained('./weights') # export weights
myPipeline.from_pretrained('./weights') # OK
# huggingface repo 'user/repo' contains the same content as in './weights'
myPipeline.from_pretrained('user/repo') # ValueError

What use case would this enable or better enable? Can you give us a code example? Instead of directly raising ValueError, maybe we could give a warning to make it really optional? In that case, the custom class could still be able to load correctly through this condition: https://github.com/huggingface/diffusers/blob/79df50388df09d9615e3c067695a453bb0a694c0/src/diffusers/pipelines/pipeline_utils.py#L342-L347

ashawkey commented 8 months ago

@patrickvonplaten @sayakpaul Hope to get your advice on this!

sayakpaul commented 8 months ago

@patrickvonplaten I'd prefer if you tackled it because I don't have much in custom pipelines.

sayakpaul commented 8 months ago

@ashawkey https://huggingface.co/Deci-early-access/DeciDiffusion-v2-0/tree/main is a good example of furthering a custom pipeline.

Additionally, using DiffusionPipeline.from_pretrained(repo_id, custom_pipeline=...) doesn't it work?

ashawkey commented 8 months ago

Thanks for your reply! custom_pipeline won't work because current implementation assumes the custom code is located in a single file. For example, in my case I imported my class from mylib/unet.py, but module_candidate will be mylib.unet and candidate_file will be unet/mylib.unet.py, which is simply an incorrect path.

ashawkey commented 8 months ago

This modification will allow importing a custom class from local files if it fails to import from remote code in huggingface repo. It won't break current custom_pipeline system, just making it more flexible for customization.

sayakpaul commented 8 months ago

Can you maybe open a PR for this with some test cases?

ashawkey commented 8 months ago

The PR is here: https://github.com/huggingface/diffusers/pull/6619. It's kind of hard to write a simple test case, but the modification is very simple.

github-actions[bot] commented 8 months ago

This issue has been automatically marked as stale because it has not had recent activity. If you think this still needs to be addressed please comment on this thread.

Please note that issues that do not follow the contributing guidelines are likely to be ignored.