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

Support using variables in base config directly as normal variables. #1651

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

mzr1996
Copy link
Member

@mzr1996 mzr1996 commented Jan 10, 2022

Motivation

A config feature improvement draft. You can directly use the variables in the base config as normal variables virtually in the Python format config file.

Modification

Parse the base config before loading the entire config file. And use eval instead of import_module to load configs.

BC-breaking (Optional)

Should be no BC-breaking. It can pass the original unit tests. But please check it carefully.

Use cases (Optional)

base.py:

list_variable = [0, 1, 2]
tuple_variable = (0, 1)
dict_variable = dict(
    a=dict(first=1, second=2),
    b=dict(first=1, second=2)
)

final.py:

_base_ = './base.py'
_base_.list_variable[1] = 5
x, y = _base_.tuple_variable
dict_variable = dict(
    a=dict(third=3, _delete_=True),
    b=dict(third=3)
)
>>> from mmcv import Config
>>> cfg = Config.fromfile("final.py")
>>> print(cfg.pretty_text)
list_variable = [0, 5, 2]
tuple_variable = (0, 1)
dict_variable = dict(
    a=dict(third=3),
    b=dict(first=1, second=2, third=3)
)
x = 0
y = 1

Checklist

Before PR:

  • I have read and followed the workflow indicated in the CONTRIBUTING.md to create this PR.
  • Pre-commit or linting tools indicated in CONTRIBUTING.md are used to fix the potential lint issues.
  • Bug fixes are covered by unit tests, the case that causes the bug should be added in the unit tests.
  • New functionalities are covered by complete unit tests. If not, please add more unit test to ensure the correctness.
  • The documentation has been modified accordingly, including docstring or example tutorials.

After PR:

  • If the modification has potential influence on downstream or other related projects, this PR should be tested with some of those projects, like MMDet or MMCls.
  • CLA has been signed and all committers have signed the CLA in this PR.

@innerlee
Copy link
Contributor

Need to add test to show case the new features

with open(temp_config_file.name, 'r') as f:
content = f.read()
codeobj = compile(content, '', mode='exec')
eval(codeobj, base_cfg_dict, cfg_dict)
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a clever trick.

One thing to note is that the eval will introduce many things into the global dict. See the following example. Whether side effects will be caused by these extra variables should be double checked.

image

Copy link
Member Author

Choose a reason for hiding this comment

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

Right, these interval variables will be removed after merged in to the cfg_dict in line 272-276

        cfg_dict = {
            k: v
            for k, v in cfg_dict.items() if not k.startswith('__')
        }

@mzr1996
Copy link
Member Author

mzr1996 commented Jan 28, 2022

Need to add test to show case the new features

Done

@zhouzaida
Copy link
Collaborator

_base_ = './base.py'
list_variable[1] = 5
x, y = tuple_variable
dict_variable = dict(
    a=dict(third=3, _delete_=True),
    b=dict(third=3)
)

Does the usage of list_variable[1] = 5 confuse the users?

Would the follow design be more appropriate?

_base_ = './base.py'
_base_.list_variable[1] = 5
x, y = tuple_variable
dict_variable = dict(
    a=dict(third=3, _delete_=True),
    b=dict(third=3)
)

@mzr1996
Copy link
Member Author

mzr1996 commented Jan 29, 2022

Sounds good, I will try it.

@mzr1996
Copy link
Member Author

mzr1996 commented Jan 29, 2022

_base_ = './base.py'
list_variable[1] = 5
x, y = tuple_variable
dict_variable = dict(
    a=dict(third=3, _delete_=True),
    b=dict(third=3)
)

Does the usage of list_variable[1] = 5 confuse the users?

Would the follow design be more appropriate?

_base_ = './base.py'
_base_.list_variable[1] = 5
x, y = tuple_variable
dict_variable = dict(
    a=dict(third=3, _delete_=True),
    b=dict(third=3)
)

Done, and the usage and the unit tests are updated.

@zhouzaida zhouzaida requested a review from HAOCHENYE January 29, 2022 04:43
@innerlee
Copy link
Contributor

In the doc, don't forget to mark it as python-config-only feature.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants