-
Notifications
You must be signed in to change notification settings - Fork 107
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
feat: add deep merging for lists #1082
Conversation
The tests show the following error in the
I didn't edit this file, so I guess that this error is due some wrong configuration or setup in CircleCI. Is there anything I have to look into? |
Thanks @MatteoVoges. @Jasha10, @odelalleau, @shagunsodhani: At a glance it looks pretty nice, but I didn't review properly.
|
I'm good with the proposed new feature. I can't commit to reviewing the code now though. |
7e0ec3c
to
9dba45d
Compare
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.
This mostly looks good to me! @omry is there a specific piece that I should look deeper into ?
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.
I'm happy with the proposed feature, and the implementation looks good to me.
One nit regarding the tests: the BaseContainer.merge_with
method is not documented and is not really intended as a public API. It would be better to test the public-facing OmegaConf.merge
facade instead of calling BaseContainer.merge_with
in the tests:
Co-authored-by: Omry Yadan <omry@yadan.net> Co-authored-by: Shagun Sodhani <1321193+shagunsodhani@users.noreply.github.com> Co-authored-by: Jasha Sommer-Simpson <8935917+Jasha10@users.noreply.github.com>
The >>> OmegaConf.merge([1,1,2], [2,2,3], remove_duplicates=True)
[2, 2, 3] Should we issue an error or warning in the case where Something like this: class OmegaConf:
...
def merge(
...,
list_merge_mode: ListMergeMode = ListMergeMode.OVERWRITE,
)
...
class ListMergeMode(Enum):
"""How `OmegaConf.merge` handles lists"""
OVERWRITE = 1 # overwrite left-hand list with right-hand list
EXTEND = 2 # extend left-hand list with right-hand list
EXTEND_NO_DUPLICATES = 3 # ... |
Also, I think the naming of >>> list1 = [1, 2, 3, 3]
>>> list2 = [3, 4, 5]
>>> OmegaConf.merge(list1, list2, extend_lists=True, remove_duplicates=True) # duplicates from list1 not removed
[1, 2, 3, 3, 4, 5] |
I thought about this before as well and my first implementation was indeed using a set. But I decided to go for the currently implemented approach, because the set was scrambling the order of the lists and it was hard to test.
I actually like the current behavior a lot, because I think if you merge something the original object should not be affected, but only extended. And if you have some duplicate values in your
I think a warning would be good here.
I don't think that this is neccessary, because we don't have many merge options yet and I think two merge modes do not require an enum. |
I think using a string an an alternative is a bit unusual and I am not a fan of this duality.
Given that the enum is becomeing a part of the public API, it should live in omegaconf/OmegaConf.py Usage can be something like: from omegaconf import OmegaConf, ListMergeMode
...
OmegaConf.merge(cfg1, cfg2, list_merge_mode = ListMergeMode.EXTEND_IGNORE_DUPLICATES) A docstring update to OmegaConf.merge is fine. Don't forget to update docs to reflect this. |
I can’t check right now but I believe there’s a way to make an enum that can be compared to a string so that just passing the string would work too. |
I think you're referring to from enum import StrEnum
class ListMergeModeString(StrEnum):
OVERWRITE = "OVERWRITE"
EXTEND = "EXTEND"
EXTEND_NO_DUPLICATES = "EXTEND_NO_DUPLICATES"
assert ListMergeModeString.OVERWRITE == "OVERWRITE" One issue with |
I even followed the SCMode and the ListMergeMode is implemented in the same way.
Unfortunately, this is then unusable for us. |
Yeah but I’m pretty sure I already implemented something similar in older versions… unfortunately I don’t have access to a computer until next week so I can’t really be more specific. |
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.
Starting to look good.
Looking at the code, I have a suggestion for slightly better enum keys. Let me know what you think (sorry for the excessive bikeshedding).
omegaconf/base.py
Outdated
class ListMergeMode(Enum): | ||
OVERRIDE = 1 # content from newer list gets taken (default) | ||
EXTEND = 2 # lists get extended | ||
EXTEND_IGNORE_DUPLICATES = 3 # only new (unique) elements get extended |
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.
I know we have went though several iteration of bikeshedding here already, but any thoughts on using append instead of extend?
Also I think for the default REPALCE is more appropriate than override.
class ListMergeMode(Enum): | |
OVERRIDE = 1 # content from newer list gets taken (default) | |
EXTEND = 2 # lists get extended | |
EXTEND_IGNORE_DUPLICATES = 3 # only new (unique) elements get extended | |
class ListMergeMode(Enum): | |
REPALCE = 1 # Replaces the target list with the new one (default) | |
APPEND= 2 # Appends the new list to the target list | |
APPEND_UNIQUE_VALUE = 3 # Appends items that do not already exist in the target list |
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.
I think APPEND
is confusing due to the way that python's builtin list.append
method works:
>>> lst = [1,2,3]; lst.extend([4,5,6]); print(lst)
[1, 2, 3, 4, 5, 6]
>>> lst = [1,2,3]; lst.append([4,5,6]); print(lst)
[1, 2, 3, [4, 5, 6]]
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.
Also I think for the default REPALCE is more appropriate than override.
👍🏼
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.
I like REPLACE
and APPEND_UNIQUE_VALUE
a lot. But I agree to Jasha, that we should name it after the python functions, so that extend
is extending the list. For APPEND_UNIQUE_VALUE
I like the way, that we don't extend lists anymore, but rather looking at the actual values and if they are unique in the target list. @omry ,what do you think?
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.
Okay, from the perspective of the target list:
# replace target list
REPLACE
# Extend target list with new list
EXTEND
# Extend target list with items not already present in it.
EXTEND_UNIQUE
Co-authored-by: Omry Yadan <omry@yadan.net>
5e5ef75
to
5d2df8f
Compare
The linting error is fixed in #1090 (and here as well) |
c71155e
to
539e0e4
Compare
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.
Looks good!
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.
Looks good, thanks!
Thanks, are there any plans for a (pre-)release in the near future? |
Maybe @shagunsodhani or @Jasha10 can do that. |
@MatteoVoges fyi I've just uploaded |
Thanks for that! ❤️ |
Fixes #1080
Proposed changes
OmegaConf.merge()
functionextend_lists
: this enables (deep) merge support forListConfigs
(default: False to ensure backwards compability)allow_duplicates
: this controls, if two equal items can be in a list (default: False)Examples
If you have a config:
and
you will get with the enabled flag
extend_lists
this result:The functionality of
allow_duplicates
is self explanatory, i guess.Tests and Documentation
docs/source/usage.rst
or indocs/notebook/Tutorial.ipynb
or somewhere else?Contributed by