Repository: iterative/dvc
Pull request #1624 (2019-02-17 23:15:07)

Cleanup metrics
* `dvc metrics *` CLI help cleanup.
* Remove deprecated `*-path` commands and related stuff.
* Apply lower() and check metrics type in metrics modify/add, metrics show.
* Change `error` -> `warning` in metrics show when it fails to read a file. 
* Make CSV and TSV path specification and implementation consistent with documentation and general conventions - first always come ROW index, then COLUMN (index, or name).
* Minor cleanup (unused imports, etc)



Patch:
diff --git a/dvc/command/metrics.py b/dvc/command/metrics.py
index ce014e436..bc7f6f5e7 100644
--- a/dvc/command/metrics.py
+++ b/dvc/command/metrics.py
@@ -14,31 +14,9 @@ class CmdMetricsShow(CmdBase):
             for fname, metric in val.items():
                 logger.info("\t{}: {}".format(fname, metric))
 
-    def _get_type_xpath(self):
-        # backward compatibility
-        if self.args.json_path:
-            typ = "json"
-            xpath = self.args.json_path
-        elif self.args.tsv_path:
-            typ = "tsv"
-            xpath = self.args.tsv_path
-        elif self.args.htsv_path:
-            typ = "htsv"
-            xpath = self.args.htsv_path
-        elif self.args.csv_path:
-            typ = "csv"
-            xpath = self.args.csv_path
-        elif self.args.hcsv_path:
-            typ = "hcsv"
-            xpath = self.args.hcsv_path
-        else:
-            typ = self.args.type
-            xpath = self.args.xpath
-
-        return typ, xpath
-
     def run(self):
-        typ, xpath = self._get_type_xpath()
+        typ = self.args.type
+        xpath = self.args.xpath
         try:
             metrics = self.repo.metrics.show(
                 self.args.path,
@@ -64,7 +42,7 @@ class CmdMetricsModify(CmdBase):
                 self.args.path, typ=self.args.type, xpath=self.args.xpath
             )
         except DvcException:
-            logger.error("failed to modify metrics")
+            logger.error("failed to modify metric file settings")
             return 1
 
         return 0
@@ -97,7 +75,7 @@ class CmdMetricsRemove(CmdBase):
 
 
 def add_parser(subparsers, parent_parser):
