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

Check enum values against strings if enum key fails #1083

Closed agkphysics closed 1 year ago

agkphysics commented 1 year ago

Is your feature request related to a problem? Please describe. When using strings as enum items, only the key is compared, not the value:

from dataclasses import dataclass
from enum import Enum
from omegaconf import OmegaConf

class Val(Enum):
    VALX = "tall"
    VALY = "short"

@dataclass
class Config:
    val: Val = Val.VALX

config = OmegaConf.structured(Config)
config = OmegaConf.merge(config, OmegaConf.create({"val": "VALX"}))  # fine
config = OmegaConf.merge(config, OmegaConf.create({"val": "tall"}))  # error

Describe the solution you'd like Using the string enum value instead of the key would be nice. In these lines, if it raises a KeyError, perhaps it could check enum_type(value) as well. It already does this if value is an integer. https://github.com/omry/omegaconf/blob/71f833b6046f0b514530ed791c57080287fa0bb0/omegaconf/nodes.py#L487-L491

The main issue with this approach would be if the enum key->value mapping is not one-to-one. So it would need to be checked. Perhaps that's too impractical.

Additional context My particular use case was where the enum value began with a number, so the key had to begin with an underscore. So I had to specify the underscore in the config.

Jasha10 commented 1 year ago

if it raises a KeyError

So as for the case where there's both an enum key and an enum value that match:

class Val(Enum):
    A = "B"
    B = "C"

In this case, merge(config, create({"val": "B"})) would give {"val": Val.B}, not {"val": Val.A}.

omry commented 1 year ago

In some cases, this would be a breaking change. Not sure this is a big enough problem to justify a fix. Per Python docs, the majority of Python enums are using integer values.

agkphysics commented 1 year ago

Yeah, it's probably too much hassle for what it's worth and might cause other issues. There are other ways around the issue.