Dict getitem consistency

[omry]

Fixes #515.

This is an historical mistake. See #515 for some explanation and migration instructions.

[Jasha10]

Currently ListConfig._get_node gives the same behavior whether throw_on_missing_key is True or False (because the method does not make any use of this keyword argument).

[omry]

Yes, this is coming from the interface. there was no need yet to support it.
This is unrelated to this PR though, feel free to post a followup issue fixing it (there aren't a lot of uses of throw_on_missing yet, we may eventually just revert it is there isn't any need).

[odelalleau]

I agree it should be fixed, as there's currently an inconsistency between dicts and lists, because dict._get_node(foo) will return None if the key is missing, while list._get_node(foo) will crash if the index is invalid.

[omry]

_get_node is an internal API. As long as there is no external API that can expose an issue because of it I don't think it's particularly important.

The behavior of _get_node() in both cases is driven by the needs of OmegaConf. consistency between ListConfig and DictConfig is not an objective.

[odelalleau]

A few comments

[odelalleau]

I agree it should be fixed, as there's currently an inconsistency between dicts and lists, because dict._get_node(foo) will return None if the key is missing, while list._get_node(foo) will crash if the index is invalid.

[odelalleau]

I wonder if missing interpolations should have their own exception? (that would neither be a KeyError nor an AttributeError)

Reason is that people may use exception-based programming with the new API, to detect missing keys:

try:
    foo = cfg.foo
except AttributeError:
    foo = cfg.foo = 42
print(foo)

and if the AttributeError is raised because foo is referring to a missing interpolation, then bad things may happen.

[omry]

Why is that bad?
you accessed a field, it was pointing to something that is not there. you assign an actual value to it. It's no longer a pointer but a real value. underlying pointed to field is unchanged.

[odelalleau]

Someone writing such code most likely only intended to override cfg.foo if there was no such key in the config already. This could lead to subtle bugs if suddenly an interpolation gets broken (e.g., because a variable was renamed in the config), because instead of seeing an error the code will run just fine (ignoring the broken interpolation).

[omry]

Create a concrete example demonstrating a problem.
writing values on an interpolation is not writing through the interpolation.
Also, your example will not even work now because the exception raised is not an ConfigAttributeError but a ConfigKeyError right now:

cfg = OmegaConf.create({"a" : "${b}"})
cfg.a # ConfigKeyError
cfg["a"] # ConfigKeyError

and in any case writing on an interpolation is not write through:

In [11]: cfg
Out[11]: {'a': '${b}'}
In [12]: cfg.a = 10
In [13]: cfg
Out[13]: {'a': 10}

I am open to changes here but it's a different topic (and does not much much to do with standardizing with dict and object (given that they do not support interpolations).

[odelalleau]

Are you on master? I'm at d018678 and in your example cfg.a raises a ConfigAttributeError for me.

Here's an example illustrating what I have in mind: note how the typo in the interpolation makes it being silently ignored.

cfg = OmegaConf.create("""
default_ram: 32
super_computer_ram: 128

machines: [m1, m2, m3, m4, m5]

ram_per_machine:
    m3: 64
    m5: ${super_computerr_ram}
""")

# Ensure `cfg.ram_per_machine` contains RAM data for all machines.
ram_per_machine = cfg.ram_per_machine
for machine in cfg.machines:
    try:
        ram = getattr(ram_per_machine, machine)
    except AttributeError:
        ram = cfg.default_ram
        setattr(ram_per_machine, machine, cfg.default_ram)
    print(f"RAM for {machine}: {ram}")

print("\nConfig:\n")
print(OmegaConf.to_yaml(cfg, resolve=True))

The output:

RAM for m1: 32
RAM for m2: 32
RAM for m3: 64
RAM for m4: 32
RAM for m5: 32

Config:

default_ram: 32
super_computer_ram: 128
machines:
- m1
- m2
- m3
- m4
- m5
ram_per_machine:
  m3: 64
  m5: 32
  m1: 32
  m2: 32
  m4: 32

It's not a huge deal, I just thought I'd point it out (it wasn't a concern before because there were no exceptions to catch)

[Jasha10]

I wonder if missing interpolations should have their own exception? (that would neither be a KeyError nor an AttributeError)

So the idea is that each of the following would raise a different type of exception:

OmegaConf.create("").a  # raises ConfigAttributeError
OmegaConf.create("a: ${xyz}").a  # also raises ConfigAttributeError

[odelalleau]

@Jasha10 said it better than I could have (thank you!)

Again, I don't feel strongly about it, since it should be pretty rare (combination of exception-based programming + interpolation + user error) => just wanted to mention the idea of maybe using a different exeption, but up to you @omry

[Jasha10]

@Jasha10 said it better than I could have (thank you!)

:) my pleasure.

We already have

class InterpolationResolutionError(OmegaConfBaseException, ValueError):

defined in omegaconf.errors. Why not throw that one?
On the master branch, this InterpolationResolutionError is not being used anywhere else in the codebase.

[omry]

Ha, looks like I added it when I added relative interpolations but somehow never used it.

I am fine with using it to indicate an that an interpolation could not be resolved.

[Jasha10]

I've opened a PR (#523) to use InterpolationResolutionError for this purpose.