From e393e29ef5c1508f2db7d0e06bfb424688edee7e Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Fri, 3 Jan 2020 04:00:02 +0200 Subject: [PATCH] metrics: introduce `diff` This first implementation is based on existing `dvc metrics show` functionality, but is also able to auto-detect json to properly show diff for it. --- dvc/command/metrics.py | 122 ++++++++++++++++++++++++++++- dvc/repo/brancher.py | 6 +- dvc/repo/metrics/__init__.py | 5 ++ dvc/repo/metrics/diff.py | 105 +++++++++++++++++++++++++ dvc/repo/metrics/show.py | 7 +- setup.py | 2 + tests/func/test_metrics.py | 95 +++++++++++++++++++--- tests/unit/command/test_metrics.py | 67 ++++++++++++++++ 8 files changed, 390 insertions(+), 19 deletions(-) create mode 100644 dvc/repo/metrics/diff.py create mode 100644 tests/unit/command/test_metrics.py diff --git a/dvc/command/metrics.py b/dvc/command/metrics.py index a2bc2587ab..ef79392fbc 100644 --- a/dvc/command/metrics.py +++ b/dvc/command/metrics.py @@ -26,7 +26,12 @@ def show_metrics(metrics, all_branches=False, all_tags=False): logger.info("{branch}:".format(branch=branch)) for fname, metric in val.items(): - lines = metric if type(metric) is list else metric.splitlines() + if isinstance(metric, dict): + lines = list(metric.values()) + elif isinstance(metric, list): + lines = metric + else: + lines = metric.splitlines() if len(lines) > 1: logger.info("\t{fname}:".format(fname=fname)) @@ -100,6 +105,59 @@ def run(self): return 0 +def _show_diff(diff): + from texttable import Texttable + + if not diff: + return "No changes." + + table = Texttable() + + # remove borders to make it easier for users to copy stuff + table.set_chars(("", "", "", "")) + table.set_deco(0) + + rows = [["Path", "Metric", "Value", "Change"]] + for fname, mdiff in diff.items(): + for metric, change in mdiff.items(): + rows.append( + [ + fname, + metric, + change["new"], + change.get("diff", "diff not supported"), + ] + ) + table.add_rows(rows) + return table.draw() + + +class CmdMetricsDiff(CmdBase): + def run(self): + try: + diff = self.repo.metrics.diff( + a_ref=self.args.a_ref, + b_ref=self.args.b_ref, + targets=self.args.targets, + typ=self.args.type, + xpath=self.args.xpath, + recursive=self.args.recursive, + ) + + if self.args.show_json: + import json + + logger.info(json.dumps(diff)) + else: + logger.info(_show_diff(diff)) + + except DvcException: + logger.exception("failed to show metrics diff") + return 1 + + return 0 + + def add_parser(subparsers, parent_parser): METRICS_HELP = "Commands to add, manage, collect and display metrics." @@ -214,3 +272,65 @@ def add_parser(subparsers, parent_parser): ) metrics_remove_parser.add_argument("path", help="Path to a metric file.") metrics_remove_parser.set_defaults(func=CmdMetricsRemove) + + METRICS_DIFF_HELP = "Output metric values." + metrics_diff_parser = metrics_subparsers.add_parser( + "diff", + parents=[parent_parser], + description=append_doc_link(METRICS_DIFF_HELP, "metrics/diff"), + help=METRICS_DIFF_HELP, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + metrics_diff_parser.add_argument( + "a_ref", + nargs="?", + help=( + "Git reference from which diff is calculated. " + "If omitted `HEAD`(latest commit) is used." + ), + ) + metrics_diff_parser.add_argument( + "b_ref", + nargs="?", + help=( + "Git reference to which diff is calculated. " + "If omitted current working tree is used." + ), + ) + metrics_diff_parser.add_argument( + "--targets", + nargs="*", + help=( + "Metric files or directories (see -R) to show diff for. " + "Shows diff for all metric files by default." + ), + ) + metrics_diff_parser.add_argument( + "-t", + "--type", + help=( + "Type of metrics (json/tsv/htsv/csv/hcsv). " + "It can be detected by the file extension automatically. " + "Unsupported types will be treated as raw." + ), + ) + metrics_diff_parser.add_argument( + "-x", "--xpath", help="json/tsv/htsv/csv/hcsv path." + ) + metrics_diff_parser.add_argument( + "-R", + "--recursive", + action="store_true", + default=False, + help=( + "If any target is a directory, recursively search and process " + "metric files." + ), + ) + metrics_diff_parser.add_argument( + "--show-json", + action="store_true", + default=False, + help="Show output in JSON format.", + ) + metrics_diff_parser.set_defaults(func=CmdMetricsDiff) diff --git a/dvc/repo/brancher.py b/dvc/repo/brancher.py index 8b096726c5..f64bf41b7e 100644 --- a/dvc/repo/brancher.py +++ b/dvc/repo/brancher.py @@ -4,7 +4,7 @@ def brancher( # noqa: E302 - self, all_branches=False, all_tags=False, all_commits=False + self, revs=None, all_branches=False, all_tags=False, all_commits=False ): """Generator that iterates over specified revisions. @@ -20,12 +20,12 @@ def brancher( # noqa: E302 - empty string it there is no branches to iterate over - "Working Tree" if there are uncommitted changes in the SCM repo """ - if not any([all_branches, all_tags, all_commits]): + if not any([revs, all_branches, all_tags, all_commits]): yield "" return saved_tree = self.tree - revs = [] + revs = revs or [] scm = self.scm diff --git a/dvc/repo/metrics/__init__.py b/dvc/repo/metrics/__init__.py index 8904a00b93..7fce413da3 100644 --- a/dvc/repo/metrics/__init__.py +++ b/dvc/repo/metrics/__init__.py @@ -21,3 +21,8 @@ def remove(self, *args, **kwargs): from dvc.repo.metrics.remove import remove return remove(self.repo, *args, **kwargs) + + def diff(self, *args, **kwargs): + from .diff import diff + + return diff(self.repo, *args, **kwargs) diff --git a/dvc/repo/metrics/diff.py b/dvc/repo/metrics/diff.py new file mode 100644 index 0000000000..aa056cadcc --- /dev/null +++ b/dvc/repo/metrics/diff.py @@ -0,0 +1,105 @@ +import json +from collections import defaultdict + +from flatten_dict import flatten + +from dvc.exceptions import NoMetricsError + + +def _parse(raw): + if isinstance(raw, (dict, list, int, float)): + return raw + + assert isinstance(raw, str) + try: + return json.loads(raw) + except json.JSONDecodeError: + return raw + + +def _diff_vals(old, new): + if ( + isinstance(new, list) + and isinstance(old, list) + and len(old) == len(new) == 1 + ): + return _diff_vals(old[0], new[0]) + + if old == new: + return {} + + res = {"old": old, "new": new} + if isinstance(new, (int, float)) and isinstance(old, (int, float)): + res["diff"] = new - old + return res + + +# dot_reducer is not released yet (flatten-dict > 0.2.0) +def _dot(k1, k2): + if k1 is None: + return k2 + return "{0}.{1}".format(k1, k2) + + +def _diff_dicts(old_dict, new_dict): + old_default = None + new_default = None + + if isinstance(new_dict, dict): + new = flatten(new_dict, reducer=_dot) + else: + new = defaultdict(lambda: "not a dict") + new_default = "unable to parse" + + if isinstance(old_dict, dict): + old = flatten(old_dict, reducer=_dot) + else: + old = defaultdict(lambda: "not a dict") + old_default = "unable to parse" + + res = defaultdict(dict) + + xpaths = set(old.keys()) + xpaths.update(set(new.keys())) + for xpath in xpaths: + old_val = old.get(xpath, old_default) + new_val = new.get(xpath, new_default) + val_diff = _diff_vals(old_val, new_val) + if val_diff: + res[xpath] = val_diff + return dict(res) + + +def _diff(old_raw, new_raw): + old = _parse(old_raw) + new = _parse(new_raw) + + if isinstance(new, dict) or isinstance(old, dict): + return _diff_dicts(old, new) + + return {"": _diff_vals(old, new)} + + +def _get_metrics(repo, *args, rev=None, **kwargs): + try: + metrics = repo.metrics.show( + *args, **kwargs, revs=[rev] if rev else None + ) + return metrics[rev or ""] + except NoMetricsError: + return {} + + +def diff(repo, *args, a_ref=None, b_ref=None, **kwargs): + old = _get_metrics(repo, *args, **kwargs, rev=(a_ref or "HEAD")) + new = _get_metrics(repo, *args, **kwargs, rev=b_ref) + + paths = set(old.keys()) + paths.update(set(new.keys())) + + res = defaultdict(dict) + for path in paths: + path_diff = _diff(old[path], new[path]) + if path_diff: + res[path] = path_diff + return dict(res) diff --git a/dvc/repo/metrics/show.py b/dvc/repo/metrics/show.py index 01df0e86df..9f0f3ef138 100644 --- a/dvc/repo/metrics/show.py +++ b/dvc/repo/metrics/show.py @@ -20,7 +20,7 @@ def _read_metric_json(fd, json_path): parser = parse(json_path) - return [x.value for x in parser.find(json.load(fd))] + return {str(x.full_path): x.value for x in parser.find(json.load(fd))} def _get_values(row): @@ -266,6 +266,7 @@ def show( all_branches=False, all_tags=False, recursive=False, + revs=None, ): res = {} found = set() @@ -274,7 +275,9 @@ def show( # Iterate once to call `_collect_metrics` on all the stages targets = [None] - for branch in repo.brancher(all_branches=all_branches, all_tags=all_tags): + for branch in repo.brancher( + revs=revs, all_branches=all_branches, all_tags=all_tags + ): metrics = {} for target in targets: diff --git a/setup.py b/setup.py index f8c4fda597..592ef584c3 100644 --- a/setup.py +++ b/setup.py @@ -75,6 +75,8 @@ def run(self): "win-unicode-console>=0.5; sys_platform == 'win32'", "pywin32>=225; sys_platform == 'win32'", "networkx>=2.1,<2.4", + "flatten-dict>=0.2.0", + "texttable>=0.5.2", ] diff --git a/tests/func/test_metrics.py b/tests/func/test_metrics.py index 8f69ec315d..42043dbd8c 100644 --- a/tests/func/test_metrics.py +++ b/tests/func/test_metrics.py @@ -4,6 +4,8 @@ import logging import os +import pytest + from dvc.exceptions import DvcException from dvc.exceptions import NoMetricsError from dvc.main import main @@ -113,9 +115,9 @@ def test_show(self): ["metric_json"], typ="json", xpath="branch", all_branches=True ) self.assertEqual(len(ret), 3) - self.assertSequenceEqual(ret["foo"]["metric_json"], ["foo"]) - self.assertSequenceEqual(ret["bar"]["metric_json"], ["bar"]) - self.assertSequenceEqual(ret["baz"]["metric_json"], ["baz"]) + self.assertSequenceEqual(ret["foo"]["metric_json"], {"branch": "foo"}) + self.assertSequenceEqual(ret["bar"]["metric_json"], {"branch": "bar"}) + self.assertSequenceEqual(ret["baz"]["metric_json"], {"branch": "baz"}) ret = self.dvc.metrics.show( ["metric_tsv"], typ="tsv", xpath="0,0", all_branches=True @@ -158,13 +160,13 @@ def test_show(self): self.assertEqual(len(ret), 1) self.assertSequenceEqual( ret["foo"]["metric_json_ext"], - [ - { + { + "metrics.[0]": { "dataset": "train", "deviation_mse": 0.173461, "value_mse": 0.421601, } - ], + }, ) self.assertRaises(KeyError, lambda: ret["bar"]) self.assertRaises(KeyError, lambda: ret["baz"]) @@ -723,9 +725,9 @@ def _test_metrics(self, func): self.assertEqual( res, { - "master": {"metrics.json": ["master"]}, - "one": {"metrics.json": ["one"]}, - "two": {"metrics.json": ["two"]}, + "master": {"metrics.json": {"metrics": "master"}}, + "one": {"metrics.json": {"metrics": "one"}}, + "two": {"metrics.json": {"metrics": "two"}}, }, ) @@ -736,9 +738,9 @@ def _test_metrics(self, func): self.assertEqual( res, { - "master": {"metrics.json": ["master"]}, - "one": {"metrics.json": ["one"]}, - "two": {"metrics.json": ["two"]}, + "master": {"metrics.json": {"metrics": "master"}}, + "one": {"metrics.json": {"metrics": "one"}}, + "two": {"metrics.json": {"metrics": "two"}}, }, ) @@ -802,6 +804,10 @@ def _do_show(self, file_name, xpath): for branch in self.branches: if isinstance(ret[branch][file_name], list): self.assertSequenceEqual(ret[branch][file_name], [branch]) + elif isinstance(ret[branch][file_name], dict): + self.assertSequenceEqual( + ret[branch][file_name], {"branch": branch} + ) else: self.assertSequenceEqual(ret[branch][file_name], branch) @@ -834,7 +840,7 @@ def test_show_xpath_should_override_stage_xpath(tmp_dir, dvc): dvc.run(cmd="", overwrite=True, metrics=["metric"]) dvc.metrics.modify("metric", typ="json", xpath="m2") - assert dvc.metrics.show(xpath="m1") == {"": {"metric": [0.1]}} + assert dvc.metrics.show(xpath="m1") == {"": {"metric": {"m1": 0.1}}} def test_show_multiple_outputs(tmp_dir, dvc, caplog): @@ -871,3 +877,66 @@ def test_show_multiple_outputs(tmp_dir, dvc, caplog): "the following metrics do not exist, " "are not metric files or are malformed: 'not-found'" ) in caplog.text + + +def test_metrics_diff_raw(tmp_dir, scm, dvc): + def _gen(val): + tmp_dir.gen({"metrics": val}) + dvc.run(cmd="", metrics=["metrics"]) + dvc.scm.add(["metrics.dvc"]) + dvc.scm.commit(str(val)) + + _gen("raw 1") + _gen("raw 2") + _gen("raw 3") + + assert dvc.metrics.diff(a_ref="HEAD~2") == { + "metrics": {"": {"old": "raw 1", "new": "raw 3"}} + } + + +@pytest.mark.parametrize("xpath", [True, False]) +def test_metrics_diff_json(tmp_dir, scm, dvc, xpath): + def _gen(val): + metrics = {"a": {"b": {"c": val, "d": 1, "e": str(val)}}} + tmp_dir.gen({"m.json": json.dumps(metrics)}) + dvc.run(cmd="", metrics=["m.json"]) + dvc.metrics.modify("m.json", typ="json") + if xpath: + dvc.metrics.modify("m.json", xpath="a.b.c") + dvc.scm.add(["m.json.dvc"]) + dvc.scm.commit(str(val)) + + _gen(1) + _gen(2) + _gen(3) + + expected = {"m.json": {"a.b.c": {"old": 1, "new": 3, "diff": 2}}} + + if not xpath: + expected["m.json"]["a.b.e"] = {"old": "1", "new": "3"} + + assert expected == dvc.metrics.diff(a_ref="HEAD~2") + + +def test_metrics_diff_broken_json(tmp_dir, scm, dvc): + metrics = {"a": {"b": {"c": 1, "d": 1, "e": "3"}}} + tmp_dir.gen({"m.json": json.dumps(metrics)}) + dvc.run(cmd="", metrics_no_cache=["m.json"]) + dvc.scm.add(["m.json.dvc", "m.json"]) + dvc.scm.commit("add metrics") + + (tmp_dir / "m.json").write_text(json.dumps(metrics) + "ma\nlformed\n") + + assert dvc.metrics.diff() == { + "m.json": { + "a.b.c": {"old": 1, "new": "unable to parse"}, + "a.b.d": {"old": 1, "new": "unable to parse"}, + "a.b.e": {"old": "3", "new": "unable to parse"}, + } + } + + +def test_metrics_diff_no_metrics(tmp_dir, scm, dvc): + tmp_dir.scm_gen({"foo": "foo"}, commit="add foo") + assert dvc.metrics.diff(a_ref="HEAD~1") == {} diff --git a/tests/unit/command/test_metrics.py b/tests/unit/command/test_metrics.py new file mode 100644 index 0000000000..fb2394bd21 --- /dev/null +++ b/tests/unit/command/test_metrics.py @@ -0,0 +1,67 @@ +from dvc.cli import parse_args +from dvc.command.metrics import CmdMetricsDiff, _show_diff + + +def test_metrics_diff(dvc, mocker): + cli_args = parse_args( + [ + "metrics", + "diff", + "HEAD~10", + "HEAD~1", + "-t", + "json", + "-x", + "x.path", + "-R", + "--show-json", + "--targets", + "target1", + "target2", + ] + ) + assert cli_args.func == CmdMetricsDiff + + cmd = cli_args.func(cli_args) + m = mocker.patch("dvc.repo.metrics.diff.diff", return_value={}) + + assert cmd.run() == 0 + + m.assert_called_once_with( + cmd.repo, + targets=["target1", "target2"], + a_ref="HEAD~10", + b_ref="HEAD~1", + typ="json", + xpath="x.path", + recursive=True, + ) + + +def test_metrics_show_json_diff(): + assert _show_diff( + {"metrics.json": {"a.b.c": {"old": 1, "new": 2, "diff": 3}}} + ) == ( + " Path Metric Value Change\n" + "metrics.json a.b.c 2 3 " + ) + + +def test_metrics_show_raw_diff(): + assert _show_diff({"metrics": {"": {"old": "1", "new": "2"}}}) == ( + " Path Metric Value Change \n" + "metrics 2 diff not supported" + ) + + +def test_metrics_diff_no_diff(): + assert _show_diff( + {"other.json": {"a.b.d": {"old": "old", "new": "new"}}} + ) == ( + " Path Metric Value Change \n" + "other.json a.b.d new diff not supported" + ) + + +def test_metrics_diff_no_changes(): + assert _show_diff({}) == "No changes."