Skip to content

🚨 out_indices always a list#30941

Merged
amyeroberts merged 5 commits intohuggingface:mainfrom
amyeroberts:consistent-out-indices-type
May 22, 2024
Merged

🚨 out_indices always a list#30941
amyeroberts merged 5 commits intohuggingface:mainfrom
amyeroberts:consistent-out-indices-type

Conversation

@amyeroberts
Copy link
Copy Markdown
Contributor

@amyeroberts amyeroberts commented May 21, 2024

What does this PR do?

Recent updates to timm changed the type of the attribute model.feature_info.out_indices. Previously, out_indices would reflect the input type of out_indices on the create_model call i.e. either tuple or list. Now, this value is always a tuple. This causes the following failure on main:

FAILED tests/models/timm_backbone/test_modeling_timm_backbone.py::TimmBackboneModelTest::test_timm_transformer_backbone_equivalence - AssertionError: (1, 2, 3) != [1, 2, 3]

Example CI run: https://app.circleci.com/pipelines/github/huggingface/transformers/93542/workflows/38bfe697-908c-4d2c-a05c-7b99090fb084/jobs/1226834

As list are more useful and consistent for us -- we cannot save tuples in configs, they must be converted to lists first -- we instead choose to cast out_indices to always be a list.

This has the possibility of being a slight breaking change if users are creating models and relying on out_indices on being a tuple. As this property only happens when a new model is created, and not if it's saved and reloaded (because of the config), then I think this has a low chance of having much of an impact.

The following tests were run to make sure everything is still OK:

pytest tests/utils/test_backbone_utils.py
pytest tests/models/timm_backbone/test_modeling_timm_backbone.py
pytest tests/models/swin/test_modeling_swin.py::SwinBackboneTest

@amyeroberts amyeroberts requested a review from ArthurZucker May 21, 2024 16:13
Comment thread src/transformers/utils/backbone_utils.py Outdated
Comment thread src/transformers/utils/backbone_utils.py Outdated
@HuggingFaceDocBuilderDev
Copy link
Copy Markdown

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.

@amyeroberts amyeroberts requested a review from ydshieh May 22, 2024 09:34
Comment thread src/transformers/utils/backbone_utils.py Outdated
Copy link
Copy Markdown
Collaborator

@ydshieh ydshieh left a comment

Choose a reason for hiding this comment

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

Sound good to me but with a nit question. Would like to hear from @NielsRogge too.

Thanks!

@amyeroberts amyeroberts force-pushed the consistent-out-indices-type branch from a1e1e50 to 66a0096 Compare May 22, 2024 12:49
Comment thread tests/utils/test_backbone_utils.py Outdated
Copy link
Copy Markdown
Collaborator

@ArthurZucker ArthurZucker left a comment

Choose a reason for hiding this comment

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

If we need them to be mutable, cool to cast to list!

@amyeroberts
Copy link
Copy Markdown
Contributor Author

If we need them to be mutable, cool to cast to list!

@ArthurZucker It's not that we need them to be mutable (in fact, we probably don't), it's to do with consistent typing when saving / loading configs. Previously, we just followed the out_indices from timm, which could be tuples or lists. However, for our models, because they're loaded through the configs, if a model is saved out with out_indices which are a tuple, it'll be cast to a list because of writing as a json. Previously, timm would respect whichever type was used to create the model, but now only uses tuples. This PR enforces a casting to list to make sure:

  • We're consistent across backbones: timm vs. transformers
  • timm models have out_indices consistently set to lists across timm versions

@amyeroberts amyeroberts merged commit dff54ad into huggingface:main May 22, 2024
@amyeroberts amyeroberts deleted the consistent-out-indices-type branch May 22, 2024 14:23
itazap pushed a commit that referenced this pull request May 30, 2024
* out_indices always a list

* Update src/transformers/utils/backbone_utils.py

* Update src/transformers/utils/backbone_utils.py

* Move type casting

* nit
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants