Move to yaml1.2 #4415

skshetry · 2020-08-18T11:38:02Z

This PR:

gets rid of pyyaml
uses ruamel everywhere
tries to unite toml and yaml
❗ 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.
Document that yaml files are expected to be in YAML v1.2 dvc.org#1704

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

This commit: * gets rid of pyyaml * uses ruamel everywhere * tries to unite toml and yaml However, this will require a few more changes that will come soonish. Eg: Current `parse_yaml` and `parse_toml` behavior could be error-prone, so, it's better to introduce `tree` inside these serialization methods (which I will try to tackle next). Another is, yaml load can be slow for json. We could speed it up just by using `json.load(s)` here (also in upcoming patch).

dvc/dependency/param.py

dvc/repo/plots/data.py

dvc/utils/__init__.py

dvc/scm/git.py

skshetry · 2020-08-18T12:31:46Z

dvc/dependency/param.py

 from collections import defaultdict

 import dpath.util
-import toml


@pmrowla, tomlkit preserves ordering and comments. We can move to it in the future.

dvc/repo/metrics/show.py

skshetry · 2020-08-19T09:33:44Z

dvc/utils/serialize/__init__.py

 from ._yaml import *  # noqa, pylint: disable=wildcard-import
+
+PARSERS = defaultdict(lambda: parse_yaml)  # noqa: F405
+PARSERS.update({".toml": parse_toml})  # noqa: F405


Also, it'd be good to have JSON utils after the upcoming PR with trees are done. Maybe even move to ujson as it has now wheels on all platforms.

In [1]: %timeit json.loads('{"ja": "son"}') 2.32 µs ± 35 ns per loop (mean ± std. dev. of 7 runs, 100000 loops each) In [2]: %timeit YAML(typ='safe').load('{"ja": "son"}') 180 µs ± 1.28 µs per loop (mean ± std. dev. of 7 runs, 10000 loops each) In [3]: %timeit ujson.loads('{"ja": "son"}') 372 ns ± 13.4 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

Refer treeverse/dvc#4415

* Document that YAML files are expected to be in YAML1.2 Refer treeverse/dvc#4415 * remove note, embed the note in the paragraph * Apply suggestions from code review Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com> * Restyled by prettier Co-authored-by: Jorge Orpinel <jorgeorpinel@users.noreply.github.com> Co-authored-by: Restyled.io <commits@restyled.io>

skshetry added 2 commits August 18, 2020 16:51

Fix tests

389bde4

skshetry requested review from efiop, pared and pmrowla August 18, 2020 11:38

skshetry self-assigned this Aug 18, 2020