-    METRICS_HELP = "Get metrics from all branches."
+    METRICS_HELP = "A set of commands to add, manage, collect and display project metrics."
     metrics_parser = subparsers.add_parser(
         "metrics",
         parents=[parent_parser],
@@ -107,12 +85,12 @@ def add_parser(subparsers, parent_parser):
 
     metrics_subparsers = metrics_parser.add_subparsers(
         dest="cmd",
-        help="Use dvc metrics CMD --help for command-specific help.",
+        help="Use dvc metrics CMD --help to display command-specific help.",
     )
 
     fix_subparsers(metrics_subparsers)
 
-    METRICS_SHOW_HELP = "Show metrics."
+    METRICS_SHOW_HELP = "Output metric values."
     metrics_show_parser = metrics_subparsers.add_parser(
         "show",
         parents=[parent_parser],
@@ -120,27 +98,13 @@ def add_parser(subparsers, parent_parser):
         help=METRICS_SHOW_HELP,
     )
     metrics_show_parser.add_argument(
-        "path", nargs="?", help="Path to metrics file or directory"
+        "path", nargs="?", help="Path to a metric file or a directory."
     )
     metrics_show_parser.add_argument(
-        "-t", "--type", help="Type of metrics(RAW/JSON/TSV/HTSV/CSV/HCSV)."
+        "-t", "--type", help="Type of metrics (raw/json/tsv/htsv/csv/hcsv)."
     )
     metrics_show_parser.add_argument(
-        "-x", "--xpath", help="JSON/TSV/HTSV/CSV/HCSV path."
-    )
-    metrics_show_group = metrics_show_parser.add_mutually_exclusive_group()
-    metrics_show_group.add_argument("--json-path", help="JSON path.")
-    metrics_show_group.add_argument(
-        "--tsv-path", help="TSV path 'row,column' (e.g. '1,2')."
-    )
-    metrics_show_group.add_argument(
-        "--htsv-path", help="Headed TSV path 'row,column (e.g. 'Name,3')."
-    )
-    metrics_show_group.add_argument(
-        "--csv-path", help="CSV path 'row,column' (e.g. '1,2')."
-    )
-    metrics_show_group.add_argument(
-        "--hcsv-path", help="Headed CSV path 'row,column' (e.g. 'Name,3')."
+        "-x", "--xpath", help="json/tsv/htsv/csv/hcsv path."
     )
     metrics_show_parser.add_argument(
         "-a",
@@ -165,7 +129,7 @@ def add_parser(subparsers, parent_parser):
     )
     metrics_show_parser.set_defaults(func=CmdMetricsShow)
 
-    METRICS_ADD_HELP = "Add metrics."
+    METRICS_ADD_HELP = "Tag file as a metric file."
     metrics_add_parser = metrics_subparsers.add_parser(
         "add",
         parents=[parent_parser],
@@ -173,15 +137,15 @@ def add_parser(subparsers, parent_parser):
         help=METRICS_ADD_HELP,
     )
     metrics_add_parser.add_argument(
-        "-t", "--type", help="Type of metrics(RAW/JSON/TSV/HTSV/CSV/HCSV)."
+        "-t", "--type", help="Type of metrics (raw/json/tsv/htsv/csv/hcsv)."
     )
     metrics_add_parser.add_argument(
-        "-x", "--xpath", help="JSON/TSV/HTSV/CSV/HCSV path."
+        "-x", "--xpath", help="json/tsv/htsv/csv/hcsv path."
     )
-    metrics_add_parser.add_argument("path", help="Path to metrics file.")
+    metrics_add_parser.add_argument("path", help="Path to a metric file.")
     metrics_add_parser.set_defaults(func=CmdMetricsAdd)
 
-    METRICS_MODIFY_HELP = "Modify metrics."
+    METRICS_MODIFY_HELP = "Modify metric file options."
     metrics_modify_parser = metrics_subparsers.add_parser(
         "modify",
         parents=[parent_parser],
@@ -189,20 +153,20 @@ def add_parser(subparsers, parent_parser):
         help=METRICS_MODIFY_HELP,
     )
     metrics_modify_parser.add_argument(
-        "-t", "--type", help="Type of metrics(RAW/JSON/TSV/HTSV/CSV/HCSV)."
+        "-t", "--type", help="Type of metrics (raw/json/tsv/htsv/csv/hcsv)."
     )
     metrics_modify_parser.add_argument(
-        "-x", "--xpath", help="JSON/TSV/HTSV/CSV/HCSV path."
+        "-x", "--xpath", help="json/tsv/htsv/csv/hcsv path."
     )
-    metrics_modify_parser.add_argument("path", help="Metrics file.")
+    metrics_modify_parser.add_argument("path", help="Path to a metric file.")
     metrics_modify_parser.set_defaults(func=CmdMetricsModify)
 
-    METRICS_REMOVE_HELP = "Remove metrics."
+    METRICS_REMOVE_HELP = "Remove files's metric tag."
     metrics_remove_parser = metrics_subparsers.add_parser(
         "remove",
         parents=[parent_parser],
         description=METRICS_REMOVE_HELP,
         help=METRICS_REMOVE_HELP,
     )
-    metrics_remove_parser.add_argument("path", help="Path to metrics file.")
+    metrics_remove_parser.add_argument("path", help="Path to a metric file.")
     metrics_remove_parser.set_defaults(func=CmdMetricsRemove)
diff --git a/dvc/logger.py b/dvc/logger.py
index 2836bcffa..abc02aed4 100644
--- a/dvc/logger.py
+++ b/dvc/logger.py
@@ -29,11 +29,20 @@ def debug(message):
     logger.debug(out)
 
 
-def warning(message):
+def warning(message, parse_exception=False):
     """Prints a warning message."""
     prefix = colorize("Warning", color="yellow")
 
-    out = "{prefix}: {message}".format(prefix=prefix, message=message)
+    exception, stack_trace = None, ""
+    if parse_exception:
+        exception, stack_trace = _parse_exc()
+
+    out = "{prefix}: {description}".format(
+        prefix=prefix, description=_description(message, exception)
+    )
+
+    if stack_trace:
+        out += "\n{stack_trace}".format(stack_trace=stack_trace)
 
     logger.warning(out)
 
diff --git a/dvc/output/base.py b/dvc/output/base.py
index 9fb8bb1ca..c956e89c9 100644
--- a/dvc/output/base.py
+++ b/dvc/output/base.py
@@ -1,10 +1,10 @@
 from __future__ import unicode_literals
-from dvc.utils.compat import str
 
 import re
 from schema import Or, Optional
 
 from dvc.exceptions import DvcException
+from dvc.utils.compat import str
 
 
 class OutputDoesNotExistError(DvcException):
@@ -77,6 +77,12 @@ class OutputBase(object):
     def __str__(self):
         return self.url
 
+    @property
+    def is_local(self):
+        raise DvcException(
+            "is local is not supported for {}".format(self.scheme)
+        )
+
     @classmethod
     def match(cls, url):
         return re.match(cls.REMOTE.REGEX, url)
@@ -174,6 +180,11 @@ class OutputBase(object):
 
         return ret
 
+    def verify_metric(self):
+        raise DvcException(
+            "verify metric is not supported for {}".format(self.scheme)
+        )
+
     def download(self, to_info, resume=False):
         self.remote.download([self.path_info], [to_info], resume=resume)
 
diff --git a/dvc/output/local.py b/dvc/output/local.py
index 142d18e16..c330e09ea 100644
--- a/dvc/output/local.py
+++ b/dvc/output/local.py
@@ -67,7 +67,7 @@ class OutputLOCAL(OutputBase):
 
         return ret
 
-    def _verify_metric(self):
+    def verify_metric(self):
         if not self.metric:
             return
 
@@ -97,7 +97,7 @@ class OutputLOCAL(OutputBase):
 
         if not self.use_cache:
             self.info = self.remote.save_info(self.path_info)
-            self._verify_metric()
+            self.verify_metric()
             if not self.IS_DEPENDENCY:
                 msg = "Output '{}' doesn't use cache. Skipping saving."
                 logger.info(msg.format(self.rel_path))
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 f26b6ff62..541446b6a 100644
--- a/dvc/repo/metrics/modify.py
+++ b/dvc/repo/metrics/modify.py
@@ -1,11 +1,10 @@
 from __future__ import unicode_literals
 
-import os
-
 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]
