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: return errors in json format #9146

Merged
merged 5 commits into from
Mar 27, 2023
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
43 changes: 37 additions & 6 deletions dvc/commands/plots.py
Original file line number Diff line number Diff line change
@@ -1,8 +1,9 @@
import argparse
import logging
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
Expand All @@ -11,14 +12,43 @@
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] = []
data = {}

for renderer, src_errors, def_errors in renderers_with_errors:
name = renderer.name
data[name] = to_json(renderer, split)
errors.extend(
{
"name": name,
"rev": 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,
"rev": 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)
ui.write_json(compact({"errors": errors, "data": data}), highlight=False)


def _adjust_vega_renderers(renderers):
Expand Down Expand Up @@ -122,15 +152,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))
Expand Down
35 changes: 29 additions & 6 deletions dvc/render/match.py
Original file line number Diff line number Diff line change
@@ -1,10 +1,10 @@
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
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
Expand All @@ -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

Expand Down Expand Up @@ -61,21 +63,31 @@ 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:
Expand All @@ -94,13 +106,24 @@ def match_defs_renderers(

converter = _get_converter(renderer_cls, inner_id, props, definitions_data)

dps, rev_props = converter.flat_datapoints(rev)
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
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this need to be broad?
flat_datapoints should only raise DvcException, right?

Copy link
Member Author

Choose a reason for hiding this comment

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

I am trying to match what we do on error_handler. I see flat_datapoints() reraising DvcException on some instances.

The question is, is there a potential to raise other errors like OSError, etc? And looking at the image.flat_datapoints(), it might raise other exceptions. So I don't want this to fail in any case.

def_errors[rev] = e
skshetry marked this conversation as resolved.
Show resolved Hide resolved
continue

if not final_props and rev_props:
final_props = rev_props
plot_datapoints.extend(dps)

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
4 changes: 2 additions & 2 deletions dvc/repo/plots/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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):
Expand Down
7 changes: 5 additions & 2 deletions tests/func/plots/test_show.py
Original file line number Diff line number Diff line change
Expand Up @@ -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]
83 changes: 59 additions & 24 deletions tests/integration/plots/test_plots.py
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand All @@ -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,
Expand All @@ -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():
Expand All @@ -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,
Expand Down Expand Up @@ -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,
Expand Down Expand Up @@ -326,24 +344,24 @@ 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,
"workspace",
"../image.png",
image_v2,
html_path,
json_result,
json_data,
)
verify_image(
path,
"HEAD",
"../image.png",
image_v1,
html_path,
json_result,
json_data,
)


Expand All @@ -361,13 +379,30 @@ 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,
"rev": "workspace",
"type": "FileNotFoundError",
"msg": "",
}
for p in [
"linear.json",
"confusion.json",
"image.png",
]
]
expected_result = {
"errors": errors,
"data": {
"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):
Expand Down
13 changes: 9 additions & 4 deletions tests/unit/command/test_plots.py
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand All @@ -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,
)
Expand Down Expand Up @@ -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)

Expand Down
Loading