output: use path instead of relpath (#9673)

Relpath might not exist (e.g. on windows between different drives),
we should just use path as is and not get too fancy with the ui,
unless we want to constantly keep ValueError in mind when using
relpath for ui needs all over the place.

These operations should use index, where we always have current
directory and relative paths, so the ui will change accordingly
anyway.

.dvc/.gitignore

@@ -0,0 +1,3 @@
+/config.local
+/tmp
+/cache

dvc/fs/callbacks.py

@@ -2,12 +2,9 @@
 from contextlib import ExitStack
 from typing import TYPE_CHECKING, Any, BinaryIO, Dict, Optional, Union
 
+from dvc.progress import Tqdm
 from dvc.utils.objects import cached_property
-from dvc_objects.fs.callbacks import (  # noqa: F401
-    DEFAULT_CALLBACK,
-    Callback,
-    TqdmCallback,
-)
+from dvc_objects.fs.callbacks import DEFAULT_CALLBACK, Callback  # noqa: F401
 
 if TYPE_CHECKING:
     from rich.progress import TaskID
@@ -15,6 +12,59 @@ if TYPE_CHECKING:
     from dvc.ui._rich_progress import RichTransferProgress
 
 
+# NOTE: this is very similar to dvc-objects.fs.callbacks.TqdmCallback,
+# but it works with our own Tqdm instance.
+class TqdmCallback(Callback):
+    def __init__(
+        self,
+        size: Optional[int] = None,
+        value: int = 0,
+        progress_bar: Optional["Tqdm"] = None,
+        **tqdm_kwargs,
+    ):
+        tqdm_kwargs["total"] = size or -1
+        self._tqdm_kwargs = tqdm_kwargs
+        self._progress_bar = progress_bar
+        self._stack = ExitStack()
+        super().__init__(size=size, value=value)
+
+    @cached_property
+    def progress_bar(self):
+        progress_bar = (
+            self._progress_bar
+            if self._progress_bar is not None
+            else Tqdm(**self._tqdm_kwargs)
+        )
+        return self._stack.enter_context(progress_bar)
+
+    def __enter__(self):
+        return self
+
+    def close(self):
+        self._stack.close()
+
+    def set_size(self, size):
+        # Tqdm tries to be smart when to refresh,
+        # so we try to force it to re-render.
+        super().set_size(size)
+        self.progress_bar.refresh()
+
+    def call(self, hook_name=None, **kwargs):  # noqa: ARG002
+        self.progress_bar.update_to(self.value, total=self.size)
+
+    def branch(
+        self,
+        path_1: "Union[str, BinaryIO]",
+        path_2: str,
+        kwargs: Dict[str, Any],
+        child: Optional[Callback] = None,
+    ):
+        child = child or TqdmCallback(
+            bytes=True, desc=path_1 if isinstance(path_1, str) else path_2
+        )
+        return super().branch(path_1, path_2, kwargs, child=child)
+
+
 class RichCallback(Callback):
     def __init__(
         self,

dvc/output.py

@@ -20,6 +20,7 @@ from dvc.exceptions import (
     DvcException,
     MergeError,
 )
+from dvc.utils import format_link
 from dvc.utils.objects import cached_property
 from dvc_data.hashfile import check as ocheck
 from dvc_data.hashfile import load as oload
@@ -678,6 +679,14 @@ class Output:
     # pylint: enable=no-member
 
     def save(self) -> None:
+        if self.use_cache and not self.is_in_repo:
+            raise DvcException(
+                f"Saving cached external output {str(self)} is not supported "
+                "since DVC 3.0. See "
+                f"{format_link('https://dvc.org/doc/user-guide/upgrade')} "
+                "for more info."
+            )
+
         if not self.exists:
             raise self.DoesNotExistError(self)
 
@@ -1270,7 +1279,7 @@ class Output:
             raise FileNotFoundError(  # noqa: B904
                 errno.ENOENT,
                 os.strerror(errno.ENOENT),
-                self.fs.path.relpath(path),
+                path,
             )
 
         new = tree.from_trie(trie)
@@ -1393,8 +1402,7 @@ class Output:
         otransfer(staging, self.cache, {obj.hash_info}, hardlink=relink, shallow=False)
 
         if relink:
-            rel = self.fs.path.relpath(path)
-            with CheckoutCallback(desc=f"Checking out {rel}", unit="files") as callback:
+            with CheckoutCallback(desc="Checking out {path}", unit="files") as callback:
                 self._checkout(
                     path,
                     self.fs,

dvc/progress.py

@@ -2,12 +2,16 @@
 import logging
 import sys
 from threading import RLock
+from typing import TYPE_CHECKING
 
 from tqdm import tqdm
 
 from dvc.env import DVC_IGNORE_ISATTY
 from dvc.utils import env2bool
 
+if TYPE_CHECKING:
+    from dvc.fs.callbacks import TqdmCallback
+
 logger = logging.getLogger(__name__)
 tqdm.set_lock(RLock())
 
@@ -158,3 +162,8 @@ class Tqdm(tqdm):
             d["ncols_desc"] = d["ncols_info"] = 1
             d["prefix"] = ""
         return d
+
+    def as_callback(self) -> "TqdmCallback":
+        from dvc.fs.callbacks import TqdmCallback
+
+        return TqdmCallback(progress_bar=self)

dvc/repo/checkout.py

@@ -8,6 +8,7 @@ from dvc.exceptions import (
     DvcException,
     NoOutputOrStageError,
 )
+from dvc.ui import ui
 from dvc.utils import relpath
 
 from . import locked
@@ -106,7 +107,6 @@ def checkout(  # noqa: C901
     allow_missing=False,
     **kwargs,
 ):
-    from dvc.fs.callbacks import Callback
     from dvc.repo.index import build_data_index
     from dvc.stage.exceptions import StageFileBadNameError, StageFileDoesNotExistError
     from dvc_data.index.checkout import ADD, DELETE, MODIFY, apply, compare
@@ -142,21 +142,25 @@ def checkout(  # noqa: C901
         onerror=onerror,
     )
 
-    with Callback.as_tqdm_callback(
+    with ui.progress(
         unit="entry",
         desc="Building data index",
-    ) as cb:
+    ) as pb:
         old = build_data_index(
-            view, self.root_dir, self.fs, compute_hash=True, callback=cb
+            view,
+            self.root_dir,
+            self.fs,
+            compute_hash=True,
+            callback=pb.as_callback(),
         )
 
     new = view.data["repo"]
 
-    with Callback.as_tqdm_callback(
+    with ui.progress(
         desc="Comparing indexes",
         unit="entry",
-    ) as cb:
-        diff = compare(old, new, relink=relink, delete=True, callback=cb)
+    ) as pb:
+        diff = compare(old, new, relink=relink, delete=True, callback=pb.as_callback())
 
     if not force:
         _check_can_delete(diff.files_delete, new, self.root_dir, self.fs)
@@ -166,22 +170,22 @@ def checkout(  # noqa: C901
 
     def checkout_onerror(src_path, dest_path, _exc):
         logger.debug(
-            "failed to create {%s} from {%s}", dest_path, src_path, exc_info=True
+            "failed to create '%s' from '%s'", dest_path, src_path, exc_info=True
         )
 
         for out_path in out_paths:
             if self.fs.path.isin_or_eq(dest_path, out_path):
                 failed.add(out_path)
 
-    with Callback.as_tqdm_callback(
+    with ui.progress(
         unit="file",
         desc="Checkout",
-    ) as cb:
+    ) as pb:
         apply(
             diff,
             self.root_dir,
             self.fs,
-            callback=cb,
+            callback=pb.as_callback(),
             update_meta=False,
             onerror=checkout_onerror,
             state=self.state,

dvc/repo/data.py

@@ -2,9 +2,11 @@ import os
 import posixpath
 from typing import TYPE_CHECKING, Any, Dict, Iterable, List, TypedDict, Union
 
+from dvc.fs.callbacks import DEFAULT_CALLBACK
 from dvc.ui import ui
 
 if TYPE_CHECKING:
+    from dvc.fs.callbacks import Callback
     from dvc.repo import Repo
     from dvc.scm import Git, NoSCM
     from dvc_data.index import DataIndex
@@ -50,6 +52,7 @@ def _diff(
     not_in_cache: bool = False,
     not_in_remote: bool = False,
     remote_refresh: bool = False,
+    callback: "Callback" = DEFAULT_CALLBACK,
 ) -> Dict[str, List[str]]:
     from dvc_data.index import StorageError
     from dvc_data.index.diff import UNCHANGED, UNKNOWN, diff
@@ -70,6 +73,7 @@ def _diff(
         shallow=not granular,
         hash_only=True,
         with_unknown=True,
+        callback=callback,
     ):
         if (
             change.typ == UNCHANGED
@@ -149,16 +153,27 @@ def _git_info(scm: Union["Git", "NoSCM"], untracked_files: str = "all") -> GitIn
 def _diff_index_to_wtree(repo: "Repo", **kwargs: Any) -> Dict[str, List[str]]:
     from .index import build_data_index
 
-    with ui.status("Building workspace index"):
+    with ui.progress(
+        desc="Building workspace index",
+        unit="entry",
+    ) as pb:
         workspace = build_data_index(
-            repo.index, repo.root_dir, repo.fs, compute_hash=True
+            repo.index,
+            repo.root_dir,
+            repo.fs,
+            compute_hash=True,
+            callback=pb.as_callback(),
         )
 
-    with ui.status("Calculating diff between index/workspace"):
+    with ui.progress(
+        desc="Calculating diff between index/workspace",
+        unit="entry",
+    ) as pb:
         return _diff(
             repo.index.data["repo"],
             workspace,
             not_in_cache=True,
+            callback=pb.as_callback(),
             **kwargs,
         )
 
@@ -171,8 +186,11 @@ def _diff_head_to_index(
     with repo.switch(head):
         head_index = repo.index.data["repo"]
 
-    with ui.status("Calculating diff between head/index"):
-        return _diff(head_index, index, **kwargs)
+    with ui.progress(
+        desc="Calculating diff between head/index",
+        unit="entry",
+    ) as pb:
+        return _diff(head_index, index, callback=pb.as_callback(), **kwargs)
 
 
 class Status(TypedDict):

dvc/repo/experiments/collect.py

@@ -10,6 +10,7 @@ from typing import (
     Iterator,
     List,
     Optional,
+    Tuple,
     Union,
 )
 
@@ -369,12 +370,13 @@ def _describe(
 
 
 def _sorted_ranges(exp_ranges: Iterable["ExpRange"]) -> List["ExpRange"]:
-    """Return list of ExpRange sorted by timestamp."""
+    """Return list of ExpRange sorted by (timestamp, rev)."""
 
-    def _head_timestamp(exp_range: "ExpRange") -> datetime:
+    def _head_timestamp(exp_range: "ExpRange") -> Tuple[datetime, str]:
         head_exp = first(exp_range.revs)
         if head_exp and head_exp.data and head_exp.data.timestamp:
-            return head_exp.data.timestamp
-        return datetime.fromtimestamp(0)
+            return head_exp.data.timestamp, head_exp.rev
+
+        return datetime.fromtimestamp(0), ""
 
     return sorted(exp_ranges, key=_head_timestamp, reverse=True)

dvc/repo/experiments/executor/ssh.py

@@ -210,8 +210,7 @@ class SSHExecutor(BaseExecutor):
         """Collect DVC cache."""
         from dvc.repo.experiments.pull import _pull_cache
 
-        with self.get_odb() as odb:
-            _pull_cache(repo, exp_ref, run_cache=run_cache, odb=odb)
+        _pull_cache(repo, exp_ref, run_cache=run_cache)
 
     @contextmanager
     def get_odb(self):

dvc/repo/experiments/pull.py

@@ -107,10 +107,9 @@ def _pull_cache(
     dvc_remote=None,
     jobs=None,
     run_cache=False,
-    odb=None,
 ):
     if isinstance(refs, ExpRefInfo):
         refs = [refs]
     revs = list(exp_commits(repo.scm, refs))
     logger.debug("dvc fetch experiment '%s'", refs)
-    repo.fetch(jobs=jobs, remote=dvc_remote, run_cache=run_cache, revs=revs, odb=odb)
+    repo.fetch(jobs=jobs, remote=dvc_remote, run_cache=run_cache, revs=revs)

tests/unit/command/test_data_status.py

@@ -75,7 +75,8 @@ def test_json(dvc, mocker, capsys, mocked_status, args, to_omit):
 
 def test_no_changes_repo(dvc, scm, capsys):
     assert main(["data", "status"]) == 0
-    assert capsys.readouterr() == ("No changes.\n", "")
+    out, _ = capsys.readouterr()
+    assert out == "No changes.\n"
 
 
 def test_empty_scm_repo(tmp_dir, capsys):
@@ -83,7 +84,7 @@ def test_empty_scm_repo(tmp_dir, capsys):
     Repo.init()
 
     assert main(["data", "status"]) == 0
-    out, err = capsys.readouterr()
+    out, _ = capsys.readouterr()
     assert (
         out
         == """\
@@ -91,7 +92,6 @@ No changes in an empty git repo.
 (there are changes not tracked by dvc, use "git status" to see)
 """
     )
-    assert not err
 
 
 @pytest.mark.parametrize(

tests/unit/repo/experiments/test_collect.py

@@ -0,0 +1,69 @@
+import datetime
+import random
+from typing import Dict, List
+
+import pytest
+
+from dvc.repo.experiments.collect import ExpRange, ExpState, SerializableExp, collect
+
+
+@pytest.mark.vscode
+def test_collect_stable_sorting(dvc, scm, mocker):
+    """
+    Check that output is deterministically sorted even for
+    commits with the same timestamp. This affects the experience
+    in vs-code to avoid experiments "bouncing around" when "exp show"
+    is called repeatedly
+    """
+    expected_revs = [
+        "c" * 40,
+        "b" * 40,
+        "a" * 40,
+        "7" * 40,
+    ]
+
+    def collect_queued_patched(_, baseline_revs) -> Dict[str, List["ExpRange"]]:
+        single_timestamp = datetime.datetime(2023, 6, 20, 0, 0, 0)
+
+        exp_ranges = [
+            ExpRange(
+                revs=[
+                    ExpState(
+                        rev=rev,
+                        name=f"exp-state-{rev[0]}",
+                        data=SerializableExp(rev=rev, timestamp=single_timestamp),
+                    )
+                ],
+                name=f"exp-range-{rev[0]}",
+            )
+            for rev in expected_revs
+        ]
+
+        # shuffle collection order
+        random.shuffle(exp_ranges)
+
+        return {baseline_rev: exp_ranges for baseline_rev in baseline_revs}
+
+    mocker.patch("dvc.repo.experiments.collect.collect_queued", collect_queued_patched)
+    mocker.patch("dvc.repo.experiments.collect.collect_active", return_value={})
+    mocker.patch("dvc.repo.experiments.collect.collect_failed", return_value={})
+    mocker.patch("dvc.repo.experiments.collect.collect_successful", return_value={})
+
+    # repeat (shuffling collection order in collect_queued_patched)
+    for _ in range(20):
+        collected = collect(repo=dvc, all_commits=True)
+        assert collected[0].rev == "workspace"
+        assert collected[0].experiments is None
+        assert collected[1].rev == scm.get_rev()
+        _assert_experiment_rev_order(collected[1].experiments, expected_revs)
+
+
+def _assert_experiment_rev_order(
+    actual: List["ExpRange"],
+    expected_revs: List[str],
+):
+    expected_revs = expected_revs.copy()
+
+    for actual_exp_range in actual:
+        for exp_state in actual_exp_range.revs:
+            assert exp_state.rev == expected_revs.pop(0)