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

Fix merging of params when empty #6378

Merged
merged 3 commits into from Aug 2, 2021
Merged

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented Aug 2, 2021

The recent release of python-benedict==0.24.1 started ignoring empty dictionaries
during the merge. The way we expected to work was questionable and also how
benedict itself tries to handle the references to an empty dictionary is
also questionable.

I consider this to be a proper way to handle this even if this gets fixed
on the benedict side.

To demonstrate the problem with referencing an empty dictionary:

>>> x = {}
>>> y = dict(x)
>>> y.update({"foo": "bar"})
>>> x
{}
>>> y
{'foo': 'bar'}

Benedict tries to handle in-place updates, so it's tricky for it to handle this case of empty dictionaries as the references are not preserved. And, since modify_data requires updating things in-place, we require references to be preserved. So, it seems it's our duty to try to preserve references than it being left to benedict alone.

Fixes #6374.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

@skshetry skshetry requested a review from a team as a code owner August 2, 2021 17:44
@skshetry skshetry self-assigned this Aug 2, 2021
@skshetry skshetry added ci I keep failing, you keep fixing testing Related to the tests and the testing infrastructure labels Aug 2, 2021
@skshetry skshetry added this to In progress in DVC 27 Jul - 10 Aug via automation Aug 2, 2021
Recent release of python-benedict==0.24.1 started ignoring empty dictionaries
during merge. The way we expected to work was questionable and also how
benedict itself tries to handle the references to empty dictionary is
also questionable.

I consider this to be a proper way to handle this even if this gets fixed
on the benedict side.
@skshetry
Copy link
Member Author

skshetry commented Aug 2, 2021

I'll try adding creating a patch to the upstream tomorrow, and we can proceed accordingly.

Copy link
Member

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Thank you! 🙏

@efiop efiop merged commit 7444696 into iterative:master Aug 2, 2021
DVC 27 Jul - 10 Aug automation moved this from In progress to Done Aug 2, 2021
@efiop efiop added the p0-critical Critical issue. Needs to be fixed ASAP. label Aug 2, 2021
@skshetry skshetry deleted the fix-benedict-6374 branch August 3, 2021 04:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ci I keep failing, you keep fixing p0-critical Critical issue. Needs to be fixed ASAP. testing Related to the tests and the testing infrastructure
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

tests: test_experiments::test_untracked is failing in CI
2 participants