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

add model registry #760

Merged
merged 23 commits into from Apr 10, 2021
Merged

add model registry #760

merged 23 commits into from Apr 10, 2021

Conversation

xvjiarui
Copy link
Collaborator

@xvjiarui xvjiarui commented Dec 25, 2020

This PR refactored Registry class in following ways:

  1. make build function an attribute of Registry class.

before:

model = build_from_cfg(cfg, MODELS)

after:

model = MODELS.build(cfg)

build function could be configured by passing argument.

  1. Added hierarchy structure for Registry.

For example, we can define MODEL registry in both MMDetection and MMClassification as followed:
In MMDetection:

from mmcv.utils import Registry
from mmcv.cnn import MODELS as MMCV_MODELS
MODELS = Registry('model', parent=MMCV_MODELS)

@MODELS.register_module()
class NetA(nn.Module):
    def forward(self, x):
        return x

In MMClassification:

from mmcv.utils import Registry
from mmcv.cnn import MODELS as MMCV_MODELS
MODELS = Registry('model', parent=MMCV_MODELS)

@MODELS.register_module()
class NetB(nn.Module):
    def forward(self, x):
        return x + 1

We could build either NetA or NetB by:

from mmcv.cnn import MODELS as MMCV_MODELS
net_a = MMCV_MODELS.build(cfg=dict(type='mmdet.NetA'))
net_b = MMCV_MODELS.build(cfg=dict(type='mmcls.NetB'))

Modification for MM repos:

before:

from mmcv.utils import Registry, build_from_cfg
BACKBONES = Registry('backbone')
def build(cfg, registry, default_args=None):
    """Build a module.

    Args:
        cfg (dict, list[dict]): The config of modules, is is either a dict
            or a list of configs.
        registry (:obj:`Registry`): A registry the module belongs to.
        default_args (dict, optional): Default arguments to build the module.
            Defaults to None.

    Returns:
        nn.Module: A built nn module.
    """
    if isinstance(cfg, list):
        modules = [
            build_from_cfg(cfg_, registry, default_args) for cfg_ in cfg
        ]
        return nn.Sequential(*modules)
    else:
        return build_from_cfg(cfg, registry, default_args)

def build_backbone(cfg):
    """Build backbone."""
    return build(cfg, BACKBONES) 

after:

from mmcv.cnn import ModelRegistry
from mmcv.cnn import MODELS as MMCV_MODELS
MODELS = ModelRegistry('models', parent=MMCV_MODELS)
def build_model(cfg):
    """Build model."""
    return MODELS.build(cfg)

@xvjiarui xvjiarui added the WIP label Dec 25, 2020
@hellock
Copy link
Member

hellock commented Dec 25, 2020

We can briefly describe the design in the PR description to involve more community discussions.

@codecov
Copy link

codecov bot commented Dec 25, 2020

Codecov Report

Merging #760 (6f97293) into master (8647b9a) will decrease coverage by 0.04%.
The diff coverage is 67.60%.

❗ Current head 6f97293 differs from pull request most recent head a0ef0a9. Consider uploading reports for the commit a0ef0a9 to get more accurate results
Impacted file tree graph

@@            Coverage Diff             @@
##           master     #760      +/-   ##
==========================================
- Coverage   64.86%   64.81%   -0.05%     
==========================================
  Files         148      149       +1     
  Lines        9189     9404     +215     
  Branches     1647     1708      +61     
==========================================
+ Hits         5960     6095     +135     
- Misses       2902     2962      +60     
- Partials      327      347      +20     
Flag Coverage Δ
unittests 64.81% <67.60%> (-0.05%) ⬇️

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

Impacted Files Coverage Δ
mmcv/cnn/bricks/conv.py 100.00% <ø> (ø)
mmcv/runner/base_module.py 79.41% <0.00%> (ø)
mmcv/runner/hooks/momentum_updater.py 51.48% <44.21%> (-6.85%) ⬇️
mmcv/runner/hooks/lr_updater.py 57.54% <60.00%> (+0.72%) ⬆️
mmcv/onnx/symbolic.py 40.60% <75.00%> (+0.71%) ⬆️
mmcv/cnn/__init__.py 100.00% <100.00%> (ø)
mmcv/cnn/builder.py 100.00% <100.00%> (ø)
mmcv/cnn/utils/weight_init.py 91.66% <100.00%> (+0.09%) ⬆️
mmcv/utils/registry.py 98.33% <100.00%> (+0.86%) ⬆️
mmcv/version.py 92.30% <100.00%> (ø)
... and 1 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8647b9a...a0ef0a9. Read the comment docs.

@xvjiarui xvjiarui removed the WIP label Dec 27, 2020
@ZwwWayne
Copy link
Collaborator

