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

[CodeCamp2023-327]Control whether methods of the Visualizer are executed only in the main process through a parameter #1353

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

DeliMm
Copy link

@DeliMm DeliMm commented Sep 11, 2023

Motivation

Control whether methods of the Visualizer are executed only in the main process through a parameter.. See more details in #1081

Modification

To implement this function, we add a parameter 'img_only_master' to visualizer by all kinds of vis_backend. During runtime, it is determined whether to retain only the images in the GPU by checking if img_only_master is passed and examining its corresponding value. The modified master_only decorator is as following:

def master_only(func: Callable) -> Callable:
    """Decorate those methods which should be executed in master process.

    Args:
        func (callable): Function to be decorated.

    Returns:
        callable: Return decorated function.
    """

    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        if ('img_only_master' in kwargs
                and kwargs['img_only_master'] is False) or is_main_process():
            return func(*args, **kwargs)

    return wrapper

When we build the runner, we can follow this way to pass the img_only_master:

visualizer=dicttype='Visualizer',
             vis_backends=[
                 dict(
                     type='LocalVisBackend',
                     save_dir='./vis',
                     img_only_master=False)
             ]),

When we use the add_img method, we need pass the img_only_master through img_only_master=runner.visualizer.img_only_master

Checklist

  1. Pre-commit or other linting tools are used to fix the potential lint issues.
  2. The modification is covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  3. If the modification has potential influence on downstream projects, this PR should be tested with downstream projects, like MMDet or MMCls.
  4. The documentation has been modified accordingly, like docstring or example tutorials.

DeliMm and others added 3 commits September 11, 2023 12:44
…ted only in the main process through a parameter
…ted only in the main process through a parameter

update test_visualizer.py
@HAOCHENYE
Copy link
Collaborator

Coupling the implementation of master_only with the Visualizer is not a good practice. I believe we can avoid using the master_only decorator inside the Visualizer and instead opt for adding a master_only parameter to the Visualizer. Then, in various methods, we can check whether master_only is set to True and control whether to invoke the corresponding method only on rank 0.

class Visualizer
    def __init__(self, ..., master_only=True):
        ...
        self.master_only = master_only

    def xxx(self, ...):
        if _should_do():
            ...
    
    def _should_do(self):
        if self.master_only and get_rank() != 0:
            return False
        else:
            return True

@DeliMm
Copy link
Author

DeliMm commented Sep 21, 2023

Thank you for your response. I initially thought that I could only add 'master_only' without modifying the decorator. And I'll make the necessary adjustments as soon as possible.

mmengine/visualization/visualizer.py Outdated Show resolved Hide resolved
def dataset_meta(self, dataset_meta: dict) -> None:
"""Set the dataset meta info to the Visualizer."""
self._dataset_meta = dataset_meta
if self._should_do():
Copy link
Collaborator

Choose a reason for hiding this comment

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

We need to reconsider that if it is necessary to apply self._should_do here. I think we could skip this check here.

Copy link
Author

Choose a reason for hiding this comment

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

For this, I added the check based on the original master_only decorator for this function. For dataset_meta, it might be better to keep the original master_only decorator.

@HAOCHENYE
Copy link
Collaborator

Hi, please check the modification will not break the CI

DeliMm and others added 3 commits October 7, 2023 10:18
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

2 participants