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

Nested structured config validation #1019

Open
4 tasks done
djkawa opened this issue Oct 4, 2022 · 3 comments · May be fixed by #1133
Open
4 tasks done

Nested structured config validation #1019

djkawa opened this issue Oct 4, 2022 · 3 comments · May be fixed by #1133
Labels
bug Something isn't working

Comments

@djkawa
Copy link

djkawa commented Oct 4, 2022

Hi folks,

Not sure if it’s a bug or just not supported, I cannot opine for sure from the docs, feel free to clarify. I have noticed that the validation on structured configs does not go deeper than one level when using a nested dataclass.

from dataclasses import dataclass, field
from omegaconf import OmegaConf


@dataclass
class DeeplyNestedConf:
    an_int: int = 10
    a_string: str = "test"


@dataclass
class StructuredConf:
    single_nest: dict[str, DeeplyNestedConf] = field(default_factory=dict)
    double_nest: dict[str, dict[str, DeeplyNestedConf]] = field(default_factory=dict)


def test_nested_struct():
    yaml = """
    single_nest:
        level_1:
            an_int: 20
    double_nest:
        level_1:
            nested_level_1:
                an_int: 30
                a_string: this is not typed
            nested_level_2:
                an_int: 40
                a_typo: this does not fail
    """

    schema = OmegaConf.structured(StructuredConf())
    untyped = OmegaConf.create(yaml)
    typed = OmegaConf.merge(schema, untyped)
    structured_conf = OmegaConf.to_object(typed)  # type: StructuredConf
    assert isinstance(structured_conf.single_nest["level_1"], DeeplyNestedConf)  # Awesome!
    not_typed = structured_conf.double_nest["level_1"]["nested_level_1"]
    assert not isinstance(not_typed, DeeplyNestedConf)  # why not?
    assert isinstance(not_typed, dict)  # it’s just a dict :-(
    did_not_fail_validation = structured_conf.double_nest["level_1"]["nested_level_2"]
    assert not isinstance(did_not_fail_validation, DeeplyNestedConf)
    assert isinstance(did_not_fail_validation, dict)

While the first level of nesting works great, I was expecting the merge to fail because of the object named did_not_fail_validation. And even if the wrong yaml was fixed, I would have expected the not_typed object to be a DeeplyNestedConf.

Looking forward to knowing your thoughts.

Additional context

  • OmegaConf version: 2.2.3
  • Python version: 3.10.6
  • Operating system: win-64
  • Please provide a minimal repro
@djkawa djkawa added the bug Something isn't working label Oct 4, 2022
@Jasha10
Copy link
Collaborator

Jasha10 commented Oct 4, 2022

Not sure if it’s a bug or just not supported

This is a bug. Thanks very much for the report, @djkawa!

@bzczb
Copy link
Contributor

bzczb commented Aug 14, 2023

My attempt: omry:omegaconf:v2.3.0...bzczb:omegaconf:8e69077aca715f57d0f028c0dc1421d643ed8018
Be warned that I don't have a great understanding of this code, and one of the tests fails for "wrong error message" now.
If it looks OK I can PR it and try to figure that test out.

@Jasha10
Copy link
Collaborator

Jasha10 commented Aug 20, 2023

@bzczb your patch looks like a good start.

We'd want to (1) add tests based on the OP's example and (2) add tests to make sure that the _metadata field on the newly created DictConfig / ListConfig object are initialized correctly. My hunch is that, as the patch stands, the key_type and element_type on DictConfig and ListConfig are not set properly.

The "wrong error message" you mentioned might not be a big deal.

@bzczb bzczb linked a pull request Oct 22, 2023 that will close this issue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants