Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
2 tasks done
ppwwyyxx opened this issue May 30, 2024 · 2 comments
Open
2 tasks done

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

ppwwyyxx opened this issue May 30, 2024 · 2 comments
Labels
bug Something isn't working

Comments

@ppwwyyxx
Copy link

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

  • OmegaConf version: 2.3.0
  • Python version: 3.12.3
@ppwwyyxx ppwwyyxx added the bug Something isn't working label May 30, 2024
@Daraan
Copy link

Daraan commented Jun 3, 2024

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
Copy link
Author

ppwwyyxx commented Jun 3, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants