Simpler param updates with python-benedict #4780

sjawhar · 2020-10-23T12:50:40Z

❗ 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 · 2020-10-23T15:45:34Z

dvc/repo/experiments/run.py

@@ -31,7 +30,7 @@ def _parse_params(path_params: Iterable):
                )
        if not path:
            path = ParamsDependency.DEFAULT_PARAMS_FILE
-        ret[path] = unflatten(params)
+        ret[path] = params


To be clear, this now returns a dict like:

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

sjawhar · 2020-10-23T15:46:07Z

dvc/repo/experiments/__init__.py

@@ -311,31 +310,18 @@ def _unpack_args(self, tree=None):

    def _update_params(self, params: dict):
        """Update experiment params files with the specified values."""
+        from benedict import benedict


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 · 2020-10-23T15:47:05Z

tests/func/experiments/test_experiments.py

+        [["foo=baz"], "{foo: baz, goo: {bag: 3}}"],
+        [["foo=baz,goo=bar"], "{foo: baz, goo: bar}"],
+        [["goo.bag=4"], "{foo: [bar: 1, baz: 2], goo: {bag: 4}}"],
+        [["foo[0]=bar"], "{foo: [bar, baz: 2], goo: {bag: 3}}"],


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

pmrowla · 2020-10-26T08:29:20Z

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 · 2020-11-11T06:14:57Z

tests/func/experiments/test_experiments.py

+            "{foo: [bar: 1, baz: 3], goo: {bag: 3}, lorem: false}",
+        ],
+        [
+            ["foo[1]=- baz\n- goo"],


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.

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.

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 · 2020-11-12T07:16:02Z

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

pmrowla · 2020-11-12T07:42:47Z

Thanks for the PR @sjawhar!

…limit * 'master' of github.com:iterative/dvc: dag: add --outs option (iterative#4739) Add test server and tests for webdav (iterative#4827) Simpler param updates with python-benedict (iterative#4780) checkpoints: set DVC_ROOT environment variable (iterative#4877) api: add support for simple wildcards (iterative#4864) tests: mark azure test as flaky (iterative#4881) setup.py: limit responses version for moto (iterative#4879) remote: avoid chunking on webdav. Fixes iterative#4796 (iterative#4828) checkpoints: `exp run` and `exp res[ume]` refactor (iterative#4855)

efiop · 2020-11-18T15:49:05Z

@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 · 2020-11-19T01:40:49Z

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

@efiop

efiop · 2020-11-19T01:59:31Z

@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).

sjawhar force-pushed the feature/exp-simpler-param-updates branch from eca373a to 8fe0b17 Compare October 23, 2020 15:41

sjawhar commented Oct 23, 2020

View reviewed changes

sjawhar force-pushed the feature/exp-simpler-param-updates branch from 8fe0b17 to 31db81e Compare October 24, 2020 16:34

tomasfarias mentioned this pull request Oct 24, 2020

Linting failures caused by using dict with keyword arguments #4783

Closed

pmrowla added enhancement Enhances DVC A: experiments Related to dvc exp labels Oct 26, 2020

efiop requested a review from skshetry November 3, 2020 15:28

skshetry reviewed Nov 11, 2020

View reviewed changes

skshetry approved these changes Nov 11, 2020

View reviewed changes

Simpler param updates with python-benedict

39847ba

pmrowla force-pushed the feature/exp-simpler-param-updates branch from 31db81e to 39847ba Compare November 12, 2020 07:12

pmrowla mentioned this pull request Nov 12, 2020

experiments: consider using hydra for CLI params syntax #4883

Closed

pmrowla merged commit 72ec2a8 into iterative:master Nov 12, 2020

efiop mentioned this pull request Nov 18, 2020

dvc v1.10.0 conda-forge/dvc-feedstock#164

Merged

3 tasks

sjawhar deleted the feature/exp-simpler-param-updates branch November 20, 2020 01:18

pmrowla mentioned this pull request Nov 25, 2020

Adding python-benedict, mailchecker conda-forge/staged-recipes#13326

Merged

9 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Simpler param updates with python-benedict #4780

Simpler param updates with python-benedict #4780

Uh oh!

sjawhar commented Oct 23, 2020 •

edited

Loading

Uh oh!

sjawhar Oct 23, 2020

Uh oh!

sjawhar Oct 23, 2020

Uh oh!

sjawhar Oct 23, 2020

Uh oh!

pmrowla commented Oct 26, 2020

Uh oh!

skshetry Nov 11, 2020 •

edited

Loading

Uh oh!

skshetry Nov 11, 2020

Uh oh!

sjawhar Nov 11, 2020 •

edited

Loading

Uh oh!

pmrowla commented Nov 12, 2020

Uh oh!

pmrowla commented Nov 12, 2020

Uh oh!

efiop commented Nov 18, 2020 •

edited

Loading

Uh oh!

pmrowla commented Nov 19, 2020 •

edited

Loading

Uh oh!

efiop commented Nov 19, 2020

Uh oh!

Uh oh!

Simpler param updates with python-benedict #4780

Simpler param updates with python-benedict #4780

Uh oh!

Conversation

sjawhar commented Oct 23, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description of Changes

TODO

Uh oh!

sjawhar Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

sjawhar Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

sjawhar Oct 23, 2020

Choose a reason for hiding this comment

Uh oh!

pmrowla commented Oct 26, 2020

Uh oh!

skshetry Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

skshetry Nov 11, 2020

Choose a reason for hiding this comment

Uh oh!

sjawhar Nov 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pmrowla commented Nov 12, 2020

Uh oh!

pmrowla commented Nov 12, 2020

Uh oh!

efiop commented Nov 18, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pmrowla commented Nov 19, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efiop commented Nov 19, 2020

Uh oh!

Uh oh!

sjawhar commented Oct 23, 2020 •

edited

Loading

skshetry Nov 11, 2020 •

edited

Loading

sjawhar Nov 11, 2020 •

edited

Loading

efiop commented Nov 18, 2020 •

edited

Loading

pmrowla commented Nov 19, 2020 •

edited

Loading