Skip to content
This repository has been archived by the owner on Mar 12, 2024. It is now read-only.

Integration with Detectron2 #11

Closed
jmspereira opened this issue May 28, 2020 · 11 comments
Closed

Integration with Detectron2 #11

jmspereira opened this issue May 28, 2020 · 11 comments

Comments

@jmspereira
Copy link

First of all, excellent work!

I know that probably the integration with detectron2 is not automatic since there are differences in the training architecture compared with the default detectron2 procedure. But there are any plans to integrate DETR in detectron2?

Thank you!

@fmassa
Copy link
Contributor

fmassa commented May 28, 2020

Hi,

Thanks for the message!

There is some work needed in order to integrate DETR with detectron2, but this is something that should definitely be considered. My initial thinking was just to add a compatibility layer in a folder d2_compat/, which uses detectron2 dataset / transforms abstractions, while re-using most of DETR codebase for model definition.

cc @ppwwyyxx @wanyenlo for thoughts if we should integrate DETR directly inside detectron2, or use D2 as a library via a compatibility layer.

@jmspereira
Copy link
Author

Thank you very much for the quick response!
I look forward to any news! 😄

@pvti
Copy link

pvti commented Jun 1, 2020

Hi there, detectron2 brings me here :)

@poodarchu
Copy link

I've implemented DETR based on Detectron2 at:
https://github.com/poodarchu/DETR.detectron2

@poodarchu
Copy link

I've implemented DETR based on Detectron2 at:
https://github.com/poodarchu/DETR.detectron2

I'm glad to share the latest progress that I've make it usable and can reproduce the results in official implementation.

@asharakeh
Copy link

@poodarchu Thank you for this!

@alcinos
Copy link
Contributor

alcinos commented Jun 25, 2020

Hi all, we are releasing the official Detectron2 wrapper (see #103), along with a conversion script.
Let us know if it fits your needs.

@jmspereira
Copy link
Author

I do not have time to test the usage in the following days, but it seems to perfectly fit my needs.
Thank you again for the amazing work!

@poodarchu
Copy link

I think the MaskBackbone and Joiner are unnecessary and will introduce burdens of understanding.

@fmassa
Copy link
Contributor

fmassa commented Jun 28, 2020

@poodarchu yes, the Joiner is unfortunate and reminiscent of previous (more complicated) implementation.
Removing it is totally possible, but care would need to be taken to avoid breaking backwards-compatibility with previously saved models, and for the sake of simplicity, we didn't do it before the release.

@alcinos
Copy link
Contributor

alcinos commented Jun 28, 2020

Thanks for your feedback @poodarchu.
We took the option to design the wrapper to be as thin as possible so as to reduce maintenance cost for us, and ensure exact reproduction of the paper's results. In particular, our wrapper doesn't require rewriting the DETR's forward function, which is the approach you took in your codebase.
As you noted, this requires a tiny packaging (aka MaskBackbone) of Detectron2's backbones so that they return features along with the mask in the way expected by DETR. The Joiner is imported directly from DETR's codebase.

Both options are viable and have their own pros and cons, I hope this clarifies the motivation behind our choices.

Since the wrapper landed on master, I'm closing this. Feel free to reach out if you have further concerns.

@alcinos alcinos closed this as completed Jun 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants