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

Forward flags when loading to creation #1051

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

famura
Copy link

@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
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
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
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
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
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
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants