Subconfig is a class attribute #41308
Conversation
|
The docs for this PR live here. All of your documentation changes will be reflected on that endpoint. The docs are available until 30 days after the last update. |
src/transformers/models/conditional_detr/configuration_conditional_detr.py
Show resolved
Hide resolved
Cyrilvallez
left a comment
Cyrilvallez
left a comment
There was a problem hiding this comment.
Looks very nice! You have a few conflicts due to #41300 though, sorry!
|
Approve pls 🙏🏻 |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: conditional_detr, d_fine, dab_detr, deformable_detr, depth_anything, detr, dpt, esm, grounding_dino, mask2former, maskformer, mm_grounding_dino, omdet_turbo, oneformer, pegasus, pegasus_x |
Cyrilvallez
left a comment
Cyrilvallez
left a comment
There was a problem hiding this comment.
Let's just revert all changes to modeling_utils.py, and instead fix where super().__init__ is called in configs (should be at the bottom, after subconfigs are set) - see e.g. dpt!
Feel free to merge afterwards!
| sub_config = getattr(config, sub_config_key) | ||
| sub_config.dtype = dtype | ||
| if (sub_config := getattr(config, sub_config_key)) is not None: | ||
| sub_config.dtype = dtype |
There was a problem hiding this comment.
I think this check is needed only if super().__init__() is called before subconfigs are set in the config no? Let's quickly move the call to the bottom on all changed configs if so instead of this!
There was a problem hiding this comment.
not only, some models do not initialize a sub config at run time and either 1) infer when modeling in load_backbone from a variety of config keys 2) do not need the path with sub-model so they don't need a config for that
These are all vision models with backbones, and maybe we can update them to follow the common standards with nested configs. I would prefer to check and do standardization much later, after making and finishing the config type validation PR. So I'll merge it for now and take note to myself to come back and dig into backbone-vision configs :)
* delete * fix this test * fix copies * oke, more tests to fix * fix last tests on DPT * deleted accidentally
4 participants
What does this PR do?
As per title, it should be a class attribute for vision-only models. VLMs already use class attribute. If it is a property, it is unavailable before we init the class. Having it as a class attribute will be needed for integrating type validation and transforming all configs to a dataclass
Also cleaned up some duplicate code. An attribute map already maps those keys, so an extra property is not needed