Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

plots/metrics: fix some communication disrepancies #4837

Merged
merged 4 commits into from
Nov 7, 2020
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion dvc/command/plots.py
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ def run(self):
)

except DvcException:
logger.exception("")
logger.exception("failed to show plots")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for a post-merge review, but we are going away from these obvious messages and deleting them everywhere. Are you sure we need it here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

not necessarily, I added it only to make it consistent with metrics - I guess we could remove it in both places. How about that?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pared Yeah, better to remove in both cases.

return 1

return 0
Expand Down
17 changes: 13 additions & 4 deletions dvc/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -174,11 +174,20 @@ class NoMetricsError(DvcException):
pass


class NoPlotsError(DvcException):
def __init__(self):
class NoMetricsParsedError(NoMetricsError):
def __init__(self, command):
super().__init__(
f"Could not parse {command} files. Use `-v` option to see more "
"details."
)


class NoMetricsFoundError(NoMetricsError):
def __init__(self, command, run_options):
super().__init__(
"no plots in this repository. Use `--plots/--plots-no-cache` "
"options for `dvc run` to mark stage outputs as plots."
f"No {command} files in this repository. "
f"Use `{run_options}` options for "
f"`dvc run` to mark stage outputs as {command}."
)


Expand Down
13 changes: 3 additions & 10 deletions dvc/repo/metrics/show.py
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
import logging

from dvc.exceptions import NoMetricsError
from dvc.exceptions import NoMetricsFoundError, NoMetricsParsedError
from dvc.repo import locked
from dvc.repo.collect import collect
from dvc.scm.base import SCMError
Expand Down Expand Up @@ -104,16 +104,9 @@ def show(

if not res:
if metrics_found:
msg = (
"Could not parse metrics files. Use `-v` option to see more "
"details."
)
raise NoMetricsParsedError("metrics")
else:
msg = (
"no metrics files in this repository. Use `-m/-M` options for "
"`dvc run` to mark stage outputs as metrics."
)
raise NoMetricsError(msg)
raise NoMetricsFoundError("metrics", "-m/-M")

# Hide workspace metrics if they are the same as in the active branch
try:
Expand Down
31 changes: 25 additions & 6 deletions dvc/repo/plots/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,13 @@

from funcy import cached_property, first, project

from dvc.exceptions import DvcException, NoPlotsError
from dvc.exceptions import (
DvcException,
NoMetricsFoundError,
NoMetricsParsedError,
)
from dvc.repo.collect import collect
from dvc.repo.plots.data import PlotParsingError
from dvc.schema import PLOT_PROPS
from dvc.tree.repo import RepoTree
from dvc.utils import relpath
Expand Down Expand Up @@ -71,10 +76,24 @@ def render(data, revs=None, props=None, templates=None):
# Merge data by plot file and apply overriding props
plots = _prepare_plots(data, revs, props)

return {
datafile: _render(datafile, desc["data"], desc["props"], templates)
for datafile, desc in plots.items()
}
result = {}
for datafile, desc in plots.items():
try:
result[datafile] = _render(
datafile, desc["data"], desc["props"], templates
)
except PlotParsingError as e:
logger.debug(
"failed to read '%s' on '%s'",
e.path,
e.revision,
exc_info=True,
)

if not any(result.values()):
raise NoMetricsParsedError("plots")

return result

def show(self, targets=None, revs=None, props=None, templates=None):
from .data import NoMetricInHistoryError
Expand All @@ -90,7 +109,7 @@ def show(self, targets=None, revs=None, props=None, templates=None):

# No data at all is a special error with a special message
if not data:
raise NoPlotsError()
raise NoMetricsFoundError("plots", "--plots/--plots-no-cache")

if templates is None:
templates = self.templates
Expand Down
25 changes: 19 additions & 6 deletions dvc/repo/plots/data.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,13 @@
import csv
import io
import json
import os
from collections import OrderedDict
from copy import copy

from funcy import first
from funcy import first, reraise

from dvc.exceptions import DvcException
from dvc.utils.serialize import loads_yaml
from dvc.utils.serialize import ParseError, parse_json, parse_yaml


class PlotMetricTypeError(DvcException):
Expand All @@ -32,6 +31,14 @@ def __init__(self, path):
super().__init__(f"Could not find '{path}'.")


class PlotParsingError(ParseError):
def __init__(self, path, revision):
self.path = path
self.revision = revision

super().__init__(path, f"revision: {revision}")


def plot_data(filename, revision, content):
_, extension = os.path.splitext(filename.lower())
if extension == ".json":
Expand Down Expand Up @@ -159,7 +166,11 @@ def _processors(self):
return [_filter_fields, _append_index, _append_revision]

def to_datapoints(self, **kwargs):
data = self.raw(**kwargs)
with reraise(
[ParseError, csv.Error],
PlotParsingError(self.filename, self.revision),
):
data = self.raw(**kwargs)

for data_proc in self._processors():
data = data_proc(
Expand All @@ -170,7 +181,9 @@ def to_datapoints(self, **kwargs):

class JSONPlotData(PlotData):
def raw(self, **kwargs):
return json.loads(self.content, object_pairs_hook=OrderedDict)
return parse_json(
self.content, self.filename, object_pairs_hook=OrderedDict
)

def _processors(self):
parent_processors = super()._processors()
Expand Down Expand Up @@ -207,7 +220,7 @@ def raw(self, header=True, **kwargs): # pylint: disable=arguments-differ

class YAMLPlotData(PlotData):
def raw(self, **kwargs):
return loads_yaml(self.content, typ="rt")
return parse_yaml(self.content, self.filename, typ="rt")

def _processors(self):
parent_processors = super()._processors()
Expand Down
4 changes: 2 additions & 2 deletions dvc/utils/serialize/_json.py
Original file line number Diff line number Diff line change
Expand Up @@ -14,9 +14,9 @@ def load_json(path, tree=None):
return _load_data(path, parser=parse_json, tree=tree)


def parse_json(text, path):
def parse_json(text, path, **kwargs):
with reraise(json.JSONDecodeError, JSONFileCorruptedError(path)):
return json.loads(text) or {}
return json.loads(text, **kwargs) or {}


def dump_json(path, data, tree=None):
Expand Down
15 changes: 0 additions & 15 deletions tests/func/metrics/metrics/test_show.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,15 +3,9 @@
import pytest

from dvc.repo import Repo
from dvc.repo.metrics.show import NoMetricsError
from dvc.utils.fs import remove


def test_show_empty(dvc):
with pytest.raises(NoMetricsError):
dvc.metrics.show()


Comment on lines -10 to -14
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to test_common.py::test_show_no_metrics_files

def test_show_simple(tmp_dir, dvc, run_copy_metrics):
tmp_dir.gen("metrics_t.yaml", "1.1")
run_copy_metrics(
Expand All @@ -36,15 +30,6 @@ def test_show_multiple(tmp_dir, dvc, run_copy_metrics):
assert dvc.metrics.show() == {"": {"foo": {"foo": 1}, "baz": {"baz": 2}}}


def test_show_invalid_metric(tmp_dir, dvc, run_copy_metrics):
tmp_dir.gen("metrics_temp.yaml", "foo:\n- bar\n- baz\nxyz: string")
run_copy_metrics(
"metrics_temp.yaml", "metrics.yaml", metrics=["metrics.yaml"]
)
with pytest.raises(NoMetricsError):
dvc.metrics.show()
Comment on lines -39 to -45
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to test_common.py::test_show_malformed_metric.



def test_show_branch(tmp_dir, scm, dvc, run_copy_metrics):
tmp_dir.gen("metrics_temp.yaml", "foo: 1")
run_copy_metrics(
Expand Down
7 changes: 0 additions & 7 deletions tests/func/metrics/plots/test_show.py
Original file line number Diff line number Diff line change
Expand Up @@ -398,13 +398,6 @@ def _replace(path, src, dst):
path.write_text(path.read_text().replace(src, dst))


def test_no_plots(tmp_dir, dvc):
from dvc.exceptions import NoPlotsError

with pytest.raises(NoPlotsError):
dvc.plots.show()
Comment on lines -401 to -405
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved to test_common.py::test_show_no_metrics_files



def test_should_raise_on_no_template(tmp_dir, dvc, run_copy_metrics):
metric = [{"val": 2}, {"val": 3}]
_write_json(tmp_dir, metric, "metric_t.json")
Expand Down
53 changes: 41 additions & 12 deletions tests/func/metrics/test_common.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,28 +2,57 @@

import pytest

from dvc.main import main
from tests.func.metrics.utils import _write_json


def metrics_diff(dvc, filename, revision):
dvc.metrics.diff(targets=[filename], a_rev=revision)


def plots_diff(dvc, filename, revision):
dvc.plots.diff(targets=[filename], revs=[revision])


@pytest.mark.parametrize(
"diff_fun, metric_value",
((metrics_diff, {"m": 1}), (plots_diff, [{"m": 1}, {"m": 2}])),
"command, metric_value",
(("metrics", {"m": 1}), ("plots", [{"m": 1}, {"m": 2}])),
)
def test_diff_no_file_on_target_rev(
tmp_dir, scm, dvc, caplog, diff_fun, metric_value
tmp_dir, scm, dvc, caplog, command, metric_value
):
with tmp_dir.branch("new_branch", new=True):
_write_json(tmp_dir, metric_value, "metric.json")

with caplog.at_level(logging.WARNING, "dvc"):
diff_fun(dvc, "metric.json", "master")
assert (
main([command, "diff", "master", "--targets", "metric.json"])
== 0
)

assert "'metric.json' was not found at: 'master'." in caplog.text


@pytest.mark.parametrize(
"command, malformed_metric",
(("metrics", '{"m": 1'), ("plots", '[{"m": 1}, {"m": 2}'),),
)
def test_show_malformed_metric(
tmp_dir, scm, dvc, caplog, command, malformed_metric
):
tmp_dir.gen("metric.json", malformed_metric)

with caplog.at_level(logging.ERROR, "dvc"):
assert main([command, "show", "metric.json"]) == 1

assert (
f"Could not parse {command} files. "
"Use `-v` option to see more details."
) in caplog.text


@pytest.mark.parametrize(
"command, run_options",
(("metrics", "-m/-M"), ("plots", "--plots/--plots-no-cache"),),
)
def test_show_no_metrics_files(tmp_dir, dvc, caplog, command, run_options):
with caplog.at_level(logging.ERROR, "dvc"):
assert main([command, "show"]) == 1

assert (
f"No {command} files in this repository. "
f"Use `{run_options}` options for "
f"`dvc run` to mark stage outputs as {command}."
) in caplog.text
Comment on lines +54 to +58
Copy link
Contributor

@efiop efiop Nov 7, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is really an exception that you are testing, it would be better to test it without going through CLI, as we did before. Just a bit cleaner. Same with tests above.