add tests for metrics wrong type and some other edge cases

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

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

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
 

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):