Repository: iterative/dvc
Pull request #9898 (2023-08-31 22:38:38)

merge plot ids across dvc.yaml files
Addresses the issue in https://github.com/iterative/dvclive/pull/687#issuecomment-1701542389.

It merges plot IDs across all `dvc.yaml` files in the repo. Technically this is a breaking change, but most likely it's an improvement for almost all use cases. We already merge plot IDs across revisions, so this PR does the same across `dvc.yaml` files. Not sure if there are valid cases to have separate plots with the same plot ID in different `dvc.yaml` files. Is it worth warning when there is an overlap?

Note: artifacts should follow the same behavior, which would help with issues like https://github.com/iterative/studio/issues/6939.

Before this PR:

![visualization(12)](https://github.com/iterative/dvc/assets/2308172/4874a030-50a1-4f15-82e4-e0e01439326f)
![visualization(13)](https://github.com/iterative/dvc/assets/2308172/afb3bd52-a86c-4204-93f9-b15464719b8c)

After this PR:

![visualization(15)](https://github.com/iterative/dvc/assets/2308172/8652a2e2-9fb6-481e-9422-8071501f5d6d)


dberenbaum: Looks like my pre-commit hooks aren't working. Taking a look.
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/iterative/dvc/pull/9898?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative) Report
Patch coverage is **`100.00%`** of modified lines.

| [Files Changed](https://app.codecov.io/gh/iterative/dvc/pull/9898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative) | Coverage |
|---|---|
| [dvc/render/match.py](https://app.codecov.io/gh/iterative/dvc/pull/9898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative#diff-ZHZjL3JlbmRlci9tYXRjaC5weQ==) | `100.00%` |
| [tests/func/plots/test\_show.py](https://app.codecov.io/gh/iterative/dvc/pull/9898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative#diff-dGVzdHMvZnVuYy9wbG90cy90ZXN0X3Nob3cucHk=) | `100.00%` |
| [tests/integration/plots/test\_plots.py](https://app.codecov.io/gh/iterative/dvc/pull/9898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative#diff-dGVzdHMvaW50ZWdyYXRpb24vcGxvdHMvdGVzdF9wbG90cy5weQ==) | `100.00%` |
| [tests/unit/render/test\_match.py](https://app.codecov.io/gh/iterative/dvc/pull/9898?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative#diff-dGVzdHMvdW5pdC9yZW5kZXIvdGVzdF9tYXRjaC5weQ==) | `100.00%` |


:loudspeaker: Thoughts on this report? [Let us know!](https://about.codecov.io/pull-request-comment-report/?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=iterative).

skshetry: > It merges plot IDs across all dvc.yaml files in the repo. Technically this is a breaking change, but most likely it's an improvement for almost all use cases. We already merge plot IDs across revisions, so this PR does the same across dvc.yaml files. Not sure if there are valid cases to have separate plots with the same plot ID in different dvc.yaml files. Is it worth warning when there is an overlap?

This goes against how everything works in `dvc`, and is problematic for several reasons.
We won't be able to create plots by just looking at the definition in one `dvc.yaml` file.
And there may be errors in some dvc.yaml files that we'll never be sure if we have all the required definitions for the plot. 

Also, a breaking change as you said, and makes it more confusing to the users if they try to share some configs across dvc.yaml files and expect them to be unique.

I don't have a strong opinion on disallowing use of same id/names across multiple dvc.yaml files, whether by enforcing it or by convention. This would make it easier to migrate plots of same ids across different `dvc.yaml` files.

But here, it feels like the correct fix should be to always use the definitions from workspace/HEAD instead of merging them (could not find the issue to link to :( ).



dberenbaum: > But here, it feels like the correct fix should be to always use the definitions from workspace/HEAD instead of merging them (could not find the issue to link to :( ).

This PR doesn't touch how we merge plot definitions or how we handle changes across revisions. It is only about cases where the same plot ID is found in different `dvc.yaml` files.

---

@skshetry I agree with your overall point that merging two plots with the same ID in different `dvc.yaml` files doesn't fit how we address other things in dvc like `stages`. I updated so that it doesn't merge anything. Instead, it drops the `dvc.yaml` file name from the plot ID if there are no conflicts (a conflict means the same plot ID exists in another `dvc.yaml` for that revision).

Examples:

- If `rev1` has `plot_id1` in `dvc.yaml`, this PR changes its ID from `dvc.yaml::plot_id1` to `plot_id1`
- If `rev1` has `plot_id1` in `dvc.yaml`, and `rev2` has `plot_id1` in `dvclive/dvc.yaml`, both will have ID `plot_id1` since there is no conflict within each revision.
- If `rev1` has `plot_id1` in both `dvc.yaml` and `dvclive/dvc.yaml`, they will have IDs `dvc.yaml::plot_id1` and `dvclive/dvc.yaml::plot_id1`.
- If `rev1` has `plot_id1` in both `dvc.yaml` and `dvclive/dvc.yaml`, and `rev2` has `plot_id1` in only `dvc.yaml`, `rev1` will have IDs `dvc.yaml::plot_id1` and `dvclive/dvc.yaml::plot_id1`, and `rev2` will have ID `plot_id1`. 
 
In the last example, should `rev2` have ID `dvc.yaml::plot_id1` to match `rev1`?

dberenbaum: Need to test how it looks in VS Code and Studio.
daavoo: > After some debugging with @mvshmakov, I'm so far unable to get the local studio dev ui working, so I might need someone who already has this setup to test these changes to unblock this.

I will test it
daavoo: > > After some debugging with @mvshmakov, I'm so far unable to get the local studio dev ui working, so I might need someone who already has this setup to test these changes to unblock this.
> 
> I will test it

Unfortunately, the change doesn't have an effect on Studio. It appears that Studio doesn't use the code modified here. I am checking the Studio backend parsing code to see how this change could be applied
daavoo: > Unfortunately, the change doesn't have an effect on Studio. It appears that Studio doesn't use the code modified here. I am checking the Studio backend parsing code to see how this change could be applied

Here is the code:

https://github.com/iterative/studio/blame/5b60ed17ecea77aec9c83cc58b5edf6616dfc4e1/backend/repos/parsing/dvcmeat.py#L194-L196

I will try to patch it but in the short term it would be good to understand why we kind of have this logic duplicated there

daavoo: > @dberenbaum Should I try to patch it there?
> 
> In the longer term, it would be good to understand why we kind of have this logic duplicated there

Ok, this is not a 5-minute change 😅 

Studio parses and stores plots separately for each commit.
I am now not sure if the ID of the plots should be overwritten in the backend after all the commits are processed or if this should be handled in the FE based on selected revisions.

cc @shcheklein 
dberenbaum: > Studio parses and stores plots separately for each commit.
> I am now not sure if the ID of the plots should be overwritten in the backend after all the commits are processed or if this should be handled in the FE based on the revisions selected for the plot.

AFAIR Studio does this in part so that it doesn't change depending on the revisions selected. 

> In the longer term, it would be good to understand why we kind of have this logic duplicated there

💯 We are duplicating a lot of plots logic.

As a small starting point, I could extract the logic in this PR to a utils function so it can be reused in the Studio code linked above.
dberenbaum: > As a small starting point, I could extract the logic in this PR to a utils function so it can be reused in the Studio code linked above.

Done. Now it should be possible to replace https://github.com/iterative/studio/blob/5b60ed17ecea77aec9c83cc58b5edf6616dfc4e1/backend/repos/parsing/dvcmeat.py#L192-L195 with:

```python
for name, definition in group_definitions_by_id(definitions).items():
    plot_inner_id, plot_properties = definition
```

We would still need to iterate over `definitions` one more time in https://github.com/iterative/studio/blob/5b60ed17ecea77aec9c83cc58b5edf6616dfc4e1/backend/repos/parsing/dvcmeat.py#L218-L219, but it at least saves from having to copy this PR's logic into Studio. Open to other ideas for how to structure this.
daavoo: > We would still need to iterate over `definitions` one more time in https://github.com/iterative/studio/blob/5b60ed17ecea77aec9c83cc58b5edf6616dfc4e1/backend/repos/parsing/dvcmeat.py#L218-L219, but it at least saves from having to copy this PR's logic into Studio. Open to other ideas for how to structure this.

Could we also collect and return errors in the utils function here:

```py
        for plot_id, plot_definition in config_file_content.get("data", {}).items():
```

I am also curious as I can't find a test in Studio passing through `error` in config_file_content. 
I wonder what these different levels of errors are and what we do with them in DVC (are we just ignoring them?)

Patch:
diff --git a/dvc/render/match.py b/dvc/render/match.py
index 86eba55c3..f9a052124 100644
--- a/dvc/render/match.py
+++ b/dvc/render/match.py
@@ -8,7 +8,7 @@ import dpath.options
 from funcy import get_in, last
 
 from dvc.repo.plots import _normpath, infer_data_sources
-from dvc.utils.plots import get_plot_id
+from dvc.utils.plots import group_definitions_by_id
 
 from .convert import _get_converter
 
@@ -36,14 +36,9 @@ class PlotsData:
     def group_definitions(self):
         groups = defaultdict(list)
         for rev, rev_content in self.data.items():
-            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))
+            definitions = rev_content.get("definitions", {}).get("data", {})
+            for plot_id, definition in group_definitions_by_id(definitions).items():
+                groups[plot_id].append((rev, *definition))
         return dict(groups)
 
     def get_definition_data(self, target_files, rev):
diff --git a/dvc/utils/plots.py b/dvc/utils/plots.py
index 636a22059..6e85bea47 100644
--- a/dvc/utils/plots.py
+++ b/dvc/utils/plots.py
@@ -1,4 +1,36 @@
+from collections import defaultdict
+from typing import Dict, Tuple
+
+
 def get_plot_id(config_plot_id: str, config_file_path: str = ""):
     return (
         f"{config_file_path}::{config_plot_id}" if config_file_path else config_plot_id
     )
+
+
+def group_definitions_by_id(
+    definitions: Dict[str, Dict]
+) -> Dict[str, Tuple[str, Dict]]:
+    """
+    Format ID and extracts plot_definition for each plot.
+
+    Arguments:
+        definitions: dict of {config_file: config_file_content}.
+
+    Returns:
+        Dict of {plot_id: (original_plot_id, plot_definition)}.
+    """
+    groups_by_config: Dict = defaultdict(dict)
+    groups_by_id: Dict = {}
+    for config_file, config_file_content in definitions.items():
+        for plot_id, plot_definition in config_file_content.get("data", {}).items():
+            groups_by_config[plot_id][config_file] = (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_config.items():
+        if len(configs) == 1:
+            groups_by_id[plot_id] = next(iter(configs.values()))
+        else:
+            for config_file, content in configs.items():
+                full_id = get_plot_id(plot_id, config_file)
+                groups_by_id[full_id] = content
+    return groups_by_id


Test patch:
diff --git a/tests/func/plots/test_show.py b/tests/func/plots/test_show.py
index e8e50133b..9eece262a 100644
--- a/tests/func/plots/test_show.py
+++ b/tests/func/plots/test_show.py
@@ -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]
diff --git a/tests/integration/plots/conftest.py b/tests/integration/plots/conftest.py
index 1e6f36ea7..13e91a87c 100644
--- a/tests/integration/plots/conftest.py
+++ b/tests/integration/plots/conftest.py
@@ -191,3 +191,58 @@ def repo_with_config_plots(tmp_dir, scm, dvc, run_copy_metrics):
         }
 
     return make
+
+
+@pytest.fixture
+def repo_with_dvclive_plots(tmp_dir, scm, dvc, run_copy_metrics):
+    def make():
+        metrics = [
+            {"step": 1, "metric": 0.1},
+            {"step": 2, "metric": 0.2},
+            {"step": 3, "metric": 0.3},
+        ]
+
+        metrics_path = tmp_dir / "dvclive" / "plots" / "metrics" / "metric.tsv"
+        metrics_path.parent.mkdir(parents=True)
+        metrics_path.dump_json(metrics)
+
+        plots_config_v1 = [
+            {
+                "plots/metrics": {
+                    "x": "step",
+                }
+            }
+        ]
+
+        from dvc.utils.serialize import modify_yaml
+
+        dvcyaml_path_v1 = tmp_dir / "dvclive" / "dvc.yaml"
+        with modify_yaml(dvcyaml_path_v1) as dvcfile_content:
+            dvcfile_content["plots"] = plots_config_v1
+        scm.add([metrics_path, dvcyaml_path_v1])
+        scm.commit("add dvclive 2.x plots")
+
+        yield {
+            "data": {metrics_path: metrics},
+            "configs": {dvcyaml_path_v1: plots_config_v1},
+        }
+
+        plots_config_v2 = [
+            {
+                "dvclive/plots/metrics": {
+                    "x": "step",
+                }
+            }
+        ]
+
+        dvcyaml_path_v1.unlink()
+        dvcyaml_path_v2 = tmp_dir / "dvc.yaml"
+        with modify_yaml(dvcyaml_path_v2) as dvcfile_content:
+            dvcfile_content["plots"] = plots_config_v2
+
+        yield {
+            "data": {metrics_path: metrics},
+            "configs": {dvcyaml_path_v2: plots_config_v2},
+        }
+
+    return make
diff --git a/tests/integration/plots/test_plots.py b/tests/integration/plots/test_plots.py
index 3c4f79df4..8a22918b9 100644
--- a/tests/integration/plots/test_plots.py
+++ b/tests/integration/plots/test_plots.py
@@ -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(
@@ -456,5 +456,20 @@ 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
+
+
+@pytest.mark.vscode
+def test_repo_with_dvclive_plots(tmp_dir, capsys, repo_with_dvclive_plots):
+    next(repo_with_dvclive_plots())
+
+    for s in ("show", "diff"):
+        _, json_result, split_json_result = call(capsys, subcommand=s)
+        expected_result = {
+            "data": {
+                "dvclive/plots/metrics/metric.tsv": [],
+            },
+        }
+        assert json_result == expected_result
+        assert split_json_result == expected_result
diff --git a/tests/unit/render/test_match.py b/tests/unit/render/test_match.py
index 822d37283..58d4b5c5b 100644
--- a/tests/unit/render/test_match.py
+++ b/tests/unit/render/test_match.py
@@ -1,3 +1,4 @@
+import pytest
 from funcy import set_in
 
 from dvc.render import VERSION_FIELD
@@ -5,37 +6,116 @@ 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_2": {"data": {"plot_id_3": {}}},
+                        }
+                    },
+                    "source": {
+                        "data": {
+                            "config_file_1": {"error": FileNotFoundError()},
+                        }
+                    },
+                },
+            },
+            {
+                "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):
@@ -57,12 +137,6 @@ def test_match_renderers(M):
                 "data": {"file.json": {"data": [{"x": 1, "y": 1}, {"x": 2, "y": 2}]}}
             },
         },
-        "errored_revision": {
-            "definitions": {
-                "data": {"config_file_1": {"error": FileNotFoundError()}},
-            },
-            "sources": {},
-        },
         "revision_with_no_data": {
             "definitions": {
                 "data": {
@@ -103,7 +177,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",
@@ -129,7 +203,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}),
@@ -139,3 +213,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}
diff --git a/tests/unit/utils/test_plots.py b/tests/unit/utils/test_plots.py
new file mode 100644
index 000000000..da69bd5c5
--- /dev/null
+++ b/tests/unit/utils/test_plots.py
@@ -0,0 +1,18 @@
+from dvc.utils.plots import get_plot_id, group_definitions_by_id
+
+
+def test_get_plot_id():
+    assert get_plot_id("plot_id", "config_path") == "config_path::plot_id"
+    assert get_plot_id("plot_id", "") == "plot_id"
+
+
+def test_group_definitions_by_id():
+    definitions = {
+        "config1": {"data": {"plot1": "definition1", "plot2": "definition2"}},
+        "config2": {"data": {"plot1": "definition1"}},
+    }
+    assert group_definitions_by_id(definitions) == {
+        "config1::plot1": ("plot1", "definition1"),
+        "config2::plot1": ("plot1", "definition1"),
+        "plot2": ("plot2", "definition2"),
+    }