Skip to content
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

MMDet MaskRCNN ResNet50/SwinTransformer Decouple #3281

Conversation

eugene123tw
Copy link
Contributor

@eugene123tw eugene123tw commented Apr 5, 2024

Summary

  • Moved MMDet MaskRCNN
  • Moved MMDet ResNet50
  • Moved MMDet SwinTransformer
  • Moved MMDeploy MaskRCNN components

How to test

Checklist

  • I have added unit tests to cover my changes.​
  • I have added integration tests to cover my changes.​
  • I have added e2e tests for validation.
  • I have added the description of my changes into CHANGELOG in my target branch (e.g., CHANGELOG in develop).​
  • I have updated the documentation in my target branch accordingly (e.g., documentation in develop).
  • I have linked related issues.

License

  • I submit my code changes under the same Apache License that covers the project.
    Feel free to contact the maintainers if that's a concern.
  • I have updated the license header for each file (see an example below).
# Copyright (C) 2024 Intel Corporation
# SPDX-License-Identifier: Apache-2.0

Copy link
Contributor

@harimkang harimkang left a comment

Choose a reason for hiding this comment

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

At first glance, it looks like you're importing and replacing modules.
@jaegukhyun Can you take a look to make sure there's no duplication with detection-related tasks?

Will the MaskRCNN in src/otx/algo/instance_segmentation/maskrcnn.py, which is used as the main, be replaced with OTXInstanceSegModel instead of MMDetInstanceSegCompatibleModel in another PR?

src/otx/core/model/instance_segmentation.py Show resolved Hide resolved
@jaegukhyun
Copy link
Contributor

I have one question during review.
Will MaskRCNN family be exported using NativeExporter or MMDeployExporter?

@eugene123tw
Copy link
Contributor Author

At first glance, it looks like you're importing and replacing modules. @jaegukhyun Can you take a look to make sure there's no duplication with detection-related tasks?

Will the MaskRCNN in src/otx/algo/instance_segmentation/maskrcnn.py, which is used as the main, be replaced with OTXInstanceSegModel instead of MMDetInstanceSegCompatibleModel in another PR?

Hi @harimkang, maybe I need to clarify some points. I've transferred all the essential MaskRCNN code and backbone code from MMDet to OTX 2.x, reflecting a similar approach with MMDet detection models or the mmcv conv/norm module you did. Also, reuse as many components as Jaeguk moved.

I'm not sure of the rationale behind replacing MMDetInstanceSegCompatibleModel with OTXInstanceSegModel. As far as I understand, we still require MMXCompatibleModel classes to execute mmlab models on OTX 2.x without installing MMDet. I believe this PR aims to enable Geti MMDet models in OTX without having to install mmdet or mmcv.

Copy link
Contributor

@jaegukhyun jaegukhyun left a comment

Choose a reason for hiding this comment

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

I think separate mmdet folder has pros and cons. My opinion is we don't need to specify that all of algorithms come from mmdet. @harimkang What do you think?

@eugene123tw
Copy link
Contributor Author

eugene123tw commented Apr 23, 2024

I have one question during review. Will MaskRCNN family be exported using NativeExporter or MMDeployExporter?

@jaegukhyun Currently, it relies on MMDeployExporter. However, if we intend to remove mmdeploy dependency, this action should be addressed in a separate PR due to its large scope.

@harimkang
Copy link
Contributor

harimkang commented Apr 23, 2024

Hi @harimkang, maybe I need to clarify some points. I've transferred all the essential MaskRCNN code and backbone code from MMDet to OTX 2.x, reflecting a similar approach with MMDet detection models or the mmcv conv/norm module you did. Also, reuse as many components as Jaeguk moved.

I'm not sure of the rationale behind replacing MMDetInstanceSegCompatibleModel with OTXInstanceSegModel. As far as I understand, we still require MMXCompatibleModel classes to execute mmlab models on OTX 2.x without installing MMDet. I believe this PR aims to enable Geti MMDet models in OTX without having to install mmdet or mmcv.

Yes, I think we're on the same page. But doesn't MMDetInstanceSegCompatibleModel also have a dependency on mm after all, and if we want to use it without installing mm, shouldn't eventually make sure that existing models don't use the MMDetInstanceSegCompatibleModel class in MaskRCNN?

Or are you saying you're going to leave the old model alone and add a new recipe (without mm)?

@eugene123tw
Copy link
Contributor Author

eugene123tw commented Apr 23, 2024

Hi @harimkang, maybe I need to clarify some points. I've transferred all the essential MaskRCNN code and backbone code from MMDet to OTX 2.x, reflecting a similar approach with MMDet detection models or the mmcv conv/norm module you did. Also, reuse as many components as Jaeguk moved.
I'm not sure of the rationale behind replacing MMDetInstanceSegCompatibleModel with OTXInstanceSegModel. As far as I understand, we still require MMXCompatibleModel classes to execute mmlab models on OTX 2.x without installing MMDet. I believe this PR aims to enable Geti MMDet models in OTX without having to install mmdet or mmcv.

Yes, I think we're on the same page. But doesn't MMDetInstanceSegCompatibleModel also have a dependency on mm after all, and if we want to use it without installing mm, shouldn't eventually make sure that existing models don't use the MMDetInstanceSegCompatibleModel class in MaskRCNN?

Or are you saying you're going to leave the old model alone and add a new recipe (without mm)?

@harimkang MMDetInstanceSegCompatibleModel is a bridge for training MMDet models. With the decoupling of MMDet MaskRCNN in this PR, we also need to untangle data structures (such as BitmapMasks and PolygonMasks) and the instance segmentation data pipeline. However, this will require a separate PR to avoid overwhelming the current one.

Of course, there are still several mm-related components, including mmcv opset, mmdet data structures, mmdeploy, and mmengine, that still remain. It's possible to migrate all these components to OTX 2.x, it's also time-consuming. Given the scope, this migration cannot be achieved in a single PR.

Looking ahead, for torchvision.MaskRCNN, a new recipe will be necessary. And, to maintain backward compatibility, we can continue utilizing MMDetInstanceSegCompatibleModel until we find a solution for full compatibility between torchvision.MaskRCNN and MMDet MaskRCNN.

@harimkang
Copy link
Contributor

Of course, there are still several mm-related components, including mmcv opset, mmdet data structures, mmdeploy, and mmengine, that still remain. It's possible to migrate all these components to OTX 2.x, it's also time-consuming. Given the scope, this migration cannot be achieved in a single PR.

Looking ahead, for torchvision.MaskRCNN, a new recipe will be necessary. And, to maintain backward compatibility, we can continue utilizing MMDetInstanceSegCompatibleModel until we find a solution for full compatibility between torchvision.MaskRCNN and MMDet MaskRCNN.

Yes I understand, thanks for the clarification! :)

@eugene123tw eugene123tw changed the title MMDet MaskRCNN ResNet50 Decouple MMDet MaskRCNN ResNet50/SwinTransformer Decouple Apr 23, 2024
jaegukhyun
jaegukhyun previously approved these changes Apr 24, 2024
@harimkang harimkang enabled auto-merge (squash) April 25, 2024 00:28
@harimkang harimkang merged commit fffa2eb into openvinotoolkit:develop Apr 25, 2024
13 checks passed
@eugene123tw eugene123tw deleted the eugene/CVS-137823-mmdet-maskrcnn-decouple branch April 25, 2024 07:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
TEST Any changes in tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants