diff --git a/dvc/commands/plots.py b/dvc/commands/plots.py index dac23b637..92b2d775e 100644 --- a/dvc/commands/plots.py +++ b/dvc/commands/plots.py @@ -25,7 +25,7 @@ def _show_json(renderers, split=False): def _adjust_vega_renderers(renderers): - from dvc.render import VERSION_FIELD + from dvc.render import REVISION_FIELD, VERSION_FIELD from dvc_render import VegaRenderer for r in renderers: @@ -42,6 +42,7 @@ def _adjust_vega_renderers(renderers): dp["rev"] = "::".join(vi.values()) else: for dp in r.datapoints: + dp[REVISION_FIELD] = dp[VERSION_FIELD]["revision"] dp.pop(VERSION_FIELD, {}) @@ -66,18 +67,6 @@ def _data_versions_count(renderer): return len(set(x)) -def _filter_unhandled_renderers(renderers): - # filtering out renderers currently unhandled by vscode extension - from dvc_render import VegaRenderer - - def _is_json_viable(r): - return not ( - isinstance(r, VegaRenderer) and _data_versions_count(r) > 1 - ) - - return list(filter(_is_json_viable, renderers)) - - class CmdPlots(CmdBase): def _func(self, *args, **kwargs): raise NotImplementedError @@ -156,12 +145,10 @@ class CmdPlots(CmdBase): templates_dir=self.repo.plots.templates_dir, ) if self.args.json: - renderers = _filter_unhandled_renderers(renderers) _show_json(renderers, self.args.split) return 0 _adjust_vega_renderers(renderers) - if self.args.show_vega: renderer = first(filter(lambda r: r.TYPE == "vega", renderers)) if renderer: diff --git a/dvc/render/convert.py b/dvc/render/convert.py index 9c9b3718e..c90fd0426 100644 --- a/dvc/render/convert.py +++ b/dvc/render/convert.py @@ -2,7 +2,13 @@ import json from collections import defaultdict from typing import Dict, List, Union -from dvc.render import REVISION_FIELD, REVISIONS_KEY, SRC_FIELD, TYPE_KEY +from dvc.render import ( + REVISION_FIELD, + REVISIONS_KEY, + SRC_FIELD, + TYPE_KEY, + VERSION_FIELD, +) from dvc.render.converter.image import ImageConverter from dvc.render.converter.vega import VegaConverter @@ -23,7 +29,7 @@ def _get_converter( def _group_by_rev(datapoints): grouped = defaultdict(list) for datapoint in datapoints: - rev = datapoint.pop(REVISION_FIELD) + rev = datapoint.get(VERSION_FIELD, {}).get("revision") grouped[rev].append(datapoint) return dict(grouped) diff --git a/dvc/render/converter/vega.py b/dvc/render/converter/vega.py index 2cf03bb9b..5568ff665 100644 --- a/dvc/render/converter/vega.py +++ b/dvc/render/converter/vega.py @@ -127,13 +127,20 @@ class VegaConverter(Converter): ), ) ) + self.steps.append( + ( + "infer_y", + partial( + self._infer_y, + ), + ) + ) self.steps.append( ( "generate_y", partial( self._generate_y_values, - y_values=self.plot_properties.get("y", None), ), ) ) @@ -156,7 +163,7 @@ class VegaConverter(Converter): } - {None} self.inferred_properties["fields"] = fields - def _infer_y(self, datapoints: List[Dict]): + def _infer_y(self, datapoints: List[Dict], **kwargs): if "y" not in self.plot_properties: data_fields = list(first(datapoints)) skip = ( @@ -178,6 +185,7 @@ class VegaConverter(Converter): ) else: self.inferred_properties["y"] = inferred_y + return datapoints def convert( self, @@ -197,7 +205,6 @@ class VegaConverter(Converter): processed = deepcopy(data) for step_name, step in self.steps: - if step_name not in skip: processed = step( # type: ignore processed, @@ -205,25 +212,28 @@ class VegaConverter(Converter): filename=filename, ) - self._infer_y(processed) # type: ignore return processed, {**self.plot_properties, **self.inferred_properties} def _generate_y_values( # noqa: C901 self, datapoints: List[Dict], - y_values: Optional[Union[str, List, Dict]], revision: str, filename: str, **kwargs, ) -> List[Dict]: + y_values = self.plot_properties.get( + "y", None + ) or self.inferred_properties.get("y", None) + + assert y_values is not None + result = [] properties_update = {} def _add_version_info(datapoint, version_info): tmp = datapoint.copy() tmp[VERSION_FIELD] = version_info - tmp[REVISION_FIELD] = version_info["revision"] return tmp def _version_info(revision, filename=None, field=None): @@ -250,12 +260,6 @@ class VegaConverter(Converter): return tmp - if not y_values: - for dp in datapoints: - result.append( - _add_version_info(dp, _version_info(revision, filename)) - ) - if isinstance(y_values, str): for datapoint in datapoints: result.append( @@ -290,13 +294,15 @@ class VegaConverter(Converter): ): # if we use the same field from all files, # we dont have to generate it + y_field = all_fields.pop() for datapoint in datapoints: result.append( _add_version_info( - datapoint, _version_info(revision, filename) + datapoint, + _version_info(revision, filename, y_field), ) ) - properties_update.update({"y": all_fields.pop()}) + properties_update.update({"y": y_field}) else: for def_filename, val in y_values.items(): if isinstance(val, str): diff --git a/tests/integration/plots/test_plots.py b/tests/integration/plots/test_plots.py index 262020f52..6d0b38b42 100644 --- a/tests/integration/plots/test_plots.py +++ b/tests/integration/plots/test_plots.py @@ -11,6 +11,7 @@ from bs4 import BeautifulSoup from funcy import first from dvc.cli import main +from dvc.render import REVISION_FIELD, VERSION_FIELD JSON_OUT = "vis_data" @@ -60,17 +61,6 @@ def extract_vega_specs(html_path, plots_ids): return result -def filter_fields(datapoints: List[Dict], fields: List[str]): - - tmp = deepcopy(datapoints) - for datapoint in tmp: - keys = set(datapoint.keys()) - for key in keys: - if key not in fields: - datapoint.pop(key) - return tmp - - def drop_fields(datapoints: List[Dict], fields: List[str]): tmp = deepcopy(datapoints) for datapoint in tmp: @@ -219,10 +209,35 @@ def test_repo_with_plots( html_result = extract_vega_specs( html_path, ["linear.json", "confusion.json"] ) + + assert json_result["linear.json"][0]["content"]["data"][ + "values" + ] == _update_datapoints( + linear_v1, + { + VERSION_FIELD: { + "revision": "workspace", + "filename": "linear.json", + "field": "y", + }, + }, + ) assert html_result["linear.json"]["data"]["values"] == _update_datapoints( linear_v1, { - "rev": "workspace", + REVISION_FIELD: "workspace", + }, + ) + assert json_result["confusion.json"][0]["content"]["data"][ + "values" + ] == _update_datapoints( + confusion_v1, + { + VERSION_FIELD: { + "revision": "workspace", + "filename": "confusion.json", + "field": "actual", + }, }, ) assert html_result["confusion.json"]["data"][ @@ -230,10 +245,9 @@ def test_repo_with_plots( ] == _update_datapoints( confusion_v1, { - "rev": "workspace", + REVISION_FIELD: "workspace", }, ) - verify_image( tmp_dir, "workspace", "image.png", image_v1, html_path, json_result ) @@ -280,17 +294,59 @@ def test_repo_with_plots( html_path, ["../linear.json", "../confusion.json"], ) + assert json_result["../linear.json"][0]["content"]["data"][ + "values" + ] == _update_datapoints( + linear_v2, + { + VERSION_FIELD: { + "revision": "workspace", + "filename": "../linear.json", + "field": "y", + }, + }, + ) + _update_datapoints( + linear_v1, + { + VERSION_FIELD: { + "revision": "HEAD", + "filename": "../linear.json", + "field": "y", + }, + }, + ) assert html_result["../linear.json"]["data"][ "values" ] == _update_datapoints( linear_v2, { - "rev": "workspace", + REVISION_FIELD: "workspace", }, ) + _update_datapoints( linear_v1, { - "rev": "HEAD", + REVISION_FIELD: "HEAD", + }, + ) + assert json_result["../confusion.json"][0]["content"]["data"][ + "values" + ] == _update_datapoints( + confusion_v2, + { + VERSION_FIELD: { + "revision": "workspace", + "filename": "../confusion.json", + "field": "actual", + }, + }, + ) + _update_datapoints( + confusion_v1, + { + VERSION_FIELD: { + "revision": "HEAD", + "filename": "../confusion.json", + "field": "actual", + }, }, ) assert html_result["../confusion.json"]["data"][ @@ -298,12 +354,12 @@ def test_repo_with_plots( ] == _update_datapoints( confusion_v2, { - "rev": "workspace", + REVISION_FIELD: "workspace", }, ) + _update_datapoints( confusion_v1, { - "rev": "HEAD", + REVISION_FIELD: "HEAD", }, ) @@ -390,11 +446,9 @@ def test_repo_with_config_plots(tmp_dir, capsys, repo_with_config_plots): repo_state = repo_with_config_plots() plots = next(repo_state) - html_path, json_result, split_json_result = call(capsys) + html_path, _, __ = call(capsys) assert os.path.exists(html_path) - assert json_result == {} - assert split_json_result == {} html_result = extract_vega_specs( html_path, [ @@ -405,9 +459,15 @@ def test_repo_with_config_plots(tmp_dir, capsys, repo_with_config_plots): assert html_result["dvc.yaml::linear_train_vs_test"]["data"][ "values" ] == _update_datapoints( - plots["data"]["linear_train.json"], {"rev": "linear_train.json"} + plots["data"]["linear_train.json"], + { + REVISION_FIELD: "linear_train.json", + }, ) + _update_datapoints( - plots["data"]["linear_test.json"], {"rev": "linear_test.json"} + plots["data"]["linear_test.json"], + { + REVISION_FIELD: "linear_test.json", + }, ) # TODO check json results once vscode is able to handle flexible plots diff --git a/tests/unit/command/test_plots.py b/tests/unit/command/test_plots.py index 753cc25e6..6165373b4 100644 --- a/tests/unit/command/test_plots.py +++ b/tests/unit/command/test_plots.py @@ -334,10 +334,6 @@ def test_plots_diff_json(dvc, mocker, capsys): ) render_mock = mocker.patch("dvc_render.render_html") - show_json_mock = mocker.patch( - "dvc.commands.plots._filter_unhandled_renderers", - return_value=renderers, - ) show_json_mock = mocker.patch("dvc.commands.plots._show_json") assert cmd.run() == 0 diff --git a/tests/unit/render/test_convert.py b/tests/unit/render/test_convert.py index 1f4294774..1fbf26b02 100644 --- a/tests/unit/render/test_convert.py +++ b/tests/unit/render/test_convert.py @@ -1,4 +1,10 @@ -from dvc.render import REVISION_FIELD, REVISIONS_KEY, SRC_FIELD, TYPE_KEY +from dvc.render import ( + REVISION_FIELD, + REVISIONS_KEY, + SRC_FIELD, + TYPE_KEY, + VERSION_FIELD, +) from dvc.render.convert import to_json @@ -7,8 +13,18 @@ def test_to_json_vega(mocker): vega_renderer.TYPE = "vega" vega_renderer.get_filled_template.return_value = '{"this": "is vega"}' vega_renderer.datapoints = [ - {"x": 1, "y": 2, REVISION_FIELD: "foo", "filename": "foo.json"}, - {"x": 2, "y": 1, REVISION_FIELD: "bar", "filename": "foo.json"}, + { + "x": 1, + "y": 2, + VERSION_FIELD: {"revision": "foo"}, + "filename": "foo.json", + }, + { + "x": 2, + "y": 1, + VERSION_FIELD: {"revision": "bar"}, + "filename": "foo.json", + }, ] result = to_json(vega_renderer) assert result[0] == { @@ -17,10 +33,20 @@ def test_to_json_vega(mocker): "content": {"this": "is vega"}, "datapoints": { "foo": [ - {"x": 1, "y": 2, "filename": "foo.json"}, + { + "x": 1, + "y": 2, + "filename": "foo.json", + VERSION_FIELD: {"revision": "foo"}, + }, ], "bar": [ - {"x": 2, "y": 1, "filename": "foo.json"}, + { + "x": 2, + "y": 1, + "filename": "foo.json", + VERSION_FIELD: {"revision": "bar"}, + }, ], }, } @@ -34,8 +60,18 @@ def test_to_json_vega_split(mocker): '{"this": "is split vega"}' ) vega_renderer.datapoints = [ - {"x": 1, "y": 2, REVISION_FIELD: "foo", "filename": "foo.json"}, - {"x": 2, "y": 1, REVISION_FIELD: "bar", "filename": "foo.json"}, + { + "x": 1, + "y": 2, + VERSION_FIELD: {"revision": "foo"}, + "filename": "foo.json", + }, + { + "x": 2, + "y": 1, + VERSION_FIELD: {"revision": "bar"}, + "filename": "foo.json", + }, ] result = to_json(vega_renderer, split=True) assert result[0] == { @@ -43,8 +79,22 @@ def test_to_json_vega_split(mocker): REVISIONS_KEY: ["bar", "foo"], "content": {"this": "is split vega"}, "datapoints": { - "foo": [{"x": 1, "y": 2, "filename": "foo.json"}], - "bar": [{"x": 2, "y": 1, "filename": "foo.json"}], + "foo": [ + { + "x": 1, + "y": 2, + "filename": "foo.json", + VERSION_FIELD: {"revision": "foo"}, + } + ], + "bar": [ + { + "x": 2, + "y": 1, + "filename": "foo.json", + VERSION_FIELD: {"revision": "bar"}, + } + ], }, } vega_renderer.get_filled_template.assert_called_with(skip_anchors=["data"]) diff --git a/tests/unit/render/test_match.py b/tests/unit/render/test_match.py index 3c00ddf9f..85bb3b2e4 100644 --- a/tests/unit/render/test_match.py +++ b/tests/unit/render/test_match.py @@ -89,14 +89,20 @@ def test_match_renderers(mocker): assert len(renderers) == 1 assert renderers[0].datapoints == [ { - VERSION_FIELD: {"revision": "v1", "filename": "file.json"}, - "rev": "v1", + VERSION_FIELD: { + "revision": "v1", + "filename": "file.json", + "field": "y", + }, "x": 1, "y": 1, }, { - VERSION_FIELD: {"revision": "v1", "filename": "file.json"}, - "rev": "v1", + VERSION_FIELD: { + "revision": "v1", + "filename": "file.json", + "field": "y", + }, "x": 2, "y": 2, }, diff --git a/tests/unit/render/test_vega_converter.py b/tests/unit/render/test_vega_converter.py index a6073d116..6117cbeae 100644 --- a/tests/unit/render/test_vega_converter.py +++ b/tests/unit/render/test_vega_converter.py @@ -57,42 +57,53 @@ def test_finding_lists(dictionary, expected_result): @pytest.mark.parametrize( "input_data,properties,expected_datapoints,expected_properties", [ - ( - # default x and y + pytest.param( {"metric": [{"v": 1}, {"v": 2}]}, {}, [ { "v": 1, "step": 0, - VERSION_FIELD: {"revision": "r", "filename": "f"}, - "rev": "r", + VERSION_FIELD: { + "revision": "r", + "filename": "f", + "field": "v", + }, }, { "v": 2, "step": 1, - VERSION_FIELD: {"revision": "r", "filename": "f"}, - "rev": "r", + VERSION_FIELD: { + "revision": "r", + "filename": "f", + "field": "v", + }, }, ], {"x": "step", "y": "v"}, + id="default_x_y", ), - ( - # filter fields + pytest.param( {"metric": [{"v": 1, "v2": 0.1}, {"v": 2, "v2": 0.2}]}, {"fields": {"v"}}, [ { "v": 1, "step": 0, - VERSION_FIELD: {"revision": "r", "filename": "f"}, - "rev": "r", + VERSION_FIELD: { + "revision": "r", + "filename": "f", + "field": "v", + }, }, { "v": 2, "step": 1, - VERSION_FIELD: {"revision": "r", "filename": "f"}, - "rev": "r", + VERSION_FIELD: { + "revision": "r", + "filename": "f", + "field": "v", + }, }, ], { @@ -100,9 +111,9 @@ def test_finding_lists(dictionary, expected_result): "y": "v", "fields": {"v", "step"}, }, + id="filter_fields", ), - ( - # choose x and y + pytest.param( {"metric": [{"v": 1, "v2": 0.1}, {"v": 2, "v2": 0.2}]}, {"x": "v", "y": "v2"}, [ @@ -114,7 +125,6 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v2", }, - "rev": "r", }, { "v": 2, @@ -124,13 +134,12 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v2", }, - "rev": "r", }, ], {"x": "v", "y": "v2"}, + id="choose_x_y", ), - ( - # append x and y to filtered fields + pytest.param( { "metric": [ {"v": 1, "v2": 0.1, "v3": 0.01, "v4": 0.001}, @@ -148,7 +157,6 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v4", }, - "rev": "r", }, { "v": 2, @@ -159,13 +167,12 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v4", }, - "rev": "r", }, ], {"x": "v3", "y": "v4", "fields": {"v", "v3", "v4"}}, + id="append_x_y_to_fields", ), - ( - # find metric in nested structure + pytest.param( { "some": "noise", "very": { @@ -184,7 +191,6 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v2", }, - "rev": "r", }, { "v": 2, @@ -194,13 +200,12 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v2", }, - "rev": "r", }, ], {"x": "v", "y": "v2"}, + id="find_in_nested_structure", ), - ( - # provide list of fields in y def + pytest.param( {"metric": [{"v": 1, "v2": 0.1}, {"v": 2, "v2": 0.2}]}, {"y": {"f": ["v", "v2"]}}, [ @@ -212,7 +217,6 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v", }, - "rev": "r", "dvc_inferred_y_value": 1, "step": 0, }, @@ -224,7 +228,6 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v2", }, - "rev": "r", "dvc_inferred_y_value": 0.1, "step": 0, }, @@ -236,7 +239,6 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v", }, - "rev": "r", "dvc_inferred_y_value": 2, "step": 1, }, @@ -248,12 +250,12 @@ def test_finding_lists(dictionary, expected_result): "filename": "f", "field": "v2", }, - "rev": "r", "dvc_inferred_y_value": 0.2, "step": 1, }, ], {"x": "step", "y": "dvc_inferred_y_value", "y_label": "y"}, + id="y_def_list", ), ], ) @@ -275,6 +277,7 @@ def test_convert( def test_convert_skip_step(): converter = VegaConverter() converter.skip_step("append_index") + converter.skip_step("generate_y") datapoints, resolved_properties = converter.convert( data={"a": "b", "metric": [{"v": 1}, {"v": 2}]}, @@ -285,13 +288,9 @@ def test_convert_skip_step(): assert datapoints == [ { "v": 1, - "rev": "r", - VERSION_FIELD: {"revision": "r", "filename": "f"}, }, { "v": 2, - "rev": "r", - VERSION_FIELD: {"revision": "r", "filename": "f"}, }, ] assert resolved_properties == {"x": "step", "y": "v"}