Workaround for Dataframe object in the config: check whether value is string before mandatory missing check

[jeffknaide]

Closes #566

sidestep source of facebookresearch/hydra#1285 at the omegaconf level

[omry]

Thanks @jeffknaide.
We have some outstanding discussions around the internal behavior of MISSING, we will address this once we comes to a conclusion there.

[omry]

Blocked by #543.

[omry]

Thanks @jeffknaide.
See inline comments.

[omry]

I find __eq__ returning self very odd. what's going on here?

[jeffknaide]

when a pandas DataFrame runs df.__eq__(val), it returns a DataFrame of booleans with the same shape as the original DataFrame. Here I was trying to mimic the behavior of a df comparison resulting in the same class being returned, as this is partially what caused issues originally. perhaps this should be made clearer with a comment?

[omry]

Gotcha.
Yes, add a comment what the behavior we are mimicking is.
I guess we can't mimic it without overriding both eq and bool?

[jeffknaide]

i don't think so -- if we don't override eq, the comparison to string would return a bool and then not raise the expected error on the conditional. eg if Dataframe() == "???" would equate to if False (which wouldn't raise the expected error) without the eq override.

[omry]

Add a comment here.
something like:

Emulates Pandas Dataframe equality behavior.

[omry]

lgtm.
@odelalleau, this could interact with your outstanding PR to change the semantics of missing.

[odelalleau]

lgtm.
@odelalleau, this could interact with your outstanding PR to change the semantics of missing.

At first glance I don't foresee any issue (crossing fingers). Just mentioning that maybe there could be a new utility function to avoid repeating if isinstance(x, str) and x == "???" everywhere?

[omry]

Yeah, was thinking that too.

[jeffknaide]

@odelalleau @omry oh, yes, that makes sense. I just added is_mandatory_missing to _utils and referenced it where i had been using if isinstance(x, str) and x == "???".

There was also an inner function defined in BaseContainer._resolve_with_default called is_mandatory_missing which was returned of get_value_kind. I just removed that inner function in favor of is_mandatory_missing defined in _utils. This seems to break the tests, though, so I must be missing something that's happening within get_value_kind

[odelalleau]

I must be missing something that's happening within get_value_kind

It’s almost certainly because that function can be called on a node: it starts with _get_value(..) to obtain the node’s value if that’s the case. So in BaseContainer the problem is likely related to if is_mandatory_missing(resolved):, and replacing ˋresolved` with its value here should fix it.

[omry]

I think we will want to take a closer look at this in a followup.
can you undo the refactoring changes?

[odelalleau]

can you undo the refactoring changes?

Most could be kept, only the change in _resolve_with_default() needs reverting

[omry]

Maybe. it just feels like there are a bit too many layers here. The logic can be slightly reorganized to be simpler.

[jeffknaide]

@omry - i just reverted previous commit. let me know if you want me to put the utils function back while leaving _resolve_with_default alone.

[omry]

Thanks @jeffknaide.