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.73k stars 5.31k forks source link

Confusion about `FrozenDict` in `configuration_utils.py` #9503

Open townwish4git opened 1 month ago

townwish4git commented 1 month ago

I am confused about the design of FrozenDict in configuration_utils.py and the usage of it.

1. Is FrozenDict really frozen?

From the code, FrozenDict sets self.__frozen = True during initialization. It then checks if hasattr(self, "__frozen") and self.__frozen in methods like __setattr__ or __setitem__ to raise an exception if it's supposed to be frozen. However, in Python, using double underscores (__) triggers name mangling, which means hasattr(self, "__frozen") couldn't find __frozen as it is _FrozenDict__frozen actually. This check would always return False, rendering the intended freeze ineffective ā€“ __setattr__ and __setitem__ can still be used, making the FrozenDict not truly frozen.

Is this what we expect?

2. If we were to modify FrozenDict to be truly immutable, how would we use it as model.config?

Typically, we register parameters needed for model initialization as a FrozenDict via register_to_config. These are often accessed during the forward method with checks like if self.config.xxx == xxx to determine execution paths. However, in some models, certain properties of self.config might need to be altered after initialization, as seen in IP-Adapter

self.config.encoder_hid_dim_type = "ip_image_proj"

Does this contradict the design philosophy of FrozenDict?

3. If models.config could be mutable, how should one go about changing it?

In the example above, should we use __setattr__ to modify self.config.encoder_hid_dim_type or __setitem__ to add an additional key-value pair to FrozenDict which appears more in line with typical dictionary usage, like

- self.config.encoder_hid_dim_type = "ip_image_proj"
+ self.config['encoder_hid_dim_type'] = "ip_image_proj"


I was just wondering if you might have a moment to clarify these points for me?

tahahah commented 1 week ago

I don't know if this will contribute to the discussion meaningfully but there is a method in ConfigMixin that allows changing of FrozenDict this way:

self.register_to_config(cross_attention_dim=256)

townwish4git commented 1 week ago

I don't know if this will contribute to the discussion meaningfully but there is a method in ConfigMixin that allows changing of FrozenDict this way:

self.register_to_config(cross_attention_dim=256)

Thanks a lot for resolving my confusion about 2&3. So it looks like self.config should not be modified directly, but should be updated through the provided register_to_config interface. Thanks again!