Skip to content

Forward flags when loading to creation#1051

Draft
famura wants to merge 2 commits intoomry:mainfrom
famura:feat/forward_flags
Draft

Forward flags when loading to creation#1051
famura wants to merge 2 commits intoomry:mainfrom
famura:feat/forward_flags

Conversation

@famura
Copy link
Copy Markdown

@famura famura commented Jan 5, 2023

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
Copy link
Copy Markdown
Owner

omry commented Jan 5, 2023

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

famura commented Jan 5, 2023

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
Copy link
Copy Markdown
Owner

omry commented Jan 7, 2023

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

famura commented Jan 9, 2023

I understand, thanks for the explanation.

@Jasha10 Jasha10 marked this pull request as draft January 11, 2023 10:47
@Jasha10
Copy link
Copy Markdown
Collaborator

Jasha10 commented Jan 11, 2023

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants