-
Notifications
You must be signed in to change notification settings - Fork 25.3k
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
Efficientformer #20459
Efficientformer #20459
Conversation
Hey @Bearnardd, thank you for working on this! Could you run Also, type casting function arguments (e.g. |
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.
Thanks for working on this @Bearnardd! I left a couple of comments to fix inconsistencies and follow the best practices. Most of the comments are easy to address, some of the main issues are:
- EfficientFormerLastStage could use some restructuring to get rid of using nn.Sequential.
- test_feature_extraction_efficientformer.py is missing (can be copied from ViT).
- EfficientFormerForImageClassificationWithTeacher uses a frozen external model (RegNetY-16GF) for distillation I'm not sure if the implementation is accurate as it consists of a single dense layer that is trained alongside the model.
- Some common modeling tests (defined in tests/test_modeling_common.py) are failing, you can run all tests with:
pytest tests/models/efficientformer/test_modeling_efficientformer.py
Hope this helps :)
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
Hi @alaradirik - thank you very much for the detailed review! I will address the changes shortly :) . I am aware of the failing tests but I am not entirely sure how to count the number of expected attentions and hidden layers for this particular model since it does not have a "standard" transformer based architecture. Nevertheless I think that I will address the current comments and as the next step I will ask you some questions about expected attention and hidden outputs. |
Hey @Bearnardd, no problem at all! We define our own small model architecture within the If you are sure the implementation is correct and this is expected (in this case or other cases), you can always override the common tests within |
…to efficientformer
The documentation is not available anymore as the PR was closed or merged. |
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.
The changes look good to me!
I left few a comments, including a code snippet to fix the model initialization test failure. My main comment is regarding the feature extractor. Could you rename EfficientFormerFeatureExtractor
as EfficientFormerImageProcessor
?
We are changing the naming to avoid confusion as users sometimes think the feature extractor is a model itself rather than the preprocessor. The renaming requires changing the filename (image_processing_efficientformer.py
), test filename, replacing all instances of feature extraction imports with the image processor and adding it to models/auto/image_processing_auto.py
.
You can see an example of this in this PR.
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
Thanks @Bearnardd ! |
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Show resolved
Hide resolved
...ers/models/efficientformer/convert_efficientformer_original_pytorch_checkpoint_to_pytorch.py
Outdated
Show resolved
Hide resolved
...ers/models/efficientformer/convert_efficientformer_original_pytorch_checkpoint_to_pytorch.py
Show resolved
Hide resolved
...ers/models/efficientformer/convert_efficientformer_original_pytorch_checkpoint_to_pytorch.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
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.
The PR is almost ready to be merged and looks great overall!
I left a few comments to update all repo names and fix the 3 failing slow tests. You can run all tests with:
RUN_SLOW=True pytest tests/models/efficientformer/test_modeling_efficientformer.py
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/configuration_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
src/transformers/models/efficientformer/modeling_efficientformer.py
Outdated
Show resolved
Hide resolved
- Adds EfficientFormer V1 to transformers - PR co-authored by @novice03 and @Bearnardd Co-authored-by: novice <pranavpulijala@gmail.com> Co-authored-by: novice <44259234+novice03@users.noreply.github.com>
Thank you so much for working on this @Bearnardd! |
- Adds EfficientFormer V1 to transformers - PR co-authored by @novice03 and @Bearnardd Co-authored-by: novice <pranavpulijala@gmail.com> Co-authored-by: novice <44259234+novice03@users.noreply.github.com>
- Adds EfficientFormer V1 to transformers - PR co-authored by @novice03 and @Bearnardd Co-authored-by: novice <pranavpulijala@gmail.com> Co-authored-by: novice <44259234+novice03@users.noreply.github.com>
This PR adds Efficientformer, a model that has similar latency as MobileNets, but achieves better accuracy on ImageNet. It is based on the closed PR: #18296
Paper: https://arxiv.org/abs/2206.01191
Code and weights: https://github.com/snap-research/EfficientFormer
Fixes #18041
Who can review?
@alaradirik @NielsRogge