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

preliminary UnionNode implementation #913

Merged
merged 4 commits into from
May 16, 2022
Merged

Conversation

Jasha10
Copy link
Collaborator

@Jasha10 Jasha10 commented May 5, 2022

This PR implements support for unions of primitive types (i.e. unions of int, float, bool, str, bytes, and None).

A few notes:

  • Union can be freely mixed with Optional and None. For example, all of the following are equivalent:
    • Optional[Union[int, str]]
    • Union[Optional[int], str]
    • Union[int, str, None]
    • Union[int, str, type(None)]
  • Containers of unions (e.g. List[Union[int, str]], or Dict[int, Union[str, bytes]]) are supported.
  • Unions of containers (e.g. Union[List[str], Dict[str, str], MyStructuredConfig]) are not yet supported. This will come in a followup PR.

Here is a demo of the API:

from omegaconf import OmegaConf, ValidationError
from dataclasses import dataclass
from typing import Union
from pytest import raises
@dataclass
class Foo:
    u: Union[int, str] = 123

cfg = OmegaConf.create(Foo)
assert cfg.u == 123
cfg.u = "abc"  # ok

with raises(ValidationError):
    cfg.u = b"binary"  # bytes not compatible with union

with raises(ValidationError):
    OmegaConf.create(Foo(1.2))  # float not compatible

Implementation Notes:

  • UnionNode is a "pass-through" node. That means most validation logic is delegated to a child node.
  • I've introduced a new method Container._get_child which parallels Container._get_node. The difference between these methods is that _get_child automatically "passes over" Unions. Similarly, I've introduced Node._get_parent_container(), which is similar to Node._get_parent() except that it passes over union nodes in the parent heirarchy.
from dataclasses import dataclass
from typing import Dict, Union
from omegaconf import MISSING, OmegaConf, StringNode, UnionNode

@dataclass
class Example:
    cfg: Dict[str, Union[str, int]] = MISSING

cfg = OmegaConf.create(Example({"foo": "bar"})).cfg

assert isinstance(cfg._content, dict)
assert isinstance(cfg._content["foo"], UnionNode)
assert isinstance(cfg._content["foo"]._content, StringNode)
assert isinstance(cfg._content["foo"]._content._val, str)
assert cfg._content["foo"]._content._val == "bar"

assert cfg._content["foo"]._metadata.ref_type == Union[int, str]
assert cfg._content["foo"]._content._metadata.ref_type == str

assert cfg._get_node("foo") is cfg._content["foo"]            # UnionNode
assert cfg._get_child("foo") is cfg._content["foo"]._content  # StringNode

TODO before merging this

  • docs
  • serialization test
  • Double-check usage of _get_parent vs _get_parent_container throughout the codebase.
  • Double-check usage of _get_node vs _get_child throughout the codebase.

@Jasha10 Jasha10 marked this pull request as draft May 5, 2022 17:27
Copy link
Collaborator

@pixelb pixelb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The Box abstraction is great, and make things very neat.
The implementation all looks good to me.

In general splitting out the union of containers support until later, is fine and good.

Pre-approving assuming the outstanding tasks are done.

Thanks!

@Jasha10 Jasha10 force-pushed the primitive-unions branch 4 times, most recently from 17d6fce to 3cdd79e Compare May 16, 2022 15:47
@Jasha10 Jasha10 marked this pull request as ready for review May 16, 2022 15:57
@Jasha10 Jasha10 merged commit 0e278b5 into omry:master May 16, 2022
@Jasha10 Jasha10 deleted the primitive-unions branch May 23, 2022 04:36
@Jasha10 Jasha10 mentioned this pull request Jun 11, 2022
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.

None yet

2 participants