Add a support display for NoneType

[atranitell]

Hi, Thanks for the works making configuration easily. But in my situation, some values could be NoneType as a kind of flag to control the program, or it could also be appeared in the config file. e.g. a simple config.yaml:

INFO:
  NAME: example
  DIR: null

As I expected to control it by:

if _C.INFO.DIR is None:
  something

And I expected that the program prints it by calling print(cfgNode)

INFO:
  NAME: example
  DIR: None

Thanks, KJ

[rbgirshick]

This seems reasonable to me and I don't think it will cause any issues.

@ppwwyyxx what do you think? Can you foresee any situations in which allowing None might be a problem?

[ppwwyyxx]

Looks reasonable to me. The only possible break I can imagine is when users use cfg.get('key') to tell whether a key exists -- a common pattern with dict, but probably not common with CfgNode?

[atranitell]

Hi, Sorry for so late to test it. I write a test but I'm not sure whether it should be expected:

# demo.yaml like this:
# INFO:
#  NAME: example
#  DIR: null

cfg = yacs.config.CfgNode(new_allowed=True)
cfg.merge_from_file('demo.yaml')
print(cfg.get('INFO'))
print(cfg.get('INFO.DIR'))

# The first print display:
#     NAME: example
#     DIR: None

# and the second print without throwing error:
#     None

[rbgirshick]

I think we don't need to worry about the cfg.get("key") case because the behavior is the same as with a dict. If that dict is d1 = {"key": None} then d1.get("key") will return None, just like d2 = {} and d2.get("key") will return None.

@atranitell if you run the unit tests (via tox) you will find that test_load_cfg_invalid_type fails because it tests type validation by using a key that is None. Can you update that test case to use an invalid type?

[atranitell]

@rbgirshick Hi, it's indeed hard to find a case that does not belong to someone type appearing in:

{tuple, list, str, int, float, bool, type(None)}

Most of special situations will be converted to string type. Could you have any ideas about it?

[rbgirshick]

Since all primitive types are allowed (other than dict, which is converted to a CfgNode and thus ok), we will need to use a custom type that yaml.safe_load can load. I recommend something like this:

      def test_load_cfg_invalid_type(self):

          class CustomClass(yaml.YAMLObject):
              """A custom class that yaml.safe_load can load."""
              yaml_loader = yaml.SafeLoader
              yaml_tag = u'!CustomClass'

          # FOO.BAR.QUUX will have type CustomClass, which is not allowed
          cfg_string = "FOO:\n BAR:\n  QUUX: !CustomClass {}"
          with self.assertRaises(AssertionError):
              yacs.config.load_cfg(cfg_string)

[atranitell]

Awesome! It indeed is a custom type that fits the YAML format but throw an error. The unittest currenty display:

..WARNING:yacs.config:Deprecated config key (ignoring): MODEL.DILATION
.WARNING:yacs.config:Deprecated config key (ignoring): FINAL_MSG
WARNING:yacs.config:Deprecated config key (ignoring): MODEL.DILATION
.DEBUG:yacs.config:Invalid type <class 'object'> for key INVALID_KEY_TYPE; valid types = {<class 'int'>, <class 'str'>, <class 'float'>, <class 'list'>, <class 'tuple'>, <class 'NoneType'>, <class 'bool'>}
..DEBUG:yacs.config:Key FOO.BAR.QUUX with value <class '__main__.TestCfg.test_load_cfg_invalid_type.<locals>.CustomClass'> is not a valid type; valid types: {<class 'int'>, <class 'str'>, <class 'float'>, <class 'list'>, <class 'tuple'>, <class 'NoneType'>, <class 'bool'>}
.......DEBUG:yacs.config:Non-existent key: MODEL.DOES_NOT_EXIST
.......
----------------------------------------------------------------------
Ran 20 tests in 0.048s

OK

That I think there is no problem to merge into master. Thanks, KJ

[qizhuli]

With None allowed as a valid value, we might want to have the freedom to modify a key, whose default is None, to a non-None value during an experiment, or vice versa. However, this cannot be done currently due to the type matching requirement implemented in yacs.config._check_and_coerce_cfg_value_type.

Example:

