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

[Refact] context rewrite #38

Merged
merged 31 commits into from
Jun 3, 2022
Merged

[Refact] context rewrite #38

merged 31 commits into from
Jun 3, 2022

Conversation

yhna940
Copy link
Contributor

@yhna940 yhna940 commented May 31, 2022

This is a PR forcing run to work with the context being changed by the context manager.
We plan to work on refactoring the context manager into a decorator type.

@yhna940 yhna940 changed the title Feature/refact context [Refact] context rewrite May 31, 2022
@github-actions
Copy link

github-actions bot commented May 31, 2022

Coverage report

The coverage rate is 83.21%

The branch rate is 60.3%

87% of new lines are covered.

Diff Coverage details (click to unfold)

mmtune/mm/tasks/mmdet.py

100.0% of new lines are covered

mmtune/mm/tasks/base.py

72.22222222222223% of new lines are covered

mmtune/utils/config.py

54.54545454545455% of new lines are covered

mmtune/mm/context/rewriters/builder.py

100.0% of new lines are covered

mmtune/mm/context/rewriters/register.py

100.0% of new lines are covered

mmtune/mm/hooks/reporter.py

100.0% of new lines are covered

mmtune/mm/context/rewriters/instantiate.py

100.0% of new lines are covered

mmtune/apis/tune.py

40.0% of new lines are covered

mmtune/mm/tasks/blackbox.py

0.0% of new lines are covered

mmtune/ray/spaces/builder.py

66.66666666666667% of new lines are covered

mmtune/mm/context/rewriters/path.py

100.0% of new lines are covered

mmtune/mm/tasks/builder.py

100.0% of new lines are covered

mmtune/utils/logger.py

66.66666666666667% of new lines are covered

mmtune/mm/tasks/mmcls.py

100.0% of new lines are covered

mmtune/utils/container.py

100.0% of new lines are covered

mmtune/ray/searchers/nevergrad.py

100.0% of new lines are covered

mmtune/mm/context/manager.py

100.0% of new lines are covered

mmtune/mm/context/rewriters/decouple.py

100.0% of new lines are covered

mmtune/mm/hooks/checkpoint.py

100.0% of new lines are covered

mmtune/mm/context/rewriters/base.py

83.33333333333334% of new lines are covered

mmtune/ray/stoppers/early_drop.py

100.0% of new lines are covered

mmtune/ray/spaces/base.py

100.0% of new lines are covered

mmtune/utils/init.py

100.0% of new lines are covered

mmtune/mm/tasks/sphere.py

100.0% of new lines are covered

mmtune/version.py

100.0% of new lines are covered

mmtune/mm/context/rewriters/dump.py

100.0% of new lines are covered

mmtune/mm/context/rewriters/patch.py

100.0% of new lines are covered

mmtune/mm/tasks/mmseg.py

50.0% of new lines are covered

mmtune/mm/context/rewriters/merge.py

100.0% of new lines are covered

mmtune/mm/context/rewriters/init.py

100.0% of new lines are covered

mmtune/mm/tasks/mmtrainbase.py

50.0% of new lines are covered

@yhna940 yhna940 requested review from nijkah and KKIEEK May 31, 2022 06:49
@yhna940 yhna940 marked this pull request as draft June 1, 2022 04:31
@yhna940 yhna940 marked this pull request as ready for review June 2, 2022 07:32
@yhna940 yhna940 requested a review from nijkah June 2, 2022 07:32
@yhna940
Copy link
Contributor Author

yhna940 commented Jun 2, 2022

The test coverage increase will be carried out in another pull request.

yhna940 and others added 4 commits June 2, 2022 17:41
Co-authored-by: Hakjin Lee <nijkah@gmail.com>
Co-authored-by: Hakjin Lee <nijkah@gmail.com>
Co-authored-by: Hakjin Lee <nijkah@gmail.com>
@yhna940 yhna940 force-pushed the feature/refact-context branch from 0a76912 to ac692b3 Compare June 2, 2022 08:42
yhna940 and others added 2 commits June 2, 2022 17:43
Co-authored-by: Hakjin Lee <nijkah@gmail.com>
Co-authored-by: Hakjin Lee <nijkah@gmail.com>
@@ -15,10 +29,27 @@ def unwrap_regexp(value, regexp=WRAPPING_REGEXP):


@REWRITERS.register_module()
class BatchConfigPathcer:
class BatchConfigPathcer(BaseRewriter):
Copy link
Member

Choose a reason for hiding this comment

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

I think its name and docstring can be improved. I cannot get it what it does until now.

Copy link
Member

Choose a reason for hiding this comment

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

Adding an example may help.

@@ -33,10 +64,28 @@ def __call__(self, context: dict):


@REWRITERS.register_module()
class SequeunceConfigPathcer:
class SequeunceConfigPathcer(BaseRewriter):
Copy link
Member

@nijkah nijkah Jun 2, 2022

Choose a reason for hiding this comment

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

Same as BatchConfigPatcher,
it seems important to its main function is to interpret - in the configuration file.



@REWRITERS.register_module()
class PathJoinTrialId(BaseRewriter):
Copy link
Member

@nijkah nijkah Jun 2, 2022

Choose a reason for hiding this comment

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

How about AppendTrialIDtoPath?

from mmcv.runner.dist_utils import get_dist_info
from mmcv.runner.hooks.logger import LoggerHook
from torch import distributed as dist


@HOOKS.register_module()
class RayTuneLoggerHook(LoggerHook):
"""Logger hook for Ray Tune."""
Copy link
Member

@nijkah nijkah Jun 2, 2022

Choose a reason for hiding this comment

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

Suggested change
"""Logger hook for Ray Tune."""
"""MMCV Logger hook for Ray Tune."""



@TASKS.register_module()
class BaseTask(metaclass=ABCMeta):
"""Wrap the apis of target task."""
Copy link
Member

@nijkah nijkah Jun 2, 2022

Choose a reason for hiding this comment

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

The docstring seems inappropriate.
"""Base class to specify the target task."""


def set_args(self, args: Sequence[str]) -> None:
self.args = self.parse_args(args)
"""Parse and Set the args.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Parse and Set the args.
"""Parse and set the args.

context_manager = ContextManager(**status)
return context_manager(self.run)(*args, **kwargs)
def context_aware_run(self, searched_cfg: Dict, **kwargs) -> Any:
"""Gathers and refines the information received by users and Raytune to
Copy link
Member

@nijkah nijkah Jun 2, 2022

Choose a reason for hiding this comment

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

Suggested change
"""Gathers and refines the information received by users and Raytune to
"""Gather and refine the information received by users and Ray.tune to


@abstractmethod
def run(self, *args, **kwargs) -> None:
def run(self, *, args: argparse.Namespace, **kwargs) -> None:
"""The objective task.
Copy link
Member

Choose a reason for hiding this comment

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

Description seems inappropriate.

Just Run the target task.

searched_cfg,
backend='nccl',
**kwargs) -> None:
"""Gathers and refines the information received by users and Raytune to
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
"""Gathers and refines the information received by users and Raytune to
"""Gather and refine the information received by users and Ray.tune to

@yhna940 yhna940 requested review from nijkah and KKIEEK June 3, 2022 01:26
Copy link
Member

@nijkah nijkah left a comment

Choose a reason for hiding this comment

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

Looks Great to Me.

@yhna940 yhna940 merged commit 2462401 into master Jun 3, 2022
@nijkah nijkah deleted the feature/refact-context branch September 8, 2022 06:41
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.

3 participants