Skip to content

fix(models): Forward timm model kwargs to timm.create_model for OmDet-Turbo #44611

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
zucchini-nlp merged 4 commits into from
Mar 13, 2026
Merged

fix(models): Forward timm model kwargs to timm.create_model for OmDet-Turbo #44611

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
357a5cf
fix: Add and forward timm_model_kwargs to timm.create_model
harshaljanjani Mar 11, 2026
414ee30
fix: Revert TimmBackboneConfig, add BC migration comment
harshaljanjani Mar 12, 2026
94ce638
change: Address review
harshaljanjani Mar 12, 2026
45ce043
nit: Add comment
harshaljanjani Mar 13, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 11 additions & 2 deletions src/transformers/models/omdet_turbo/configuration_omdet_turbo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,8 +149,7 @@ def __init__(
# Init timm backbone with hardcoded values for BC
timm_default_kwargs = {
"out_indices": [1, 2, 3],
"img_size": image_size,
"always_partition": True,
"timm_model_kwargs": {"img_size": image_size, "always_partition": True},
}
backbone_config, kwargs = consolidate_backbone_kwargs_to_config(
backbone_config=backbone_config,
Expand All @@ -161,6 +160,16 @@ def __init__(
**kwargs,
)

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

@zucchini-nlp zucchini-nlp Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same here, the consolidate_backbone_kwargs_to_config utility should work ootb. If not, can you check why?

@harshaljanjani harshaljanjani Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

image

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.

@zucchini-nlp zucchini-nlp Mar 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

@zucchini-nlp zucchini-nlp Mar 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

transformers/src/transformers/models/omdet_turbo/modeling_omdet_turbo.py

Line 282 in aad13b8

self.vision_backbone = load_backbone(config)

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

@zucchini-nlp zucchini-nlp Mar 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

after unification we'll have more freedom to pass any timm kwargs when creating the model by config.model_args

@harshaljanjani harshaljanjani Mar 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your time @zucchini-nlp; attempted to resolve it accordingly 🤗

Comment on lines +165 to +170

@zucchini-nlp zucchini-nlp Mar 13, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's add a comment on why this is needed pls

@harshaljanjani harshaljanjani Mar 13, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added :)

if text_config is None:
logger.info("`text_config` is `None`. Initializing the config with the default `clip_text_model`")
text_config = CONFIG_MAPPING["clip_text_model"]()
Expand Down
4 changes: 4 additions & 0 deletions src/transformers/models/timm_backbone/configuration_timm_backbone.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,8 @@ class TimmBackboneConfig(BackboneConfigMixin, PreTrainedConfig):
Whether to output only the features or also the logits.
freeze_batch_norm_2d (`bool`, *optional*, defaults to `False`):
Converts all `BatchNorm2d` and `SyncBatchNorm` layers of provided module into `FrozenBatchNorm2d`.
timm_model_kwargs (`dict`, *optional*):
Additional keyword arguments to pass to `timm.create_model` (e.g. `{"img_size": 640}`).

Example:
```python
Expand All @@ -57,6 +59,7 @@ def __init__(
out_indices=None,
freeze_batch_norm_2d=False,
output_stride=None,
timm_model_kwargs=None,

@zucchini-nlp zucchini-nlp Mar 12, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@harshaljanjani harshaljanjani Mar 12, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense; reverted :)

**kwargs,
):
self.backbone = backbone
Expand All @@ -65,6 +68,7 @@ def __init__(
self.out_indices = out_indices if out_indices is not None else [-1]
self.output_stride = output_stride
self.freeze_batch_norm_2d = freeze_batch_norm_2d
self.timm_model_kwargs = timm_model_kwargs if timm_model_kwargs is not None else {}

super().__init__(**kwargs)

Expand Down
2 changes: 2 additions & 0 deletions src/transformers/models/timm_backbone/modeling_timm_backbone.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ def __init__(self, config, **kwargs):
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", {})

@cursor cursor Bot Mar 12, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Fix in Cursor Fix in Web

@harshaljanjani harshaljanjani Mar 12, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's no codepath tmk that produces None here?


backbone = timm.create_model(
config.backbone,
Expand All @@ -58,6 +59,7 @@ def __init__(self, config, **kwargs):
in_chans=in_chans,
out_indices=out_indices,
output_stride=config.output_stride,
**timm_model_kwargs,
**kwargs,
)

Expand Down
Loading