Skip to content

[Feature] Add efficientnet_fpn_backbone #5546

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

Closed
wants to merge 1 commit into from

Conversation

talregev
Copy link
Contributor

@talregev talregev commented Mar 3, 2022

Add efficientnet_fpn_backbone

@facebook-github-bot
Copy link

Hi @talregev!

Thank you for your pull request and welcome to our community.

Action Required

In order to merge any pull request (code, docs, etc.), we require contributors to sign our Contributor License Agreement, and we don't seem to have one on file for you.

Process

In order for us to review and merge your suggested changes, please sign at https://code.facebook.com/cla. If you are contributing on behalf of someone else (eg your employer), the individual CLA may not be sufficient and your employer may need to sign the corporate CLA.

Once the CLA is signed, our tooling will perform checks and validations. Afterwards, the pull request will be tagged with CLA signed. The tagging process may take up to 1 hour after signing. Please give it that time before contacting us about it.

If you have received this in error or have any questions, please contact us at [email protected]. Thanks!

@facebook-github-bot
Copy link

facebook-github-bot commented Mar 3, 2022

💊 CI failures summary and remediations

As of commit d4a21c0 (more details on the Dr. CI page):


  • 1/1 failures introduced in this PR

🕵️ 1 new failure recognized by patterns

The following CI failures do not appear to be due to upstream breakages:

See CircleCI build unittest_linux_cpu_py3.9 (1/1)

Step: "Run tests" (full log | diagnosis details | 🔁 rerun)

