From 7a0540970a25339cdfa7a0fe860a2c6097f89676 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Thu, 9 Mar 2023 18:48:15 +0545 Subject: [PATCH 1/5] plots: return errors in json format --- dvc/commands/plots.py | 45 ++++++++++++++++++++++++--- dvc/render/match.py | 35 ++++++++++++++++++--- dvc/repo/plots/__init__.py | 4 +-- tests/integration/plots/test_plots.py | 29 ++++++++++++----- tests/unit/command/test_plots.py | 13 +++++--- tests/unit/render/test_match.py | 8 ++--- 6 files changed, 107 insertions(+), 27 deletions(-) diff --git a/dvc/commands/plots.py b/dvc/commands/plots.py index b999759482..eb3f5c1f97 100644 --- a/dvc/commands/plots.py +++ b/dvc/commands/plots.py @@ -1,6 +1,7 @@ import argparse import logging import os +from typing import TYPE_CHECKING, Dict, List from funcy import first @@ -11,14 +12,47 @@ from dvc.ui import ui from dvc.utils import format_link +if TYPE_CHECKING: + from dvc.render.match import RendererWithErrors + + logger = logging.getLogger(__name__) -def _show_json(renderers, split=False): +def _show_json(renderers_with_errors: List["RendererWithErrors"], split=False): from dvc.render.convert import to_json + from dvc.utils.serialize import encode_exception + + errors: List[Dict] = [] + out = {} + + for renderer, src_errors, def_errors in renderers_with_errors: + name = renderer.name + out[name] = to_json(renderer, split) + if src_errors: + errors.extend( + { + "name": name, + "revision": rev, + "source": source, + **encode_exception(e), + } + for rev, per_rev_src_errors in src_errors.items() + for source, e in per_rev_src_errors.items() + ) + if def_errors: + errors.extend( + { + "name": name, + "revision": rev, + **encode_exception(e), + } + for rev, e in def_errors.items() + ) - result = {renderer.name: to_json(renderer, split) for renderer in renderers} - ui.write_json(result, highlight=False) + if errors: + out = {"errors": errors, **out} + ui.write_json(out, highlight=False) def _adjust_vega_renderers(renderers): @@ -122,15 +156,16 @@ def run(self) -> int: # noqa: C901, PLR0911, PLR0912 renderers_out = out if self.args.json else os.path.join(out, "static") - renderers = match_defs_renderers( + renderers_with_errors = match_defs_renderers( data=plots_data, out=renderers_out, templates_dir=self.repo.plots.templates_dir, ) if self.args.json: - _show_json(renderers, self.args.split) + _show_json(renderers_with_errors, self.args.split) return 0 + renderers = [r.renderer for r in renderers_with_errors] _adjust_vega_renderers(renderers) if self.args.show_vega: renderer = first(filter(lambda r: r.TYPE == "vega", renderers)) diff --git a/dvc/render/match.py b/dvc/render/match.py index 196f47f65e..c3aed3db45 100644 --- a/dvc/render/match.py +++ b/dvc/render/match.py @@ -1,6 +1,6 @@ import os from collections import defaultdict -from typing import TYPE_CHECKING, Dict, List, Optional +from typing import TYPE_CHECKING, DefaultDict, Dict, List, NamedTuple, Optional import dpath import dpath.options @@ -13,6 +13,8 @@ if TYPE_CHECKING: from dvc.types import StrPath + from dvc_render.base import Renderer + dpath.options.ALLOW_EMPTY_STRING_KEYS = True @@ -61,26 +63,38 @@ def get_definition_data(self, target_files, rev): return result -def match_defs_renderers( +class RendererWithErrors(NamedTuple): + renderer: "Renderer" + source_errors: Dict[str, Dict[str, Exception]] + definition_errors: Dict[str, Exception] + + +def match_defs_renderers( # noqa: C901, PLR0912 data, out=None, templates_dir: Optional["StrPath"] = None, -): +) -> List[RendererWithErrors]: from dvc_render import ImageRenderer, VegaRenderer plots_data = PlotsData(data) renderers = [] renderer_cls = None + for plot_id, group in plots_data.group_definitions().items(): plot_datapoints: List[Dict] = [] props = _squash_plots_properties(group) final_props: Dict = {} + def_errors: Dict[str, Exception] = {} + src_errors: DefaultDict[str, Dict[str, Exception]] = defaultdict(dict) + if out is not None: props["out"] = out if templates_dir is not None: props["template_dir"] = templates_dir + from funcy import get_in + for rev, inner_id, plot_definition in group: plot_sources = infer_data_sources(inner_id, plot_definition) definitions_data = plots_data.get_definition_data(plot_sources, rev) @@ -94,13 +108,24 @@ def match_defs_renderers( converter = _get_converter(renderer_cls, inner_id, props, definitions_data) - dps, rev_props = converter.flat_datapoints(rev) + try: + dps, rev_props = converter.flat_datapoints(rev) + except Exception as e: # noqa: BLE001, pylint: disable=broad-except + def_errors[rev] = e + continue + if not final_props and rev_props: final_props = rev_props plot_datapoints.extend(dps) + for src in plot_sources: + if error := get_in(data, [rev, "sources", "data", src, "error"]): + src_errors[rev][src] = error + if "title" not in final_props: final_props["title"] = renderer_id + if renderer_cls is not None: - renderers.append(renderer_cls(plot_datapoints, renderer_id, **final_props)) + renderer = renderer_cls(plot_datapoints, renderer_id, **final_props) + renderers.append(RendererWithErrors(renderer, dict(src_errors), def_errors)) return renderers diff --git a/dvc/repo/plots/__init__.py b/dvc/repo/plots/__init__.py index fd7a12303e..281a8e8b28 100644 --- a/dvc/repo/plots/__init__.py +++ b/dvc/repo/plots/__init__.py @@ -20,7 +20,7 @@ import dpath import dpath.options -from funcy import distinct, first, project, reraise +from funcy import first, ldistinct, project, reraise from dvc.exceptions import DvcException from dvc.utils import error_handler, errored_revisions, onerror_collect @@ -356,7 +356,7 @@ def infer_data_sources(plot_id, config=None): if isinstance(x, dict): sources.append(first(x.keys())) - return distinct(source for source in sources) + return ldistinct(source for source in sources) def _matches(targets, config_file, plot_id): diff --git a/tests/integration/plots/test_plots.py b/tests/integration/plots/test_plots.py index c8db3923ec..5af9385b6b 100644 --- a/tests/integration/plots/test_plots.py +++ b/tests/integration/plots/test_plots.py @@ -361,13 +361,28 @@ def test_repo_with_removed_plots(tmp_dir, capsys, repo_with_plots): for s in {"show", "diff"}: _, json_result, split_json_result = call(capsys, subcommand=s) - for p in { - "linear.json", - "confusion.json", - "image.png", - }: - assert json_result[p] == [] - assert split_json_result[p] == [] + errors = [ + { + "name": p, + "source": p, + "revision": "workspace", + "type": "FileNotFoundError", + "msg": "", + } + for p in [ + "linear.json", + "confusion.json", + "image.png", + ] + ] + expected_result = { + "errors": errors, + "image.png": [], + "confusion.json": [], + "linear.json": [], + } + assert json_result == expected_result + assert split_json_result == expected_result def test_config_output_dir(tmp_dir, dvc, capsys): diff --git a/tests/unit/command/test_plots.py b/tests/unit/command/test_plots.py index bb2f67ce4d..1c9e5d4610 100644 --- a/tests/unit/command/test_plots.py +++ b/tests/unit/command/test_plots.py @@ -7,6 +7,7 @@ from dvc.cli import parse_args from dvc.commands.plots import CmdPlotsDiff, CmdPlotsShow, CmdPlotsTemplates +from dvc.render.match import RendererWithErrors @pytest.fixture @@ -268,8 +269,11 @@ def test_should_call_render(tmp_dir, mocker, capsys, plots_data, output): output = output or "dvc_plots" index_path = tmp_dir / output / "index.html" - renderers = mocker.MagicMock() - mocker.patch("dvc.render.match.match_defs_renderers", return_value=renderers) + renderer = mocker.MagicMock() + mocker.patch( + "dvc.render.match.match_defs_renderers", + return_value=[RendererWithErrors(renderer, {}, {})], + ) render_mock = mocker.patch("dvc_render.render_html", return_value=index_path) assert cmd.run() == 0 @@ -278,7 +282,7 @@ def test_should_call_render(tmp_dir, mocker, capsys, plots_data, output): assert index_path.as_uri() in out render_mock.assert_called_once_with( - renderers=renderers, + renderers=[renderer], output_file=Path(tmp_dir / output / "index.html"), html_template=None, ) @@ -354,12 +358,13 @@ def test_show_json(split, mocker, capsys): import dvc.commands.plots renderer = mocker.MagicMock() + renderer_obj = RendererWithErrors(renderer, {}, {}) renderer.name = "rname" to_json_mock = mocker.patch( "dvc.render.convert.to_json", return_value={"renderer": "json"} ) - dvc.commands.plots._show_json([renderer], split) + dvc.commands.plots._show_json([renderer_obj], split) to_json_mock.assert_called_once_with(renderer, split) diff --git a/tests/unit/render/test_match.py b/tests/unit/render/test_match.py index 02316559ab..104721c157 100644 --- a/tests/unit/render/test_match.py +++ b/tests/unit/render/test_match.py @@ -77,9 +77,9 @@ def test_match_renderers(mocker): }, } - renderers = match_defs_renderers(data) - assert len(renderers) == 1 - assert renderers[0].datapoints == [ + (renderer_with_errors,) = match_defs_renderers(data) + renderer = renderer_with_errors[0] + assert renderer.datapoints == [ { VERSION_FIELD: { "revision": "v1", @@ -99,7 +99,7 @@ def test_match_renderers(mocker): "y": 2, }, ] - assert renderers[0].properties == { + assert renderer.properties == { "title": "config_file_1::plot_id_1", "x": "x", "y": "y", From 64b06b3e1afe1116b0424044f514e2e09e1c6b18 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Tue, 14 Mar 2023 08:57:11 +0545 Subject: [PATCH 2/5] plots --json: move plots data into data key --- dvc/commands/plots.py | 48 ++++++++++----------- tests/func/plots/test_show.py | 7 +++- tests/integration/plots/test_plots.py | 60 ++++++++++++++++++--------- 3 files changed, 67 insertions(+), 48 deletions(-) diff --git a/dvc/commands/plots.py b/dvc/commands/plots.py index eb3f5c1f97..2b92bcadbc 100644 --- a/dvc/commands/plots.py +++ b/dvc/commands/plots.py @@ -3,7 +3,7 @@ import os from typing import TYPE_CHECKING, Dict, List -from funcy import first +from funcy import compact, first from dvc.cli import completion from dvc.cli.command import CmdBase @@ -24,35 +24,31 @@ def _show_json(renderers_with_errors: List["RendererWithErrors"], split=False): from dvc.utils.serialize import encode_exception errors: List[Dict] = [] - out = {} + data = {} for renderer, src_errors, def_errors in renderers_with_errors: name = renderer.name - out[name] = to_json(renderer, split) - if src_errors: - errors.extend( - { - "name": name, - "revision": rev, - "source": source, - **encode_exception(e), - } - for rev, per_rev_src_errors in src_errors.items() - for source, e in per_rev_src_errors.items() - ) - if def_errors: - errors.extend( - { - "name": name, - "revision": rev, - **encode_exception(e), - } - for rev, e in def_errors.items() - ) + data[name] = to_json(renderer, split) + errors.extend( + { + "name": name, + "revision": rev, + "source": source, + **encode_exception(e), + } + for rev, per_rev_src_errors in src_errors.items() + for source, e in per_rev_src_errors.items() + ) + errors.extend( + { + "name": name, + "revision": rev, + **encode_exception(e), + } + for rev, e in def_errors.items() + ) - if errors: - out = {"errors": errors, **out} - ui.write_json(out, highlight=False) + ui.write_json(compact({"errors": errors, "data": data}), highlight=False) def _adjust_vega_renderers(renderers): diff --git a/tests/func/plots/test_show.py b/tests/func/plots/test_show.py index 0d49745abc..0a31c7fcf7 100644 --- a/tests/func/plots/test_show.py +++ b/tests/func/plots/test_show.py @@ -418,5 +418,8 @@ def test_show_plots_defined_with_native_os_path(tmp_dir, dvc, scm, capsys): assert main(["plots", "show", "--json"]) == 0 out, _ = capsys.readouterr() json_out = json.loads(out) - assert json_out[f"dvc.yaml::{top_level_plot}"] - assert json_out[stage_plot] + assert "errors" not in json_out + + json_data = json_out["data"] + assert json_data[f"dvc.yaml::{top_level_plot}"] + assert json_data[stage_plot] diff --git a/tests/integration/plots/test_plots.py b/tests/integration/plots/test_plots.py index 5af9385b6b..3c7b05e91b 100644 --- a/tests/integration/plots/test_plots.py +++ b/tests/integration/plots/test_plots.py @@ -182,7 +182,13 @@ def test_repo_with_plots(tmp_dir, scm, dvc, capsys, run_copy_metrics, repo_with_ html_path, json_result, split_json_result = call(capsys) html_result = extract_vega_specs(html_path, ["linear.json", "confusion.json"]) - assert json_result["linear.json"][0]["content"]["data"][ + assert "errors" not in json_result + assert "errors" not in split_json_result + + json_data = json_result["data"] + split_json_data = split_json_result["data"] + + assert json_data["linear.json"][0]["content"]["data"][ "values" ] == _update_datapoints( linear_v1, @@ -200,7 +206,7 @@ def test_repo_with_plots(tmp_dir, scm, dvc, capsys, run_copy_metrics, repo_with_ REVISION_FIELD: "workspace", }, ) - assert json_result["confusion.json"][0]["content"]["data"][ + assert json_data["confusion.json"][0]["content"]["data"][ "values" ] == _update_datapoints( confusion_v1, @@ -218,34 +224,40 @@ def test_repo_with_plots(tmp_dir, scm, dvc, capsys, run_copy_metrics, repo_with_ REVISION_FIELD: "workspace", }, ) - verify_image(tmp_dir, "workspace", "image.png", image_v1, html_path, json_result) + verify_image(tmp_dir, "workspace", "image.png", image_v1, html_path, json_data) for plot in ["linear.json", "confusion.json"]: verify_vega( "workspace", html_result[plot], - json_result[plot], - split_json_result[plot], + json_data[plot], + split_json_data[plot], ) - verify_vega_props("confusion.json", json_result, **confusion_props) + verify_vega_props("confusion.json", json_data, **confusion_props) image_v2, linear_v2, confusion_v2, confusion_props = next(repo_state) html_path, json_result, split_json_result = call(capsys, subcommand="diff") html_result = extract_vega_specs(html_path, ["linear.json", "confusion.json"]) - verify_image(tmp_dir, "workspace", "image.png", image_v2, html_path, json_result) - verify_image(tmp_dir, "HEAD", "image.png", image_v1, html_path, json_result) + assert "errors" not in json_result + assert "errors" not in split_json_result + + json_data = json_result["data"] + split_json_data = split_json_result["data"] + + verify_image(tmp_dir, "workspace", "image.png", image_v2, html_path, json_data) + verify_image(tmp_dir, "HEAD", "image.png", image_v1, html_path, json_data) for plot in ["linear.json", "confusion.json"]: verify_vega( ["HEAD", "workspace"], html_result[plot], - json_result[plot], - split_json_result[plot], + json_data[plot], + split_json_data[plot], ) - verify_vega_props("confusion.json", json_result, **confusion_props) + verify_vega_props("confusion.json", json_data, **confusion_props) path = tmp_dir / "subdir" path.mkdir() with path.chdir(): @@ -254,7 +266,13 @@ def test_repo_with_plots(tmp_dir, scm, dvc, capsys, run_copy_metrics, repo_with_ html_path, ["../linear.json", "../confusion.json"], ) - assert json_result["../linear.json"][0]["content"]["data"][ + + assert "errors" not in json_result + assert "errors" not in split_json_result + + json_data = json_result["data"] + split_json_data = split_json_result["data"] + assert json_data["../linear.json"][0]["content"]["data"][ "values" ] == _update_datapoints( linear_v2, @@ -286,7 +304,7 @@ def test_repo_with_plots(tmp_dir, scm, dvc, capsys, run_copy_metrics, repo_with_ REVISION_FIELD: "HEAD", }, ) - assert json_result["../confusion.json"][0]["content"]["data"][ + assert json_data["../confusion.json"][0]["content"]["data"][ "values" ] == _update_datapoints( confusion_v2, @@ -326,8 +344,8 @@ def test_repo_with_plots(tmp_dir, scm, dvc, capsys, run_copy_metrics, repo_with_ verify_vega( ["HEAD", "workspace"], html_result[plot], - json_result[plot], - split_json_result[plot], + json_data[plot], + split_json_data[plot], ) verify_image( path, @@ -335,7 +353,7 @@ def test_repo_with_plots(tmp_dir, scm, dvc, capsys, run_copy_metrics, repo_with_ "../image.png", image_v2, html_path, - json_result, + json_data, ) verify_image( path, @@ -343,7 +361,7 @@ def test_repo_with_plots(tmp_dir, scm, dvc, capsys, run_copy_metrics, repo_with_ "../image.png", image_v1, html_path, - json_result, + json_data, ) @@ -377,9 +395,11 @@ def test_repo_with_removed_plots(tmp_dir, capsys, repo_with_plots): ] expected_result = { "errors": errors, - "image.png": [], - "confusion.json": [], - "linear.json": [], + "data": { + "image.png": [], + "confusion.json": [], + "linear.json": [], + }, } assert json_result == expected_result assert split_json_result == expected_result From 79be5671a42c0ad686acd47e6b07670e391ee51e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Wed, 15 Mar 2023 15:01:42 +0545 Subject: [PATCH 3/5] rename revision key to rev --- dvc/commands/plots.py | 4 ++-- tests/integration/plots/test_plots.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dvc/commands/plots.py b/dvc/commands/plots.py index 2b92bcadbc..9382ce9c23 100644 --- a/dvc/commands/plots.py +++ b/dvc/commands/plots.py @@ -32,7 +32,7 @@ def _show_json(renderers_with_errors: List["RendererWithErrors"], split=False): errors.extend( { "name": name, - "revision": rev, + "rev": rev, "source": source, **encode_exception(e), } @@ -42,7 +42,7 @@ def _show_json(renderers_with_errors: List["RendererWithErrors"], split=False): errors.extend( { "name": name, - "revision": rev, + "rev": rev, **encode_exception(e), } for rev, e in def_errors.items() diff --git a/tests/integration/plots/test_plots.py b/tests/integration/plots/test_plots.py index 3c7b05e91b..4119f46918 100644 --- a/tests/integration/plots/test_plots.py +++ b/tests/integration/plots/test_plots.py @@ -383,7 +383,7 @@ def test_repo_with_removed_plots(tmp_dir, capsys, repo_with_plots): { "name": p, "source": p, - "revision": "workspace", + "rev": "workspace", "type": "FileNotFoundError", "msg": "", } From 867d97beea4879ce96ffaa4165300c3313c8500f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Sat, 18 Mar 2023 10:09:38 +0545 Subject: [PATCH 4/5] move imports to top-level --- dvc/render/match.py | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/dvc/render/match.py b/dvc/render/match.py index c3aed3db45..b3161cb213 100644 --- a/dvc/render/match.py +++ b/dvc/render/match.py @@ -4,7 +4,7 @@ import dpath import dpath.options -from funcy import last +from funcy import get_in, last from dvc.repo.plots import _normpath, infer_data_sources from dvc.utils.plots import get_plot_id @@ -93,8 +93,6 @@ def match_defs_renderers( # noqa: C901, PLR0912 if templates_dir is not None: props["template_dir"] = templates_dir - from funcy import get_in - for rev, inner_id, plot_definition in group: plot_sources = infer_data_sources(inner_id, plot_definition) definitions_data = plots_data.get_definition_data(plot_sources, rev) From 864b68dee5a041a3c094959841be4c0ea3402fa4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Mon, 27 Mar 2023 12:30:30 +0545 Subject: [PATCH 5/5] add tests --- dvc/render/match.py | 8 ++++---- tests/unit/render/test_match.py | 23 ++++++++++++++++++++++- 2 files changed, 26 insertions(+), 5 deletions(-) diff --git a/dvc/render/match.py b/dvc/render/match.py index b3161cb213..3f36e9adc0 100644 --- a/dvc/render/match.py +++ b/dvc/render/match.py @@ -106,6 +106,10 @@ def match_defs_renderers( # noqa: C901, PLR0912 converter = _get_converter(renderer_cls, inner_id, props, definitions_data) + for src in plot_sources: + if error := get_in(data, [rev, "sources", "data", src, "error"]): + src_errors[rev][src] = error + try: dps, rev_props = converter.flat_datapoints(rev) except Exception as e: # noqa: BLE001, pylint: disable=broad-except @@ -116,10 +120,6 @@ def match_defs_renderers( # noqa: C901, PLR0912 final_props = rev_props plot_datapoints.extend(dps) - for src in plot_sources: - if error := get_in(data, [rev, "sources", "data", src, "error"]): - src_errors[rev][src] = error - if "title" not in final_props: final_props["title"] = renderer_id diff --git a/tests/unit/render/test_match.py b/tests/unit/render/test_match.py index 104721c157..822d37283a 100644 --- a/tests/unit/render/test_match.py +++ b/tests/unit/render/test_match.py @@ -1,4 +1,7 @@ +from funcy import set_in + from dvc.render import VERSION_FIELD +from dvc.render.converter.vega import VegaConverter from dvc.render.match import PlotsData, _squash_plots_properties, match_defs_renderers @@ -35,7 +38,7 @@ def test_group_definitions(): } -def test_match_renderers(mocker): +def test_match_renderers(M): data = { "v1": { "definitions": { @@ -106,6 +109,24 @@ def test_match_renderers(mocker): "x_label": "x", "y_label": "y", } + assert renderer_with_errors.source_errors == { + "revision_with_no_data": {"file.json": M.instance_of(FileNotFoundError)} + } + assert not renderer_with_errors.definition_errors + + +def test_flat_datapoints_errors_are_caught(M, mocker): + d = {} + d = set_in( + d, + ["v1", "definitions", "data", "dvc.yaml", "data", "plot_id_1"], + {"x": "x", "y": {"file.json": "y"}}, + ) + d = set_in(d, ["v1", "sources", "data", "file.json", "data"], [{"x": 1, "y": 1}]) + mocker.patch.object(VegaConverter, "flat_datapoints", side_effect=ValueError) + (renderer_with_errors,) = match_defs_renderers(d) + assert not renderer_with_errors.source_errors + assert renderer_with_errors.definition_errors == {"v1": M.instance_of(ValueError)} def test_squash_plots_properties():