metrics: fix bad metric file case handling

Also some refactoring got into this, sorry :(

Signed-off-by: Ruslan Kuprieiev <[email protected]>

dvc/command/metrics.py

@@ -6,29 +6,41 @@ from dvc.command.base import CmdBase, fix_subparsers
 
 
 class CmdMetricsShow(CmdBase):
-    def run(self):
-        try:
-            # 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
+    def _show(self, metrics):
+        for branch, val in metrics.items():
+            if self.args.all_branches or self.args.all_tags:
+                logger.info("{}:".format(branch))
 
-            self.repo.metrics.show(
+            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()
+        try:
+            metrics = self.repo.metrics.show(
                 self.args.path,
                 typ=typ,
                 xpath=xpath,
@@ -36,6 +48,8 @@ class CmdMetricsShow(CmdBase):
                 all_tags=self.args.all_tags,
                 recursive=self.args.recursive,
             )
+
+            self._show(metrics)
         except DvcException:
             logger.error("failed to show metrics")
             return 1

dvc/exceptions.py

@@ -4,6 +4,7 @@ from __future__ import unicode_literals
 
 from dvc.utils.compat import str, builtin_str
 
+import os
 import traceback
 
 
@@ -51,6 +52,21 @@ class OutputDuplicationError(DvcException):
         super(OutputDuplicationError, self).__init__(msg)
 
 
+class OutputNotFoundError(DvcException):
+    """Thrown if a file/directory not found in repository pipelines.
+
+    Args:
+        output (unicode): path to the file/directory.
+    """
+
+    def __init__(self, output):
+        super(OutputNotFoundError, self).__init__(
+            "unable to find stage file with output '{path}'".format(
+                path=os.path.relpath(output)
+            )
+        )
+
+
 class WorkingDirectoryAsOutputError(DvcException):
     """Thrown if directory that stage is going to be saved in is specified as
     an output of anothe stage.
@@ -175,3 +191,20 @@ class ReproductionError(DvcException):
         self.path = dvc_file_name
         msg = "failed to reproduce '{}'".format(dvc_file_name)
         super(ReproductionError, self).__init__(msg, cause=ex)
+
+
+class BadMetricError(DvcException):
+    def __init__(self, path):
+        super(BadMetricError, self).__init__(
+            "'{}' does not exist, not a metric or is malformed".format(
+                os.path.relpath(path)
+            )
+        )
+
+
+class NoMetricsError(DvcException):
+    def __init__(self):
+        super(NoMetricsError, self).__init__(
+            "no metric files in this repository. "
+            "Use 'dvc metrics add' to add a metric file to track."
+        )

dvc/repo/__init__.py

@@ -7,7 +7,7 @@ import os
 import dvc.prompt as prompt
 import dvc.logger as logger
 
-from dvc.exceptions import DvcException, NotDvcRepoError
+from dvc.exceptions import DvcException, NotDvcRepoError, OutputNotFoundError
 
 
 class Repo(object):
@@ -299,7 +299,7 @@ class Repo(object):
 
         G = nx.DiGraph()
         G_active = nx.DiGraph()
-        stages = stages or self.stages(from_directory)
+        stages = stages or self.stages(from_directory, check_dag=False)
         stages = [stage for stage in stages if stage]
         outs = []
 
@@ -361,7 +361,7 @@ class Repo(object):
             G.subgraph(c).copy() for c in nx.weakly_connected_components(G)
         ]
 
-    def stages(self, from_directory=None):
+    def stages(self, from_directory=None, check_dag=True):
         """
         Walks down the root directory looking for Dvcfiles,
         skipping the directories that are related with
@@ -399,6 +399,9 @@ class Repo(object):
 
             dirs[:] = list(filter(filter_dirs, dirs))
 
+        if check_dag:
+            self.check_dag(stages)
+
         return stages
 
     def active_stages(self, from_directory=None):
@@ -408,3 +411,25 @@ class Repo(object):
         for G in self.pipelines(from_directory):
             stages.extend(list(nx.get_node_attributes(G, "stage").values()))
         return stages
+
+    def find_outs_by_path(self, path, outs=None, recursive=False):
+        if not outs:
+            outs = [out for stage in self.stages() for out in stage.outs]
+
+        abs_path = os.path.abspath(path)
+        is_dir = os.path.isdir(abs_path)
+
+        def func(out):
+            if out.path == abs_path:
+                return True
+
+            if is_dir and recursive and out.path.startswith(abs_path + os.sep):
+                return True
+
+            return False
+
+        matched = list(filter(func, outs))
+        if not matched:
+            raise OutputNotFoundError(path)
+
+        return matched

dvc/repo/metrics/modify.py

@@ -2,44 +2,12 @@ from __future__ import unicode_literals
 
 import os
 
-from dvc.exceptions import DvcException, OutputDuplicationError
-
-
-def find_output_by_path(repo, path, outs=None, recursive=False):
-    if not outs:
-        astages = repo.active_stages()
-        outs = [out for stage in astages for out in stage.outs]
-
-    abs_path = os.path.abspath(path)
-    if os.path.isdir(abs_path) and recursive:
-        matched = [
-            out
-            for out in outs
-            if os.path.abspath(out.path).startswith(abs_path)
-        ]
-    else:
-        matched = [out for out in outs if out.path == abs_path]
-        stages = [out.stage.relpath for out in matched]
-        if len(stages) > 1:
-            raise OutputDuplicationError(path, stages)
-
-    return matched if matched else []
+from dvc.exceptions import DvcException
 
 
 def modify(repo, path, typ=None, xpath=None, delete=False):
-    outs = find_output_by_path(repo, path)
-
-    if not outs:
-        msg = "unable to find file '{}' in the pipeline".format(path)
-        raise DvcException(msg)
-
-    if len(outs) != 1:
-        msg = (
-            "-R not yet supported for metrics modify. "
-            "Make sure only one metric is referred to by '{}'".format(path)
-        )
-        raise DvcException(msg)
-
+    outs = repo.find_outs_by_path(path)
+    assert len(outs) == 1
     out = outs[0]
 
     if out.scheme != "local":

dvc/repo/metrics/show.py

@@ -6,8 +6,13 @@ import json
 from jsonpath_rw import parse
 
 import dvc.logger as logger
-from dvc.exceptions import OutputDuplicationError, DvcException
-from dvc.repo.metrics.modify import find_output_by_path
+from dvc.exceptions import (
+    OutputDuplicationError,
+    DvcException,
+    OutputNotFoundError,
+    BadMetricError,
+    NoMetricsError,
+)
 from dvc.utils.compat import str, builtin_str, open
 
 
@@ -67,6 +72,50 @@ def _read_metric(path, typ=None, xpath=None):
     return ret
 
 
+def _collect_metrics(self, path, recursive, typ, xpath):
+    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:
+            return []
+
+    res = []
+    for o in outs:
+        if not o.metric:
+            continue
+
+        if not typ and isinstance(o.metric, dict):
+            t = o.metric.get(o.PARAM_METRIC_TYPE, typ)
+            x = o.metric.get(o.PARAM_METRIC_XPATH, xpath)
+        else:
+            t = typ
+            x = xpath
+
+        res.append((o, t, x))
+
+    return res
+
+
+def _read_metrics(self, metrics):
+    res = {}
+    for out, typ, xpath in metrics:
+        assert out.scheme == "local"
+        if out.use_cache:
+            path = self.cache.local.get(out.checksum)
+        else:
+            path = out.path
+
+        metric = _read_metric(path, typ=typ, xpath=xpath)
+        if not metric:
+            continue
+
+        res[out.rel_path] = metric
+
+    return res
+
+
 def show(
     self,
     path=None,
@@ -80,71 +129,14 @@ def show(
     for branch in self.scm.brancher(
         all_branches=all_branches, all_tags=all_tags
     ):
-        astages = self.active_stages()
-        outs = [out for stage in astages for out in stage.outs]
+        entries = _collect_metrics(self, path, recursive, typ, xpath)
+        metrics = _read_metrics(self, entries)
+        if metrics:
+            res[branch] = metrics
 
+    if not res:
         if path:
-            outs = find_output_by_path(
-                self, path, outs=outs, recursive=recursive
-            )
+            raise BadMetricError(path)
+        raise NoMetricsError()
 
-        metrics = filter(lambda o: o.metric, outs)
-        stages = set()
-        entries = []
-
-        for o in metrics:
-            if not typ and isinstance(o.metric, dict):
-                t = o.metric.get(o.PARAM_METRIC_TYPE, typ)
-                x = o.metric.get(o.PARAM_METRIC_XPATH, xpath)
-            else:
-                t = typ
-                x = xpath
-            entries.append((o.path, t, x))
-            stages.add(o.stage.path)
-
-        if path and not entries:
-            if os.path.isdir(path):
-                logger.warning(
-                    "Path '{path}' is a directory. "
-                    "Consider running with '-R'.".format(path=path)
-                )
-                return {}
-
-        for fname, t, x in entries:
-            if stages:
-                for stage in stages:
-                    self.checkout(stage, force=True)
-
-            rel = os.path.relpath(fname)
-            metric = _read_metric(fname, typ=t, xpath=x)
-            if not metric:
-                continue
-
-            if branch not in res:
-                res[branch] = {}
-
-            res[branch][rel] = metric
-
-    for branch, val in res.items():
-        if all_branches or all_tags:
-            logger.info("{}:".format(branch))
-        for fname, metric in val.items():
-            logger.info("\t{}: {}".format(fname, metric))
-
-    if res:
-        return res
-
-    if path and os.path.isdir(path):
-        return res
-
-    if path:
-        msg = "file '{}' does not exist, not a metric file or is malformed".format(
-            path
-        )
-    else:
-        msg = (
-            "no metric files in this repository."
-            " use 'dvc metrics add' to add a metric file to track."
-        )
-
-    raise DvcException(msg)
+    return res

dvc/repo/move.py

@@ -34,19 +34,10 @@ def move(self, from_path, to_path):
 
     to_path = _expand_target_path(from_path, to_path)
 
-    try:
-        stage, out = next(
-            (stage, out)
-            for stage in self.stages()
-            for out in stage.outs
-            if from_out.path == out.path
-        )
-    except StopIteration:
-        raise DvcException(
-            "unable to find stage file with output '{path}'".format(
-                path=from_path
-            )
-        )
+    outs = self.find_outs_by_path(from_out.path)
+    assert len(outs) == 1
+    out = outs[0]
+    stage = out.stage
 
     if not stage.is_data_source:
         raise MoveNotDataSourceError(stage.relpath)

tests/test_metrics.py

@@ -3,7 +3,7 @@ import json
 
 from dvc.repo import Repo as DvcRepo
 from dvc.main import main
-from dvc.exceptions import DvcException
+from dvc.exceptions import DvcException, BadMetricError, NoMetricsError
 from tests.basic_env import TestDvc
 
 
@@ -144,11 +144,10 @@ class TestMetricsRecursive(TestDvc):
         self.dvc.scm.checkout("master")
 
     def test(self):
-
-        ret = self.dvc.metrics.show(
-            "nested", all_branches=True, recursive=False
-        )
-        self.assertEqual(len(ret), 0)
+        with self.assertRaises(BadMetricError):
+            ret = self.dvc.metrics.show(
+                "nested", all_branches=True, recursive=False
+            )
 
         ret = self.dvc.metrics.show(
             "nested", all_branches=True, recursive=True
@@ -357,6 +356,10 @@ class TestMetricsCLI(TestMetrics):
 
 class TestNoMetrics(TestDvc):
     def test(self):
+        with self.assertRaises(NoMetricsError):
+            self.dvc.metrics.show()
+
+    def test_cli(self):
         ret = main(["metrics", "show"])
         self.assertNotEqual(ret, 0)