🚨 🚨Bring some dinos to modern standards#46266
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. |
guarin
left a comment
There was a problem hiding this comment.
Thanks, this will make things much easier! Left more questions than comments :)
guarin
left a comment
There was a problem hiding this comment.
Nice refactor! Looks mostly good to me, left some comments and questions :)
vasqu
left a comment
There was a problem hiding this comment.
Only took a look at dinov2, I think we need to make sure this is good and then we can propogate based on that.
Sorry for the confusion on some comments, please read everything before responding 🫠
| pass | ||
|
|
||
|
|
||
| class Dinov2Embeddings(nn.Module): |
There was a problem hiding this comment.
Any reason we don't inherit from e.g. BeitEmbeddings. Not convinced that we cannot use modular here
There was a problem hiding this comment.
I couldv'e sworn I had answered these 👁️
however it's a bit tricky, it can change logits a bit due to upcasting. Trying a run-slow though
| return hidden_state * self.lambda1 | ||
|
|
||
|
|
||
| class Dinov2MLP(nn.Module): |
There was a problem hiding this comment.
Imo can be CLIPMLP for the forward
|
|
||
| def __init__(self, config: Dinov2Config) -> None: | ||
| super().__init__() | ||
| self.norm1 = nn.LayerNorm(config.hidden_size, eps=config.layer_norm_eps) |
There was a problem hiding this comment.
Imo, we could copy llama here and add the layer_scales (which are the only new thing)
There was a problem hiding this comment.
Inheriting LlamaDecoderLayer means overriding both init and forward, beyond the layer scales we'd also be swapping RMSNorm→LayerNorm and dropping all the cache/RoPE 😬
There was a problem hiding this comment.
Makes sense, always annoying 😢
| self.embeddings = Dinov2Embeddings(config) | ||
| self.encoder = Dinov2Encoder(config) | ||
| self.layernorm = nn.LayerNorm(config.hidden_size, eps=config.layer_norm_eps) | ||
| self.pooler = None |
There was a problem hiding this comment.
| self.pooler = None |
| Dinov2 backbone, to be used with frameworks like DETR and MaskFormer. | ||
| """ | ||
| ) | ||
| class Dinov2Backbone(BackboneMixin, Dinov2PreTrainedModel): |
There was a problem hiding this comment.
No similar backbone to use?
There was a problem hiding this comment.
unfortunately, not really! (I think answered elsewhere but worth adding back)
|
run-slow: depth_anything, dinov2, dinov2_with_registers, dinov3_convnext, dinov3_vit, eomt, eomt_dinov3, pixio, rf_detr, sapiens2, videomt |
|
This comment contains models: ["models/depth_anything", "models/dinov2", "models/dinov2_with_registers", "models/dinov3_convnext", "models/dinov3_vit", "models/eomt", "models/eomt_dinov3", "models/pixio", "models/rf_detr", "models/sapiens2", "models/videomt"] |
|
View the CircleCI Test Summary for this PR: https://huggingface.co/spaces/transformers-community/circle-ci-viz?pr=46266&sha=b0ea71 |
|
@guarin if you can have another look, I updated the swiglu-related reverse load test, I think it's an acceptable workaround. LMK! |
guarin
left a comment
There was a problem hiding this comment.
Nice refactor! Just some minor points/questions here and there, otherwise LGTM!
| patch_size = self.patch_size if isinstance(self.patch_size, Iterable) else (self.patch_size, self.patch_size) | ||
| new_height = height // patch_size[0] | ||
| new_width = width // patch_size[1] | ||
|
|
There was a problem hiding this comment.
Would it be possible to already create self.patch_size as a tuple? Or does this break BC?
There was a problem hiding this comment.
yeah I think it breaks BC here, in particular for rf_detr IIRC
| image_size = config.image_size | ||
| patch_size = config.patch_size | ||
| image_size = image_size if isinstance(image_size, Iterable) else (image_size, image_size) | ||
| patch_size = patch_size if isinstance(patch_size, Iterable) else (patch_size, patch_size) |
There was a problem hiding this comment.
Nit: Inline to self.image_size = config.image_size if isinstance(config.image_size, Iterable) else (config.image_size, config.image_size) to allow overwriting self.image_size and self.patch_size with modular?
There was a problem hiding this comment.
This is coming from VitPatchEmbeddings IIRC, so it's not modifiable directly
| down_proj = self.down_proj(self.act_fn(self.gate_proj(x)) * self.up_proj(x)) | ||
| return down_proj |
There was a problem hiding this comment.
| down_proj = self.down_proj(self.act_fn(self.gate_proj(x)) * self.up_proj(x)) | |
| return down_proj | |
| return self.down_proj(self.act_fn(self.gate_proj(x)) * self.up_proj(x)) |
There was a problem hiding this comment.
eh, that's llama-inherited 😬
| _supports_attention_backend = True | ||
| _can_compile_fullgraph = True |
| ```""" | ||
| kwargs["output_hidden_states"] = True # required to extract layers for the stages | ||
|
|
||
| kwargs["output_hidden_states"] = True # required to extract per-stage feature maps from hidden_states |
There was a problem hiding this comment.
Could this be replaced by a decorator?
There was a problem hiding this comment.
AH indeed, I think #46422 (that I merged...) covers it. checking
| patch_size = ( | ||
| self.config.patch_size | ||
| if isinstance(self.config.patch_size, Iterable) | ||
| else (self.config.patch_size, self.config.patch_size) | ||
| ) |
There was a problem hiding this comment.
Looking at all these I wonder whether we don't want to convert to tuple in config __post_init__. Not sure if that works though if users set config.patch_size = 14 though.
There was a problem hiding this comment.
Not sure if that works though if users set config.patch_size = 14 though.
Yes, I think it'll break unfortunately
|
|
||
| class Dinov2WithRegistersEmbeddings(nn.Module): | ||
| """ | ||
| Construct the CLS token, mask token, register tokens, position and patch embeddings. |
There was a problem hiding this comment.
Could we inherit from Dinov2Embeddings?
There was a problem hiding this comment.
we can, but it's a lot of overrides, so the reuse isn't bringing much IMO
There was a problem hiding this comment.
Imo we can inherit for the init? And highlight the diffs with small comments
|
|
||
| # Validate output dimensions if not tracing | ||
| if not torch.jit.is_tracing(): | ||
| if int(height) != patch_pos_embed.shape[-2] or int(width) != patch_pos_embed.shape[-1]: |
There was a problem hiding this comment.
Is there a particular reason why we have this check? Because it essentially checks if nn.functional.interpolate resized the embeddings to the correct size which seems redundant.
There was a problem hiding this comment.
uh indeed that seems to be a dead check. The point of these is to guarantee static shapes/force interpolate during tracing, which is what it does on other models, but is not useful/wrongly placed here
|
[For maintainers] Suggested jobs to run (before merge) run-slow: depth_anything, dinov2, dinov2_with_registers, dinov3_convnext, dinov3_vit, eomt, eomt_dinov3, pixio, rf_detr, sapiens2, videomt |
|
run-slow: depth_anything, dinov2, dinov2_with_registers, dinov3_convnext, dinov3_vit, eomt, eomt_dinov3, pixio, rf_detr, sapiens2, videomt |
|
This comment contains models: ["models/depth_anything", "models/dinov2", "models/dinov2_with_registers", "models/dinov3_convnext", "models/dinov3_vit", "models/eomt", "models/eomt_dinov3", "models/pixio", "models/rf_detr", "models/sapiens2", "models/videomt"] |
CI recapDashboard: View test results in Grafana |
|
Let me know if I should make a last pass 🤗 |
|
No rush, but glad to have a review if you have time! I iterated mostly on |
vasqu
left a comment
There was a problem hiding this comment.
Looking super solid, I have a few smaller comments but nothing major. I think we are close to getting this merged 🫡
| def __init__(self, config: Dinov2Config) -> None: | ||
| nn.Module.__init__(self) | ||
| self.cls_token = nn.Parameter(torch.randn(1, 1, config.hidden_size)) | ||
| self.mask_token = nn.Parameter(torch.zeros(1, config.hidden_size)) if config.use_mask_token else None | ||
| self.patch_embeddings = Dinov2PatchEmbeddings(config) | ||
| self.position_embeddings = nn.Parameter( | ||
| torch.randn(1, self.patch_embeddings.num_patches + 1, config.hidden_size) | ||
| ) | ||
| self.dropout = nn.Dropout(config.hidden_dropout_prob) | ||
| self.patch_size = config.patch_size | ||
| self.config = config |
There was a problem hiding this comment.
| def __init__(self, config: Dinov2Config) -> None: | |
| nn.Module.__init__(self) | |
| self.cls_token = nn.Parameter(torch.randn(1, 1, config.hidden_size)) | |
| self.mask_token = nn.Parameter(torch.zeros(1, config.hidden_size)) if config.use_mask_token else None | |
| self.patch_embeddings = Dinov2PatchEmbeddings(config) | |
| self.position_embeddings = nn.Parameter( | |
| torch.randn(1, self.patch_embeddings.num_patches + 1, config.hidden_size) | |
| ) | |
| self.dropout = nn.Dropout(config.hidden_dropout_prob) | |
| self.patch_size = config.patch_size | |
| self.config = config | |
| def __init__(self, config: Dinov2Config) -> None: | |
| super().__init__(self, config) | |
| del num_patches | |
| self.position_embeddings = nn.Parameter( | |
| torch.randn(1, self.patch_embeddings.num_patches + 1, config.hidden_size) | |
| ) |
can we not simplify a bit?
| patch_size = self.patch_size if isinstance(self.patch_size, Iterable) else (self.patch_size, self.patch_size) | ||
| new_height = height // patch_size[0] | ||
| new_width = width // patch_size[1] | ||
|
|
||
| sqrt_num_positions = torch_int(num_positions**0.5) | ||
| patch_pos_embed = patch_pos_embed.reshape(1, sqrt_num_positions, sqrt_num_positions, dim) | ||
| patch_pos_embed = patch_pos_embed.permute(0, 3, 1, 2) | ||
| target_dtype = patch_pos_embed.dtype | ||
| patch_pos_embed = nn.functional.interpolate( | ||
| patch_pos_embed.to(torch.float32), | ||
| size=(new_height, new_width), | ||
| mode="bicubic", | ||
| align_corners=False, | ||
| ).to(dtype=target_dtype) |
There was a problem hiding this comment.
Ok so I see 2 key differences
- The patch size can be different for width and height
- Dtype casting around interpolation
Let's highlight those with small comments
| nn.Module.__init__(self) | ||
| in_features = out_features = config.hidden_size | ||
| hidden_features = int(config.hidden_size * config.mlp_ratio) | ||
| self.fc1 = nn.Linear(in_features, hidden_features, bias=True) | ||
| self.activation_fn = ACT2FN[config.hidden_act] | ||
| self.fc2 = nn.Linear(hidden_features, out_features, bias=True) |
There was a problem hiding this comment.
| nn.Module.__init__(self) | |
| in_features = out_features = config.hidden_size | |
| hidden_features = int(config.hidden_size * config.mlp_ratio) | |
| self.fc1 = nn.Linear(in_features, hidden_features, bias=True) | |
| self.activation_fn = ACT2FN[config.hidden_act] | |
| self.fc2 = nn.Linear(hidden_features, out_features, bias=True) | |
| super().__init__(self, config) | |
| # Key difference: intermediate size is calculated based on ratio | |
| self.fc1 = nn.Linear(config.hidden_size, int(config.hidden_size * config.mlp_ratio)) | |
| self.fc2 = nn.Linear(int(config.hidden_size * config.mlp_ratio), config.hidden_size) |
Imo we can even fully copy based on adding a post init and intermediate size attr in the config
| nn.Module.__init__(self) | ||
| hidden_features = int(config.hidden_size * config.mlp_ratio) | ||
| hidden_features = (int(hidden_features * 2 / 3) + 7) // 8 * 8 | ||
| self.gate_proj = nn.Linear(config.hidden_size, hidden_features, bias=True) | ||
| self.up_proj = nn.Linear(config.hidden_size, hidden_features, bias=True) | ||
| self.down_proj = nn.Linear(hidden_features, config.hidden_size, bias=True) | ||
| self.act_fn = nn.functional.silu |
There was a problem hiding this comment.
Same here, ok but this will break with different intermediate sizes? maybe can be a conditional that calculates it based on whether swiglu is used or not
Is the activation only used in the mlp? Imo, we can also override the activation in post init when we use swiglu
|
|
||
| def __init__(self, config: Dinov2Config) -> None: | ||
| super().__init__() | ||
| self.norm1 = nn.LayerNorm(config.hidden_size, eps=config.layer_norm_eps) |
There was a problem hiding this comment.
Makes sense, always annoying 😢
| hidden_state = embedding_output | ||
| hidden_states = (hidden_state,) | ||
| for layer in self.encoder.layer: | ||
| hidden_state = layer(hidden_state, **kwargs) | ||
| hidden_states = hidden_states + (hidden_state,) |
There was a problem hiding this comment.
hmm what happened here that we don't have an encoder here? That was more elegant no?
| hidden_states=hidden_states, | ||
| attentions=output.attentions, | ||
| ) | ||
| return BackboneOutput(feature_maps=tuple(feature_maps)) |
| super()._init_weights(module) | ||
| if hasattr(module, "segmentation_bias") and isinstance(module.segmentation_bias, nn.Parameter): | ||
| nn.init.constant_(module.segmentation_bias, 0.0) | ||
| init.constant_(module.segmentation_bias, 0.0) |
There was a problem hiding this comment.
Oh wow how did we miss this yikes
| expected_slice = torch.tensor( | ||
| [[8.8223, 8.6483, 8.6216], [8.3332, 8.6047, 8.7545], [8.6547, 8.6885, 8.7472]], | ||
| ).to(torch_device) | ||
| # Re-baselined after the dinov2 refactor adopted ViT's fp32-softmax `eager_attention_forward`. |
There was a problem hiding this comment.
Hmm so it didnt have fp32 before in the softmax? Maybe we should revert on that and keep it as close as we can to the original. Pretty sure there is a eager attn with no fp32 softmax like bart
| # Re-baselined after the dinov2 refactor adopted ViT's fp32-softmax `eager_attention_forward`. | ||
| slice_expectations = Expectations( | ||
| { | ||
| ("cpu", None): [[8.8223, 8.6483, 8.6215], [8.3332, 8.6047, 8.7545], [8.6547, 8.6885, 8.7472]], |
There was a problem hiding this comment.
Since when do we run on cpu 👀
What does this PR do?
Part of the larger vision model refactor #41693 focused on dinov2, which has still some usage and downloads, but mostly serves as a basis for many other models. Attempt at putting this in line with the rest of the lib.