@@ -14,12 +13,18 @@ def modify(repo, path, typ=None, xpath=None, delete=False):
         msg = "output '{}' scheme '{}' is not supported for metrics"
         raise DvcException(msg.format(out.path, out.path_info["scheme"]))
 
-    if typ:
+    if typ is not None:
+        typ = typ.lower().strip()
+        if typ not in ["raw", "json", "csv", "tsv", "hcsv", "htsv"]:
+            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
 
-    if xpath:
+    if xpath is not None:
         if not isinstance(out.metric, dict):
             out.metric = {}
         out.metric[out.PARAM_METRIC_XPATH] = xpath
@@ -27,6 +32,6 @@ def modify(repo, path, typ=None, xpath=None, delete=False):
     if delete:
         out.metric = None
 
-    out._verify_metric()
+    out.verify_metric()
 
     out.stage.dump()
diff --git a/dvc/repo/metrics/show.py b/dvc/repo/metrics/show.py
index 1024c89c0..f273d2c22 100644
--- a/dvc/repo/metrics/show.py
+++ b/dvc/repo/metrics/show.py
@@ -6,14 +6,8 @@ import json
 from jsonpath_rw import parse
 
 import dvc.logger as logger
-from dvc.exceptions import (
-    OutputDuplicationError,
-    DvcException,
-    OutputNotFoundError,
-    BadMetricError,
-    NoMetricsError,
-)
-from dvc.utils.compat import str, builtin_str, open
+from dvc.exceptions import OutputNotFoundError, BadMetricError, NoMetricsError
+from dvc.utils.compat import builtin_str, open
 
 
 def _read_metric_json(fd, json_path):
