OmegaConf.select raises an exception when selecting an invalid key in some cases

[omry]

#444 fixed an assertion error but the fix is inconsistent:
Generally, the contact of OmegaConf.select is that it returns None if the field is not found.
This fix is causing inconsistent behavior:

In [14]: cfg
Out[14]: {'a': 10, 'b': None}

In [15]: print(OmegaConf.select(cfg, "foo.bar"))
None

In [16]: print(OmegaConf.select(cfg, "b.bar"))
...
ConfigKeyError: Error trying to access b.bar: node `b` is not a container and thus cannot contain `bar`
    full_key: b.bar
    object_type=dict

I can see the value of raising an exception instead of returning None, but:

1. it should be optional to maintain backward compatibility (default to return None).
2. It should be consistent (always or never).

Adding support for it is potentially beyond the scope of fixing this bug.

[omry]

cc @odelalleau.

[odelalleau]

Yeah, the fix in #444 was for interpolations -- I didn't consider OmegaConf.select() at that time.

That being said, I'm not entirely sure this should be changed:

1. There is no significant backward compatibility concern since it wasn't working in 2.0 (it threw an AssertionError)
2. It's not exactly the same setting as a missing key: here, we are querying a non-container node for a key, which has no way to work

But if we keep it it probably shouldn't raise a ConfigKeyError -- ConfigTypeError would seem like a better fit.

I think the example below would be a stronger case to return None (currently it crashes with an AttributeError due to calling get() on None):

cfg = OmegaConf.create({"x": DictConfig(None)})
OmegaConf.select(cfg, "x.y")

(though, giving it more thought, I think it could also be argued that asking for a key of None should raise an exception -- I'm fine with either)

[omry]

The case that made me look at this was exactly the None case (see b in my example).
This is causing an unexpected issue in Hydra when a user is using +foo.bar=10 and foo is None.
A reasonable expecation is that + would force an update here (converting foo to a dictionary), but Hydra is trying to check if foo.bar exists (using OmegaConf.select()) to prevent accidental override, and it blows up with the mentioned error.

What do you think about OmegaConf.select(cfg, "foo.bar") returning None if foo is None?
I think I am okay with it raising an exception if foo is a ValueNode.

[odelalleau]

What do you think about OmegaConf.select(cfg, "foo.bar") returning None if foo is None?
I think I am okay with it raising an exception if foo is a ValueNode.

I'm ok with it. I would indeed prefer if it was only allowed for container nodes, but if it's somehow more convenient to also allow it for ValueNodes, I won't fight against it.