omry / omegaconf

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

Forward flags when loading to creation #1051

Open famura opened 1 year ago

famura commented 1 year ago

What?

This is a minimal PR providing the possibibility to pass the same flags to OmegaConf.load() that can be used when calling OmegaConf.create().

Why?

I created a custom resolver that allows setting tuples as config values. It worked perfectly when testing it like this OmegaConf.create("tup: ${as_tuple:1,2,3}"). However, it failed when loading the config file

tup:  ${as_tuple:1,2,3}

since I am calling OmegaConf.resolve() after OmegaConf.load() (for a good reason that is out of scope here). The error was

omegaconf.errors.UnsupportedValueType: Value 'tuple' is not a supported primitive type

I could resolve that error by altering the allow_objects flag

config._set_flag("allow_objects", True)

Despite the disclaimers in the comments about the flags API, I think it would be nice to be able to directly set them when creating a config from a file the same way we can do it when creating it programmatically.

What do you think? Did I miss anything?

How?

Please see the code. The change is straightforward.

omry commented 1 year ago

We are actually considering introducing a headers mechanism to files similar to # @package abc in Hydra (#anchor Such a mechanism could be used to support allow_objects or other OmegaConf flags and would somewhat conflict with this PR (Need to determine the strategy for merging in-file flags with the provided flags, a likely solution is to override the flags in the file with the flags from the API call).

famura commented 1 year ago

Thanks, @omry for your quick reply. I am not familiar with that header mechanism you mentioned, thus don't immediately see how these are conflicting. Anyways, if you don't want this PR, we can close it.

omry commented 1 year ago

I will let @Jasha10 decide on accepting this. I am not too thrilled because it will make the headers support more complicated and the mechanism there would in principle allow for a cleaner solution (specifying the flags in the file itself).

famura commented 1 year ago

I understand, thanks for the explanation.

Jasha10 commented 1 year ago

Thanks for the PR, @famura! Let's leave this PR open for now and re-evaluate one we make progress on the header support that Omry mentioned.