From 2057c471bb6bbf85fd7fe807f5a9194bdbe7b49f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Thu, 19 May 2022 11:52:51 +0545 Subject: [PATCH] data.checkout: remove dependency on DVC; prompts and DvcException --- dvc/data/checkout.py | 54 +++++++++++++------ dvc/data/slow_link_detection.py | 45 ---------------- dvc/dependency/repo.py | 3 ++ dvc/output.py | 21 +++++++- tests/unit/remote/test_slow_link_detection.py | 45 ---------------- 5 files changed, 60 insertions(+), 108 deletions(-) delete mode 100644 tests/unit/remote/test_slow_link_detection.py diff --git a/dvc/data/checkout.py b/dvc/data/checkout.py index 803746b67..0840e7d63 100644 --- a/dvc/data/checkout.py +++ b/dvc/data/checkout.py @@ -1,15 +1,12 @@ import logging from itertools import chain -from typing import TYPE_CHECKING, Optional +from typing import TYPE_CHECKING, List, Optional -from dvc import prompt -from dvc.exceptions import CacheLinkError, CheckoutError, ConfirmRemoveError from dvc.objects.fs._callback import FsspecCallback from dvc.objects.fs.generic import test_links, transfer from .diff import ROOT from .diff import diff as odiff -from .slow_link_detection import slow_link_guard # type: ignore[attr-defined] from .stage import stage if TYPE_CHECKING: @@ -18,7 +15,25 @@ if TYPE_CHECKING: logger = logging.getLogger(__name__) -def _remove(fs_path, fs, in_cache, force=False): +class PromptError(Exception): + def __init__(self, path: str) -> None: + self.path = path + super().__init__(f"unable to remove '{path}' without a confirmation.") + + +class CheckoutError(Exception): + def __init__(self, paths: List[str]) -> None: + self.paths = paths + super().__init__("Checkout failed") + + +class LinkError(Exception): + def __init__(self, path: str) -> None: + self.path = path + super().__init__("No possible cache link types for '{path}'.") + + +def _remove(fs_path, fs, in_cache, force=False, prompt=None): if not fs.exists(fs_path): return @@ -32,14 +47,14 @@ def _remove(fs_path, fs, in_cache, force=False): "Are you sure you want to proceed?" ) - if not prompt.confirm(msg): - raise ConfirmRemoveError(fs_path) + if prompt is None or not prompt(msg): + raise PromptError(fs_path) fs.remove(fs_path) -def _relink(link, cache, cache_info, fs, path, in_cache, force): - _remove(path, fs, in_cache, force=force) +def _relink(link, cache, cache_info, fs, path, in_cache, force, prompt=None): + _remove(path, fs, in_cache, force=force, prompt=prompt) link(cache, cache_info, fs, path) # NOTE: Depending on a file system (e.g. on NTFS), `_remove` might reset # read-only permissions in order to delete a hardlink to protected object, @@ -57,6 +72,7 @@ def _checkout_file( force, relink=False, state=None, + prompt=None, ): """The file is changed we need to checkout a new copy""" modified = False @@ -75,6 +91,7 @@ def _checkout_file( fs_path, change.old.in_cache, force=force, + prompt=prompt, ) else: modified = True @@ -86,6 +103,7 @@ def _checkout_file( fs_path, change.old.in_cache, force=force, + prompt=prompt, ) else: link(cache, cache_fs_path, fs, fs_path) @@ -130,7 +148,6 @@ class Link: def __init__(self, links): self._links = links - @slow_link_guard def __call__(self, cache, from_path, to_fs, to_path, callback=None): if to_fs.exists(to_path): to_fs.remove(to_path) # broken symlink @@ -153,7 +170,7 @@ class Link: except FileNotFoundError as exc: raise CheckoutError([to_path]) from exc except OSError as exc: - raise CacheLinkError([to_path]) from exc + raise LinkError(to_path) from exc def _checkout( @@ -165,22 +182,24 @@ def _checkout( progress_callback=None, relink=False, state=None, + prompt=None, ): if not diff: return links = test_links(cache.cache_types, cache.fs, cache.fs_path, fs, fs_path) if not links: - raise CacheLinkError([fs_path]) + raise LinkError(fs_path) link = Link(links) - for change in diff.deleted: entry_path = ( fs.path.join(fs_path, *change.old.key) if change.old.key != ROOT else fs_path ) - _remove(entry_path, fs, change.old.in_cache, force=force) + _remove( + entry_path, fs, change.old.in_cache, force=force, prompt=prompt + ) failed = [] for change in chain(diff.added, diff.modified): @@ -203,11 +222,12 @@ def _checkout( force, relink, state=state, + prompt=prompt, ) if progress_callback: progress_callback(entry_path) except CheckoutError as exc: - failed.extend(exc.target_infos) + failed.extend(exc.paths) if failed: raise CheckoutError(failed) @@ -224,6 +244,7 @@ def checkout( quiet=False, ignore: Optional["Ignore"] = None, state=None, + prompt=None, ): # if protocol(fs_path) not in ["local", cache.fs.protocol]: # raise NotImplementedError @@ -258,9 +279,10 @@ def checkout( progress_callback=progress_callback, relink=relink, state=state, + prompt=prompt, ) except CheckoutError as exc: - failed.extend(exc.target_infos) + failed.extend(exc.paths) if diff and state: state.save_link(fs_path, fs) diff --git a/dvc/data/slow_link_detection.py b/dvc/data/slow_link_detection.py index 67f82dab2..e69de29bb 100644 --- a/dvc/data/slow_link_detection.py +++ b/dvc/data/slow_link_detection.py @@ -1,45 +0,0 @@ -# type: ignore -import logging -import sys -import time -from functools import wraps - -import colorama - -logger = logging.getLogger(__name__) -this = sys.modules[__name__] - -this.timeout_seconds = 10.0 -this.already_displayed = False -this.message = ( - "You can cut execution time considerably by using a different " - "`cache.type` configuration.\n" - "See {blue}https://dvc.org/doc/commands-reference/config#cache{reset} " - "for more information.\n" - "To disable this message, run:\n" - "`dvc config cache.slow_link_warning false`".format( - blue=colorama.Fore.BLUE, reset=colorama.Fore.RESET - ) -) - - -def slow_link_guard(f): - @wraps(f) - def wrapper(self, odb, *args, **kwargs): - if this.already_displayed: - return f(self, odb, *args, **kwargs) - - if not odb.slow_link_warning or odb.cache_types: - return f(self, odb, *args, **kwargs) - - start = time.time() - result = f(self, odb, *args, **kwargs) - delta = time.time() - start - - if delta >= this.timeout_seconds: - logger.warning(this.message) - this.already_displayed = True - - return result - - return wrapper diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index cbafacc63..84bf790dd 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -5,6 +5,8 @@ from typing import TYPE_CHECKING, Dict, Optional, Set, Tuple from voluptuous import Required +from dvc.prompt import confirm + from .base import Dependency if TYPE_CHECKING: @@ -74,6 +76,7 @@ class RepoDependency(Dependency): self.repo.odb.local, ignore=None, state=self.repo.state, + prompt=confirm, ) def update(self, rev=None): diff --git a/dvc/output.py b/dvc/output.py index 2c5f4022a..57254910e 100644 --- a/dvc/output.py +++ b/dvc/output.py @@ -9,8 +9,10 @@ from voluptuous import And, Any, Coerce, Length, Lower, Required, SetTo from dvc import prompt from dvc.exceptions import ( + CacheLinkError, CheckoutError, CollectCacheError, + ConfirmRemoveError, DvcException, MergeError, RemoteCacheRequiredError, @@ -577,6 +579,19 @@ class Output: if self.isfile() and self.meta.isexec: self.odb.set_exec(self.fs_path) + def _checkout(self, *args, **kwargs): + from dvc.data.checkout import CheckoutError as _CheckoutError + from dvc.data.checkout import LinkError, PromptError + + try: + return checkout(*args, **kwargs) + except PromptError as exc: + raise ConfirmRemoveError(exc.path) + except LinkError as exc: + raise CacheLinkError([exc.path]) + except _CheckoutError as exc: + raise CheckoutError(exc.paths) + def commit(self, filter_info=None): if not self.exists: raise self.DoesNotExistError(self) @@ -606,7 +621,7 @@ class Output: shallow=False, hardlink=True, ) - checkout( + self._checkout( filter_info or self.fs_path, self.fs, obj, @@ -614,6 +629,7 @@ class Output: relink=True, ignore=self.dvcignore, state=self.repo.state, + prompt=prompt.confirm, ) self.set_exec() @@ -766,7 +782,7 @@ class Output: added = not self.exists try: - modified = checkout( + modified = self._checkout( filter_info or self.fs_path, self.fs, obj, @@ -775,6 +791,7 @@ class Output: progress_callback=progress_callback, relink=relink, state=self.repo.state, + prompt=prompt.confirm, **kwargs, ) except CheckoutError: diff --git a/tests/unit/remote/test_slow_link_detection.py b/tests/unit/remote/test_slow_link_detection.py deleted file mode 100644 index 46066e3ae..000000000 --- a/tests/unit/remote/test_slow_link_detection.py +++ /dev/null @@ -1,45 +0,0 @@ -import pytest - -import dvc.data.slow_link_detection -from dvc.data.slow_link_detection import slow_link_guard - - -@pytest.fixture(autouse=True) -def timeout_immediately(monkeypatch): - monkeypatch.setattr(dvc.data.slow_link_detection, "timeout_seconds", 0.0) - - -@pytest.fixture -def make_remote(mocker): - def _make_remote(cache_type=None, should_warn=True): - remote = mocker.Mock() - remote.cache_types = cache_type - remote.slow_link_warning = should_warn - return remote - - return _make_remote - - -def test_show_warning_once(caplog, make_remote): - remote = make_remote() - slow_link_guard(lambda x, y: None)(None, remote) - slow_link_guard(lambda x, y: None)(None, remote) - - slow_link_detection = dvc.data.slow_link_detection - message = slow_link_detection.message # noqa, pylint: disable=no-member - assert len(caplog.records) == 1 - assert caplog.records[0].message == message - - -def test_dont_warn_when_cache_type_is_set(caplog, make_remote): - remote = make_remote(cache_type="copy") - slow_link_guard(lambda x, y: None)(None, remote) - - assert len(caplog.records) == 0 - - -def test_dont_warn_when_warning_is_disabled(caplog, make_remote): - remote = make_remote(should_warn=False) - slow_link_guard(lambda x, y: None)(None, remote) - - assert len(caplog.records) == 0