🚨 Refactor DETR to updated standards #41549

yonigozlan · 2025-10-13T16:57:33Z

What does this PR do?

This PR aims at refactoring DETR as part of an effort to standardize vision models in the library, in the same vein as #41546.
Expect to see much more PRs like this for vision models as we approach v5!

HuggingFaceDocBuilderDev · 2025-10-13T17:07:47Z

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.

…sks, vision input embeds and query embeds

yonigozlan · 2025-10-14T13:14:32Z

src/transformers/safetensors_conversion.py

+            if not isinstance(line, str):
+                line = line.decode()


line was an str when I tried to use this, not sure why! I can open a separate PR for it though

yonigozlan · 2025-10-14T13:21:12Z

src/transformers/models/detr/modeling_detr.py

+        if pixel_values is None and inputs_embeds is None:
+            raise ValueError("You have to specify either pixel_values or inputs_embeds")
+
+        if inputs_embeds is None:
+            batch_size, num_channels, height, width = pixel_values.shape
+            device = pixel_values.device
+
+            if pixel_mask is None:
+                pixel_mask = torch.ones(((batch_size, height, width)), device=device)
+            vision_features = self.backbone(pixel_values, pixel_mask)
+            feature_map, mask = vision_features[-1]
+
+            # Apply 1x1 conv to map (N, C, H, W) -> (N, d_model, H, W), then flatten to (N, HW, d_model)
+            # (feature map and position embeddings are flattened and permuted to (batch_size, sequence_length, hidden_size))
+            projected_feature_map = self.input_projection(feature_map)
+            flattened_features = projected_feature_map.flatten(2).permute(0, 2, 1)
+            spatial_position_embeddings = (
+                self.position_embedding(shape=feature_map.shape, device=device, dtype=pixel_values.dtype, mask=mask)
+                .flatten(2)
+                .permute(0, 2, 1)
+            )
+            flattened_mask = mask.flatten(1)
+        else:
+            batch_size = inputs_embeds.shape[0]
+            device = inputs_embeds.device
+            flattened_features = inputs_embeds
+            # When using inputs_embeds, we need to infer spatial dimensions for position embeddings
+            # Assume square feature map
+            seq_len = inputs_embeds.shape[1]
+            feat_dim = int(seq_len**0.5)
+            # Create position embeddings for the inferred spatial size
+            spatial_position_embeddings = (
+                self.position_embedding(
+                    shape=torch.Size([batch_size, self.config.d_model, feat_dim, feat_dim]),
+                    device=device,
+                    dtype=inputs_embeds.dtype,
+                )
+                .flatten(2)
+                .permute(0, 2, 1)
+            )
+            # If a pixel_mask is provided with inputs_embeds, interpolate it to feat_dim, then flatten.
+            if pixel_mask is not None:
+                mask = nn.functional.interpolate(pixel_mask[None].float(), size=(feat_dim, feat_dim)).to(torch.bool)[0]
+                flattened_mask = mask.flatten(1)
+            else:
+                # If no mask provided, assume all positions are valid
+                flattened_mask = torch.ones((batch_size, seq_len), device=device, dtype=torch.long)


Now truly supports passing input_embeds instead of silently doing nothing with it

yonigozlan · 2025-10-14T13:21:41Z

src/transformers/models/detr/modeling_detr.py

+        if decoder_inputs_embeds is not None:
+            queries = decoder_inputs_embeds
+        else:
+            queries = torch.zeros_like(object_queries_position_embeddings)


Same, truly supports decoder_inputs_embeds as input

yonigozlan · 2025-10-14T13:22:32Z

src/transformers/models/detr/modeling_detr.py

-            attention_mask=None,
-            object_queries=object_queries,
-            query_position_embeddings=query_position_embeddings,
+            attention_mask=decoder_attention_mask,


Supports masking of queries (as advertised)

yonigozlan · 2025-10-14T13:23:56Z

src/transformers/models/detr/modeling_detr.py

