Support optional _parent_ and _root_ parameters in custom resolvers

[omry]

Custom resolvers can now receive optional parent and root parameters.

This is the doc example:

OmegaConf.register_new_resolver(
    "sum_friends",
    lambda a, b, *, _parent_: _parent_[a] + _parent_[b],
    use_cache=False,
)
cfg = OmegaConf.create(
    {
        "a": {
            "b": 1,
            "c": 2,
            "sum": "${sum_friends:b,c}",
        },
    }
)

assert cfg.a.sum == 3

[Jasha10]

Why write _node_ and not _parent_?

[omry]

To confuse the enemy, shhhh, the enemy is listening!

[odelalleau]

Just some minor doc tweaks (+ @Jasha10's comment)

[omry]

I think I responded to all the comments. Take a second look (not always exactly as requested).

[odelalleau]

Just a minor doc formatting issue

[odelalleau]

Great!

[omry]

The last PR I just merged is breaking one of the tests.
It's hard to debug because it gets into an infinite loop, but I suspect the problem is that the tuple(cfg, cfg) is being converted to a ListConfig which in turns causes some issues.

[omry]

I worked around it by not returning a tuple, but this is something we did not anticipate.

[omry]

It might make sense to allow control over the generation of config nodes from dicts, list and tuples returned by custom resolvers.
to avoid surprises like this one we might want to default to not generate.

Side note, this exposed a bug: the code before my fix got into an infinite loop. Not sure what this is about, worth digging further.

[odelalleau]

It might make sense to allow control over the generation of config nodes from dicts, list and tuples returned by custom resolvers.
to avoid surprises like this one we might want to default to not generate.

Side note, this exposed a bug: the code before my fix got into an infinite loop. Not sure what this is about, worth digging further.

That's interesting one. It's caused by a cycle in the parent -> child dependency graph. Minimum repro:

c = OmegaConf.create({"x": []})
c.x.append(c)
c.x

I'm working on a PR to detect such cycles in typical situations, to at least get an explicit error instead of an infinite loop. Regarding an actual fix, not sure if it'd be possible without major changes (like allowing multiple parents), but a couple of ideas (which I haven't fully thought through):

• When appending a node to a ListConfig (and possibly in other similar situations), don't set the parent if the node is already an ancestor of the ListConfig
• Or, create a wrapper node that would behave a bit like current interpolated nodes (could even treat them as another kind of interpoolation). The wrapper node would have the ListConfig as parent, but the underlying node's parent wouldn't be modified

[omry]

Yeah, I dug a bit as well and I think it's a pretty unique circumstances.
Essentially the object contains cycles which means it cannot be traversed. Normal traversal would cause a stack-overflow which is not that bad, but somehow this results in an infinite loop.

I didn't dug enough to see why this is happening, but my intuition matches what are describing.
Ideally this would result in an exception, but I think it's not a huge problem.

I have a handling for this situation here for some scenarios when adding a node that belongs to the same config.
I think a fix here can be similar but to check if a added node is in the parent chain of the node it's being added to.

Supporting two parents would break interpolation.

parent1:

node:
  foo: 10

parent2:

node:
  foo: 20

cfg1 = OmegaConf.create(parent1)
cfg2 = OmegaConf.create(parent2)
cfg3 = OmegaConf.create({"bar" : "${a.node.foo}"})
cfg1.a = cfg3 # cfg1 is now a parent
cfg2.a = cfg3 # cfg2 is now a parent too (?)

# At this point, what is the value of `cfg3.bar`?

[odelalleau]

Ideally this would result in an exception, but I think it's not a huge problem.

Good :) Because I gave up on my PR, it turns out there are too many intertwined functionalities that break when there is a cycle in the parents. The simple approach I started with is too limited, and other potential solutions I can think of would be trickier to implement, so I'm not going to address this now.

I have a handling for this situation here for some scenarios when adding a node that belongs to the same config.
I think a fix here can be similar but to check if a added node is in the parent chain of the node it's being added to.

The specific line you pointed too isn't super relevant though (I think), but what's around it is: the important point is that currently the node is deepcopied in _set_item_impl() by default. This happens in a ListConfig when calling c[idx] = node, but not in c.append(node). Is that intended?

Supporting two parents would break interpolation.

I'm sure it would break a lot of things :) Not going to suggest that we should get into that.

[omry]

The specific line you pointed too isn't super relevant though (I think), but what's around it is: the important point is that currently the node is deepcopied in _set_item_impl() by default. This happens in a ListConfig when calling c[idx] = node, but not in c.append(node). Is that intended?

Not intentional.
I think we can deepcopy an input Node (unless that flag I pointed to is set).
That might actually solve this issue (is append on the path here?)

[odelalleau]

That might actually solve this issue (is append on the path here?)

Yes. The path is _node_wrap() -> ListConfig.init() -> _set_value() -> _set_value_impl() -> append()

I created an issue to keep track of it: #601