omry / omegaconf

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

OmegaConf.update AssertionError on key pointing into None-valued DictConfig. #1035

Open Jasha10 opened 1 year ago

Jasha10 commented 1 year ago

Describe the bug The OmegaConf.update method throws an AssertionError when the key argument passed to OmegaConf.update points inside of a DictConfig instance whose _content is None.

To Reproduce

from dataclasses import dataclass
from typing import Optional
from omegaconf import OmegaConf
from pytest import raises

@dataclass
class Child:
    x: int = 123
    y: int = 456

@dataclass
class Parent:
    child: Optional[Child] = None

cfg = OmegaConf.structured(Parent)

with raises(AssertionError, match="Unexpected type for root: NoneType"):
    OmegaConf.update(cfg=cfg, key="child.x", value=789)

Expected behavior I think OmegaConf should fail gracefully (or maybe not fail at all) in this situation rather than throwing an AssertionError.

Additional context

Edit:

Here is the full traceback: ```text $ OC_CAUSE=1 python repro2.py Traceback (most recent call last): File "/home/homestar/dev/mrepo/pysc/repro2.py", line 17, in OmegaConf.update(cfg=cfg, key="child.x", value=789) File "/home/homestar/miniconda3/envs/pysc/lib/python3.10/site-packages/omegaconf/omegaconf.py", line 709, in update assert isinstance( AssertionError: Unexpected type for root: NoneType ```
Jasha10 commented 1 year ago

OmegaConf.merge does not exhibit the same failure mode.

-- Click here for an example -- ```python from dataclasses import dataclass from typing import Optional from omegaconf import OmegaConf @dataclass class Child: x: int = 123 y: int = 456 @dataclass class Parent: child: Optional[Child] = None cfg = OmegaConf.structured(Parent) other = {"child": {"x": 789}} out = OmegaConf.merge(cfg, other) assert OmegaConf.get_type(out.child) is Child assert out == {"child": {"x": 789, "y": 456}} ```

I feel that OmegaConf.update's behavior should be consistent with that of OmegaConf.merge: calling update should result in a node out.child that is a non-None structured config backed the Child class.

omry commented 1 year ago

I feel that OmegaConf.update's behavior should be consistent with that of OmegaConf.merge: calling update should result in a node out.child that is a non-None structured config backed the Child class.

Dunno, that would be an insert, not an update.