omry / omegaconf

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

Interpolation breaking change, scope of interpolation (is this intentional?) #690

Closed oliver-batchelor closed 3 years ago

oliver-batchelor commented 3 years ago

Describe the bug

I have some config file which looks like this (used from hydra). I'm trying out the latest hydra dev installed with --pre. (1.1.0dev5), with omegaconf (2.1.0dev24), I tried 25 as well and has the same behavior.

camera:
  rotation: [0, 90, 0]

  cameras:
    - name: cam1
      location: [0, 0.0, 0.26]
      rotation: ${camera.rotation}
    - name: cam2
      location: [0, 0.0, 0.26]
      rotation: ${camera.rotation}

It no longer finds camera.rotation. I'm not sure why? omegaconf.errors.InterpolationKeyError: Interpolation key 'camera.rotation' not found

Is this expected?

oliver-batchelor commented 3 years ago

I see the defaulting behavior has changed - will read the documentation more carefully. Thanks

omry commented 3 years ago

I am not sure I understand the issue you are reporting. Can you provide a minimal runnable repro showing the difference between the two versions? If that minimal repro requires Hydra, file it in the Hydra repo. if you can do it with OmegaConf only you can add it here.

I am expecting ${camera.rotation} in the above example to always work.

side note: 1.1 adds relative interpolation, you can now do:

camera:
  rotation: [0, 90, 0]

  cameras:
    - name: cam1
      location: [0, 0.0, 0.26]
      rotation: ${...rotation}
oliver-batchelor commented 3 years ago

There is no issue. I just upgraded from before 1.0 to 1.1 and didn’t realise the package directive was important.

Thanks for your great work!

On Tue, 20 Apr 2021 at 7:07 AM, Omry Yadan @.***> wrote:

I am not sure I understand the issue you are reporting. Can you provide a minimal runnable repro showing the difference between the two versions? If that minimal repro requires Hydra, file it in the Hydra repo. if you can do it with OmegaConf only you can add it here.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/omry/omegaconf/issues/690#issuecomment-822712149, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAITRZJJ3ZBWQLK5Q4FQF3DTJR5OPANCNFSM43GBJLEQ .

omry commented 3 years ago

Oh, I see. Yeah - I generally recommend not skipping major Hydra versions. For the best upgrade experience, you should upgrade from 0.11 to 1.0, make sure you addressed all the warning and only then upgrade from 1.0 to 1.1 (and again address all the warnings).

oliver-batchelor commented 3 years ago

Yeah, that's a good idea. I've been using 1.0 for a while in other projects but didn't think to do it that way. 1.1 is looking really nice, especially nested interpolations make some things much easier.

The only other change which tripped me up for a minute was the ordering in the default list with self.

On Tue, Apr 20, 2021 at 12:01 PM Omry Yadan @.***> wrote:

Oh, I see. Yeah - I generally recommend not skipping Hydra major versions. For the best upgrade experience, you should upgrade from 0.11 to 1.0, make sure you addressed all the warning and only then upgrade from 1.0 to 1.1 (and again address all the warnings).

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/omry/omegaconf/issues/690#issuecomment-822866665, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAITRZJOVQXW4YFBI3G3ZBDTJS76BANCNFSM43GBJLEQ .

omry commented 3 years ago

The default composition order (ordering with _self_ is one of the few breaking changes that does not have an explicit warning in Hydra 1.1. I think you are the first person reporting that it got bit by it. I am expecting it to only affect a very small subset of the users. I hope it wasn't too hard to figure out. I added a note in another page most people migrating will see to draw attention to it.

The killer feature of 1.1 is the recursive Defaults List.

By the way: https://twitter.com/Hydra_Framework/status/1382410154952458241