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

Flag for automatic conversion to native types from OmegaConf Container #419

Closed
snisarg opened this issue Oct 23, 2020 · 4 comments
Closed

Comments

@snisarg
Copy link

snisarg commented Oct 23, 2020

Is your feature request related to a problem? Please describe.

OmegaConf does a great job of being transparent from a configuration point of view to pass values to constructor of classes as an example. Since the object passed is an OmegaConf Container for types like dict and lists, this can become a problem where types play a role in the code.

For example, if there is an isinstance check in the code, the OmegaConf object will fail.

from omegaconf import OmegaConf

def check_list(data: List[str]):
    if not isinstance(data, list):
      raise Exception("Illegal arguments")

# Say this comes from Hydra
conf = OmegaConf.create([1, 2, 3])

# Expected PASS, but this will FAIL
check_list(conf)

A more concrete example is when we use OmegaConf to init a PyTorch nn.Module, which would fail in torchscript as it can't script Container objects.

Describe the solution you'd like

The solution for this is to use OmegaConf.to_container(), but it would be great to provide a flag that does it automatically once MISSING features are filled in, for example. This would prevent hand picking fields and adding OmegaConf.to_container() to them, beating the principle of transparency.

Describe alternatives you've considered

Adding OmegaConf.to_container() by hand to relevant fields inside the code.

Additional context

For those who have access to phabricator: https://www.internalfb.com/diff/D24382743

@omry
Copy link
Owner

omry commented Oct 23, 2020

The solution for this is to use OmegaConf.to_container(), but it would be great to provide a flag that does it automatically once MISSING features are filled in, for example. This would prevent hand picking fields and adding OmegaConf.to_container() to them, beating the principle of transparency.

I am not sure I understand what you are asking here.

  • A flag that does what automatically?
  • When does the flag logic executes?

@snisarg
Copy link
Author

snisarg commented Oct 27, 2020

I'm assuming we are using OmegaConf containers for a good reason, but it'll be great to not have to call to_container(). The flag is a suggestion, where we can call to_container implicitly when say there are no MISSING fields present.

I wonder if we could track if nested objects have all their MISSING values filled up via callbacks from the child and then convert the nested object via to_container()?

I'm not aware of all the reasons for having OmegaConf container, so I may be completely off base here.

@omry
Copy link
Owner

omry commented Oct 27, 2020

Okay, I suggest that you go over the slides to get a sense of what are the advantages and constraints imposed by using OmegaConf. This can help in two ways:

  1. It will enable you to take full advantage of OmegaConf features.
  2. It will enable you to ask for features in a more meaningful way.

OmegaConf containers are not the same as the primitive dict and list containers. they do offer many advantages and go to great length to be drop-in replacement, but there are some differences. As you pointed out one of those is that code that is doing instance checks for dict or list will fail.

A solution is to instancecheck against MutableSequence and MutableMapping instead of against list and dict.
I am no plans to extend Dict and List (See link in comments on #270 for some of the reasons). In addition to the reasons there this would be a breaking change (e.g: code that is currently differentiating between list and ListConfig based on instance checks will break).

First of all, you can always convert OmegaConf to a primitive container early on and stick to primitive dicts and lists. If you can't relax the type checks in your code it may be the best option (you will lose the value provided by OmegaConf containers though and I don't recommend that).
You mention in the original diff that seeing the problem with Hydra's instantiate method.
Hydra 1.1 will add some support for automatic conversion to primitive containers during instantiate.
Please go over this link to see if you think that would solve your problem.

@omry
Copy link
Owner

omry commented Feb 16, 2021

Reported again in #472. There is some progress on it in #502.

Closing as dup.

@omry omry closed this as completed Feb 16, 2021
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

No branches or pull requests

2 participants