Pros and cons of the configuration setup

[LysandreJik]

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?

[patrickvonplaten]

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 :-))

[patrickvonplaten]

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