Calling OmegaConf.to_container on interpolation-to-missing errors

[Jasha10]

The behavior of the OmegaConf.to_container method has changed with respect to interpolation-to-missing nodes.

With OmegaConf v2.0.6, no exception is raised on encountering interpolation-to-missing:

>>> cfg = OmegaConf.create({"missing": "???", "x": "${missing}"})
>>> OmegaConf.to_container(cfg, resolve=True)
{'missing': '???', 'x': '???'}

Currently on the master branch, this no longer works: an InterpolationToMissingValueError is raised.
This behavior change resulted from #545.

[Jasha10]

I am currently working on a PR to close #501, introducing a throw_on_missing keyword arg to the OmegaConf.to_container signature. MandatoryMissingValue exceptions are raised or suppressed depending on whether that keyword arg is set.

From the perspective of API design, I think it makes sense that InterpolationToMissingValueError should also be raised or suppressed depending on whether throw_on_missing is set.

[omry]

This is a collateral damage from a breaking change. Not really a bug, more like something we failed to mention as a breaking change (we can add a news fragment for it now).

When introducing throw_on_missing, I agree that having it set to False should suppress the InterpolationToMissingValueError as well.

[odelalleau]

I actually disagree with to_container(..., resolve=True, throw_on_missing=False) allowing interpolations to missing values. The main reason is that this would be inconsistent with (at least) OmegaConf.select() where throw_on_missing=False doesn't allow this, and instead one should use throw_on_resolution_failure=False (resulting in a value of None).

This is because trying to resolve an interpolation to a missing value is now considered an error (instead of being considered as yielding a missing value). This is indeed a breaking change, and it should have probably been mentioned in another news item attached to #545 (@omry should I submit a PR for this?)

I would suggest not to try and support the example given at the top unless there is a good reason for it. And if we do want to support it, I think it shouldn't be with throw_on_missing=False. This may actually require revisiting some key decisions made in #545 so it should be considered carefully.

[Jasha10]

this would be inconsistent with (at least) OmegaConf.select() where throw_on_missing=False doesn't allow this, and instead one should use throw_on_resolution_failure=False

Good point.

I would suggest not to try and support the example given at the top unless there is a good reason for it.

My assumption was that most users of OmegaConf.to_container are in one of two categories:

• I don't want to check each value in the to_container output to make sure it's not equal to "???".
  Please raise an error to fail fast.
• I don't want any errors raised by to_container.
  Please replace missing values with "???" as necessary.

For users in the second category, missing values feel the same as interpolation-to-missing values.
But my categorization did not account for other possible resolution failures...

Anyway, I'd be fine with not supporting the example at the top. If we do decide to support it, one idea is to add a throw_on_interpolation_to_missing flag to the to_container signature, which would suppress InterpolationToMissingValueError but allow other subclasses of
InterpolationResolutionError to be raised.

This may actually require revisiting some key decisions made in #545 so it should be considered carefully.

Ok :)

[odelalleau]

But my categorization did not account for other possible resolution failures...

Yeah, if someone really doesn't want any error raised, probably that a "throw_on_resolution_failure_flag" would make more sense (and would also intercept InterpolationToMissingValueError -- that's already done in other functions using this flag).

Anyway, I'd be fine with not supporting the example at the top. If we do decide to support it, one idea is to add a throw_on_interpolation_to_missing flag to the to_container signature, which would suppress InterpolationToMissingValueError but allow other subclasses of
InterpolationResolutionError to be raised.

Yes, that's an option, though it starts making the API somewhat complex just to handle this special case in a special way that may not be consistent with the rest of the codebase.

Just to share one example why this might be an issue: I think that in general we want code based on some config cfg to behave the same if we convert cfg to a container (through to_container()) then back to a config (through OmegaConf.create()), i.e. replace cfg by OmegaConf.create(OmegaConf.to_container(cfg, throw_on_missing=False)).
Currently, if cfg is equal to {"x": "???", "y": "${x}"}, then OmegaConf.is_missing(cfg, "y") is False. However, if to_container() was converting y to a missing value, then after the round-trip conversion above this would become True.

[omry]

I think your last example is wrong. round trip conversion is incompatible with resolve=True.
If someone does not care about resolving (or want to be able to roundtrip the config) can just not pass a resolve flag and have it default to False.

Side note: Converting to container also loses all type safety, so it's not really round trip on that dimension as well.

My assumption was that most users of OmegaConf.to_container are in one of two categories:

• I don't want to check each value in the to_container output to make sure it's not equal to "???".
  Please raise an error to fail fast.
• I don't want any errors raised by to_container.
  Please replace missing values with "???" as necessary.

Currently, people that do not want any errors raised can use the default resolve=False.
Right now, resolve=True is resolving interpolations, but also raises on missing values.
Maybe the solution is to disentangle those two behaviors, making resolve apply to interpolations only and add a second flag to control the behavior for missing values. That flag could be true by default to maintain compatibility.

This would mean that while converting a container, to_container would resolve interpolations if and only if resolve is True. That might raise a resolution error if there is an interpolation to a missing value.
The second flag controls the behavior of to_container when it encounters a (direct) missing value. if it's true it raises, otherwise it returns "???".

Since we have 2 boolean flags, let's list all modes:

resolve=False, throw_on_missing=False

No interpolation resolution, interpolations are returned as strings.
Direct missing values are returned as "???".

resolve=False, throw_on_missing=True

No interpolation resolution, interpolations are returned as strings.
Direct missing values are raising MandatoryMissingValue.

resolve=True, throw_on_missing=False

Interpolations are resolved. Interpolation to missing would raise InterpolationToMissingValueError.
Direct missing values are returned as "???".

resolve=True, throw_on_missing=True

Interpolations are resolved. Interpolation to missing would raise InterpolationToMissingValueError.
Direct missing values are raising MandatoryMissingValue.

[odelalleau]

I think your last example is wrong. round trip conversion is incompatible with resolve=True.

Oh, yeah, I made it sound like the round-trip would return an equivalent object. That's definitely not true (at least interpolations & typing are lost).
What I meant to say is essentially that the more we can preserve in the round-trip, the better.

Since we have 2 boolean flags, let's list all modes:
(...)

As far as I can tell this is the current behavior from #730, up to the handling of interpolations to missing values (which is the point up for debate).
I agree with what you're proposing.

[Jasha10]

I agree with what you're proposing.

Ok, great! I'll update the implementation.

[Jasha10]

Closing this now since the current behavior for the resolve flag is desirable. #730 will not change this behavior.