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

runner constructor #1238

Closed
wants to merge 0 commits into from
Closed

runner constructor #1238

wants to merge 0 commits into from

Conversation

densechen
Copy link
Contributor

Add RunnerConstructor feature to build_runner.

It will not effect the original code feature, but just an enhancement.

Refer here for more details: issue1225

@CLAassistant
Copy link

CLAassistant commented Aug 3, 2021

CLA assistant check
All committers have signed the CLA.

@densechen
Copy link
Contributor Author

import in __init__.py

@zhouzaida zhouzaida requested review from nbei, xvjiarui and ZwwWayne August 4, 2021 09:02
@codecov
Copy link

codecov bot commented Aug 4, 2021

Codecov Report

Merging #1238 (d3841d2) into master (1790e9f) will increase coverage by 0.03%.
The diff coverage is 90.00%.

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

@@            Coverage Diff             @@
##           master    #1238      +/-   ##
==========================================
+ Coverage   68.23%   68.26%   +0.03%     
==========================================
  Files         160      161       +1     
  Lines       10721    10739      +18     
  Branches     1969     1971       +2     
==========================================
+ Hits         7315     7331      +16     
- Misses       3022     3023       +1     
- Partials      384      385       +1     
Flag Coverage Δ
unittests 68.26% <90.00%> (+0.03%) ⬆️

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

Impacted Files Coverage Δ
mmcv/runner/default_constructor.py 77.77% <77.77%> (ø)
mmcv/runner/__init__.py 100.00% <100.00%> (ø)
mmcv/runner/builder.py 100.00% <100.00%> (ø)

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 1790e9f...dcabcff. Read the comment docs.

@ZwwWayne
Copy link
Collaborator

ZwwWayne commented Aug 5, 2021

Hi @densechen
Thanks for the PR. Could you show the new use cases used in your projects after introducing RunnerConstructor? The information would be beneficial for us to consider the new design.

@densechen
Copy link
Contributor Author

Hi, @ZwwWayne , if a RunnerConstructor is provided, we can have the ability to make some extra operations on the build runner without the necessary to change the user code.
By the way, we have developed a new open source pytorch-style federated learning library, which will be released soon.
We are currently palning to write an extra package that constumed for mm-xxx project. It will make all the mm-xxx project enable to do federated learning experiments without the necessary to change the user code, but just import our extra package at the beginning of code.
While developing this extra package, we found that it may be more flexible to provide a RunnerConstructor in our case.

@zhouzaida
Copy link
Collaborator

Hi, @ZwwWayne , if a RunnerConstructor is provided, we can have the ability to make some extra operations on the build runner without the necessary to change the user code.
By the way, we have developed a new open source pytorch-style federated learning library, which will be released soon.
We are currently palning to write an extra package that constumed for mm-xxx project. It will make all the mm-xxx project enable to do federated learning experiments without the necessary to change the user code, but just import our extra package at the beginning of code.
While developing this extra package, we found that it may be more flexible to provide a RunnerConstructor in our case.

Got it. Thanks for your contribution. And could you provide an example to show how to use the RunnerConstructor

@densechen
Copy link
Contributor Author

Currently, if we want to do something with runner provided by mmcv, we need such piece of code:

from mmcv.runner.builder import RUNNERS, build_runner
from openfed.common.gluer import glue
from typing import Any

from .builder import build_openfed

def openfed_runner(model,
                   batch_processor=None,
                   optimizer=None,
                   work_dir=None,
                   logger=None,
                   meta=None,
                   max_iters=None,
                   max_epochs=None,
                   runner_cfg=None):
    # Build Runner
    runner = build_runner(
        runner_cfg,
        default_args=dict(
            model=model,
            batch_processor=batch_processor,
            optimizer=optimizer,
            work_dir=work_dir,
            logger=logger,
            meta=meta,
            max_iters=max_iters,
            max_epochs=max_epochs,
        )
    )
    # DO SOMETHING HERE
    runner.openfed_name = 'XXX'
    return runner


