Simpler param updates with python-benedict

[sjawhar]

• ❗ 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.

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

Description of Changes

Following up on my last comment on #4769

So I did some testing and my idea to use benedict to simplify the code actually works great... almost. (Update: figured it out) As you can see in the changes here, you can replace a lot of code with a single built-in function call. This also has the benefit of using more standard [0] syntax for list accessing.

I added more tests to make sure that everything was working, then I ran into this strange behavior on a single test case:

The params are being correctly updated and written, but for some reason the experiment doesn't seem to actually run the copy step. It's strange, and I would appreciate any help chasing this down...

I just needed to add goo to the list of stage params 🤦

Linting is failing on changes I didn't make.

TODO

• What's up with that weird behavior?!
• Run the full test suite to make sure I didn't break anything elsewhere 😄

[sjawhar]

To be clear, this now returns a dict like:

{'path.to.key': yamlValue1, 'path[2]key': yamlValue2}

[sjawhar]

Not sure if you prefer local or global imports. Since this is only needed for the beta experiments feature, I figured local was appropriate.

[sjawhar]

Note the change in syntax, which I think is more in line with your future plans.

[pmrowla]

This is very cool, thanks for the PR!

Since this will add a dependency I want to discuss this internally with the rest of the DVC team first before merging it, but everything looks good to me for now.

[skshetry]

This syntax is not easy to understand/use. At a first glance, I associated those =- together and assumed it was removing items from the list.

Ideally, I'd like to have the following syntax:

$ dvc exp run --params 'foo[1]=[baz, goo]' 'bar={a:2, b: 3}'

I understand that we split it up by comma, so we cannot achieve that. We should consider using a different delimiter, maybe even space or semicolon and make it look JSON-like.

[skshetry]

It seems a natural progression to support appending and removing similar to Hydra's syntax (they use ~foo to remove and +foo=value to append).

Or, maybe even use Hydra later directly.

[sjawhar]

Note that I didn't change anything regarding the syntax. This is the syntax already accepted by the yaml parser being used, which is separate from this change. I'm simply documenting what currently works.

That said, I agree that it it would be nice to be able to specify arguments that include commas, and I think that your suggestion of simply changing --params to read a list instead of a single string would be the most straight-forward way of doing it.

[pmrowla]

Rebased this PR and created #4883 for future consideration regarding hydra

[pmrowla]

Thanks for the PR @sjawhar!

[efiop]

@sjawhar @pmrowla Unfortunately there is no such package in conda, which means we can't use benedict as is (this exp feature won't work in conda's dvc 1.10 package 🙁 ). Is there an alternative lib that has conda package?

[pmrowla]

Hydra does have a conda package (#4883). This can probably just be reverted until we get to that issue.

@efiop

[efiop]

@pmrowla Since hydra would change the syntax for this, I guess we better to do the switch before 2.0 if we indeed decide to go with it? Your call here. Could revert, or could submit benedict to conda-forge if we want (will be happy to help with sending one, we've sent a few already).