/root/project/torchvision/io/video.py:406: Runt...log: [mov,mp4,m4a,3gp,3g2,mj2] moov atom not found
test/test_datasets_video_utils.py::TestVideo::test_video_clips_custom_fps
  /root/project/torchvision/datasets/video_utils.py:217: UserWarning: There aren't enough frames in the current video to get a clip for the given clip length and frames between clips. The video (and potentially others) will be skipped.
    warnings.warn(

test/test_image.py::test_decode_png[L-ImageReadMode.GRAY-palette_pytorch.png]
test/test_image.py::test_decode_png[RGB-ImageReadMode.RGB-palette_pytorch.png]
  /root/project/env/lib/python3.9/site-packages/PIL/Image.py:945: UserWarning: Palette images with Transparency expressed in bytes should be converted to RGBA images
    warnings.warn(

test/test_io.py::TestVideo::test_read_video_timestamps_corrupted_file
  /root/project/torchvision/io/video.py:406: RuntimeWarning: Failed to open container for /tmp/tmpzvrlprce.mp4; Caught error: [Errno 1094995529] Invalid data found when processing input: '/tmp/tmpzvrlprce.mp4'; last error log: [mov,mp4,m4a,3gp,3g2,mj2] moov atom not found
    warnings.warn(msg, RuntimeWarning)

test/test_models.py::test_memory_efficient_densenet[densenet121]
test/test_models.py::test_memory_efficient_densenet[densenet169]
test/test_models.py::test_memory_efficient_densenet[densenet201]
test/test_models.py::test_memory_efficient_densenet[densenet161]
  /root/project/env/lib/python3.9/site-packages/torch/nn/modules/module.py:1383: UserWarning: positional arguments and argument "destination" are deprecated. nn.Module.state_dict will not accept them in the future. Refer to https://pytorch.org/docs/master/generated/torch.nn.Module.html#torch.nn.Module.state_dict for details.
    warnings.warn(

test/test_models.py::test_memory_efficient_densenet[densenet121]

This comment was automatically generated by Dr. CI (expand for details).

Please report bugs/suggestions to the (internal) Dr. CI Users group.

Click here to manually regenerate this comment.

@facebook-github-bot
Copy link

Thank you for signing our Contributor License Agreement. We can now accept your code for this (and any) Meta Open Source project. Thanks!

@talregev talregev force-pushed the Talr/efficientnet_backbone branch 2 times, most recently from dd9e92f to f5758af Compare March 4, 2022 05:54
Copy link
Contributor

@datumbox datumbox left a comment

Choose a reason for hiding this comment

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

@talregev Thanks for the contribution, welcome to our community.

Though your code on the PR looks good, I'm skeptical merging this for a few reasons:

  1. TorchVision focuses on providing canonical implementations. The specific FPN Backbone that you propose is aligned with the ResNet approach but it's not something that is described on a paper.
  2. The method is not used by any pre-trained model or model builder.
  3. It's rather a utility that fits better in downstream applications. Even the decision over which backbones should use a FrozenBN is somehow arbitrary and possibly application specific.

Note that writing an issue prior sending a PR is not mandatory, but it can help you avoid doing throwaway work. Have a look for more information here.

If you have concerns about anything I'm mentioning above, please let me know. Thanks!

@talregev
Copy link
Contributor Author

talregev commented Mar 4, 2022

@datumbox @oke-aditya

  1. The specific FPN Backbone that you propose is aligned with the ResNet approach

This is "maskrcnn" approach to build easily backbond for other networks for detection.

  1. The method is not used by any pre-trained model or model builder.

The method used pre-train efficientnet backbond that come with pytorch. you must missed that.

  1. It's rather a utility that fits better in downstream applications.

It the same as resnet and mobilenet function that help you to build backbond for maskrcnn and other network for detection.

@talregev talregev force-pushed the Talr/efficientnet_backbone branch from f5758af to 24d7ea6 Compare March 4, 2022 12:11
@talregev talregev force-pushed the Talr/efficientnet_backbone branch from 24d7ea6 to d4a21c0 Compare March 4, 2022 12:40
@datumbox
Copy link
Contributor

datumbox commented Mar 4, 2022

@talregev I'm aware the we offer an Efficientnet backbone; I'm the one who added it. :)

My point is that, even though you follow a "maskrcnn" approach, this doesn't mean it's a canonical implementation of a model described on a paper. Unless you can send us a reference of a specific paper you are implementing and also decide to offer pre-trained weights for that model, then this PR can't be merged.

I don't doubt that this utility method can be useful for users (this is why you could consider creating a small project for it at Github). But consider what would happen if we try to create methods for virtually every architecture/backbone that TorchVision supports. We would have to add tens of methods, duplicating massive amount of code. It would be nice at one point to offer a utility that creates an FPN out of arbitrary backbones but the main problem is that there are too many ways to build an FPN and each paper tends to do something different. That's the reason we currently don't offer such a general purpose utility.

@talregev
Copy link
Contributor Author

talregev commented Mar 4, 2022

But consider what would happen if we try to create methods for virtually every architecture/backbone that TorchVision supports

There is only 17 files inside models, with ~15 backbond models. most of them is not used for detection.
This is a detection utils file. it tend to help to build backbond for detections networks, as the name of the file imply.
We should add as many backbond support for detections, and if we see duplications, we can start refactoring and generalize.
until it happen, we can have another useful support and easy access method like mobilnet and resnt that found in the same file.

Unless you can send us a reference of a specific paper you are implementing

Not sure I understand this policy. This is raise a difficult questions for the community.
Are you accepting only novel things that come from papers?
There is no new things that come from engineer and try new stuff?

Also there is a paper (not my paper) that use cascade mask-rcnn with efficientnet backbond with NAS-FPN:
https://arxiv.org/pdf/2012.07177v2.pdf

@datumbox
Copy link
Contributor

datumbox commented Mar 4, 2022

@talregev Please review our general contribution and the model contribution guides for details on what kind of improvements we usually accept and what is the recommended approach for maximizing the chances of getting them merged.

As I explained earlier, this utility can't be added in TorchVision without having a specific canonical architecture using it. Moreover, prior implementing new architectures you would need to open an issue and discuss it with the maintainers as per the guide above. I see that you already separated some of the test improvements you made to a different PR at #5552. I'm happy to help you review and merge the other PR but unfortunately this one is not something we can accept. I don't argue that some people will find it useful; I'm saying that this is not the kind of utility we want to add in the library.

@datumbox datumbox closed this Mar 4, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants