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.

Sign up for GitHub

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

merge plot ids across dvc.yaml files #9898

Merged
merged 14 commits into from
Sep 14, 2023
Merged

merge plot ids across dvc.yaml files #9898

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
14 commits
Select commit Hold shift + click to select a range
a3f6002
merge plot ids across dvc.yaml files
Aug 31, 2023
60c8355
[pre-commit.ci] auto fixes from pre-commit.com hooks
pre-commit-ci[bot] Aug 31, 2023
6b89ea3
add test for merging plot ids across config files
Aug 31, 2023
003cfef
Merge branch 'merge-plots' of github.com:iterative/dvc into merge-plots
Aug 31, 2023
e59d3f9
Update dvc/render/match.py
dberenbaum Sep 1, 2023
a21a047
drop unused arg
Sep 1, 2023
d4d8b60
dropped config_file from calls to _matches
Sep 1, 2023
dc578af
keep config_file in plot_id if multiple config_files
Sep 5, 2023
237d44a
add test cases for plots group_definitions
Sep 6, 2023
81b33b3
add integration test for dvclive 2.x->3.x plots
Sep 7, 2023
d62cb55
add util function to group plot definitions
Sep 8, 2023
6336def
Merge branch 'main' into merge-plots
Sep 11, 2023
77a6fcd
drop irrelevant errors in tests/unit/render/test_match.py
Sep 11, 2023
159b032
Merge branch 'main' into merge-plots
dberenbaum Sep 13, 2023
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
12 changes: 10 additions & 2 deletions dvc/render/match.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,22 @@ def __init__(self, data: Dict):
def group_definitions(self):
groups = defaultdict(list)
for rev, rev_content in self.data.items():
groups_by_id: Dict = defaultdict(dict)
for config_file, config_file_content in (
rev_content.get("definitions", {}).get("data", {}).items()
):
for plot_id, plot_definition in config_file_content.get(
"data", {}
).items():
full_id = get_plot_id(plot_id, config_file)
groups[full_id].append((rev, plot_id, plot_definition))
groups_by_id[plot_id][config_file] = (rev, plot_id, plot_definition)
# only keep config_file if the same plot_id is in multiple config_files
for plot_id, configs in groups_by_id.items():
if len(configs) == 1:
groups[plot_id].append(next(iter(configs.values())))
daavoo marked this conversation as resolved.
Show resolved Hide resolved
else:
for config_file, content in configs.items():
full_id = get_plot_id(plot_id, config_file)
groups[full_id].append(content)
return dict(groups)

def get_definition_data(self, target_files, rev):
Expand Down
2 changes: 1 addition & 1 deletion tests/func/plots/test_show.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,5 +426,5 @@ def test_show_plots_defined_with_native_os_path(tmp_dir, dvc, scm, capsys):
assert "errors" not in json_out

json_data = json_out["data"]
assert json_data[f"dvc.yaml::{top_level_plot}"]
assert json_data[f"{top_level_plot}"]
assert json_data[stage_plot]
6 changes: 3 additions & 3 deletions tests/integration/plots/test_plots.py
View file
Open in desktop
dberenbaum marked this conversation as resolved.
Show resolved Hide resolved
Original file line number Diff line number Diff line change
Expand Up @@ -440,8 +440,8 @@ def test_repo_with_config_plots(tmp_dir, capsys, repo_with_config_plots):
html_result = extract_vega_specs(
html_path,
[
"dvc.yaml::linear_train_vs_test",
"dvc.yaml::confusion_train_vs_test",
"linear_train_vs_test",
"confusion_train_vs_test",
],
)
ble = _update_datapoints(
Expand All @@ -456,5 +456,5 @@ def test_repo_with_config_plots(tmp_dir, capsys, repo_with_config_plots):
},
)

assert html_result["dvc.yaml::linear_train_vs_test"]["data"]["values"] == ble
assert html_result["linear_train_vs_test"]["data"]["values"] == ble
# TODO check json results once vscode is able to handle flexible plots
148 changes: 118 additions & 30 deletions tests/unit/render/test_match.py
View file
Open in desktop
Copy link
Contributor

@daavoo daavoo Sep 7, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

Not a blocker: I prefer higher-level tests, like setting up dvc.yaml definitions and asserting on plots.show output. At least for me, these ones feel harder to follow/read and are kind of brittle because they are tied to the internal structures.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Added an integration test for the dvclive 2.x->3.x transition. I don't want to get into testing every combination of functionality in the high-level tests.

Original file line number Diff line number Diff line change
@@ -1,41 +1,117 @@
import pytest
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


def test_group_definitions():
error = FileNotFoundError()
data = {
"v1": {
"definitions": {
"data": {
"config_file_1": {"data": {"plot_id_1": {}, "plot_id_2": {}}},
"config_file_2": {"data": {"plot_id_3": {}}},
@pytest.mark.parametrize(
"data,expected",
[
pytest.param(
{
"v1": {
"definitions": {
"data": {"config_file_1": {"data": {"plot_id_1": {}}}}
}
}
}
},
"v2": {
"definitions": {
"data": {
"config_file_1": {"error": error},
"config_file_2": {"data": {"plot_id_3": {}}},
},
{"plot_id_1": [("v1", "plot_id_1", {})]},
id="simple",
),
pytest.param(
{
"v1": {
"definitions": {
"data": {
"config_file_1": {"data": {"plot_id_1": {}}},
"config_file_2": {"data": {"plot_id_1": {}}},
}
}
}
}
},
}

},
{
"config_file_1::plot_id_1": [("v1", "plot_id_1", {})],
"config_file_2::plot_id_1": [("v1", "plot_id_1", {})],
},
id="multi_config",
),
pytest.param(
{
"v1": {
"definitions": {
"data": {"config_file_1": {"data": {"plot_id_1": {}}}}
}
},
"v2": {
"definitions": {
"data": {"config_file_2": {"data": {"plot_id_1": {}}}}
}
},
},
{"plot_id_1": [("v1", "plot_id_1", {}), ("v2", "plot_id_1", {})]},
id="multi_rev",
),
pytest.param(
{
"v1": {
"definitions": {
"data": {
"config_file_1": {"data": {"plot_id_1": {}}},
"config_file_2": {"data": {"plot_id_1": {}}},
}
}
},
"v2": {
"definitions": {
"data": {"config_file_1": {"data": {"plot_id_1": {}}}}
}
},
},
{
"config_file_1::plot_id_1": [("v1", "plot_id_1", {})],
"config_file_2::plot_id_1": [("v1", "plot_id_1", {})],
"plot_id_1": [("v2", "plot_id_1", {})],
},
id="multi_rev_multi_config",
),
pytest.param(
{
"v1": {
"definitions": {
"data": {
"config_file_1": {
"data": {"plot_id_1": {}, "plot_id_2": {}}
},
"config_file_2": {"data": {"plot_id_3": {}}},
}
}
},
"v2": {
"definitions": {
"data": {
"config_file_1": {"error": FileNotFoundError()},
"config_file_2": {"data": {"plot_id_3": {}}},
}
}
},
},
{
"plot_id_1": [("v1", "plot_id_1", {})],
"plot_id_2": [("v1", "plot_id_2", {})],
"plot_id_3": [
("v1", "plot_id_3", {}),
("v2", "plot_id_3", {}),
],
},
id="all",
),
],
)
def test_group_definitions(data, expected):
grouped = PlotsData(data).group_definitions()

assert grouped == {
"config_file_1::plot_id_1": [("v1", "plot_id_1", {})],
"config_file_1::plot_id_2": [("v1", "plot_id_2", {})],
"config_file_2::plot_id_3": [
("v1", "plot_id_3", {}),
("v2", "plot_id_3", {}),
],
}
assert grouped == expected


def test_match_renderers(M):
Expand Down Expand Up @@ -103,7 +179,7 @@ def test_match_renderers(M):
},
]
assert renderer.properties == {
"title": "config_file_1::plot_id_1",
"title": "plot_id_1",
"x": "x",
"y": "y",
"x_label": "x",
Expand All @@ -129,7 +205,7 @@ def test_flat_datapoints_errors_are_caught(M, mocker):
assert renderer_with_errors.definition_errors == {"v1": M.instance_of(ValueError)}


def test_squash_plots_properties():
def test_squash_plots_properties_revs():
group = [
("v3", "config_file", "plot_id", {"foo": 1}),
("v2", "config_file", "plot_id", {"foo": 2, "bar": 2}),
Expand All @@ -139,3 +215,15 @@ def test_squash_plots_properties():
plot_properties = _squash_plots_properties(group)

assert plot_properties == {"foo": 1, "bar": 2, "baz": 3}


def test_squash_plots_properties_config_files():
group = [
("v1", "config_file1", "plot_id", {"foo": 1}),
("v1", "config_file2", "plot_id", {"foo": 2, "bar": 2}),
("v1", "config_file3", "plot_id", {"baz": 3}),
]

plot_properties = _squash_plots_properties(group)

assert plot_properties == {"foo": 1, "bar": 2, "baz": 3}