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

Experiments: allow setting of list parameters #4769

Merged
merged 3 commits into from
Oct 23, 2020

Conversation

sjawhar
Copy link
Contributor

@sjawhar sjawhar commented Oct 21, 2020

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

Will close #4768

As stated in the issue, I'm sure more work will be needed, but this little tweak was enough to at least allow the experiment to run. Some more work would be required to get parameter diffs to show up nicely in dvc exp show (screenshot of current display is included below, see the change in activation function), but that might be out of scope for this issue.

image

If there are no major objections, I can go ahead and write up some tests and finish this PR off proper-like.

Check if dict is Mapping or list before using .get()
@sjawhar sjawhar changed the title Allow setting of list parameteres Experiments: allow setting of list parameters Oct 21, 2020
@efiop efiop requested a review from pmrowla October 21, 2020 20:33
@pmrowla
Copy link
Contributor

pmrowla commented Oct 22, 2020

@sjawhar thanks for identifying this issue and for the PR! This looks like a great start, and worked for me in a test project.

Don't worry about cleaning up the exp show display for this PR, we can consider how to address that in the future. For now, lists and dicts being displayed in the string [..., {..}] format is at least consistent with other DVC commands (params diff/metrics diff/etc).

For a test case, a modified version of

def test_new_simple(tmp_dir, scm, dvc, mocker):

should work.

Your test case will just need to generate a params.yaml file containing a small list, and when running the stage it would look something like

dvc.experiments.run(stage.addressing, params=["foo.<list_index>...=modified_value"])

The test stage we use just copies a params into a metrics file, so after calling experiments.run, the final output metrics file should contain the modified_value from the run command, rather than initial params.yaml value.

@sjawhar
Copy link
Contributor Author

sjawhar commented Oct 22, 2020

OK, test added. Thanks for pointing me to the right spot, that made it super easy for me πŸ™

@sjawhar sjawhar force-pushed the feature/experiment-list-params branch from 1953822 to fe29f59 Compare October 22, 2020 20:22
@sjawhar sjawhar force-pushed the feature/experiment-list-params branch from fe29f59 to 033c773 Compare October 22, 2020 20:24
@sjawhar
Copy link
Contributor Author

sjawhar commented Oct 23, 2020

I've been thinking about this and wondering if there's an opportunity to simplify things and delete a bunch of code by using something like https://github.com/fabiocaccamo/python-benedict. If the existing project params were instantiated as a benedict instance, then you could do stuff like params['foo.bar[0].bar'] = 1 with no fuss:

>>> from benedict import benedict
>>> params = benedict({'foo': { 'bar': [{'baz': 3}, 2]}})
>>> params['foo.bar[0].baz']=1
>>> params
{'foo': {'bar': [{'baz': 1}, 2]}}

@pmrowla pmrowla merged commit 831e856 into iterative:master Oct 23, 2020
@pmrowla
Copy link
Contributor

pmrowla commented Oct 23, 2020

Thanks for the PR! πŸ™

The suggestion regarding benedict is interesting and we could potentially consider using something like that in the future. If you'd like to file a separate feature request for it that would be great

@pmrowla pmrowla added enhancement Enhances DVC A: experiments Related to dvc exp labels Oct 23, 2020
@sjawhar sjawhar deleted the feature/experiment-list-params branch October 23, 2020 12:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: experiments Related to dvc exp enhancement Enhances DVC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Experiments: Modifying parameters in lists
2 participants