Skip to content

[AutoBackbone] Improve API#20407

Merged
NielsRogge merged 10 commits into
huggingface:mainfrom
NielsRogge:improve_backbone
Nov 28, 2022
Merged

[AutoBackbone] Improve API#20407
NielsRogge merged 10 commits into
huggingface:mainfrom
NielsRogge:improve_backbone

Conversation

@NielsRogge

@NielsRogge NielsRogge commented Nov 23, 2022

Copy link
Copy Markdown
Collaborator

What does this PR do?

As backbones themselves have hidden states and optional attentions, this PR adds them to the BackboneOutput.

This way, frameworks that leverage backbones can return hidden states/attentions of the backbone if the user specifies output_hidden_states=True or output_attentions=True.

To do:

  • perhaps we should test backbones with all common tests

@HuggingFaceDocBuilderDev

HuggingFaceDocBuilderDev commented Nov 23, 2022

Copy link
Copy Markdown

The documentation is not available anymore as the PR was closed or merged.

Comment thread src/transformers/models/resnet/test.py Outdated
@NielsRogge
NielsRogge requested a review from ydshieh November 24, 2022 08:49
@NielsRogge

Copy link
Copy Markdown
Collaborator Author

Pinging @ydshieh on this PR as I made 2 updates to test_modeling_common.py to support models which output a tuple of tensors as their first output. I've updated test_determinism and test_save_load to make them more general.

@NielsRogge

Copy link
Copy Markdown
Collaborator Author

@michaelbenayoun as seen on the CI, the torch fx tests fail because backbones aren't supported yet.

Could you add support for them in a separate PR?

@ydshieh

ydshieh commented Nov 24, 2022

Copy link
Copy Markdown
Collaborator

Pinging @ydshieh on this PR as I made 2 updates to test_modeling_common.py to support models which output a tuple of tensors as their first output. I've updated test_determinism and test_save_load to make them more general.

Looks good for me regarding the tests.

I would personally put the check isinstance(first, tuple) inside the check_xxx methods, and call them recursively.
This way we don't need to worry if each element in the list/tuple would contain list/tuple. But no obligation for now.

@NielsRogge

Copy link
Copy Markdown
Collaborator Author

@ydshieh for some reason the CI isn't run when I push new commits, do you know why?

@ydshieh

ydshieh commented Nov 24, 2022

Copy link
Copy Markdown
Collaborator

Not really, try again with a new empty commit?

git commit --allow-empty -m "empty commit to trigger CI"

@NielsRogge

Copy link
Copy Markdown
Collaborator Author

@ydshieh Thanks a lot! @sgugger feel free to approve :)

Comment thread tests/models/resnet/test_modeling_resnet.py Outdated
@NielsRogge
NielsRogge merged commit 0bae286 into huggingface:main Nov 28, 2022
mpierrau pushed a commit to mpierrau/transformers that referenced this pull request Dec 15, 2022
* Add hidden states and attentions to backbone outputs

* Update ResNet

* Fix more tests

* Debug test

* Fix test_determinism

* Fix test_save_load

* Remove file

* Disable fx tests

* Test

* Add fx support for backbones

Co-authored-by: Niels Rogge <nielsrogge@Nielss-MacBook-Pro.local>
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