import yacs.config
# Generate a dummy config as the default config; Note that default value of QUUX is None
default_cfg_string = "FOO:\n BAR:\n  QUUX:"
default_cfg = yacs.config.load_cfg(default_cfg_string)
# Dummy experiment setting; Note that we want to change QUUX to path/to/file
exp_cfg_string = "FOO:\n BAR:\n  QUUX: path/to/file"
exp_cfg = yacs.config.load_cfg(exp_cfg_string)
default_cfg.merge_from_other_cfg(exp_cfg)

...throws an error:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "yacs/yacs/config.py", line 217, in merge_from_other_cfg
    _merge_a_into_b(cfg_other, self, self, [])
  File "yacs/yacs/config.py", line 460, in _merge_a_into_b
    _merge_a_into_b(v, b[k], root, key_list + [k])
  File "yacs/yacs/config.py", line 460, in _merge_a_into_b
    _merge_a_into_b(v, b[k], root, key_list + [k])
  File "yacs/yacs/config.py", line 456, in _merge_a_into_b
    v = _check_and_coerce_cfg_value_type(v, b[k], k, full_key)
  File "yacs/yacs/config.py", line 513, in _check_and_coerce_cfg_value_type
    original_type, replacement_type, original, replacement, full_key
ValueError: Type mismatch (<class 'NoneType'> vs. <class 'str'>) with values (None vs. path/to/file) for config key: FOO.BAR.QUUX

Therefore, should we perhaps also loosen up the type matching requirement to make None useful in practice? A quick way to achieve so would be to add the following lines after L487 inside method yacs.config._check_and_coerce_cfg_value_type:

# If either of them is None, allow type mismatch
if replacement_type == type(None) or original_type == type(None):
    return replacement

With this proposed change, tox still maintains full success rate.

Happy to hear your thoughts on this.

[atranitell]

Hi,

@qizhuli , happy to receive the attention. Sorry to reply this comment for a long time due to heavy works. Basically, I think the suggest is reasonable for the latent semantic of NoneType.

But, considering a situation with two configuration.

FOO:
  BAR:
    QUUX: None

FOO:
  BAR:
    QUUX:
      QQ: None

A possible expectation is that the field of QUUX will be replaced by QQ: None, but the proposed method throw an error:

AssertionError: `b` (cur type <class 'NoneType'>) must be an instance of <class 'yacs.config.CfgNode'>

Here, I have a consideration, if the conversion between replacement and original type should only be allowed among basic type mentioned by _VALID_TYPES = {tuple, list, str, int, float, bool, type(None)}.

And if we could change it by:

if replacement_type in _VALID_TYPES and original_type in _VALID_TYPES:
    return replacement

Best, KJ

[qizhuli]

@atranitell Thank you for the response. I agree that we should allow conversion between None to one of the _VALID_TYPES.

However, further allowing conversion from any of the _VALID_TYPES to any of the _VALID_TYPES is probably too drastic a change that requires careful deliberation, and also out of scope for this PR. Therefore, I would suggest to modify the proposed if block in yacs.config._check_and_coerce_cfg_value_type to:

# If either of them is None, allow type conversion to one of the valid types
if (replacement_type == type(None) and original_type in _VALID_TYPES) \
        or (original_type == type(None) and replacement_type in _VALID_TYPES):
    return replacement

Happy to hear your thoughts on this.

[atranitell]

Hi qizhu,

Thanks for your suggestion. The conversion is in practice if it only constrains among basic types. I have added it into the PR. :)

Also, I think it is now reasonable to merge into the master. @rbgirshick , what do you think about this?

[rbgirshick]

Sorry for the latency. I'm ok with this now given the type conversion discussion and implementation. Before merging, can you rebase and then run

$ cd yacs
$ isort -rc -sl -t 1 --atomic yacs
$ black yacs

Thanks.

[qizhuli]

Hi @rbgirshick, are there other changes you would like to make to this PR?

[lijiaqi]

It is useful to add type(None) into _VALID_TYPES and allow the extra type conversions. Hope this PR can be merged ASAP. @rbgirshick

[DKandrew]

It is useful to add type(None) into _VALID_TYPES and allow the extra type conversions. Hope this PR can be merged ASAP. @rbgirshick

Agree. Now it is June of 2020. Hope this PR can be merged ASAP @rbgirshick