@@ -21,64 +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):
-    col, row = 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):
-    col, row = 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.error("unable to read metric in '{}'".format(path))
+        logger.warning(
+            "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 = []
@@ -98,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"
@@ -107,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
 
@@ -129,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
 


Test patch:
diff --git a/tests/test_metrics.py b/tests/test_metrics.py
index 8409850ef..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
         )
         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="branch,0", all_branches=True
+            "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
         )
         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.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="branch,0", all_branches=True
+            "metric_hcsv", typ="hcsv", xpath=",", 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"])
+        for b in ["foo", "bar", "baz"]:
+            self.assertSequenceEqual(ret[b]["metric_hcsv"], [[b]])
 
 
 class TestMetricsRecursive(TestDvc):
@@ -145,9 +212,7 @@ class TestMetricsRecursive(TestDvc):
 
     def test(self):
         with self.assertRaises(BadMetricError):
-            ret = self.dvc.metrics.show(
-                "nested", all_branches=True, recursive=False
-            )
+            self.dvc.metrics.show("nested", all_branches=True, recursive=False)
 
         ret = self.dvc.metrics.show(
             "nested", all_branches=True, recursive=True
@@ -183,7 +248,7 @@ class TestMetricsReproCLI(TestDvc):
         ret = main(["metrics", "add", "metrics"])
         self.assertEqual(ret, 0)
 
-        ret = main(["metrics", "modify", "-t", "csv", "-x", "0,0", "metrics"])
+        ret = main(["metrics", "modify", "-t", "CSV", "-x", "0,0", "metrics"])
         self.assertEqual(ret, 0)
 
         ret = main(["repro", "-f", "-m", stage.path])
@@ -203,73 +268,6 @@ class TestMetricsReproCLI(TestDvc):
             self.dvc.run(metrics_no_cache=["metrics_bin"])
 
 
-class TestMetricsCLICompat(TestMetrics):
-    def test(self):
-        # FIXME check output
-        ret = main(["metrics", "show", "--all-branches", "metric", "-v"])
-        self.assertEqual(ret, 0)
-
-        ret = main(
-            [
-                "metrics",
-                "show",
-                "--all-branches",
-                "metric_json",
-                "--json-path",
-                "branch",
-            ]
-        )
-        self.assertEqual(ret, 0)
-
-        ret = main(
-            [
-                "metrics",
-                "show",
-                "--all-branches",
-                "metric_tsv",
-                "--tsv-path",
-                "0,0",
-            ]
-        )
-        self.assertEqual(ret, 0)
-
-        ret = main(
-            [
-                "metrics",
-                "show",
-                "--all-branches",
-                "metric_htsv",
-                "--htsv-path",
-                "branch,0",
-            ]
-        )
-        self.assertEqual(ret, 0)
-
-        ret = main(
-            [
-                "metrics",
-                "show",
-                "--all-branches",
-                "metric_csv",
-                "--csv-path",
-                "0,0",
-            ]
-        )
-        self.assertEqual(ret, 0)
-
-        ret = main(
-            [
-                "metrics",
-                "show",
-                "--all-branches",
-                "metric_hcsv",
-                "--hcsv-path",
-                "branch,0",
-            ]
-        )
-        self.assertEqual(ret, 0)
-
-
 class TestMetricsCLI(TestMetrics):
     def test(self):
         # FIXME check output
@@ -289,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",
@@ -304,7 +300,7 @@ class TestMetricsCLI(TestMetrics):
                 "-t",
                 "htsv",
                 "-x",
-                "branch,0",
+                "0,branch",
             ]
         )
         self.assertEqual(ret, 0)
@@ -323,7 +319,7 @@ class TestMetricsCLI(TestMetrics):
                 "-t",
                 "hcsv",
                 "-x",
-                "branch,0",
+                "0,branch",
             ]
         )
         self.assertEqual(ret, 0)
@@ -353,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):