Repository navigation
Common API #13
Description
How much of a common API do you expect there to be across different objects? For example, the unet.py file contains a Trainer, while other models seem to only rely on their forward method, but they have different APIs and signatures.
Same with pipelines, for example with the BDDM pipeline having the following arguments for it's __call__ method: mel_spectrogram, generator, torch_device=None, while the DDIM pipeline has batch_size=1, generator=None, torch_device=None, eta=0.0, num_inference_steps=50.
Do we expect all of these models, pipelines, schedulers to have a common API at the end? Is that even possible with diffusers?
It seems like most arguments are similar, but with a few specificities for models, pipelines and schedulers. That's where having a configuration system would arguably work quite well as it would show very visibly what each of them has in terms of arguments for customization.
This reminds me a bit of the do_lower_case problem we have in transformers: some tokenizers have it, some don't, but users don't necessarily understand that and try to use it for all tokenizers.
Activity
patrickvonplaten commentedon Jun 15, 2022
My take here is that:
models: All models should follow the same API meaning that all classes defined in the folder should all only provide one and only oneforward(...)method, be framework specific, and have more or less the same function signature inforward(...)(conditional models will require one more function argument). This is still a big TODO - we need to clean up this folder.schedulers: All models should follow a very similar API and all provide one or multiplestep(...)methods that all have a very similar signature. All schedulers should be aligned as much as possible in terms of API and should be framework independant. I don't think we can force them though to have the exact same API (e.g. PNDM requires two loops), but each step method in the loop follows more or less the same signature. This is already done to a big extent - e.g. compare the different files in the above directory to each other.pipelines: It'll be harder to force a common API for pipelines given that they will be very different depending on which modality they are used etc... Here, I'd like to force every pipeline to implement one and only oneforward(...)method and have pipelines be framework specific, but I don't think we can/should force the function signature to be the same really. The idea is that new pipelines will be closely follow published papers and easy-to-add for a new contributor. Note however that pipelines will be only used for inference.patrickvonplaten commentedon Jun 15, 2022
Think this is a very important discussion to have. What do you think @patil-suraj @anton-l @thomwolf ?
anton-l commentedon Jun 15, 2022
models: the UNet variations can definitely be consolidated into one. However, I'm in favor of keeping modality-specific subclasses, similar to...ForTaskNameintranformers. See, for example,GLIDETextToImageUNetModelandGLIDESuperResUNetModelhere, where we have differentforwards depending on how the model is conditioned.schedulers: basically we just need a "forward diffusion process"step()for training and a "backward diffusion process"step()for inference. Hopefully that holds for all models.pipelines: We might need to separate__call__into sub-steps for pipelines with multiple steps, e.g. text2image + upscale1 + upscale2. And the arguments, of course, are very different between implementations. I would even suggest aPipelineConfigclass for extremely parameterized pipelines like Majesty Diffusionpatil-suraj commentedon Jun 15, 2022
models: Agree with @anton-l here, having modality/task (ex: text/embedding conditioned, class conditioned) specific sub-classes would be nice. This will make the code much more readable.schedulers: Pretty much the same comment as Patrick and Antonpipelines: Agree with @anton-l here again. Some pipelines do multiple things and users may want to do one or multiple or all of the steps. Having some way of specifying sub-steps would be useful.kashif commentedon Jun 15, 2022
One thing that is sometimes missing with the
models'sforwardis the notion of acontextorconditioning... typically for the image gen. there is no conditioning, however for other modalities conditioning might be needed so if we can also add that in theforwardand by default make it none, it will be helpful!patil-suraj commentedon Jun 15, 2022
Thanks @kashif . I think this is exactly what @anton-l proposed with task/modality specific sub-classes.
patrickvonplaten commentedon Jun 15, 2022
@anton-l RE:
Haven't looked too much into the models yet, but do we really need them to be modality specific? E.g. couldn't a vision unet also work on a spectrogram image? Think I'd be at the moment more in favor of naming the modules in
src/diffusers/modelsafter their input / output format and model type - e.g. unet_2d.py, unet_3d.py. In the case of glide, could that maybe mean that we have aunet_conditional_2d.pyand aunnet_upsample.pymodule? I'm a bit worried that if we addFor<TaskName>orFor<Modality>too early and then find out later that those models work for other modalities it'll be a bit difficult to refactor them later on. Note here the target group is also more experienced users so they might not need the "guidance" of a modality specific name.However, for the pipelines I'd be very much in favor of quickly making the distinction between modalities as the pipelines should 1-to-1 reflect architectures proposed in papers. Here the target group is less experienced users which just want to run the model as presented in a paper.
What do you think @patil-suraj @anton-l ?
patrickvonplaten commentedon Jun 15, 2022
Not super excited about early-on introducing a pipeline config, but happy to brainstorm about it (don't know much about majestic yet :-))
patrickvonplaten commentedon Jul 21, 2022
Also closing as the API is now finalized and explain on the main README: https://github.com/huggingface/diffusers
stable-diffusion support multiple shapes (huggingface#13)
Merge pull request huggingface#13 from dotieuthien/add-convert-tensorrt
Merge pull request huggingface#13 from yuyanpeng-google/yuyan-mh
support ostris loras. (#13)
let's go Flux2 🚀 (#12711)
refactor(dreamlite): address review feedback from huggingface#13815
[Pipelines] Add DreamLite text-to-image and image-edit pipelines (#13815
[Pipelines] Add DreamLite text-to-image and image-edit pipelines (#13815