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’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

handle SCMode.INSTANTIATE with throw_on_missing=False #1104

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented Jul 16, 2023

This PR enables OmegaConf.to_container(..., throw_on_missing=False) to work for structured configs when structured_config_mode=SCMode.INSTANTIATE. Closes #1103.

In addition, this PR moves MISSING from omegaconf.py into base.py to avoid a circular import issue.

Here is an example of the behavior enabled by this PR:

# repro.py
from dataclasses import dataclass
from enum import Enum
import omegaconf
from omegaconf import OmegaConf

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

a = OmegaConf.create(A)
container = OmegaConf.to_container(
    a,
    throw_on_missing=False,
    structured_config_mode=omegaconf.SCMode.INSTANTIATE,
)

assert isinstance(container, A)
print(container)

BEFORE:

$ python repro.py
Traceback (most recent call last):
  File "/home/homestar/dev/omegaconf/repro.py", line 12, in <module>
    cfg = OmegaConf.to_container(
...
omegaconf.errors.MissingMandatoryValue: Structured config of type `A` has missing mandatory value: x
    full_key: x
    object_type=A

AFTER:

$ python repro.py
A(x='???')

@@ -378,7 +378,8 @@
def get_dataclass_data(
obj: Any, allow_objects: Optional[bool] = None
) -> Dict[str, Any]:
from omegaconf.omegaconf import MISSING, OmegaConf, _maybe_wrap
from omegaconf import MISSING, OmegaConf
from omegaconf.omegaconf import _maybe_wrap

Check notice

Code scanning / CodeQL

Cyclic import

Import of module [omegaconf.omegaconf](1) begins an import cycle.
@@ -38,7 +38,7 @@
is_structured_config_frozen,
type_str,
)
from .base import Box, Container, ContainerMetadata, DictKeyType, Node
from .base import MISSING, Box, Container, ContainerMetadata, DictKeyType, Node

Check notice

Code scanning / CodeQL

Cyclic import

Import of module [omegaconf.base](1) begins an import cycle.
),
)
else:
v = MISSING

Check warning

Code scanning / CodeQL

Variable defined multiple times

This assignment to 'v' is unnecessary as it is [redefined](1) before this value is used. This assignment to 'v' is unnecessary as it is [redefined](2) before this value is used.
@omry
Copy link
Owner

omry commented Jul 21, 2023

Looks good overall and I agree that it's a better behavior.
I think there is still some circular dependency.

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

Successfully merging this pull request may close these issues.

One should be allowed to re-instantiate a structured object with missing values
2 participants