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

[Refactor] Support MMEngine-based & MMCV-based #108

Merged
merged 15 commits into from
Dec 29, 2022

Conversation

nijkah
Copy link
Member

@nijkah nijkah commented Dec 19, 2022

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

  • Support MMEngine-based Frameworks
  • Support MMCV-based Frameworks

Modification

Support MMEngine-based Frameworks manually
Support MMCV-based Frameworks
Replace MMCV utils with MMEngine utils

@KKIEEK
Copy link
Contributor

KKIEEK commented Dec 19, 2022

아직 러프하게 작업 중이시기도 하고 이미 염두해두고 계실 수도 있겠습니다만, 제 생각을 정리할 겸 미리 몇 자 적어봅니다.

  1. mm component를 try-except 구문을 이용하여 import 해서 쓰는 방향성에 대해서는 동의합니다.
  2. 하나의 class 안에 if branch를 걸어서 mmcv와 mmengine을 구분하는 것은 로직을 복잡하게 보이도록 만듭니다. class 두 개를 각각 구현하시고 try-except 구문을 이용해 동적으로 import 하는 방식을 차용한다면 mmcv 및 mmengine 서로의 구현을 더욱 명료하게 보일 수 있을 듯합니다.

예시 1)

# at /mm/tasks/mmdet.py
if HAS_MMENGINE:  # or MMCV_VERSION > 2.0.0
  @TASKS.register_module()
  class MMDetection:
    # mmengine based implementation ...
else:
  @TASKS.register_module()
  class MMDetection:
    # mmcv based implementation ...

예시 2)

# folder
/mm/tasks
  - ...
  - mmtrainbase.py
  - mmcv
    - mmdet.py
  - mmengine
    - mmdet.py
# at /mm/tasks/__init__.py
import ...
try:
  import mmengine
  from .mmengine import MMDetection
except:
  import mmcv
  from .mmcv import MMDetection
__all__ = [..., 'MMDetection', ...]

@nijkah nijkah changed the title MMEngine Draft - Run starts, but throw Error [Refactor] Support MMEgnine-based & MMCV-based Dec 23, 2022
Co-authored-by: Junhwa Song <[email protected]>
Signed-off-by: Hakjin Lee <[email protected]>
@KKIEEK KKIEEK changed the title [Refactor] Support MMEgnine-based & MMCV-based [Refactor] Support MMEngine-based & MMCV-based Dec 23, 2022
@nijkah nijkah marked this pull request as ready for review December 26, 2022 05:21
@nijkah nijkah requested a review from KKIEEK December 26, 2022 05:21
@codecov-commenter
Copy link

codecov-commenter commented Dec 26, 2022

Codecov Report

❗ No coverage uploaded for pull request base (main@36ca780). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #108   +/-   ##
=======================================
  Coverage        ?   62.01%           
=======================================
  Files           ?       58           
  Lines           ?     1951           
  Branches        ?      313           
=======================================
  Hits            ?     1210           
  Misses          ?      633           
  Partials        ?      108           
Flag Coverage Δ
unittests 62.01% <0.00%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@nijkah nijkah requested a review from yhna940 December 26, 2022 05:59
Copy link
Contributor

@KKIEEK KKIEEK left a comment

Choose a reason for hiding this comment

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

LGTM

@nijkah nijkah merged commit 68d479d into SIAnalytics:main Dec 29, 2022
@nijkah nijkah deleted the feature/mmengine branch December 29, 2022 06:00
@yhna940 yhna940 mentioned this pull request Jan 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants