From a3099bca6a0d0f826a5d1033dc77c8a1bee95177 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Wed, 13 Feb 2019 06:32:12 +0200 Subject: [PATCH] metrics: fix bad metric file case handling Also some refactoring got into this, sorry :( Signed-off-by: Ruslan Kuprieiev --- dvc/command/metrics.py | 58 ++++++++++------- dvc/exceptions.py | 33 ++++++++++ dvc/repo/__init__.py | 31 ++++++++- dvc/repo/metrics/modify.py | 38 +---------- dvc/repo/metrics/show.py | 126 +++++++++++++++++-------------------- dvc/repo/move.py | 17 ++--- tests/test_metrics.py | 15 +++-- 7 files changed, 172 insertions(+), 146 deletions(-) diff --git a/dvc/command/metrics.py b/dvc/command/metrics.py index 522195dab..ce014e436 100644 --- a/dvc/command/metrics.py +++ b/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 diff --git a/dvc/exceptions.py b/dvc/exceptions.py index 74d41e93d..eff0ae227 100644 --- a/dvc/exceptions.py +++ b/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." + ) diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py index fe2911f4f..7b7a74291 100644 --- a/dvc/repo/__init__.py +++ b/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 diff --git a/dvc/repo/metrics/modify.py b/dvc/repo/metrics/modify.py index a45a93d68..f26b6ff62 100644 --- a/dvc/repo/metrics/modify.py +++ b/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": diff --git a/dvc/repo/metrics/show.py b/dvc/repo/metrics/show.py index 84f8b5cc1..1024c89c0 100644 --- a/dvc/repo/metrics/show.py +++ b/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 diff --git a/dvc/repo/move.py b/dvc/repo/move.py index e35e16ef8..6b1e0a6f6 100644 --- a/dvc/repo/move.py +++ b/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) diff --git a/tests/test_metrics.py b/tests/test_metrics.py index 24e699f79..8409850ef 100644 --- a/tests/test_metrics.py +++ b/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)