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

OmegaConf.resolve: handle custom resolvers returning dict/list #1093

Closed Jasha10 closed 1 year ago

Jasha10 commented 1 year ago

The OmegaConf.resolve function was erroring when used with a custom resolver that returns a dict or list.

Here's a minimal repro:

# repro.py
from omegaconf import OmegaConf

def my_resolver():
    return {"first": 1, "second": 2}

OmegaConf.register_new_resolver("my_resolver", my_resolver)

yaml = """
test: ${my_resolver:}
"""

config = OmegaConf.create(yaml)
OmegaConf.resolve(config)  # raises omegaconf.errors.UnsupportedValueType
$ python repro.py
Traceback (most recent call last):
  File "/home/homestar/dev/omegaconf/repro.py", line 14, in <module>
    OmegaConf.resolve(config)  # raises omegaconf.errors.UnsupportedValueType
  File "/home/homestar/dev/omegaconf/omegaconf/omegaconf.py", line 785, in resolve
    omegaconf._impl._resolve(cfg)
  File "/home/homestar/dev/omegaconf/omegaconf/_impl.py", line 40, in _resolve
    _resolve_container_value(cfg, k)
  File "/home/homestar/dev/omegaconf/omegaconf/_impl.py", line 23, in _resolve_container_value
    node._set_value(_get_value(resolved))
  File "/home/homestar/dev/omegaconf/omegaconf/nodes.py", line 46, in _set_value
    self._val = self.validate_and_convert(value)
  File "/home/homestar/dev/omegaconf/omegaconf/nodes.py", line 76, in validate_and_convert
    return self._validate_and_convert_impl(value)
  File "/home/homestar/dev/omegaconf/omegaconf/nodes.py", line 154, in _validate_and_convert_impl
    raise UnsupportedValueType(
omegaconf.errors.UnsupportedValueType: Value 'dict' is not a supported primitive type

Closes #1092.

omry commented 1 year ago

lgtm, please fix the lint issue.

omry commented 1 year ago

Also, what's with the circular import warnings?

Jasha10 commented 1 year ago

I've rebased to fix the lint.

Also, what's with the circular import warnings?

It's just that _impl.py imports from _utils.py and _utils.py imports from _impl.py.

The warning comes from a CodeQL CI workflow added in PR #1029. I'm not sure how to disable the circular import warning specifically.

omry commented 1 year ago

It's just that _impl.py imports from _utils.py and _utils.py imports from _impl.py.

The warning comes from a CodeQL CI workflow added in PR #1029. I'm not sure how to disable the circular import warning specifically.

Why is utils importing from impl.py? (and where is it happening?) It feels like utils should be low level helpers that do not depend on implementation details.

Jasha10 commented 1 year ago

Oh, I guess _util isn't importing from _impl directly. My guess is that the lint is referring to import statements within function calls, e.g.:

omry commented 1 year ago

Ah. Those functions do seem like they belong higher up the import stack.

Jasha10 commented 1 year ago

Hmm, think we should move them to _impl.py? Edit: It looks like Hydra imports several of these functions directly from _utils.