OmegaConf.missing_keys to get a set of missing keys

[Ohad31415]

No description provided.

[Jasha10]

Closes #720

[omry]

As I said, this is on hold until we start working on 2.2. In the mean time:

1. Please add tests (including for list and mixing of lists and dicts).
2. Please add a news fragment
3. Please update the documentation.

[lgtm-com]

This pull request introduces 1 alert when merging 49b9d46 into 6c732c1 - view on LGTM.com

new alerts:

• 1 for Unused local variable

[Ohad31415]

@omry @Jasha10 I have a gap at the docs session, I've added a segment under docs/source/usage.rst for sphinx and seems like I'm missing something.
Would like to get a hand here 😀

[Jasha10]

From the doctest output:

File "usage.rst", line 791, in default
Failed example:
    OmegaConf.missing_keys({
        "foo": {"bar": "???"},
        "missing": "???",
         "list": ["a", None, "???"]
    })
Expected:
    {'list.2', 'foo.bar', 'missing'}
Got:
    {'missing', 'foo.bar', 'list.2'}

The elements of a set are not guaranteed to be printed in any particular order...
Here is something that should make the tests pass, though it is sacrifices clarity for the reader:

>>> sorted(OmegaConf.missing_keys({
...     "foo": {"bar": "???"},
...     "missing": "???",
...     "list": ["a", None, "???"]
... }))
['foo.bar', 'list.2', 'missing']

[Ohad31415]

From the doctest output:

File "usage.rst", line 791, in default
Failed example:
    OmegaConf.missing_keys({
        "foo": {"bar": "???"},
        "missing": "???",
         "list": ["a", None, "???"]
    })
Expected:
    {'list.2', 'foo.bar', 'missing'}
Got:
    {'missing', 'foo.bar', 'list.2'}

The elements of a set are not guaranteed to be printed in any particular order...
Here is something that should make the tests pass, though it is sacrifices clarity for the reader:

>>> sorted(OmegaConf.missing_keys({
...     "foo": {"bar": "???"},
...     "missing": "???",
...     "list": ["a", None, "???"]
... }))
['foo.bar', 'list.2', 'missing']

Yeah it does sacrifice readability, I wonder if there is a way around it to make it verify the output rather than the output's representation.

[omry]

Yeah it does sacrifice readability, I wonder if there is a way around it to make it verify the output rather than the output's representation.

assert ret == {'list.2', 'foo.bar', 'missing'}

[Jasha10]

I think that it would be appropriate to use the _ensure_container function here.

Suggested change

	if not isinstance(cfg, Container):
	try:
	cfg = OmegaConf.create(cfg)
	except ValidationError:
	raise ValueError(f"Could not create a config out of {cfg}")
	cfg = _ensure_container(cfg)

[omry]

Yes. once you do, you can also change your tests to take plain dict/list instead of a Container to simplfy.

[Ohad31415]

@omry The tests contain inputs of all types: ListConfig, DictConfig, plain list, dict. I think it doesn't hurt having it so, but if you think it's better with only plain lists/dicts I'll change it 😃

[omry]

Let's test everything with plain dict and list, but keep a single test for DictConfig and a single test for ListConfig (just for the top level element).

[Ohad31415]

👌 I've simplified it adding two tests at the end one with DictConfig and one with ListConfig.

[omry]

Please a news fragment file. This is similar to how it's done for Hydra.
Context here.

[omry]

Yes. once you do, you can also change your tests to take plain dict/list instead of a Container to simplfy.

[Ohad31415]

Please a news fragment file. This is similar to how it's done for Hydra.
Context here.

I've added news/720.feature. Let me know if something is wrong/missing.

[Jasha10]

A few docs updates:

[Jasha10]

Looks good to me! Thanks @Ohad31415.

[Jasha10]

I think we can now revisit this PR since work on OmegaConf 2.2 has started.

[Jasha10]

Made a merge commit to resolve conflicts with master branch.

[Jasha10]

Merged.
Thanks again @Ohad31415!