Added test_set_empty_params

[daavoo]

• ❗ I have followed the Contributing to DVC checklist.

• 📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

• Added explicit test for exp run: validate params filename and parameter names #5477
• Added regression test for exp run: unexpected error - cannot represent an object: {'lr': 0.1}  #6476

[skshetry]

benedict should never escape when we merge params from --set-params. This may result in loss of comments.
A fix might be to merge the params in the file recursively from the --set-params benedict dictionary.

[daavoo]

Not sure I'm fully understanding, what do you mean by escape ?

[skshetry]

Because of #6423, I'd like not to use or import benedict elsewhere than the following location, until it is resolved:

dvc/dvc/utils/collections.py

Line 83 in 73811f6

def merge_params(src: Dict, to_update: Dict) -> Dict:

So, the merge_params should not return any benedict object, it should return the data of same type as src, which is usually dict/CommentedMap (which preserves comments, coming from ruamel.yaml).

EDIT: updates on src should happen in-place, as required by modify_data contextmanager.

[daavoo]

Removed that and replaced with just a regression test currently checking that a exception is being raised. I guess that if we go with #6423 (comment) we should make that test pass

[skshetry]

#6476 is more urgent than #6423, though we could do it together too.

[daavoo]

I'm not sure but maybe we could add this tests for now and discuss in #6521 how we want to handle #5477 (as it would make #6476 no longer a bug but a different exception)

[skshetry]

Sorry for the late review. I have incorporated this test in #6706 (comment), as it gets automatically fixed after updating benedict to the latest version. This bug happened because we were handling an empty dictionary in a different way because of a bug in benedict which we don't need to handle now at all.