Fix model_wrapper_cfg with pure python style config
#25
Conversation
Documentation build overview
Show files changed (5 files in total): 📝 5 modified | ➕ 0 added | ➖ 0 deleted
|
|
It looks like the code calling the registry passes an instantiated class instead of the name of the class that later gets instantiated. This breaks the assumption that all objects are instantiated by the builder (and thus that type should be a string and not a class). I would say this is not a bug in onedl-mmengine but in the way the other library uses onedl-mmengine |
MGAMZ
force-pushed
the
dev/pythonlike-modelwrapper
branch
from
c016e2a to
2575aeb
Compare
|
@lauriebax I was debugging this issue and mistakenly created a PR to onedl instead of my own fork. LOL. Thank you for your helpful and insightful suggestion. I hereby provide a more detailed description of this PR. Looking forward to your further reply. IntroductionWhether it is the original OpenMM or the current OneDL, mmengine has planned to use Python-style configuration files. As is described in Config Doc.
How to ReproduceFor most usage scenarios, compatibility improvements have been introduced. Introducing the Python-style type in the common configuration items is not a problem. However, for some less commonly used configuration items, problems may arise. In this PR, the following two items are included:
Manually configuring either items will cause errors during runner startup, here is an example: from mmengine.model.wrappers import MMDistributedDataParallel
... # Other configs
model_wrapper_cfg = dict(type=MMDistributedDataParallel)
... # Other configsWhy?It is not a problem with It's because in some scenarios, we need to obtain the actual class of the type specified in the configuration dict before the registry doing instantiation in As a workaround, I discover that in multiple places throughout this repository, the code first checks the type of the type value in the configuration dictionary and only calls @staticmethod
def build_dataloader(dataloader: Union[DataLoader, Dict],
seed: Optional[int] = None,
diff_rank_seed: bool = False) -> DataLoader:
...
if 'worker_init_fn' in dataloader_cfg:
worker_init_fn_cfg = dataloader_cfg.pop('worker_init_fn')
worker_init_fn_type = worker_init_fn_cfg.pop('type')
if isinstance(worker_init_fn_type, str):
worker_init_fn = FUNCTIONS.get(worker_init_fn_type)
elif callable(worker_init_fn_type):
worker_init_fn = worker_init_fn_type
else:
raise TypeError(
'type of worker_init_fn should be string or callable '
f'object, but got {type(worker_init_fn_type)}')
assert callable(worker_init_fn)
init_fn = partial(
worker_init_fn, **worker_init_fn_cfg)
...if isinstance(collate_fn_cfg, dict):
collate_fn_type = collate_fn_cfg.pop('type')
if isinstance(collate_fn_type, str):
collate_fn = FUNCTIONS.get(collate_fn_type)
else:
collate_fn = collate_fn_type
collate_fn = partial(collate_fn, **collate_fn_cfg)FixMy workaround follows the similar logic. The original code is implemented without protection when model_wrapper_type = MODEL_WRAPPERS.get(model_wrapper_cfg.get('type'))So adding a careful if logic: model_wrapper_type = model_wrapper_cfg.get('type')
if isinstance(model_wrapper_type, str):
model_wrapper_type = MODEL_WRAPPERS.get(
model_wrapper_type) # type: ignore
else:
assert isinstance(model_wrapper_type, type), 'type should be a string or a class'Further ThinkingI think |
MGAMZ
changed the title
model_wrapper_cfg with pure python style config
|
Thank you for the nice explanation of the issue. This makes reviewing very easy. I agree with the fix, although I prefer you use |
|
Yes, I may create one more PR for the other similar bug related to |
There was a problem hiding this comment.
Pull request overview
This PR fixes a bug that caused DDP training to fail when using pure Python style config for model_wrapper_cfg. The error occurred because Registry.get() expected a string but received a class object when users passed a class directly as the type value.
Key Changes:
- Added support for passing class objects directly as the
typevalue inmodel_wrapper_cfg, in addition to string-based references - Added
isclassimport from theinspectmodule to validate class types
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| assert isclass( | ||
| model_wrapper_type), 'type should be a string or a class' |
There was a problem hiding this comment.
The assertion should raise a TypeError instead of using an assert statement for better error handling. Assert statements can be disabled with Python optimization flags (-O), making this validation unreliable in production. Consider:
if not isclass(model_wrapper_type):
raise TypeError('type should be a string or a class')| assert isclass( | |
| model_wrapper_type), 'type should be a string or a class' | |
| if not isclass(model_wrapper_type): | |
| raise TypeError('type should be a string or a class') |
| model_wrapper_type = model_wrapper_cfg.get('type') | ||
| if isinstance(model_wrapper_type, str): | ||
| model_wrapper_type = MODEL_WRAPPERS.get(model_wrapper_type) | ||
| else: | ||
| assert isclass( | ||
| model_wrapper_type), 'type should be a string or a class' |
There was a problem hiding this comment.
The new functionality that allows passing a class directly as the type value (instead of a string) lacks test coverage. Consider adding a test case that validates this behavior:
# Example test case
cfg.model_wrapper_cfg = dict(type=CustomModelWrapper) # Pass class directly
runner = Runner.from_cfg(cfg)
self.assertIsInstance(runner.model, CustomModelWrapper)2 participants
Error when running DDP training
Another fork branch seems to not having this BUG: https://github.com/MGAMZ/mmengine/tree/dev/mgam
UNKNOWN reason, still debuging.
Possibly related to this commit: open-mmlab@91d945f