Skip to content
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

🚨 out_indices always a list #30941

Merged
merged 5 commits into from
May 22, 2024

Conversation

amyeroberts
Copy link
Collaborator

@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

@HuggingFaceDocBuilderDev

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
Copy link
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!

Copy link
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
Collaborator 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
21 checks passed
@amyeroberts amyeroberts deleted the consistent-out-indices-type branch May 22, 2024 14:23
itazap pushed a commit that referenced this pull request May 24, 2024
* out_indices always a list

* Update src/transformers/utils/backbone_utils.py

* Update src/transformers/utils/backbone_utils.py

* Move type casting

* nit
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
zucchini-nlp pushed a commit to zucchini-nlp/transformers that referenced this pull request Jun 11, 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.

None yet

4 participants