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.

Sign up for GitHub

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

Already on GitHub? Sign in to your account

Jump to bottom

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

Open
amatsukawa opened this issue Jul 14, 2023 · 3 comments · May be fixed by #1104
Open

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

amatsukawa opened this issue Jul 14, 2023 · 3 comments · May be fixed by #1104
Labels
bug Something isn't working

Comments

@amatsukawa
Copy link

amatsukawa commented Jul 14, 2023 •
edited by Jasha10
Loading

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:

omegaconf/omegaconf/basecontainer.py

Line 289 in 7dae67e

return conf._to_object()

and inside this function, it could be allowed here:

omegaconf/omegaconf/dictconfig.py

Line 741 in 7dae67e

if node._is_missing():

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.
@amatsukawa amatsukawa added the bug Something isn't working label Jul 14, 2023
@Jasha10
Copy link
Collaborator

Jasha10 commented Jul 15, 2023 •
edited
Loading

@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
Copy link
Author

amatsukawa commented Jul 15, 2023 via email

@Jasha10 Jasha10 linked a pull request Jul 16, 2023 that will close this issue
Draft
@Jasha10
Copy link
Collaborator

Jasha10 commented Jul 16, 2023 •
edited
Loading

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
bug Something isn't working
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

handle SCMode.INSTANTIATE with throw_on_missing=False Jasha10/omegaconf
2 participants
@amatsukawa @Jasha10