Pros and cons of the configuration setup

Could you mention the reasons why you opted for a configuration setup that is different from transformers'?

From a previous conversation I remember it was in order to not repeat twice the arguments, however when looking at schedulers it seems like it is still the case:

diffusers/src/diffusers/schedulers/scheduling_ddpm.py

Lines 23 to 45 in 850d434

    
           def __init__( 
        
               self, 
        
               timesteps=1000, 
        
               beta_start=0.0001, 
        
               beta_end=0.02, 
        
               beta_schedule="linear", 
        
               trained_betas=None, 
        
               timestep_values=None, 
        
               variance_type="fixed_small", 
        
               clip_predicted_image=True, 
        
               tensor_format="np", 
        
           ): 
        
               super().__init__() 
        
               self.register( 
        
                   timesteps=timesteps, 
        
                   beta_start=beta_start, 
        
                   beta_end=beta_end, 
        
                   beta_schedule=beta_schedule, 
        
                   trained_betas=trained_betas, 
        
                   timestep_values=timestep_values, 
        
                   variance_type=variance_type, 
        
                   clip_predicted_image=clip_predicted_image, 
        
               )

From a quick read through the file, I don't understand why I have to register something, and what exactly I need to register. Some values are directly set as attribute below the register, while others are passed to the register method, which seems to act as an __init__ method with required arguments.

Is it in order to isolate a dict_to_save that will serialize only the kwargs passed to the register method? Wouldn't it be simpler with an __init__ method in the ConfigMixin instead?

-> Or is it a choice so as to not have a configuration object that you move around everywhere, instead choosing to have it as a mixin for both schedulers and pipelines?

@sgugger

The main reason for why I think a ConfigMixin makes more sense:

Configs are always just json files that should ideally all follow the same format and provide some simply load and save method for the Hub. Having one class for all models, pipelines, schedulers ensures that all configs stay in the same format.
Configs should only define the class they are mixed in with. By doing UNet(nn.Module, ConfigMixin) and having ConfigMixin save only the arguments one passes to the __init__(...) method, we can be sure that no unrelated config attributes will find its way into the config and thus ensure that configs stay clean. E.g. one would not pass gradient_checkpointing to the __init__ of a model so it should also not be saved in the config
Lots of duplicated code IMO if we model- and scheduler specific configs which is unnecessary IMO. The code doesn't do anything.
IMO it's also nicer to be able to do unet = UNet(num_layers=3) instead of having to do unet = UNet(UNetConfig(num_layers=3)) + I think it's nice if the model config inputs are directly shown with the model's init in the docs
Agree that maybe we should remove the register method and just use __init__ instead.

Would be very interested in hearing @sgugger's @thomwolf @rwightman opinion here as well (in case you have some time :-))

Closing this issue for now as it was discussed quite extensively offline

Merge pull request huggingface#11 from dotieuthien/add-convert-tensorrt

Merge pull request #11 from scxue/feat/sa-solver

Merge pull request huggingface#11 from yuyanpeng-google/yuyan-mh

Merge pull request #11 from huggingface/fix-fused-projections

Denormalize audio latents in I2V pipeline (analogous to T2V change) (#11

Add LTX 2.0 Video Pipelines (#12915)

refactor(dreamlite): address review feedback from huggingface#13815

[Pipelines] Add DreamLite text-to-image and image-edit pipelines (#13815

Navigation Menu

Repository navigation

Pros and cons of the configuration setup #11

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	def __init__(
	self,
	timesteps=1000,
	beta_start=0.0001,
	beta_end=0.02,
	beta_schedule="linear",
	trained_betas=None,
	timestep_values=None,
	variance_type="fixed_small",
	clip_predicted_image=True,
	tensor_format="np",
	):
	super().__init__()
	self.register(
	timesteps=timesteps,
	beta_start=beta_start,
	beta_end=beta_end,
	beta_schedule=beta_schedule,
	trained_betas=trained_betas,
	timestep_values=timestep_values,
	variance_type=variance_type,
	clip_predicted_image=clip_predicted_image,
	)

Pros and cons of the configuration setup #11

Description

Activity

patrickvonplaten commented on Jun 15, 2022

patrickvonplaten commented on Jul 21, 2022

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions