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

[Fix] Fix the logic of setting lazy_import #1239

Merged
merged 6 commits into from
Jul 11, 2023

Conversation

Li-Qingyun
Copy link
Contributor

Failure case:

a config which is allowed by previous mmengine:

from mmdet.datasets import CocoDataset
... = dict(
    ...=dict(
        class_names_txt=CocoDataset.METAINFO['classes']
    )
)

# delete python style config to allow printing and dumping config
del CocoDataset

This is non-lazy_import, hence i provide a hack arg in train.py:

cfg = Config.from_file(arg.cfg, lazy_import=False)

But there is still auto lazy_import detected:
ad3c410a64868dec60df6cc26d9cfd3

I find auto detecting lazy_import maintains in _file2dict().
But If a user provide lazy_import=False, why auto detecting lazy_import again? If whether lazy_import is True or False, it must be equal to Config._is_lazy_import(filename), what is the meaning of lazy_import=False arg.

Solutions:

  1. also add lazy_import arg to _file2dict() (but _file2dict() seems customized for non-lazy_import, seems not pretty)
  2. delete the auto detecting (but may raise anothor error)
  3. add an self.lazy_import attribute.

@CLAassistant
Copy link

CLAassistant commented Jul 6, 2023

CLA assistant check
All committers have signed the CLA.

@HAOCHENYE HAOCHENYE requested a review from zhouzaida as a code owner July 6, 2023 17:27
zhouzaida
zhouzaida previously approved these changes Jul 11, 2023
@zhouzaida zhouzaida changed the title [Suggest] fix failure case of non-lazy_import [Fix] Fix the logic of setting lazy_import Jul 11, 2023
@zhouzaida zhouzaida merged commit de81d29 into open-mmlab:main Jul 11, 2023
8 of 9 checks passed
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