fix(models): Forward timm model kwargs to timm.create_model for OmDet-Turbo#44611
Conversation
|
run-slow: omdet_turbo |
|
This comment contains models: ["models/omdet_turbo"] |
| out_indices=None, | ||
| freeze_batch_norm_2d=False, | ||
| output_stride=None, | ||
| timm_model_kwargs=None, |
There was a problem hiding this comment.
not really what we want for now. The above fix you did with passing it as timm_kwargs should be enough and consolidate_backbone_kwargs_to_config will put it inside timm config
There was a problem hiding this comment.
That makes sense; reverted :)
| if getattr(backbone_config, "model_type", None) == "timm_backbone" and not getattr( | ||
| backbone_config, "timm_model_kwargs", None | ||
| ): | ||
| timm_extra = {} | ||
| for attr in ("img_size", "always_partition"): | ||
| if hasattr(backbone_config, attr): | ||
| timm_extra[attr] = getattr(backbone_config, attr) | ||
| if timm_extra: | ||
| backbone_config.timm_model_kwargs = timm_extra | ||
|
|
There was a problem hiding this comment.
same here, the consolidate_backbone_kwargs_to_config utility should work ootb. If not, can you check why?
There was a problem hiding this comment.
Actually, the omdet-turbo-swin-tiny-hf/config.json has backbone_config:null and backbone_kwargs: {"img_size": 640, "always_partition": true, ...}.
→ consolidate_backbone_kwargs_to_config takes this path bec backbone_kwargs is non-empty, the timm_default_kwargs path is skipped entirely, so img_size/always_partition end up as direct attrs with no timm_model_kwargs. I've added a corresponding comment in the code as well on why the block is required (happy to know if there's a better direction). So consolidate_backbone_kwargs_to_config works for fresh configs OOTB, but omdet-turbo-swin-tiny-hf seems to require the BC check :)
→ I thought about whether to change consolidate_backbone_kwargs_to_config or write the BC block, but writing a check of TimmBackboneConfig params vs timm.create_model params might be fragile, like, DETR passes use_pretrained_backbone in timm_default_kwargs, which is neither a TimmBackboneConfig param nor a valid timm.create_model kwarg, so it would get incorrectly forwarded to timm.create_model and break.
There was a problem hiding this comment.
ahhh right, TimmBackbone doesn't accept arbitrary kwargs like TimmWrapper does so it gets popped, even if we pass it when creating the config
Good catch!
There was a problem hiding this comment.
Actually I am working on subsequent PR here (#44252). It is blocked currently by a few other required fixes, so until then let's patch OmdetTurbo model code
We can change:
to smth like:
backbone = AutoBackbone.from_config(config=config.backbone_config, **config.timm_kwargs)
And make sure that timm_kwargs isn't saved when serializing the config. We don't want it in hub checkpoints, it'll cause more headache
There was a problem hiding this comment.
after unification we'll have more freedom to pass any timm kwargs when creating the model by config.model_args
There was a problem hiding this comment.
Thank you for your time @zucchini-nlp; attempted to resolve it accordingly 🤗
|
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. |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
| out_indices = config.out_indices if getattr(config, "out_indices", None) is not None else (-1,) | ||
| pretrained = kwargs.pop("pretrained", False) | ||
| in_chans = kwargs.pop("in_chans", config.num_channels) | ||
| timm_model_kwargs = getattr(config, "timm_model_kwargs", {}) |
There was a problem hiding this comment.
Unpacking None as kwargs causes TypeError at init
Medium Severity
getattr(config, "timm_model_kwargs", {}) returns None (not {}) when the attribute exists on the config but is explicitly None. This happens when a saved config JSON contains "timm_model_kwargs": null or when a backbone_config dict passes it as None. The subsequent **timm_model_kwargs unpacking then raises TypeError. Using getattr(config, "timm_model_kwargs", None) or {} would guard against this.
There was a problem hiding this comment.
There's no codepath tmk that produces None here?
|
Updated with the suggested fix; happy to make further changes if needed :) |
| timm_kwargs = {} | ||
| if getattr(backbone_config, "model_type", None) == "timm_backbone": | ||
| for attr in ("img_size", "always_partition"): | ||
| if hasattr(backbone_config, attr): | ||
| timm_kwargs[attr] = getattr(backbone_config, attr) | ||
|
|
There was a problem hiding this comment.
let's add a comment on why this is needed pls
zucchini-nlp
left a comment
There was a problem hiding this comment.
Great, will merge after adding the comment
|
run-slow: omdet_turbo |
|
This comment contains models: ["models/omdet_turbo"] |
|
[For maintainers] Suggested jobs to run (before merge) run-slow: omdet_turbo |



What does this PR do?
The following issue was identified and fixed in this PR:
→ This PR (🚨 Delete duplicate code in backbone utils) structured config loading to use BackboneMixin.consolidate_backbone_kwargs_to_config. For the DETR-family; the current state works correctly because timm_default_kwargs only contains keys that map to TimmBackboneConfig.init. There might be others; but OmDet-Turbo is a model that passes kwargs meant for timm.create_model itself, and which are not
TimmBackboneConfigparams and were dropped.→ From the prev PR's diff, before the refactor, the implementation forwarded these params via
**kwargstotimm.create_modeland it worked before, but after the refactor they were stored as attributes onPreTrainedConfigand never forwarded, and parameters likeimg_sizeare ignored.Fixes #44610.
cc: @zucchini-nlp
CI Failures
Before the fix (feel free to cross-check; these errors are reproducible):
After the fix (feel free to cross-check):
Before submitting
Pull Request section?
to it if that's the case.
documentation guidelines, and
here are tips on formatting docstrings.
Note
Medium Risk
Changes timm backbone construction and argument forwarding; while scoped to an optional
timm_model_kwargsdict, it can affect any model relying onTimmBackboneif those kwargs are set or collide with existing**kwargs.Overview
Fixes OmDet-Turbo’s timm backbone initialization by storing timm-only parameters (e.g.
img_size,always_partition) underbackbone_config.timm_model_kwargsinstead of top-level config fields.Updates
TimmBackboneto forwardconfig.timm_model_kwargsintotimm.create_model, and adds a backward-compatibility shim to migrate older hub configs that hadimg_size/always_partitionas direct attributes.Written by Cursor Bugbot for commit 414ee30. This will update automatically on new commits. Configure here.