Add support for Union type in structured configs.

[pereman2]

To add support for UnionTypes a new type of node is created UnionNode. This node holds a value which is another node so it's easier to check values. I plan to modify _validate_value which validates that the input is correct with the values implementation of validate_and_convert because UnionNode should not know what a ListConfig or DictConfig is.

I added some tests to check the behaviour. At this time I don't know which ones I missed, I'll check later.

Still have to check optional unions like Optional[Union[str, int]] and I will change when pickling unions on 3.6 still fails.

Tests list

A list of use cases to cover with tests, add more here if you think of more.

• [x] Unions with optional:

Optional[Union[int, str]]
Union[int,str,None] # I think this is legal to define Optional

• [x] Union with containers, Lists, Dicts Union[str, List[str]]
• [x] Containers with Unions: List[Union[int, str]]
• [x] Assignment to union fields, make sure it respects the type and that the field is still a UnionNode
• [x] Merging into Union fields, , make sure it respects the type and that the field is still a UnionNode
• [x] As you pointed out, pickling with Union nodes.

Closes #144 .

[omry]

Cool. A few surface area points to test/consider:

• Unions with optional:

Optional[Union[int, str]]
Union[int,str,None] # I think this is legal to define Optional

• Union with containers, Lists, Dicts Union[str, List[str]]
• Containers with Unions: List[Union[int, str]]
• Assignment to union fields, make sure it respects the type and that the field is still a UnionNode
• Merging into Union fields, , make sure it respects the type and that the field is still a UnionNode
• As you pointed out, pickling with Union nodes.

[ghost]

Congratulations :tada:. DeepCode analyzed your code in 6.138 seconds and we found no issues. Enjoy a moment of no bugs :sunny:.

👉 View analysis in DeepCode’s Dashboard | Configure the bot

[pereman2]

I've done several things. I've removed validation with other functions since as you pointed out, it doesn't make much sense. I've done Containers with Union element_types with more or less test. The same goes to Optional and merge with union node.

I'll try to add more test cases and wait for your review because I think you might want to comment a lot.

Still pickle not done.

[omry]

Re-request review when you want me to review again.

[pereman2]

Hey! I'll comment every change by the tasks:

1. Union with containers, Lists, Dicts Union[str, List[str]]

I've removed the function (662772bfbf6603145d5ad647275688436ac40197) to check whether the value is a valid type and changed it so it's checked when wraping the value with each element_type(until it finds a valid one, if not, it raises error).

2. Containers with Unions: (662772bfbf6603145d5ad647275688436ac40197) List[Union[int, str]]

In this case it didn't need that much work. I you add a new value to a DictConfig or ListConfig it will wrap with type_ Union and union will handle the rest.

3. Optional[Union[int, str]] (19a4012d1093705520234c0e1ef5c1826e039627)

Optional[Union[int ,str]] returns Union[int, str, NoneType] directly so if none is inside Union, UnionNode's optional will be true then, it removes None from Union[int, str] because optional it's checked in Node's validate.

4. Assignment to union fields, make sure it respects the type and that the field is still a UnionNode

I added test for this one in 9037ea0728e5c3f42dbf26257feea00270b86624 and the tests passed.

5. Merge with UnionNodes (9037ea0728e5c3f42dbf26257feea00270b86624)

After adding tests here I foound that I didn't test structured config assignment to union nodes and I had to add specific validation for it. Moreover since I use wrap node to check if a value is correct I didn't want to wrap Nodes again(it will return the node and not check) so,I used the value of the node to wrap.

6. Pickle with UnionNodes.

The way I faced this is transforming Union[vt1, vt2] to the list of element_types and then when loading I use the list to reconstruct the Union type as before. It looks strange but for example: u = Union[str, int] and then u = Union[u, float] will return `Union[str, int, Float. Furthermore, I had to override getstate and setstate in UnionNode for pickle to work.

I believe there's refactoring to do in validate_and_convert() and more test cases maybe but, I think if you review this now it'll be better.

Also, _bring_type_to_front is implemented in case there's a value that can be converted in more than 1 element_type for example, 1 can be a str and an int therefore, if 1 is an int it should wrap first a IntegerNode instead of StringNode which would be incorrect.

[pereman2]

I added more test cases for Union inside Dict and List

[pereman2]

Test are failing now because of I added test cases which include the bug solved in #443.

Should the try block be wrapped in a function. There are two instances of this in the same method.

[omry]

FYI: I am still working on the recursive defaults in Hydra. It's a very large pull request and I am delaying reviewing of other large changes in Hydra and OmegaConf until I am done with it. I will hopefully get it over with early after the holidays.

[pereman2]

Thanks for the info. Anyways, I thought I had more time but I'm full of college work for this vacations.

[omry]

I bounced the Union support to OmegaConf 2.2, we will resume working on it after 2.1 is released. Thanks for working on it.

Since many things have changed in 2.1, we will likely start from scratch but the tests will for sure be very useful.