metrics/plots/params: return ordered (#5297)

* metrics/plots/params: return ordered

* metrics/params/plots: add tests for return order

dvc/repo/collect.py

@@ -1,6 +1,6 @@
 import logging
 import os
-from typing import TYPE_CHECKING, Callable, Iterable, Set, Tuple
+from typing import TYPE_CHECKING, Callable, Iterable, List, Tuple
 
 from dvc.path_info import PathInfo
 from dvc.types import DvcPath
@@ -13,19 +13,19 @@ logger = logging.getLogger(__name__)
 
 
 FilterFn = Callable[["BaseOutput"], bool]
-Outputs = Set["BaseOutput"]
-DvcPaths = Set[DvcPath]
+Outputs = List["BaseOutput"]
+DvcPaths = List[DvcPath]
 
 
 def _collect_outs(
     repo: "Repo", output_filter: FilterFn = None, deps: bool = False,
 ) -> Outputs:
-    outs = {
+    outs = [
         out
         for stage in repo.graph  # using `graph` to ensure graph checks run
         for out in (stage.deps if deps else stage.outs)
-    }
-    return set(filter(output_filter, outs)) if output_filter else outs
+    ]
+    return list(filter(output_filter, outs)) if output_filter else outs
 
 
 def _collect_paths(
@@ -36,14 +36,14 @@ def _collect_paths(
 ):
     from dvc.tree.repo import RepoTree
 
-    path_infos = {PathInfo(os.path.abspath(target)) for target in targets}
+    path_infos = [PathInfo(os.path.abspath(target)) for target in targets]
     tree = RepoTree(repo)
 
-    target_infos = set()
+    target_infos = []
     for path_info in path_infos:
 
         if recursive and tree.isdir(path_info):
-            target_infos.update(set(tree.walk_files(path_info)))
+            target_infos.extend(tree.walk_files(path_info))
 
         if not tree.exists(path_info):
             if not recursive:
@@ -51,19 +51,19 @@ def _collect_paths(
                     "'%s' was not found at: '%s'.", path_info, rev,
                 )
             continue
-        target_infos.add(path_info)
+        target_infos.append(path_info)
     return target_infos
 
 
 def _filter_duplicates(
     outs: Outputs, path_infos: DvcPaths
 ) -> Tuple[Outputs, DvcPaths]:
-    res_outs: Outputs = set()
-    res_infos = set(path_infos)
+    res_outs: Outputs = []
+    res_infos = path_infos
 
     for out in outs:
         if out.path_info in path_infos:
-            res_outs.add(out)
+            res_outs.append(out)
             res_infos.remove(out.path_info)
 
     return res_outs, res_infos
@@ -82,7 +82,7 @@ def collect(
     outs: Outputs = _collect_outs(repo, output_filter=output_filter, deps=deps)
 
     if not targets:
-        path_infos: DvcPaths = set()
+        path_infos: DvcPaths = []
         return outs, path_infos
 
     target_infos = _collect_paths(repo, targets, recursive=recursive, rev=rev)

dvc/repo/params/show.py

@@ -35,11 +35,13 @@ def _collect_configs(repo: "Repo", rev, targets=None) -> List["DvcPath"]:
         output_filter=_is_params,
         rev=rev,
     )
-    path_infos.update({p.path_info for p in params})
+    path_infos.extend([p.path_info for p in params])
     if not targets:
-        path_infos.add(
+        default_params = (
             PathInfo(repo.root_dir) / ParamsDependency.DEFAULT_PARAMS_FILE
         )
+        if default_params not in path_infos:
+            path_infos.append(default_params)
     return list(path_infos)
 
 

dvc/repo/plots/__init__.py

@@ -102,7 +102,7 @@ class Plots:
         plots = _prepare_plots(data, revs, props)
 
         result = {}
-        for datafile, desc in sorted(plots.items()):
+        for datafile, desc in plots.items():
             try:
                 result[datafile] = _render(
                     datafile, desc["data"], desc["props"], templates

tests/conftest.py

@@ -138,3 +138,17 @@ def pytest_configure(config):
             enabled_remotes.discard(remote_name)
         if enabled:
             enabled_remotes.add(remote_name)
+
+
+@pytest.fixture
+def dummy_stage(tmp_dir, dvc):
+    def make(path="dvc.yaml", name="dummy_stage", **kwargs):
+        from dvc.stage import PipelineStage, create_stage
+
+        stage = create_stage(
+            PipelineStage, dvc, path, name=name, cmd="", **kwargs
+        )
+        stage.dump()
+        return stage
+
+    return make

tests/unit/test_metrics.py

@@ -0,0 +1,28 @@
+import json
+import os
+
+
+def test_metrics_order(tmp_dir, dvc, dummy_stage):
+    tmp_dir.gen(
+        {
+            "p.json": json.dumps({"p1": 1}),
+            "p1.json": json.dumps({"p2": 1}),
+            "sub": {
+                "p3.json": json.dumps({"p3": 1}),
+                "p4.json": json.dumps({"p4": 1}),
+            },
+        }
+    )
+
+    dummy_stage(metrics=["p.json", str(tmp_dir / "sub" / "p4.json")])
+    dummy_stage(
+        path=str(tmp_dir / "sub" / "dvc.yaml"),
+        metrics=["p1.json", str(tmp_dir / "sub" / "p3.json")],
+    )
+
+    assert list(dvc.metrics.show()[""]) == [
+        "p.json",
+        os.path.join("sub", "p4.json"),
+        "p1.json",
+        os.path.join("sub", "p3.json"),
+    ]

tests/unit/test_params.py

@@ -0,0 +1,22 @@
+import os
+
+from dvc.utils.serialize import dumps_yaml
+
+
+def test_params_order(tmp_dir, dvc, dummy_stage):
+    tmp_dir.gen(
+        {
+            "params.yaml": dumps_yaml({"p1": 1}),
+            "p1.yaml": dumps_yaml({"p2": 1}),
+            "sub": {"p2.yaml": dumps_yaml({"p3": 1})},
+        }
+    )
+
+    p2_path = os.path.join("sub", "p2.yaml")
+    sub_stage = os.path.join("sub", "dvc.yaml")
+
+    dummy_stage(params=[{p2_path: ["p3"]}, {"p1.yaml": ["p2"]}])
+    dummy_stage(path=sub_stage, params=["p1"])
+
+    # params are sorted during dumping, therefore p1 is first
+    assert list(dvc.params.show()[""]) == ["p1.yaml", p2_path, "params.yaml"]

tests/unit/test_plots.py

@@ -0,0 +1,28 @@
+import json
+import os
+
+
+def test_plots_order(tmp_dir, dvc, dummy_stage):
+    tmp_dir.gen(
+        {
+            "p.json": json.dumps([{"p1": 1}, {"p1": 2}]),
+            "p1.json": json.dumps([{"p2": 1}, {"p2": 2}]),
+            "sub": {
+                "p3.json": json.dumps([{"p3": 1}, {"p3": 2}]),
+                "p4.json": json.dumps([{"p4": 1}, {"p4": 2}]),
+            },
+        }
+    )
+
+    dummy_stage(plots=["p.json", str(tmp_dir / "sub" / "p4.json")])
+    dummy_stage(
+        path=str(tmp_dir / "sub" / "dvc.yaml"),
+        plots=["p1.json", str(tmp_dir / "sub" / "p3.json")],
+    )
+
+    assert list(dvc.plots.show()) == [
+        "p.json",
+        os.path.join("sub", "p4.json"),
+        "p1.json",
+        os.path.join("sub", "p3.json"),
+    ]