@RUNNERS.register_module(name='OpenFedRunner')
class OpenFedRunner(object):
    def __init__(self, *args, **kwargs):
        super().__init__()
        self.openfed_runner = openfed_runner(*args, **kwargs)

    def __getattribute__(self, name: str) -> Any:
        if name == 'openfed_runner':
            return super().__getattribute__(name)
        return getattr(self.openfed_runner, name)

    def __setattr__(self, name: str, value: Any) -> None:
        if name == 'openfed_runner':
            return super().__setattr__(name, value)
        return setattr(self.openfed_runner, name, value)

This code is quite ugly.
But if a runner_constructor is provided, we only need to define a new runner_constructor for doing this.
It seems more beautiful and flexible.
We are looking forward it.

@zhouzaida
Copy link
Collaborator

got it

@zhouzaida
Copy link
Collaborator

hi, please fix the CI

@densechen
Copy link
Contributor Author

fix yapf

@zhouzaida
Copy link
Collaborator

fix yapf

you can refer to https://github.com/open-mmlab/mmcv/blob/master/CONTRIBUTING.md

@densechen
Copy link
Contributor Author

fix yapf

@densechen
Copy link
Contributor Author

Waiting for merging
@zhouzaida

@zhouzaida
Copy link
Collaborator

zhouzaida commented Aug 18, 2021

Waiting for merging
@zhouzaida

hi @densechen , thanks for your contribution again. Before merging, An unittest should be added (refer to https://github.com/open-mmlab/mmcv/blob/master/tests/test_runner/test_optimizer.py). In addition, maybe we can enhance the docstring of DefaultRunnerConstructor like example or Args. BTW, DefaultRunnerConstructor is a really great design.

mmcv/runner/builder.py Outdated Show resolved Hide resolved
@ZwwWayne
Copy link
Collaborator

Overall, this PR looks good to me. We need to resolve conflict before the merge.

@densechen
Copy link
Contributor Author

Hi, @zhouzaida Can we have a further discussion via WeChat? My Email is densechen@foxmail.com

@zhouzaida
Copy link
Collaborator

Hi, @zhouzaida Can we have a further discussion via WeChat? My Email is densechen@foxmail.com

Great, I send an email to you

@ZwwWayne
Copy link
Collaborator

This PR can be merged after resolving linting issues and conflicts.

@zhouzaida zhouzaida mentioned this pull request Aug 19, 2021
13 tasks
@densechen
Copy link
Contributor Author

Using Examples: Inject some new properties and functions for existing Runner, such as EpochBasedRunner, though a RunnerReconstructor.

1. Define a new RunnerReconstructor:

from mmcv.runner import RUNNER_BUILDERS

@RUNNER_BUILDERS.register_module()
class MyRunnerConstructor:
    def __init__(self, runner_cfg, default_args=None):
        if not isinstance(runner_cfg, dict):
            raise TypeError('runner_cfg should be a dict',
                            f'but got {type(runner_cfg)}')
        self.runner_cfg = runner_cfg
        self.default_args = default_args

    def __call__(self):
        runner = RUNNERS.build(self.runner_cfg, default_args=self.default_args)
        # Add new properties for existing runner
        runner.my_name = 'my_runner'
        runner.my_function = lambda self: print(self.my_name)
        ...

2. Use this RunnerConstructor, the config file is like:

runner = dict(type='EpochBasedRunner', max_epochs=40, constructor='MyRunnerConstructor')

@ZwwWayne
Copy link
Collaborator

  1. need to resolve conflicts.
  2. Would you like to also update the usage of the runner in the documentation? You can also do that in a later PR.

Copy link
Collaborator

@zhouzaida zhouzaida left a comment

Choose a reason for hiding this comment

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

LGTM

@zhouzaida zhouzaida closed this Aug 23, 2021
@densechen
Copy link
Contributor Author

re-commit

@zhouzaida
Copy link
Collaborator

I am so sorry to force push to your master branch

@densechen
Copy link
Contributor Author

recommit

@densechen
Copy link
Contributor Author

#1296

@zhouzaida
Copy link
Collaborator

Thanks

@zhouzaida zhouzaida mentioned this pull request Aug 23, 2021
7 tasks
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