The original goal of design seems to be achieved perfectly. One remaining question: is there a guide to bypass the 3-level hierarchy? Because from the code here, it seems that only a 2-level hierarchy is accepted.

@ZwwWayne
Copy link
Collaborator

We may also provide a guide for migrating from the old code to the new code (or what do we need to take care of to avoid BC-breaking) in the doc if necessary.

@xvjiarui
Copy link
Collaborator Author

The original goal of design seems to be achieved perfectly. One remaining question: is there a guide to bypass the 3-level hierarchy? Because from the code here, it seems that only a 2-level hierarchy is accepted.

Multi-level is supported.

@xvjiarui
Copy link
Collaborator Author

We may also provide a guide for migrating from the old code to the new code (or what do we need to take care of to avoid BC-breaking) in the doc if necessary.

I will provide a illustration in this PR. And I will create a PR in model for demo.

mmcv/utils/registry.py Outdated Show resolved Hide resolved
@ZwwWayne
Copy link
Collaborator

We may also need to provide one more registry for DATASET, thus downstream tasks can use similar build function from MMCV to build model, dataset, and runner in the unified entry point/engine.

mmcv/utils/registry.py Outdated Show resolved Hide resolved
return self._module_dict.get(key, None)
scope, real_key = self.split_scope_key(key)
if scope is not None:
return self._children[scope].get(real_key)
Copy link
Member

Choose a reason for hiding this comment

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

Does this satisfy the desired use case?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yep. The object could be built with inheritance in a recursive way.

@xvjiarui xvjiarui requested a review from hellock January 5, 2021 02:37
docs/registry.md Outdated Show resolved Hide resolved
@ZwwWayne
Copy link
Collaborator

ZwwWayne commented Jan 5, 2021

We may also need to provide one more registry for DATASET, thus downstream tasks can use similar build function from MMCV to build model, dataset, and runner in the unified entry point/engine.

Ping this part. Are we going to add this in this PR or in the future when necessary?

@xvjiarui
Copy link
Collaborator Author

xvjiarui commented Jan 5, 2021

We may also need to provide one more registry for DATASET, thus downstream tasks can use similar build function from MMCV to build model, dataset, and runner in the unified entry point/engine.

Ping this part. Are we going to add this in this PR or in the future when necessary?

The dataset build function is not unified yet. I suggest add DATASET in a new PR>

@ZwwWayne
Copy link
Collaborator

ZwwWayne commented Jan 5, 2021

We may also need to provide one more registry for DATASET, thus downstream tasks can use similar build function from MMCV to build model, dataset, and runner in the unified entry point/engine.

Ping this part. Are we going to add this in this PR or in the future when necessary?

The dataset build function is not unified yet. I suggest add DATASET in a new PR>

OK.

@xvjiarui xvjiarui requested a review from hellock January 15, 2021 01:50
mmcv/utils/registry.py Outdated Show resolved Hide resolved
mmcv/utils/registry.py Show resolved Hide resolved
mmcv/utils/registry.py Show resolved Hide resolved
mmcv/utils/registry.py Outdated Show resolved Hide resolved
mmcv/utils/registry.py Show resolved Hide resolved
mmcv/utils/registry.py Show resolved Hide resolved
@xvjiarui
Copy link
Collaborator Author

There is a remaining issue, how could build modules in mmcv.MODELS with mmdet.MODELS? What prefix should be used?
Currently, if mmdet.MODELS.build get "ResNet", it will search mmdet.MODELS only. If mmdet.MODELS.build get "mmaction.ResNet3D" it will search mmcv.MODELS from top to bottom.

@xvjiarui xvjiarui requested a review from hellock March 2, 2021 22:28
docs/registry.md Outdated Show resolved Hide resolved
docs/registry.md Show resolved Hide resolved
docs/registry.md Outdated Show resolved Hide resolved
docs/registry.md Outdated Show resolved Hide resolved
docs/registry.md Outdated Show resolved Hide resolved
mmcv/cnn/builder.py Outdated Show resolved Hide resolved
mmcv/utils/registry.py Outdated Show resolved Hide resolved
@xvjiarui
Copy link
Collaborator Author

xvjiarui commented Mar 23, 2021

  • Refactor registry.md

@xvjiarui xvjiarui requested a review from hellock March 28, 2021 03:55
@xvjiarui
Copy link
Collaborator Author

@hellock doc refactor done.

docs/registry.md Outdated Show resolved Hide resolved
docs/registry.md Outdated Show resolved Hide resolved
docs/registry.md Outdated Show resolved Hide resolved
@xvjiarui
Copy link
Collaborator Author

xvjiarui commented Apr 7, 2021

@hellock another doc refactor done.

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.

None yet

3 participants