Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Feature Request] instantiate(..., _convert_="object") to integrate with OmegaConf.to_object #1719

Closed
Jasha10 opened this issue Jul 17, 2021 · 2 comments 路 Fixed by #2451
Closed
Labels
enhancement Enhanvement request priority_low
Milestone

Comments

@Jasha10
Copy link
Collaborator

Jasha10 commented Jul 17, 2021

馃殌 Feature Request

It would be nice to combine the benefits of hydra.utils.instantiate with OmegaConf.to_object.

Motivation

Currently users of Hydra have several ways to recursively convert OmegaConf objects (DictConfig and ListConfig) into pure python objects:

  • OmegaConf.to_container converts OmegaConf objects into nested dicts and lists.
  • hydra.utils.instantiate looks for _target_ fields nested inside an OmegaConf object; _target_ is used to name a specific class to instantiate (or a specific function to call) using the config's fields as parameters.
  • OmegaConf.to_object recursively converts OmegaConf objects to dicts and lists, giving special treatment to structured configs (which are instantiated as actual instances of the underlying dataclass or attrs class).

The motivation for this feature request has to do with combining structured configs and _target_ keywords in the same config object. Consider the following example:

# schemas.py
from dataclasses import dataclass
from typing import Any

@dataclass
class Structured:
    x: int

class UnStructured:
    y: int

    def __init__(self, y: int):
        self.y = y

@dataclass
class MainConf:
    s: Structured
    u: Any
# main.py
from omegaconf import DictConfig, OmegaConf
from hydra.utils import instantiate
from schemas import MainConf, Structured, UnStructured

cfg = OmegaConf.create(
    MainConf(s=Structured(x=123), u={"_target_": "schemas.UnStructured", "y": 456})
)

# Neither `instantiate` nor `OmegaConf.to_object` gets the job done:

obj = OmegaConf.to_object(cfg)
assert isinstance(obj, MainConf)
assert isinstance(obj.s, Structured)
assert isinstance(obj.u, dict)  # field `u` is not converted to `UnStructured`

inst = instantiate(cfg)
assert isinstance(inst, DictConfig)  # top-level config is not converted to `MainConf`
assert isinstance(inst.s, DictConfig)  # field `s` is not converted to `Structured`
assert isinstance(inst.u, UnStructured)

A Workaround

Combining OmegaConf.to_object with hydra.utils.instantiate often works as expected:

obj_inst = OmegaConf.to_object(instantiate(cfg))
assert isinstance(obj_inst, MainConf)
assert isinstance(obj_inst.s, Structured)
assert isinstance(obj_inst.u, UnStructured)

Edge Cases

There are some edge cases that are not handled so well by to_object(instantiate(...)):

For example things do not work as expected if the _target_-config takes a structured config as an argument:

class UnStructured2:
    y: int
    z: Structured
    def __init__(self, y: int, z: Structured):
        self.y = y
        self.z = z
from schemas import UnStructured2
cfg2 = OmegaConf.create(
    MainConf(s=Structured(x=123), u={"_target_": "schemas.UnStructured2", "y": 456, "z": Structured(x=789)})
)
# cfg2 = OmegaConf.create({"_target_": "schemas.UnStructured2", "y": 456, "z": Structured(x=123)})
obj_inst2 = OmegaConf.to_object(instantiate(cfg2))
assert isinstance(obj_inst2, MainConf)
assert isinstance(obj_inst2.u, UnStructured2)
assert isinstance(obj_inst2.u.y, int)
assert isinstance(obj_inst2.u.z, Structured)  # Assertion Error: actual type is DictConfig

Another edge case: calling to_object(instantiate(...)) does not work if there is a _target_ keyword at the top-level of the config object:

from pytest import raises
cfg3 = OmegaConf.create({"_target_": "schemas.UnStructured", "y": 456})
with raises(ValueError):
    OmegaConf.to_object(instantiate(cfg3))
    # "ValueError: Input cfg is not an OmegaConf config object (UnStructured)"

Pitch

I propose expanding the _convert_ parameter to instantiate with a new "object" value, so that calling

instantiate(cfg, _convert_="object")

is almost equivalent to

OmegaConf.to_object(instantiate(cfg))

with special handling of the above edge-cases.

Since calling OmegaConf.to_object(instantiate(cfg)) handles almost all use-cases, I expect that this feature request will not be of high priority.

@Jasha10 Jasha10 added the enhancement Enhanvement request label Jul 17, 2021
@omry
Copy link
Collaborator

omry commented Jul 21, 2021

Thanks Jasha.
Using to_object as a conversion strategy is something worth consider for the next version of Hydra.

As you are saying, it does not seem high pri at the moment but we can look at it more closely for the next version.

@Jasha10
Copy link
Collaborator Author

Jasha10 commented Sep 21, 2021

One design question to ask in this context:
When _convert_="object", what if a structured config has a field named _target_?
My instinct is that the config should be instantiated (i.e. _target_ should get called) rather than the config being converted to an instance of the underlying dataclass/attrs class.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhanvement request priority_low
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants