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

[bug] OmegaConf.merge should respect interpolations #184

Closed
apsdehal opened this issue Apr 3, 2020 · 4 comments · Fixed by #191
Closed

[bug] OmegaConf.merge should respect interpolations #184

apsdehal opened this issue Apr 3, 2020 · 4 comments · Fixed by #191

Comments

@apsdehal
Copy link

apsdehal commented Apr 3, 2020

from omegaconf import OmegaConf

first = OmegaConf.create({
    "a": {
        "aa": 1,
        "ab": 2
    },
    "b": "${a}"
})

second = OmegaConf.create({
    "b": {
        "ba": 1,
        "bb": 2
    }
})

third = OmegaConf.merge(first, second)


expected = OmegaConf.create({
   "a": {
        "aa": 1,
        "ab": 2
   },
   "b": { 
        "aa": 1,
        "ab": 2,
        "ba": 1,
        "bb": 2
    }       
})
assert third == expected
# throws AssertionError

Version: 2.0.0rc16

@omry omry added this to the OmegaConf 2.0 milestone Apr 3, 2020
@omry
Copy link
Owner

omry commented Apr 3, 2020

Thanks, I will take a look before 2.0 is released.

By the way, you can compare to primitive maps directly:

assert third == {
   "a": {
        "aa": 1,
        "ab": 2
   },
   "b": { 
        "aa": 1,
        "ab": 2,
        "ba": 1,
        "bb": 2
    }       
}

@omry
Copy link
Owner

omry commented Apr 3, 2020

What you expect is not what should happen.
Interpolation is a pointer.
You should end up with:

{
    "a": {
        "aa": 1,
        "ab": 2,
        "ba": 1,
        "bb": 2
    },
    "b": "${a}"
}

Which is equivalent but not the same.

@omry
Copy link
Owner

omry commented Apr 3, 2020

However, interpolations are not writethrough.
by the same logic, you would expect:

cfg = OmegaConf.create({
  "a":  10,
  "b" : "${a}"
})

cfg.b = 20

To result in

a: 20
b: ${a}

But this is useful: you can achieve the same result by just modifying a directly.

instead, this results in:

a: 10
b: 20

This is a conscience choice, and I don't want to make node interpolation inconsistent with this choice.

@apsdehal
Copy link
Author

apsdehal commented Apr 3, 2020

Regardless of pointer interpretation I would still expect Merge to respect interpolations. For the example you gave, I would say it is more of the choice on your end on how you would like to handle it in OmegaConf. Nevertheless, I have been thinking of interpolations as pass by value rather than pass by reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants