benedict: monkeypatch, performance improvements, bugfixes #6706
Conversation
Fixes iterative#6476, fixes iterative#6423 and closes iterative#6484.
skshetry
added
enhancement
| else: | ||
| # NOTE: the following line may seem like an unnecessary duplication | ||
| # data.merge might affect the `src` if it's not empty, so we cannot | ||
| # check `if src` later, as it may have been mutated already. | ||
| data.merge(to_update, overwrite=True) | ||
| # benedict has issues keeping references to an empty dictionary | ||
| # see: https://github.com/iterative/dvc/issues/6374. | ||
| src.update(data) |
There was a problem hiding this comment.
This else was for a bug in the benedict, and it has been fixed. To resolve this bug, that monkeypatch I mentioned was introduced 😢. We no longer need this else block, and tests should cover this if it breaks again in the future.
| def test_benedict_rollback_its_monkeypatch(): | ||
| from dvc.utils._benedict import benedict | ||
|
|
||
| assert benedict({"foo": "foo"}) == {"foo": "foo"} | ||
| assert encoder.c_make_encoder |
There was a problem hiding this comment.
Test for the monkeypatch that we rollback.
| @@ -203,10 +215,20 @@ def test_merge_params(changes, expected): | |||
| [{"foo": "baz"}, {"foo": "baz"}], | |||
| [{"foo": "baz", "goo": "bar"}, {"foo": "baz", "goo": "bar"}], | |||
| [{"foo[1]": ["baz", "goo"]}, {"foo": [None, ["baz", "goo"]]}], | |||
| [{"foo.bar": "baz"}, {"foo": {"bar": "baz"}}], | |||
There was a problem hiding this comment.
Testing for scenario mentioned in #6476, coupled with is_serializable assertion below.
|
With this #6521 also requiring significant part of benedict and that we need to imitate it's syntax, I think it's much easier to workaround performance issue. I do believe that we should have these small utilities in DVC itself, and try to avoid small dependencies moving forward. |
Fixes #6476, fixes #6423 and closes #6484.