-
Notifications
You must be signed in to change notification settings - Fork 33.4k
[DETR and friends] Remove is_timm_available #21814
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.
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
Changes from all commits
cc20eb8
3d6ac04
9cee8d3
5364802
bc1baf3
fff2697
d403567
665c93d
adbdc29
f981818
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,8 +14,9 @@ | |
| # limitations under the License. | ||
| """ DETR model configuration""" | ||
|
|
||
| import copy | ||
| from collections import OrderedDict | ||
| from typing import Mapping | ||
| from typing import Dict, Mapping | ||
|
|
||
| from packaging import version | ||
|
|
||
|
|
@@ -187,6 +188,8 @@ def __init__( | |
| backbone_model_type = backbone_config.get("model_type") | ||
| config_class = CONFIG_MAPPING[backbone_model_type] | ||
| backbone_config = config_class.from_dict(backbone_config) | ||
| # set timm attributes to None | ||
| dilation, backbone, use_pretrained_backbone = None, None, None | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To manage the different configuration options, I'd introduce additional argument checks and have defaults which are None to prevent clashes between timm and transformer backbone expected behaviour. As a user, I'd find it confusing to pass in i.e. something like this:
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed! Note that this can go in a followup PR. |
||
|
|
||
| self.use_timm_backbone = use_timm_backbone | ||
| self.backbone_config = backbone_config | ||
|
|
@@ -233,6 +236,28 @@ def num_attention_heads(self) -> int: | |
| def hidden_size(self) -> int: | ||
| return self.d_model | ||
|
|
||
| @classmethod | ||
| def from_backbone_config(cls, backbone_config: PretrainedConfig, **kwargs): | ||
| """Instantiate a [`DetrConfig`] (or a derived class) from a pre-trained backbone model configuration. | ||
| Args: | ||
| backbone_config ([`PretrainedConfig`]): | ||
| The backbone configuration. | ||
| Returns: | ||
| [`DetrConfig`]: An instance of a configuration object | ||
| """ | ||
| return cls(backbone_config=backbone_config, **kwargs) | ||
|
|
||
| def to_dict(self) -> Dict[str, any]: | ||
| """ | ||
| Serializes this instance to a Python dictionary. Override the default [`~PretrainedConfig.to_dict`]. Returns: | ||
| `Dict[str, any]`: Dictionary of all the attributes that make up this configuration instance, | ||
| """ | ||
| output = copy.deepcopy(self.__dict__) | ||
| if output["backbone_config"] is not None: | ||
| output["backbone_config"] = self.backbone_config.to_dict() | ||
| output["model_type"] = self.__class__.model_type | ||
| return output | ||
|
|
||
|
|
||
| class DetrOnnxConfig(OnnxConfig): | ||
| torch_onnx_minimum_version = version.parse("1.11") | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I realise this is already in the design and not introduced in this PR - but it feels a bit funny to have arguments which configure the timm checkpoint to be at the model level e.g.
dilation, whereas for a transformers backbone they're contained within their own config.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, however note that this is a legacy way of using timm models as backbone.
In the future, one should always leverage the
AutoBackboneclass for frameworks that make use of several backbones