-
Notifications
You must be signed in to change notification settings - Fork 613
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
Add Modulated Deformable Convolution layer #2196
Conversation
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). 📝 Please visit https://cla.developers.google.com/ to sign. Once you've signed (or fixed any issues), please reply here with What to do if you already signed the CLAIndividual signers
Corporate signers
ℹ️ Googlers: Go here for more info. |
I signed CLA. |
This reverts commit 44bf775.
Compliant test fails due to |
I found that Addons has |
All green! Ready for review! |
fa722c2
to
7e9654f
Compare
I couldn't wait to make the GPU kernel until the CPU one reviewed! |
/cc @tanzhenyu @dynamicwebpaige for ecosystem check. |
I put this under ecosystem review. If no project in TF ecosystem is interested in this or has this in It own internal roadmap we will try to review It here. |
This is one single feature, deformable convolution layer. I don't see how this can be split into multiple pull requests. |
Thanks @Licht-T I look forward to this feature! Can't wait to use this for experiments. |
This PR Is now near 3k lines. Have we already evaluated the performance gap of a compositional (also if v1) implementation like https://tensorlayer.readthedocs.io/en/latest/_modules/tensorlayer/layers/convolution/deformable_conv.html ? |
if self.data_format != "channels_first": | ||
raise ValueError("`channels_last` data format is not supported.") |
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.
"channels_last" is the default data_format in tf.keras.layers.Conv2D. If user is selecting "channels_last", the output should be transposed, instead of raising error.
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.
swapaxis
makes a lot of computing cost. IMO, swapaxis
should not be implicitly applied.
The code looks great, we are using it successfully in one of our projects. Is anybody still working on it? It's been more than a year of waiting on deformable conv :) ... |
Thanks for a lot of comments. I'm back! Now I have much time to focus on this PR and will resolve some conflicts this week. |
That's difficult as
|
@bhack Not tested yet. I'll try. According to tensorlayer/TensorLayer#641 (comment), TensorLayer implementation is 100 times slower than the original MXNet implementation. That's why we need the optimized C++ code for Deformable Convolution.
|
It is 3 years old. We could check what happens now with the current compile stack. As a general side note: /cc @yarri-oss @tomerk As custom kernels are going to create a quite large code and maintainership overhead in the repo can we have a contact point in the XLA/HLO team to just undertestand if and when we could achieve enought performance with a compositional/compiling path (e.g. trigger some FR to the compiler stack)? We really want reduce the number of maitnained custom ops in the library. See also https://discuss.tensorflow.org/t/tfr-compositional-ops/ |
@bhack Thanks for your comments! I did some tests. In the best case, this PR is approx. 1000 times faster than the TensorLayer one.
|
I know this PR is quite a large code. I don't want to make maintainers annoyed by the large custom ops code, but I just want to provide the Deformable Convolution, which is one of the greatest achievements in CNN research, to TensorFlow users. If we can achieve good performance in another way like XLA/HLO, I should/will re-implement this PR in that way. |
@Licht-T Yes thank you for effort. In this specific case we will have near 3k lines to maintain. Also I think that you need to test the tensorlayer implementation with You can inspect the compiled function with : https://www.tensorflow.org/xla#inspect_compiled_programs If you could profile it could be also interesting to see if there is any specific dominant ops. https://www.tensorflow.org/tensorboard/tensorboard_profiling_keras You can use profiler start and stop if you don't want to use callbacks |
That looks really great! We are currently using deformable convolutions for a project and are using the tensorflowlayer version. But it's too slow for our use case. So I tried your implementation but ran into problems. How do I correctly build the code including the deformable conv layer? So far I cloned your repo, switched to add-deformable-conv branch (tried the add-deformable-conv-internal as well) and followed the instructions to install tensorflow_addons from source. The build process runs fine (just some bazel warnings). However, installing the wheel and trying a simple code
throws this error:
What am I doing wrong? Some help would be much appreciated :) Some additional info: I'm using Ubuntu 18.04 and the warnings are several
and a
|
Hey, I've tried this deformable convolution implementation in my project, and it seems to work as expected. However the performance hit compared to a normal convolution is huge. Would it be possible to support NHWC ordering and mixed_fp16/fp16 ? An additional thought: unless I'm mistaken, it seems that the modulated convolution could be implemented as first a layer that grabs the values at the predicted positions (with bilinear interpolation), and applies the mask, and concatenates the values (for example for a 3x3 convolution the number of channels of the output would be multiplied by 9), then a standard 1x1 convolution. |
@Licht-T I tried compiling it using cuda 11.6, cudnn 8.3 but getting following error on usage:
|
Changing every occurrence of |
Thanks for the contribution but I don't think we have here the bandwidth here to add another custom-ops in the repo. Please try to contribute a python only version in Keras-cv https://github.com/keras-team/keras-cv |
Hi @bhack, If I was to give this PR another try but attempt to split it down into smaller more bite sized PRs would you/others be willing to review and merge? Another caveat, is I would want to have my initial "hello world" implementation be a 3D deformable convolution as opposed to 2D within this PR. It would be my intent to leave a 2D implementation to someone else who wants to modify/extend my work (where I would be happy to support and review in that endeavour, but would be leaving someone else to spearhead it). Although I can't prove that I'll stick around after the PR, I do maintain a package in my field which pulls in outside contributions, and by feeling some of the pain of having code-owners disappear, I hope my word has enough weight when I say I want to stick around and help when there is future work/issues/PRs going on around the code that I contribute. Cheers, |
@axeldavy, if I was to take this on, might you want to help me? |
@SimonBiggs Can you open a ticket to propose this in Keras-CV https://github.com/keras-team/keras-cv/ As many of the computer vision related contributions are converging there. |
I notice the following comment re custom ops: Would that make this contribution the first custom op in Keras-CV that pulls in CUDA code? I would aim to use the approach that @axeldavy mentioned, so the custom op section would hopefully have a reasonably smaller surface area. |
We could start with a ticket there to discuss the topic and how to cover this convolution before creating a PR. |
Thanks @bhack, New issue now over at keras-team/keras-cv#1140. Cheers :), |
Description
This is the implementation of Modulated Deformable Convolution. This fixes #179. There is another PR #1129, but that is stale and contaminated by unknown license codes. So I re-implemented this which is based on the Torchvision's implementation.
I knew this would be too big to get reviewed, so, at first, I made the CPU only kernel with Eigen. When the CPU kernel is good enough, I will move to the next step: the GPU kernel implementation.UPDATE: GPU kernel available!
Type of change
Checklist:
How Has This Been Tested?
pytest
unit-testing