+        if attention_mask is not None:
+            # [batch_size, num_queries] -> [batch_size, 1, num_queries, num_queries]
+            attention_mask = _prepare_4d_attention_mask(attention_mask, inputs_embeds.dtype, tgt_len=input_shape[-1])

-        # expand encoder attention mask
+        # expand encoder attention mask (for cross-attention on encoder outputs)
        if encoder_hidden_states is not None and encoder_attention_mask is not None:
            # [batch_size, seq_len] -> [batch_size, 1, target_seq_len, source_seq_len]
            encoder_attention_mask = _prepare_4d_attention_mask(
                encoder_attention_mask, inputs_embeds.dtype, tgt_len=input_shape[-1]
            )

        # optional intermediate hidden states
        intermediate = () if self.config.auxiliary_loss else None

        # decoder layers
-        all_hidden_states = () if output_hidden_states else None
-        all_self_attns = () if output_attentions else None
-        all_cross_attentions = () if (output_attentions and encoder_hidden_states is not None) else None

        for idx, decoder_layer in enumerate(self.layers):
            # add LayerDrop (see https://huggingface.co/papers/1909.11556 for description)
-            if output_hidden_states:
-                all_hidden_states += (hidden_states,)
            if self.training:
                dropout_probability = torch.rand([])
                if dropout_probability < self.layerdrop:
                    continue

-            layer_outputs = decoder_layer(
+            hidden_states = decoder_layer(
                hidden_states,
-                combined_attention_mask,
-                object_queries,
-                query_position_embeddings,
+                attention_mask,
+                spatial_position_embeddings,
+                object_queries_position_embeddings,
                encoder_hidden_states,  # as a positional argument for gradient checkpointing
                encoder_attention_mask=encoder_attention_mask,
-                output_attentions=output_attentions,
+                **kwargs,


Truly supports attention mask on vision features (it was always None before)

yonigozlan · 2025-10-14T13:27:25Z

Hello @molbap @ArthurZucker!
The long overdue refactor of DETR is ready for a first review. I'm waiting for your reviews to run fix-copies as this will have a lot of impacts on other models (through # Copied from for now, modular later ;) )

vasqu

Some initial thoughts, focused on the masks/interface part

src/transformers/models/detr/modeling_detr.py

vasqu · 2025-10-15T16:57:42Z

tests/models/detr/test_modeling_detr.py

+    ):
+        if use_attention_mask:
+            self.skipTest(
+                "This test uses attention masks which are not compatible with DETR. Skipping when use_attention_mask is True."


Hmm, why tho? Are the attention masks perhaps 3D instead?

It's more that _test_eager_matches_sdpa_inference is not adapted to the vision space (+object queries here). It tries to add a "decoder_input_ids" to the inputs, plus the seqlen created for the dummy masks were wrong. Seeing as the function is already quite cluttered and difficult to read, I figured trying to add support for vision model directly there would not be ideal. We can either override the tests in this model specifically, or try to have a more general test for vision models. Another option would be to be able to parameterize the tests by providing how to find the correct seqlen and input names.
I would love some help on this!

I see, is this specific to detr or will we encounter more so for other models in the vision family? It's best to not skip too much if it comes down the line. Depending on how many are affected by this, we either should

Fix the base test, e.g. with parametrization, splitting the test a bit (more models with similar problems)

Overwrite the test and make specific changes (low amount of models with similar problems)

The problem is with the test's base design indeed. It will lead to more skipped tests down the line because the division encoder/encoder-decoder/decoder isn't that clearly made. The amount of models with similar problems isn't "low" imo.

Yes I think it will increase too with us fixing the attention masks for vision models, so we definitely need to improve the base test

yonigozlan · 2025-10-16T10:34:40Z

Thanks for the review @vasqu ! I standardized attention and masking following your advice :)

vasqu

Looking good from my side, amazing work! Just left some smaller comments but nothing crazy

vasqu · 2025-10-16T12:07:57Z

src/transformers/models/detr/modeling_detr.py

-        position_embedding = DetrLearnedPositionEmbedding(n_steps)
-    else:
-        raise ValueError(f"Not supported {config.position_embedding_type}")
+def eager_attention_forward(


Let's use a # Copied from ... statement so that during changes we have the connection here

I'd prefer not to come back to Copied from statements 😅

It's only for models that do not use modular yet. The issue is that if we don't do this and change anything upstream on Bert, it won't be reflected here (and the dependent models) - otherwise, we need to manually keep track of these deps 😅 Don't mind this if we refactor to modular here!

The goal is to use modular eventually (in this PR even) :)

Perfect! Then dont mind this comment :p only want to have some dep here

vasqu · 2025-10-16T12:09:17Z

src/transformers/models/detr/modeling_detr.py

    _no_split_modules = [r"DetrConvEncoder", r"DetrEncoderLayer", r"DetrDecoderLayer"]
+    _supports_sdpa = True
+    _supports_flash_attn = True
+    _supports_attention_backend = True


We should be able to _support_flex_attention (not 100% sure about the flag name) since we use create_bidirectional_masks

it's _supports_flex_attn, and yep

src/transformers/models/detr/modeling_detr.py

vasqu · 2025-10-16T12:14:16Z

tests/models/detr/test_modeling_detr.py

+    ):
+        if use_attention_mask:
+            self.skipTest(
+                "This test uses attention masks which are not compatible with DETR. Skipping when use_attention_mask is True."


I see, is this specific to detr or will we encounter more so for other models in the vision family? It's best to not skip too much if it comes down the line. Depending on how many are affected by this, we either should

Fix the base test, e.g. with parametrization, splitting the test a bit (more models with similar problems)

Overwrite the test and make specific changes (low amount of models with similar problems)

molbap

Looks niiiice
For the unhappy CI, let's throw the Check Copies away!

src/transformers/models/detr/modeling_detr.py

molbap · 2025-10-17T15:53:25Z

tests/models/detr/test_modeling_detr.py

+    ):
+        if use_attention_mask:
+            self.skipTest(
+                "This test uses attention masks which are not compatible with DETR. Skipping when use_attention_mask is True."


The problem is with the test's base design indeed. It will lead to more skipped tests down the line because the division encoder/encoder-decoder/decoder isn't that clearly made. The amount of models with similar problems isn't "low" imo.

molbap · 2025-10-17T15:53:53Z

src/transformers/modeling_utils.py

    "qwen2_5_vl",
    "videollava",
    "vipllava",
+    "detr",


I'm not sure, do we need to add this here?

Yes that's what made me go crazy haha otherwise _checkpoint_conversion_mapping doesn't work.
Note that this is temporary and will be replaced by the new way to convert weights on the fly that @ArthurZucker and @Cyrilvallez are working on.

molbap · 2025-10-17T15:55:53Z

src/transformers/models/detr/modeling_detr.py

    def __init__(self, config: DetrConfig):
        super().__init__()
-        self.embed_dim = config.d_model
+        self.hidden_size = config.d_model


won't that break BC? (at least on the attribute names)

In what way? If users access it directly? In any case I think we really need to standardize these types of variable names, it might be worth slightly breaking BC imo

yeah in case of non-config access. I agree I prefer to standardize

molbap · 2025-10-17T15:56:56Z

src/transformers/models/detr/modeling_detr.py

    _no_split_modules = [r"DetrConvEncoder", r"DetrEncoderLayer", r"DetrDecoderLayer"]
+    _supports_sdpa = True
+    _supports_flash_attn = True
+    _supports_attention_backend = True


it's _supports_flex_attn, and yep

molbap · 2025-10-17T16:00:22Z

src/transformers/models/detr/modeling_detr.py

            if self.training:
                dropout_probability = torch.rand([])
                if dropout_probability < self.layerdrop:
                    continue


not exactly the typical dropout interface, we can maybe take the occasion to update it?

Yes 😫, I was scared of breaking BC in that case, but maybe it's not so important. It would be great to get rid of non standards dropout elsewhere as well really

I think it's ok to break it in here, it does not affect inference and clearly it would be an improvement to get rid of it haha

molbap · 2025-10-17T16:01:18Z

src/transformers/models/detr/modeling_detr.py

    def freeze_backbone(self):
-        for name, param in self.backbone.conv_encoder.model.named_parameters():
+        for _, param in self.backbone.model.named_parameters():
            param.requires_grad_(False)

    def unfreeze_backbone(self):
-        for name, param in self.backbone.conv_encoder.model.named_parameters():
+        for _, param in self.backbone.model.named_parameters():
            param.requires_grad_(True)


these methods should really be user-side responsibilities 😨 I would be pro-removal! We can always communicate on it

Yes agreed, we could start a deprecation cycle, or just remove it for v5. It's present in several other vision models

Just asked @merveenoyan who's an avid finetuner and is not using these methods anymore, I think they were good initially but they're ok to go now. Agreed it's out of scope for current PR will create another to remove all of it (cc @ariG23498 as we chatted on finetuning too)

src/transformers/models/detr/modeling_detr.py

molbap · 2025-10-17T16:04:00Z

src/transformers/models/detr/modeling_detr.py

+    def forward(self, q, k, mask: Optional[torch.Tensor] = None):
        q = self.q_linear(q)
        k = nn.functional.conv2d(k, self.k_linear.weight.unsqueeze(-1).unsqueeze(-1), self.k_linear.bias)
        queries_per_head = q.view(q.shape[0], q.shape[1], self.num_heads, self.hidden_dim // self.num_heads)


on here my nit would be, if we can update a bit the single-letter variable names, that'd be great!

Yes I think we could even try to refactor this to use the standard attention module and only take the attention weights! It could be interesting to compare the performance of eager attention vs this implementation (conv2d instead of linear for key proj, and no multiplication by value) vs other attention impl.

ahah that's a tough one to benchmark but indeed sounds good, LMK if you want to do that in this PR or move to another

github-actions · 2025-10-17T22:33:45Z

[For maintainers] Suggested jobs to run (before merge)

run-slow: detr

yonigozlan added 2 commits October 13, 2025 16:55

refactor attention and many other

18e7172

Merge remote-tracking branch 'upstream/main' into refactor-detr

a931c26

yonigozlan added 4 commits October 13, 2025 17:07

remove return_dict interface

f7536b9

improve variable names

c016021

use _can_record_outputs and add real support for pixel and queries ma…

7f21985

…sks, vision input embeds and query embeds

split self attention and cross attention

d0494e9

yonigozlan commented Oct 14, 2025

View reviewed changes

nits

2f0752f

yonigozlan commented Oct 14, 2025

View reviewed changes

yonigozlan changed the title ~~[WIP] Refactor DETR to updated standards~~ Refactor DETR to updated standards Oct 14, 2025

yonigozlan requested review from ArthurZucker and molbap October 14, 2025 13:24

yonigozlan changed the title ~~Refactor DETR to updated standards~~ 🚨 Refactor DETR to updated standards Oct 14, 2025

vasqu reviewed Oct 15, 2025

View reviewed changes

standardize mask handling

67a007a

vasqu reviewed Oct 16, 2025

View reviewed changes

ArthurZucker removed their request for review October 16, 2025 14:11

yonigozlan mentioned this pull request Oct 17, 2025

🚨 Refactor ViT to updated standards #41693

Open

molbap reviewed Oct 17, 2025

View reviewed changes

yonigozlan added 2 commits October 17, 2025 17:39

update DetrMHAttentionMap

eab4e85

refactor mlp detr

ac8d387

🚨 Refactor DETR to updated standards #41549

Are you sure you want to change the base?

🚨 Refactor DETR to updated standards #41549

Conversation

yonigozlan commented Oct 13, 2025

What does this PR do?

Uh oh!

HuggingFaceDocBuilderDev commented Oct 13, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yonigozlan commented Oct 14, 2025

Uh oh!

vasqu left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yonigozlan commented Oct 16, 2025

Uh oh!

vasqu left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

molbap left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

vasqu left a comment •

edited

Loading