omry / omegaconf

Flexible Python configuration system. The last one you will ever need.
BSD 3-Clause "New" or "Revised" License
1.99k stars 115 forks source link

Assignment `cfg.a = cfg.a` behaves unexpectedly #1177

Open ppwwyyxx opened 6 months ago

ppwwyyxx commented 6 months ago

Describe the bug

from omegaconf import DictConfig

x = DictConfig({"a": 1, "b": {"c": "hello"}})
b = x.b
x.b = x.b

b.c = 'x'
print(x.b.c)  # prints hello
assert b is x.b

Expected behavior Expect x.b.c == 'x' and b is x.b

The line x.b = x.b changes the identity of x.b. Perhaps it recreates a new dict from x.b during the assignment.

This is very unintuitive and unexpected to a user, because in the Python ecosystem the assignment operator typically doesn't behave like this. It could result in mistakes shown above: modification to b is not reflected in x.b.

Additional context

Daraan commented 5 months ago

Just some thoughts from a random user.

At one hand it is unintuitive and we want objects to be equal assigned like that, however I think its very problematic to change it underneath if we think about how the dicts are structured with children and parents

x = DictConfig({"a": "X", "b": {"c": "Im X"}})
y = DictConfig({"a": "Y", "b": {"c": "Im Y"}})

x.b = y.b

b.c (in x) needs to be a child of x b.y (in y) needs to stay a child in y as well making its parent ambiguous if we allow it to be the same object. Interpolation will become more intuitive and hard to traceback where something was picked from.

If c is some interpolation (e.g. a) to the top level this should hold: x.b is y.b and x.b.c != y.b.c, we could argue yah thats still okay they are the same interpolation string, however looking at this example, or the steps underneath it shows the problem:

y.b = x.b # assume the same object
b1 = x.b
b2 = y.b
assert b1 == b2 # is True

Is b2 now bound to y or to x ? b1 is b2 would be true but how to interpolate it? If they are the same object it must be b1.c == b2.c. The call of b2=y.b should not affect b1, the only solution would be for b2 to interpolate to x which would also be unintuitive as we call it from y.


TL;DR: I agree that excluding the creation of a new node in an identity assignment in the __setattr__ function is worth considering. For other cases it will cause problems.

x.b = x.b # change nothing underneath in the 

...

def __setattr__(self, key, other):
    if getattr(self, key) is other: return
ppwwyyxx commented 5 months ago

excluding the creation of a new node in an identity assignment in the setattr function is worth considering. For other cases it will cause problems.

I think that makes sense.