From b79085ec6901fede910ac0da6a1caeff2de006e5 Mon Sep 17 00:00:00 2001 From: Ivan Shcheklein Date: Mon, 18 Feb 2019 17:17:20 -0800 Subject: [PATCH] add tests for metrics wrong type and some other edge cases --- dvc/repo/__init__.py | 3 +- dvc/repo/metrics/modify.py | 7 +- dvc/repo/metrics/show.py | 78 ++++++++++----- tests/test_metrics.py | 190 +++++++++++++++++++++++++++++++------ 4 files changed, 221 insertions(+), 57 deletions(-) diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py index 889abc768..025bfd2ef 100644 --- a/dvc/repo/__init__.py +++ b/dvc/repo/__init__.py @@ -1,8 +1,7 @@ from __future__ import unicode_literals -from dvc.utils.compat import str, builtin_str, open +from dvc.utils.compat import str -import collections import os import dvc.prompt as prompt import dvc.logger as logger diff --git a/dvc/repo/metrics/modify.py b/dvc/repo/metrics/modify.py index c28c8bfd8..541446b6a 100644 --- a/dvc/repo/metrics/modify.py +++ b/dvc/repo/metrics/modify.py @@ -4,6 +4,7 @@ from dvc.exceptions import DvcException def modify(repo, path, typ=None, xpath=None, delete=False): + supported_types = ["raw", "json", "csv", "tsv", "hcsv", "htsv"] outs = repo.find_outs_by_path(path) assert len(outs) == 1 out = outs[0] @@ -15,8 +16,10 @@ def modify(repo, path, typ=None, xpath=None, delete=False): if typ is not None: typ = typ.lower().strip() if typ not in ["raw", "json", "csv", "tsv", "hcsv", "htsv"]: - msg = "metric type '{}' is not supported" - raise DvcException(msg.format(typ)) + msg = "metric type '{typ}' is not supported, must be one of [{types}]" + raise DvcException( + msg.format(typ=typ, types=", ".join(supported_types)) + ) if not isinstance(out.metric, dict): out.metric = {} out.metric[out.PARAM_METRIC_TYPE] = typ diff --git a/dvc/repo/metrics/show.py b/dvc/repo/metrics/show.py index d86391bb5..f273d2c22 100644 --- a/dvc/repo/metrics/show.py +++ b/dvc/repo/metrics/show.py @@ -15,69 +15,95 @@ def _read_metric_json(fd, json_path): return [x.value for x in parser.find(json.load(fd))] +def _get_values(row): + if isinstance(row, dict): + return list(row.values()) + else: + return row + + def _do_read_metric_xsv(reader, row, col): if col is not None and row is not None: return [reader[row][col]] elif col is not None: return [r[col] for r in reader] elif row is not None: - return reader[row] - return None + return _get_values(reader[row]) + return [_get_values(r) for r in reader] def _read_metric_hxsv(fd, hxsv_path, delimiter): - row, col = hxsv_path.split(",") - row = int(row) + indices = hxsv_path.split(",") + row = indices[0] + row = int(row) if row else None + col = indices[1] if len(indices) > 1 and indices[1] else None reader = list(csv.DictReader(fd, delimiter=builtin_str(delimiter))) return _do_read_metric_xsv(reader, row, col) def _read_metric_xsv(fd, xsv_path, delimiter): - row, col = xsv_path.split(",") - row = int(row) - col = int(col) + indices = xsv_path.split(",") + row = indices[0] + row = int(row) if row else None + col = int(indices[1]) if len(indices) > 1 and indices[1] else None reader = list(csv.reader(fd, delimiter=builtin_str(delimiter))) return _do_read_metric_xsv(reader, row, col) -def _read_metric(path, typ=None, xpath=None): - ret = None +def _read_typed_metric(typ, xpath, fd): + if typ == "json": + ret = _read_metric_json(fd, xpath) + elif typ == "csv": + ret = _read_metric_xsv(fd, xpath, ",") + elif typ == "tsv": + ret = _read_metric_xsv(fd, xpath, "\t") + elif typ == "hcsv": + ret = _read_metric_hxsv(fd, xpath, ",") + elif typ == "htsv": + ret = _read_metric_hxsv(fd, xpath, "\t") + else: + ret = fd.read().strip() + return ret + +def _read_metric(path, typ=None, xpath=None, branch=None): + ret = None if not os.path.exists(path): return ret typ = typ.lower().strip() if typ else typ + xpath = xpath.strip() if xpath else xpath try: with open(path, "r") as fd: - if typ == "json": - ret = _read_metric_json(fd, xpath) - elif typ == "csv": - ret = _read_metric_xsv(fd, xpath, ",") - elif typ == "tsv": - ret = _read_metric_xsv(fd, xpath, "\t") - elif typ == "hcsv": - ret = _read_metric_hxsv(fd, xpath, ",") - elif typ == "htsv": - ret = _read_metric_hxsv(fd, xpath, "\t") - else: + if not xpath: ret = fd.read().strip() + else: + ret = _read_typed_metric(typ, xpath, fd) # Json path library has to be replaced or wrapped in # order to fix this too broad except clause. except Exception: logger.warning( - "unable to read metric in '{}'".format(path), parse_exception=True + "unable to read metric in '{}' in branch '{}'".format( + path, branch + ), + parse_exception=True, ) return ret -def _collect_metrics(self, path, recursive, typ, xpath): +def _collect_metrics(self, path, recursive, typ, xpath, branch): outs = [out for stage in self.stages() for out in stage.outs] if path: try: outs = self.find_outs_by_path(path, outs=outs, recursive=recursive) except OutputNotFoundError: + logger.debug( + "stage file not for found for '{}' in branch '{}'".format( + path, branch + ) + ) return [] res = [] @@ -97,7 +123,7 @@ def _collect_metrics(self, path, recursive, typ, xpath): return res -def _read_metrics(self, metrics): +def _read_metrics(self, metrics, branch): res = {} for out, typ, xpath in metrics: assert out.scheme == "local" @@ -106,7 +132,7 @@ def _read_metrics(self, metrics): else: path = out.path - metric = _read_metric(path, typ=typ, xpath=xpath) + metric = _read_metric(path, typ=typ, xpath=xpath, branch=branch) if not metric: continue @@ -128,8 +154,8 @@ def show( for branch in self.scm.brancher( all_branches=all_branches, all_tags=all_tags ): - entries = _collect_metrics(self, path, recursive, typ, xpath) - metrics = _read_metrics(self, entries) + entries = _collect_metrics(self, path, recursive, typ, xpath, branch) + metrics = _read_metrics(self, entries, branch) if metrics: res[branch] = metrics diff --git a/tests/test_metrics.py b/tests/test_metrics.py index d3172bdd0..169707897 100644 --- a/tests/test_metrics.py +++ b/tests/test_metrics.py @@ -6,6 +6,9 @@ from dvc.main import main from dvc.exceptions import DvcException, BadMetricError, NoMetricsError from tests.basic_env import TestDvc +import dvc.logger as logger +from dvc.utils.compat import StringIO + class TestMetrics(TestDvc): def setUp(self): @@ -52,52 +55,116 @@ class TestMetrics(TestDvc): self.dvc.scm.checkout("master") - def test(self): + def test_show(self): ret = self.dvc.metrics.show("metric", all_branches=True) self.assertEqual(len(ret), 3) - self.assertTrue(ret["foo"]["metric"] == "foo") - self.assertTrue(ret["bar"]["metric"] == "bar") - self.assertTrue(ret["baz"]["metric"] == "baz") + self.assertEqual(ret["foo"]["metric"], "foo") + self.assertEqual(ret["bar"]["metric"], "bar") + self.assertEqual(ret["baz"]["metric"], "baz") ret = self.dvc.metrics.show( "metric_json", typ="json", xpath="branch", all_branches=True ) self.assertEqual(len(ret), 3) - self.assertTrue(ret["foo"]["metric_json"] == ["foo"]) - self.assertTrue(ret["bar"]["metric_json"] == ["bar"]) - self.assertTrue(ret["baz"]["metric_json"] == ["baz"]) + self.assertSequenceEqual(ret["foo"]["metric_json"], ["foo"]) + self.assertSequenceEqual(ret["bar"]["metric_json"], ["bar"]) + self.assertSequenceEqual(ret["baz"]["metric_json"], ["baz"]) ret = self.dvc.metrics.show( - "metric_tsv", typ="TsV", xpath="0,0", all_branches=True + "metric_tsv", typ="tsv", xpath="0,0", all_branches=True ) self.assertEqual(len(ret), 3) - self.assertTrue(ret["foo"]["metric_tsv"] == ["foo"]) - self.assertTrue(ret["bar"]["metric_tsv"] == ["bar"]) - self.assertTrue(ret["baz"]["metric_tsv"] == ["baz"]) + self.assertSequenceEqual(ret["foo"]["metric_tsv"], ["foo"]) + self.assertSequenceEqual(ret["bar"]["metric_tsv"], ["bar"]) + self.assertSequenceEqual(ret["baz"]["metric_tsv"], ["baz"]) ret = self.dvc.metrics.show( "metric_htsv", typ="htsv", xpath="0,branch", all_branches=True ) self.assertEqual(len(ret), 3) - self.assertTrue(ret["foo"]["metric_htsv"] == ["foo"]) - self.assertTrue(ret["bar"]["metric_htsv"] == ["bar"]) - self.assertTrue(ret["baz"]["metric_htsv"] == ["baz"]) + self.assertSequenceEqual(ret["foo"]["metric_htsv"], ["foo"]) + self.assertSequenceEqual(ret["bar"]["metric_htsv"], ["bar"]) + self.assertSequenceEqual(ret["baz"]["metric_htsv"], ["baz"]) ret = self.dvc.metrics.show( - "metric_csv", typ="CSV", xpath="0,0", all_branches=True + "metric_csv", typ="csv", xpath="0,0", all_branches=True ) self.assertEqual(len(ret), 3) - self.assertTrue(ret["foo"]["metric_csv"] == ["foo"]) - self.assertTrue(ret["bar"]["metric_csv"] == ["bar"]) - self.assertTrue(ret["baz"]["metric_csv"] == ["baz"]) + self.assertSequenceEqual(ret["foo"]["metric_csv"], ["foo"]) + self.assertSequenceEqual(ret["bar"]["metric_csv"], ["bar"]) + self.assertSequenceEqual(ret["baz"]["metric_csv"], ["baz"]) ret = self.dvc.metrics.show( "metric_hcsv", typ="hcsv", xpath="0,branch", all_branches=True ) self.assertEqual(len(ret), 3) - self.assertTrue(ret["foo"]["metric_hcsv"] == ["foo"]) - self.assertTrue(ret["bar"]["metric_hcsv"] == ["bar"]) - self.assertTrue(ret["baz"]["metric_hcsv"] == ["baz"]) + self.assertSequenceEqual(ret["foo"]["metric_hcsv"], ["foo"]) + self.assertSequenceEqual(ret["bar"]["metric_hcsv"], ["bar"]) + self.assertSequenceEqual(ret["baz"]["metric_hcsv"], ["baz"]) + + def test_unknown_type_ignored(self): + ret = self.dvc.metrics.show( + "metric_hcsv", typ="unknown", xpath="0,branch", all_branches=True + ) + self.assertEqual(len(ret), 3) + for b in ["foo", "bar", "baz"]: + self.assertSequenceEqual(ret[b]["metric_hcsv"], "branch\n" + b) + + def test_type_case_normalized(self): + ret = self.dvc.metrics.show( + "metric_hcsv", typ=" hCSV ", xpath="0,branch", all_branches=True + ) + self.assertEqual(len(ret), 3) + for b in ["foo", "bar", "baz"]: + self.assertSequenceEqual(ret[b]["metric_hcsv"], [b]) + + def test_xpath_is_empty(self): + ret = self.dvc.metrics.show( + "metric_json", typ="json", xpath="", all_branches=True + ) + self.assertEqual(len(ret), 3) + for b in ["foo", "bar", "baz"]: + self.assertEqual(ret[b]["metric_json"], json.dumps({"branch": b})) + + def test_xpath_is_none(self): + ret = self.dvc.metrics.show( + "metric_json", typ="json", xpath=None, all_branches=True + ) + self.assertEqual(len(ret), 3) + for b in ["foo", "bar", "baz"]: + self.assertEqual(ret[b]["metric_json"], json.dumps({"branch": b})) + + def test_xpath_all_columns(self): + ret = self.dvc.metrics.show( + "metric_hcsv", typ="hcsv ", xpath="0,", all_branches=True + ) + self.assertEqual(len(ret), 3) + for b in ["foo", "bar", "baz"]: + self.assertSequenceEqual(ret[b]["metric_hcsv"], [b]) + + def test_xpath_all_rows(self): + ret = self.dvc.metrics.show( + "metric_csv", typ="csv", xpath=",0", all_branches=True + ) + self.assertEqual(len(ret), 3) + for b in ["foo", "bar", "baz"]: + self.assertSequenceEqual(ret[b]["metric_csv"], [b]) + + def test_xpath_all(self): + ret = self.dvc.metrics.show( + "metric_csv", typ="csv", xpath=",", all_branches=True + ) + self.assertEqual(len(ret), 3) + for b in ["foo", "bar", "baz"]: + self.assertSequenceEqual(ret[b]["metric_csv"], [[b]]) + + def test_xpath_all_with_header(self): + ret = self.dvc.metrics.show( + "metric_hcsv", typ="hcsv", xpath=",", all_branches=True + ) + self.assertEqual(len(ret), 3) + for b in ["foo", "bar", "baz"]: + self.assertSequenceEqual(ret[b]["metric_hcsv"], [[b]]) class TestMetricsRecursive(TestDvc): @@ -181,9 +248,6 @@ class TestMetricsReproCLI(TestDvc): ret = main(["metrics", "add", "metrics"]) self.assertEqual(ret, 0) - ret = main(["metrics", "modify", "-t", "XSV", "-x", "0,0", "metrics"]) - self.assertEqual(ret, 1) - ret = main(["metrics", "modify", "-t", "CSV", "-x", "0,0", "metrics"]) self.assertEqual(ret, 0) @@ -223,12 +287,10 @@ class TestMetricsCLI(TestMetrics): ] ) self.assertEqual(ret, 0) - ret = main( ["metrics", "show", "-a", "metric_tsv", "-t", "tsv", "-x", "0,0"] ) self.assertEqual(ret, 0) - ret = main( [ "metrics", @@ -244,7 +306,7 @@ class TestMetricsCLI(TestMetrics): self.assertEqual(ret, 0) ret = main( - ["metrics", "show", "-a", "metric_csv", "-t", "CSV", "-x", "0,0"] + ["metrics", "show", "-a", "metric_csv", "-t", "csv", "-x", "0,0"] ) self.assertEqual(ret, 0) @@ -287,6 +349,80 @@ class TestMetricsCLI(TestMetrics): ret = main(["metrics", "remove", "non-existing"]) self.assertNotEqual(ret, 0) + def test_wrong_type_add(self): + with open("metric.unknown", "w+") as fd: + fd.write("unknown") + fd.flush() + + ret = main(["add", "metric.unknown"]) + self.assertEqual(ret, 0) + + logger.logger.handlers[1].stream = StringIO() + ret = main(["metrics", "add", "metric.unknown", "-t", "unknown"]) + self.assertEqual(ret, 1) + self.assertIn( + "failed to add metric file 'metric.unknown' - metric type 'unknown'" + " is not supported, must be one of [raw, json, csv, tsv, hcsv, htsv]", + logger.logger.handlers[1].stream.getvalue(), + ) + + ret = main(["metrics", "add", "metric.unknown", "-t", "raw"]) + self.assertEqual(ret, 0) + + logger.logger.handlers[0].stream = StringIO() + ret = main(["metrics", "show", "metric.unknown"]) + self.assertEqual(ret, 0) + self.assertIn( + "\tmetric.unknown: unknown", + logger.logger.handlers[0].stream.getvalue(), + ) + + def test_wrong_type_modify(self): + with open("metric.unknown", "w+") as fd: + fd.write("unknown") + fd.flush() + + ret = main(["run", "-m", "metric.unknown"]) + self.assertEqual(ret, 0) + + logger.logger.handlers[1].stream = StringIO() + ret = main(["metrics", "modify", "metric.unknown", "-t", "unknown"]) + self.assertEqual(ret, 1) + self.assertIn( + "failed to modify metric file settings - metric type 'unknown'" + " is not supported, must be one of [raw, json, csv, tsv, hcsv, htsv]", + logger.logger.handlers[1].stream.getvalue(), + ) + + ret = main(["metrics", "modify", "metric.unknown", "-t", "CSV"]) + self.assertEqual(ret, 0) + + logger.logger.handlers[0].stream = StringIO() + ret = main(["metrics", "show", "metric.unknown"]) + self.assertEqual(ret, 0) + self.assertIn( + "\tmetric.unknown: unknown", + logger.logger.handlers[0].stream.getvalue(), + ) + + def test_wrong_type_show(self): + with open("metric.unknown", "w+") as fd: + fd.write("unknown") + fd.flush() + + ret = main(["run", "-m", "metric.unknown"]) + self.assertEqual(ret, 0) + + logger.logger.handlers[0].stream = StringIO() + ret = main( + ["metrics", "show", "metric.unknown", "-t", "unknown", "-x", "0,0"] + ) + self.assertEqual(ret, 0) + self.assertIn( + "\tmetric.unknown: unknown", + logger.logger.handlers[0].stream.getvalue(), + ) + class TestNoMetrics(TestDvc): def test(self):