omry / omegaconf

Flexible Python configuration system. The last one you will ever need.
BSD 3-Clause "New" or "Revised" License
1.91k stars 104 forks source link

Validation error merging structured configs containing enum in a nested list #1095

Open edlanglois opened 1 year ago

edlanglois commented 1 year ago

Describe the bug When merging into a structured config with a field of type List[List[SomeEnum]], any enums in the second (non-structured) config fail validation with

omegaconf.errors.ValidationError: Value 'SOME_VALUE' (str) is incompatible with type hint 'SomeEnum'

This does not occur for direct use of enums (value: SomeEnum) or non-nested lists (value: List[SomeEnum]).

To Reproduce

import enum
from dataclasses import dataclass, field
from typing import List

from omegaconf import OmegaConf

class MyEnum(enum.Enum):
    X = 1
    Y = 2

@dataclass
class Config:
    bare: MyEnum = MyEnum.X
    list: List[MyEnum] = field(default_factory=lambda: [MyEnum.X])
    list_list: List[List[MyEnum]] = field(default_factory=lambda: [[MyEnum.X]])

print("Base")
base = Config()
print(OmegaConf.to_yaml(base))

print("No nested list")
no_nested_list = OmegaConf.create({"bare": "Y", "list": ["Y"]})  # OK
print(OmegaConf.to_yaml(no_nested_list))

print("Merged without nested list")
print(OmegaConf.to_yaml(OmegaConf.merge(base, no_nested_list)))  # OK

print("Nested list")
nested_list = OmegaConf.create({"list_list": [["Y"]]})  # OK
print(OmegaConf.to_yaml(nested_list))

print("Merged with nested list")
print(OmegaConf.to_yaml(OmegaConf.merge(base, nested_list)))  # ValueError

Expected behavior I expect the merge of base and nested_list to complete successfully, resulting in the following structure:

bare: X
list:
- X
list_list:
- - 'Y'

instead I get

omegaconf.errors.ValidationError: Value 'Y' (str) is incompatible with type hint 'MyEnum'
    full_key: list_list[0]
    reference_type=List[List[MyEnum]]
    object_type=list

Additional context

Jasha10 commented 1 year ago

Thanks for the report, @edlanglois.

It is indeed inconsistent that merge(base, no_nested_list) works and merge(base, nested_list) gets rejected. What you've put your finger on is that merge semantics for nested containers do not perform type coercion aggressively.

>>> @dataclass
... class Nested:
...  l: list[list[str]]
...
>>> OmegaConf.merge(Nested, {"l": [[2]]})  # rejects `2` as not a string
Traceback ...

>>> @dataclass
... class Shallow:
...  l: list[str]
...
>>> OmegaConf.merge(Shallow, {"l": [2]})  # coerces `2` to a string
{'l': ['2']}

Motivation for this design decision was that aggressive coercion might be unwanted (i.e. a ValidationError might be more desirable if the data are mistyped).

Your writeup also highlights the case of coercion from str to Enum. It might make sense to add a special case handling for str -> Enum coercion in nested containers since .yaml files don't support enum values natively. @omry, what do you think?

edlanglois commented 1 year ago

Thanks for explaining the underlying cause. Yes, my issue is that I'm unable to merge a .yaml config into the nested structured config.

omry commented 1 year ago

Your writeup also highlights the case of coercion from str to Enum. It might make sense to add a special case handling for str -> Enum coercion in nested containers since .yaml files don't support enum values natively. @omry, what do you think?

I am open to a fix in that direction. Ideally it would not be handled as a special condition though.

Jasha10 commented 1 year ago

Ideally it would not be handled as a special condition though.

What do you have in mind? Enabling type coercion fully within nested containers?

omry commented 1 year ago

I think so.