-
Notifications
You must be signed in to change notification settings - Fork 123
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
Improve Optional[type] support #749
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall looks great, just a few small things (NB: there's also a pickling error with 3.6 to look into)
# merging into a new node. Use element_type as a base | ||
dest[key] = DictConfig(content=dest._metadata.element_type, parent=dest) | ||
dest[key] = DictConfig( | ||
et, parent=dest, ref_type=et, is_optional=is_optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
More of a meta question for @omry: should a DictConfig
created from a structured config type automatically set its ref_type
to that type? (here, that would mean not needing to specify ref_type=et
)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of merge, I think the ref_type should be preserved from the target node.
If there is no target node (new node), I think the ref type should be Any (default, irrc).
The concern is that this will change the assignment semantics of the node.
@dataclass
class Config:
x : int = 10
cfg = OmegaConf.create({
"a": Config(),
"b": DictConfig(Config(), ref_type=Config)
})
cfg.a = "aaa" # ok
cfg.b = "aaa" # error
@Jasha10, what is the motivation for specifying the ref_type here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of merge, I think the ref_type should be preserved from the target node.
Agreed. This PR is consistent with preserving ref_type from the target node.
If there is no target node (new node), I think the ref type should be Any (default, irrc).
Usually, but I think that if parent's element_type
is set
then (for a new node) we should have parent_element_type == child_ref_type
.
For example:
from typing import Any
from omegaconf import DictConfig, OmegaConf
from tests import User
cfg = OmegaConf.merge(DictConfig({}, element_type=User), {"a": User("Bond")})
# On this branch:
assert cfg.a._metadata.ref_type == User
# On master branch:
assert cfg.a._metadata.ref_type == Any
This change is consistent with how assignment works: child_ref_type
is
inherited from parent_element_type
.
cfg2 = DictConfig({}, element_type=User)
cfg2.a = User("Bond")
# On this branch AND master branch:
assert cfg2.a._metadata.ref_type == User
Apparently Python 3.6 pickling doesn't play well with Optional: $ python
Python 3.6.13
>>> from typing import Optional
>>> import pickle
>>> pickle.dumps(Optional[int])
Traceback (most recent call last):
File "<stdin>", line 1, in <module>
_pickle.PicklingError: Can't pickle typing.Union[int, NoneType]: it's not the same object as typing.Union The CI failure was triggered because I added fields to the |
Resolved in e2b2800 using |
# merging into a new node. Use element_type as a base | ||
dest[key] = DictConfig(content=dest._metadata.element_type, parent=dest) | ||
dest[key] = DictConfig( | ||
et, parent=dest, ref_type=et, is_optional=is_optional |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In the context of merge, I think the ref_type should be preserved from the target node.
If there is no target node (new node), I think the ref type should be Any (default, irrc).
The concern is that this will change the assignment semantics of the node.
@dataclass
class Config:
x : int = 10
cfg = OmegaConf.create({
"a": Config(),
"b": DictConfig(Config(), ref_type=Config)
})
cfg.a = "aaa" # ok
cfg.b = "aaa" # error
@Jasha10, what is the motivation for specifying the ref_type here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good for me once ongoing discussions with @omry are resolved, thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few comments on the latest commit
tests/test_merge.py
Outdated
), | ||
param( | ||
(DictConfig(content={}, element_type=User), {"foo": None}), | ||
raises(ValidationError, match="child 'foo' is not Optional"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor comment: may be worth looking at the full error message to make sure it makes sense (the excerpt being tested could be confusing)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In 0d8196c I've updated the tests to match the full error message (excluding the one with id="new_str_none"
, which is currently not passing).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks -- though personally I tend to prefer checking snippets to make tests more concise (leaving full checks to test_errors.py) -- but anyway the new version looks good to me.
This actually confirms that this error message ("child 'foo' is not Optional") is a bit confusing (IMO). I can see where it may be coming from (we first add a foo
node of type User
then try to set it to None
), but a clearer error could be something like: "Can't merge None values into a DictConfig whose element type is not Optional".
Definitely not something to change in this PR (and may not be worth changing it at all) -- I just thought I'd mention it because it caught my eye :)
(edit: feel free to resolve)
Python 3.6 eol is coming in a couple of months: https://endoflife.date/python It would be good to issue a better error than |
Currently I can't reproduce that pickling error on the Here is a minimal repro for this branch: import pickle
from dataclasses import dataclass, field
from typing import Dict, Optional
from omegaconf import OmegaConf
@dataclass
class DictOptInt:
dict_opt: Dict[str, Optional[int]]
cfg = OmegaConf.structured(DictOptInt)
pickle.dump(cfg, open("tmp.pkl", "wb")) # causes error in python 3.6 I think catching the pickling error would require adding logic to Edit: or should I add that logic to this PR? |
Future PR sounds better to me, can you just make sure there is an xfail test for Python 3.6 in this one? (if not already the case) |
Meta-discussion: I personally prefer to rebase on top of master and force-push (doing it only if needed) => this allows to easily see all changes (commits) done on top of master, without having obscure merge commits that may hide how some conflicts were handled. => This is only me though and I wonder if there's a good argument in favor of merging master like you did |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be good for me once everyone else's happy (including CI)
Good point about hiding how merge conflicts were handled.
My main motivation was to prevent earlier commits from becoming orphaned (so that review comments linking to specific commits would not be out-of-date). |
Not 100% sure we are talking about the same thing, but I prefer not to force push to master because it breaks anyone that has a checkout (or is keeping a git ref id stored). |
Yeah it's been working fine for me so far, I haven't noticed any major issue. The main problem I've found is that comments referencing specific commits would now refer to the old version of these commits. It's often not a real problem because the diff is usually the same after rebase, but it's something to be aware of (especially if you have many unresolved discussions pointing to various commits).
We're not talking about the same thing :) What I was talking about is when you want to bring in some changes from |
Rebased against |
@odelalleau would you be willing to take a quick look at the recent commit b593f4f? Outside of that commit, there have been no significant changes since your previous review (besides a |
Yeah, sorry I said nothing but I had a look and it seems good to me. Since you're asking though, I remember @omry's been a bit reluctant in the past to add new exception subclasses unless there's a clear need, but that's ok as far as I'm concerned :) |
Ok, thanks! I'll mention it to Omry before landing this. |
After exchanging emails with Omry, I'm changing back to |
This PR is to improve validation for
Optional[...]
typing.The main contribution is to allow
element_type
to be optional, e.g.is now legal.
Summary of changes:
wrap
function, check whether the parent's_metadata.element_type
is optional. This means that, for the new child value,is_optional==True
ifelement_type==Any
andis_optional==False
if e.g.element_type==int
. This closes When validating the assignment to a list,None
is not properly validated #579._utils.valid_value_annotation_type
so thatOptional[...]
values are considered valid. Checkvalid_value_annotation_type(element_type)
inContainerMetadata.__post_init__
. This closes Dict[str, Optional[str]] fails #460.ContainerMetadata.__post_init__
: replace an assertion with araise ValidationError
; remove redundantraise ValidationError
statements fromListConfig.__init__
andDictConfig.__init__
._utils._resolve_optional
as needed to adapt code for optionalelement_type
.OmegaConf.merge
such that optional element_type is handled properly.