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

DictConfig merge throws away data when element_type is structured #496

Closed
Jasha10 opened this issue Jan 25, 2021 · 2 comments · Fixed by #499
Closed

DictConfig merge throws away data when element_type is structured #496

Jasha10 opened this issue Jan 25, 2021 · 2 comments · Fixed by #499
Labels
bug Something isn't working

Comments

@Jasha10
Copy link
Collaborator

Jasha10 commented Jan 25, 2021

$ cat tmp.py
from dataclasses import dataclass
from omegaconf import DictConfig, OmegaConf


@dataclass
class User:
    name: str
    age: int


cfg1 = DictConfig({"user007": User("bond", 7)}, element_type=User)
cfg2 = {"user007": {"age": 99}}
print(OmegaConf.merge(cfg1, cfg2))
$ python tmp.py
{'user007': {'name': '???', 'age': 99}}

Desired output is

{'user007': {'name': 'bond', 'age': 99}}
@Jasha10 Jasha10 added the bug Something isn't working label Jan 25, 2021
@Jasha10
Copy link
Collaborator Author

Jasha10 commented Jan 25, 2021

This is related to the following lines in BaseContainer._map_merge, where dest[key] gets overwritten with an empty element_type instance before merging src_value with dest_node:

            if (
                is_structured_config(dest._metadata.element_type)
                and not missing_src_value
            ):
                dest[key] = DictConfig(content=dest._metadata.element_type, parent=dest)
                dest_node = dest._get_node(key)

@omry
Copy link
Owner

omry commented Jan 26, 2021

Thanks for reporting and for the PR! will take a look soon.

omry pushed a commit that referenced this issue Jan 27, 2021
@omry omry closed this as completed in #499 Jan 27, 2021
omry added a commit that referenced this issue Jan 27, 2021
* Merge: Don't overwrite element_type=strucutured

Closes #496.

* add another merge test case

* simplified and added additional test cases

* removed duplicate test and moved new tests to bottom of block

Co-authored-by: Jasha <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants