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

One should be allowed to re-instantiate a structured object with missing values #1103

Open amatsukawa opened 1 year ago

amatsukawa commented 1 year ago

Defining dataclasses to introduce schema seems like a great idea. But, sometimes we have missing values, and there is omegaconf.MISSING to express this. However:

from omegaconf import OmegaConf, MISSING
import omegaconf
from dataclasses import dataclass

@dataclass(frozen=True)
class A:
    x: int = MISSING

# If we are allowed to instantiate this
a = OmegaConf.create(a)
print(a)
# ==> {'a': {'x': '???'}}

# Then we should be allowed to instantiate it again:
OmegaConf.to_container(
    a, 
    throw_on_missing=False, 
    structured_config_mode = omegaconf.SCMode.INSTANTIATE
)
# ...
# MissingMandatoryValue: Structured config of type `A` has missing mandatory value: x
#     full_key: x
#     object_type=A

The use case is we may have partially filled objects that we want to manipulate, construct new configs out of, etc.

The problem is that this line forgets .to_container's throw_on_missing argument:

https://github.com/omry/omegaconf/blob/7dae67e4a3869b584fd6d6408b2f916c176755db/omegaconf/basecontainer.py#L289

and inside this function, it could be allowed here:

https://github.com/omry/omegaconf/blob/7dae67e4a3869b584fd6d6408b2f916c176755db/omegaconf/dictconfig.py#L741

It's currently hard for me logistically to contribute the simple PR, but hopefully I have identified the changes necessary sufficiently if the maintainers agree this should be supported.

Jasha10 commented 1 year ago

@amatsukawa what do you expect the output of OmegaConf.to_container to be in your example above? Do you want the output to be equal to A(x="???")? Note that such output would contradict the type hint given in the definition of A, potentially causing problems if your program assumes that the type hints are correct.

amatsukawa commented 1 year ago

Thanks for the response! Yes, that’s what I hope to get back.

I agree that it violates the type annotation. But we’ve already given up on that in the initial construction of A() with MISSING as the default value, before creating the structured DictConfig.

I wish to be able to convert to AND from template dataclasses with MISSING fiends and structured DictConfigs. Right now I can only go in one direction but I can’t come back.

On Sat, Jul 15, 2023 at 3:37 AM Jasha Sommer-Simpson < @.***> wrote:

@amatsukawa https://github.com/amatsukawa what do you expect the output of OmegaConf.to_container to be in your example above? Do you want the output to be equal to A("???")? Note that such output would violate the type hint given in the definition of A.

— Reply to this email directly, view it on GitHub https://github.com/omry/omegaconf/issues/1103#issuecomment-1636699467, or unsubscribe https://github.com/notifications/unsubscribe-auth/AADRHQNEA2FUKIMEHZB7EXTXQJCE5ANCNFSM6AAAAAA2K2R3XE . You are receiving this because you were mentioned.Message ID: @.***>

Jasha10 commented 1 year ago

But we’ve already given up on that in the initial construction of A() with MISSING as the default value

Good point.

hopefully I have identified the changes necessary sufficiently if the maintainers agree this should be supported.

Thanks for identifying what changes are necessary. I've created draft PR #1104 which implements the change. @omry, if you get the chance, could you please give an opinion on whether this behavior change makes sense?