OmegaConf.to_container(..., throw_on_missing) keyword arg

[Jasha10]

This PR introduces a throw_on_missing keyword arg to the OmegaConf.to_container signature.

• MandatoryMissingValue exceptions are raised or suppressed depending on whether throw_on_missing is set (closes Feature: throw_on_missing keyword argument for OmegaConf.to_container #501).
• InterpolationToMissingValueError exceptions are raised or suppressed depending on whether throw_on_missing is set (closes Calling OmegaConf.to_container on interpolation-to-missing errors #727). As discussed in Calling OmegaConf.to_container on interpolation-to-missing errors #727, we will keep the current behavior for raising InterpolationResolutionError: they are raised if and only if OmegaConf.to_container is called with resolve=True.

The OmegaConf.to_object method uses throw_on_missing==True when calling into OmegaConf.to_container.

This PR replaces #503, which is outdated.

TODO:

• Update the docs (I'll write the docs after the discussion in Calling OmegaConf.to_container on interpolation-to-missing errors #727 is resolved)
• news fragment

[odelalleau]

Just a note that it's going to take me a little while to get through this one (due to other priorities).

[odelalleau]

Looks mostly good to me -- the main point to discuss is whether or not we want to allow interpolations to missing values when throw_on_missing is True

[omry]

Generally speaking, it's better to format exceptions at high level functions, otherwise you risk formatting them multiple times.

(e.g: in OmegaConf.to_container() or other high level functions calling _to_content()).

[Jasha10]

Hmm... If I do formatting in OmegaConf.to_container(), then I will not be able to set the $KEY properly (since the for loop iterating through keys happens in this lower-level _to_content function).

[omry]

Is this related to this change? raise MissingMandatoryValue("Missing mandatory value: $KEY") ?
Why not just use the key directly when raising the exception?

[Jasha10]

Is this related to this change?

Yes, I think it's related -- In the past, a MissingMandatoryValue would not have been raised.

Why not just use the key directly when raising the exception?

Something like this?

raise MissingMandatoryValue(f"Missing mandatory value: {key}")

My point was: if we call format_and_raise in OmegaConf.to_container instead of here in BaseContainer._to_content, then the format_and_raise function will not have the correct key passed in to it (since the high-level function does not know what key caused the exception).

[omry]

Something like this?

raise MissingMandatoryValue(f"Missing mandatory value: {key}")

Yes.
Depending on exactly what is going on here, you may need to pass the key from the outside or use the key from the offending node (node._key()).

[Jasha10]

I tried it in this commit on a separate branch (which is based off of this PR branch).
The tests pass, but we are losing some information in the error messages. For example, here is the error before the commit:

>>> OmegaConf.to_object(OmegaConf.create({"a": {"b": "???"}}))
Traceback (most recent call last):
...
omegaconf.errors.MissingMandatoryValue: Missing mandatory value: b
    full_key: a.b
    object_type=dict

and here is the error after the commit:

>>> OmegaConf.to_object(OmegaConf.create({"a": {"b": "???"}}))
Traceback (most recent call last):
...
omegaconf.errors.MissingMandatoryValue: Missing mandatory value: b
    full_key:
    object_type=dict

There is no longer information about the full_key that caused the error.

Generally speaking, it's better to format exceptions at high level functions, otherwise you risk formatting them multiple times.

Is it really that bad to format multiple times?

[omry]

1. Update the PR to indicate that issue(s) it's closing.
2. I think we should be able to get the docs and news fragment already.

[omry]

Unsubscribing for now, please request review when ready.

[omry]

Something like this?

raise MissingMandatoryValue(f"Missing mandatory value: {key}")

Yes.
Depending on exactly what is going on here, you may need to pass the key from the outside or use the key from the offending node (node._key()).

[omry]

You can try this here:

            raise MissingMandatoryValue("Missing mandatory value: '{value._key()}'")

[Jasha10]

See my comment above.

[omry]

This is adding almost 200 lines to an already huge test file.
We should refactor this file somehow at some point.

[odelalleau]

One minor typo in the doc

[odelalleau]

Good on my side, thanks @Jasha10!

[omry]

lgtm, thanks.