From 32cd6c2c03652011b5d2d432a645d52f3950eb08 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: Wed, 18 May 2022 11:48:31 +0545 Subject: [PATCH] dvc.objects: use stub ignore in dvc.objects/dvc.data This commit: * defines a `Ignore` protocol in `dvc.objects`, that expects a `find` method. This will be used in dvc-data and dvc-objects instead of depending on dvc.ignore. dvcignore is expected to respect this protocol, which it does. * renames `dvcignore` kwarg to `ignore` in `dvc-data` and `dvc-objects`. * teaches dvc.cli that `IgnoreInCollectedError` is a known exception and is logged correctly and adds a test for the behaviour. I am not happy that we need to hardcode `.dvcignore` in dvc-data but I don't see any easy way, since we always try to avoid staging if the tree contains .dvcignore file, even if .dvcignore is not passed. --- dvc/cli/__init__.py | 57 +++++++++++++++++++++----------- dvc/data/checkout.py | 14 ++++---- dvc/data/stage.py | 30 +++++++++++++---- dvc/dependency/repo.py | 2 +- dvc/exceptions.py | 8 ----- dvc/objects/_ignore.py | 11 ++++++ dvc/objects/state.py | 24 +++++++++----- dvc/objects/utils.py | 7 ++-- dvc/output.py | 12 +++---- dvc/repo/diff.py | 2 +- tests/func/test_external_repo.py | 2 +- tests/func/test_ignore.py | 4 +-- tests/unit/cli/test_main.py | 16 +++++++++ 13 files changed, 125 insertions(+), 64 deletions(-) create mode 100644 dvc/objects/_ignore.py diff --git a/dvc/cli/__init__.py b/dvc/cli/__init__.py index a3f22f646..7aa7e072c 100644 --- a/dvc/cli/__init__.py +++ b/dvc/cli/__init__.py @@ -6,6 +6,7 @@ import sys # Workaround for CPython bug. See [1] and [2] for more info. # [1] https://github.com/aws/aws-cli/blob/1.16.277/awscli/clidriver.py#L55 # [2] https://bugs.python.org/issue29288 +from typing import Optional "".encode("idna") @@ -36,11 +37,37 @@ def parse_args(argv=None): return args -def _log_exceptions(exc: Exception): +def _log_unknown_exceptions() -> None: + from dvc.info import get_dvc_info + from dvc.logger import FOOTER + + logger.exception("unexpected error") + if logger.isEnabledFor(logging.DEBUG): + logger.debug("Version info for developers:\n%s", get_dvc_info()) + logger.info(FOOTER) + return None + + +def _log_exceptions(exc: Exception) -> Optional[int]: """Try to log some known exceptions, that are not DVCExceptions.""" - from dvc.fs import AuthError, ConfigError, RemoteMissingDepsError from dvc.utils import error_link, format_link + if isinstance(exc, OSError): + import errno + + if exc.errno == errno.EMFILE: + logger.exception( + "too many open files, please visit " + "{} to see how to handle this " + "problem".format(error_link("many-files")), + extra={"tb_only": True}, + ) + else: + _log_unknown_exceptions() + return None + + from dvc.fs import AuthError, ConfigError, RemoteMissingDepsError + if isinstance(exc, RemoteMissingDepsError): from dvc.utils.pkg import PKG @@ -69,7 +96,7 @@ def _log_exceptions(exc: Exception): f"dependencies: {exc.missing_deps}. {hint}", extra={"tb_only": True}, ) - return + return None if isinstance(exc, (AuthError, ConfigError)): link = format_link("https://man.dvc.org/remote/modify") @@ -92,26 +119,16 @@ def _log_exceptions(exc: Exception): f"\nSee {error_link('pickle')} for more information.", extra={"tb_only": True}, ) - return + return None - if isinstance(exc, OSError): - import errno + from dvc.data.stage import IgnoreInCollectedDirError - if exc.errno == errno.EMFILE: - logger.exception( - "too many open files, please visit " - "{} to see how to handle this " - "problem".format(error_link("many-files")), - extra={"tb_only": True}, - ) - return + if isinstance(exc, IgnoreInCollectedDirError): + logger.exception("") + return None - from dvc.info import get_dvc_info - from dvc.logger import FOOTER - - logger.exception("unexpected error") - logger.debug("Version info for developers:\n%s", get_dvc_info()) - logger.info(FOOTER) + _log_unknown_exceptions() + return None def main(argv=None): # noqa: C901 diff --git a/dvc/data/checkout.py b/dvc/data/checkout.py index e935acb01..803746b67 100644 --- a/dvc/data/checkout.py +++ b/dvc/data/checkout.py @@ -1,10 +1,9 @@ import logging from itertools import chain -from typing import Optional +from typing import TYPE_CHECKING, Optional from dvc import prompt from dvc.exceptions import CacheLinkError, CheckoutError, ConfirmRemoveError -from dvc.ignore import DvcIgnoreFilter from dvc.objects.fs._callback import FsspecCallback from dvc.objects.fs.generic import test_links, transfer @@ -13,6 +12,9 @@ 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: + from dvc.objects._ignore import Ignore + logger = logging.getLogger(__name__) @@ -101,7 +103,7 @@ def _diff( obj, cache, relink=False, - dvcignore: Optional[DvcIgnoreFilter] = None, + ignore: Optional["Ignore"] = None, ): old = None try: @@ -111,7 +113,7 @@ def _diff( fs, obj.hash_info.name if obj else cache.fs.PARAM_CHECKSUM, dry_run=True, - dvcignore=dvcignore, + ignore=ignore, ) except FileNotFoundError: pass @@ -220,7 +222,7 @@ def checkout( progress_callback=None, relink=False, quiet=False, - dvcignore: Optional[DvcIgnoreFilter] = None, + ignore: Optional["Ignore"] = None, state=None, ): # if protocol(fs_path) not in ["local", cache.fs.protocol]: @@ -232,7 +234,7 @@ def checkout( obj, cache, relink=relink, - dvcignore=dvcignore, + ignore=ignore, ) failed = [] diff --git a/dvc/data/stage.py b/dvc/data/stage.py index 84cb35daa..f512357b0 100644 --- a/dvc/data/stage.py +++ b/dvc/data/stage.py @@ -6,8 +6,6 @@ from concurrent.futures import ThreadPoolExecutor from functools import partial from typing import TYPE_CHECKING, Dict, Optional, Tuple -from dvc.exceptions import DvcIgnoreInCollectedDirError -from dvc.ignore import DvcIgnore from dvc.objects.file import HashFile from dvc.objects.hash import hash_file from dvc.objects.hash_info import HashInfo @@ -17,12 +15,24 @@ from dvc.progress import Tqdm from .db.reference import ReferenceObjectDB if TYPE_CHECKING: + from dvc.objects._ignore import Ignore from dvc.objects.db import ObjectDB from dvc.objects.fs.base import AnyFSPath, FileSystem from .tree import Tree +DefaultIgnoreFile = ".dvcignore" + + +class IgnoreInCollectedDirError(Exception): + def __init__(self, ignore_file: str, ignore_dirname: str) -> None: + super().__init__( + f"{ignore_file} file should not be in collected dir path: " + f"'{ignore_dirname}'" + ) + + logger = logging.getLogger(__name__) @@ -73,13 +83,13 @@ def _build_objects( fs_path, fs, name, - dvcignore=None, + ignore: "Ignore" = None, jobs=None, no_progress_bar=False, **kwargs, ): - if dvcignore: - walk_iterator = dvcignore.find(fs, fs_path) + if ignore: + walk_iterator = ignore.find(fs, fs_path) else: walk_iterator = fs.find(fs_path) with Tqdm( @@ -109,10 +119,16 @@ def _build_tree(fs_path, fs, name, **kwargs): from .tree import Tree tree_meta = Meta(size=0, nfiles=0) + # assuring mypy that they are not None but integer + assert tree_meta.size is not None + assert tree_meta.nfiles is not None + tree = Tree(None, None, None) for file_fs_path, meta, obj in _iter_objects(fs_path, fs, name, **kwargs): - if DvcIgnore.DVCIGNORE_FILE == fs.path.name(file_fs_path): - raise DvcIgnoreInCollectedDirError(fs.path.parent(file_fs_path)) + if fs.path.name(file_fs_path) == DefaultIgnoreFile: + raise IgnoreInCollectedDirError( + DefaultIgnoreFile, fs.path.parent(file_fs_path) + ) # NOTE: this is lossy transformation: # "hey\there" -> "hey/there" diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index 5651d5501..cbafacc63 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -72,7 +72,7 @@ class RepoDependency(Dependency): to.fs, obj, self.repo.odb.local, - dvcignore=None, + ignore=None, state=self.repo.state, ) diff --git a/dvc/exceptions.py b/dvc/exceptions.py index 127d46362..b85ef8e0a 100644 --- a/dvc/exceptions.py +++ b/dvc/exceptions.py @@ -202,14 +202,6 @@ class FileMissingError(DvcException): ) -class DvcIgnoreInCollectedDirError(DvcException): - def __init__(self, ignore_dirname): - super().__init__( - ".dvcignore file should not be in collected dir path: " - "'{}'".format(ignore_dirname) - ) - - class FileTransferError(DvcException): _METHOD = "transfer" diff --git a/dvc/objects/_ignore.py b/dvc/objects/_ignore.py new file mode 100644 index 000000000..da3dcc57a --- /dev/null +++ b/dvc/objects/_ignore.py @@ -0,0 +1,11 @@ +from typing import TYPE_CHECKING, Iterator, Protocol + +if TYPE_CHECKING: + from .fs.base import AnyFSPath, FileSystem + + +class Ignore(Protocol): + def find( + self, fs: "FileSystem", path: "AnyFSPath" + ) -> Iterator["AnyFSPath"]: + ... diff --git a/dvc/objects/state.py b/dvc/objects/state.py index 3761a7322..03f9609b3 100644 --- a/dvc/objects/state.py +++ b/dvc/objects/state.py @@ -3,6 +3,7 @@ import logging import os from abc import ABC, abstractmethod +from typing import TYPE_CHECKING from .fs import LocalFileSystem from .fs.system import inode as get_inode @@ -10,6 +11,10 @@ from .fs.utils import relpath from .hash_info import HashInfo from .utils import get_mtime_and_size +if TYPE_CHECKING: + from ._ignore import Ignore + + logger = logging.getLogger(__name__) @@ -46,21 +51,22 @@ class StateNoop(StateBase): class State(StateBase): # pylint: disable=too-many-instance-attributes - def __init__(self, root_dir=None, tmp_dir=None, dvcignore=None): + def __init__(self, root_dir=None, tmp_dir=None, ignore: "Ignore" = None): from .cache import Cache super().__init__() self.tmp_dir = tmp_dir self.root_dir = root_dir - self.dvcignore = dvcignore + self.ignore = ignore if not tmp_dir: return - config = {"eviction_policy": "least-recently-used"} - self.links = Cache(directory=os.path.join(tmp_dir, "links"), **config) - self.md5s = Cache(directory=os.path.join(tmp_dir, "md5s"), **config) + links_dir = os.path.join(tmp_dir, "links") + md5s_dir = os.path.join(tmp_dir, "md5s") + self.links = Cache(links_dir, eviction_policy="least-recently-used") + self.md5s = Cache(md5s_dir, eviction_policy="least-recently-used") def close(self): self.md5s.close() @@ -77,7 +83,7 @@ class State(StateBase): # pylint: disable=too-many-instance-attributes if not isinstance(fs, LocalFileSystem): return - mtime, size = get_mtime_and_size(path, fs, self.dvcignore) + mtime, size = get_mtime_and_size(path, fs, self.ignore) inode = get_inode(path) logger.debug( @@ -107,7 +113,7 @@ class State(StateBase): # pylint: disable=too-many-instance-attributes return None, None try: - mtime, size = get_mtime_and_size(path, fs, self.dvcignore) + mtime, size = get_mtime_and_size(path, fs, self.ignore) except FileNotFoundError: return None, None @@ -131,7 +137,7 @@ class State(StateBase): # pylint: disable=too-many-instance-attributes return try: - mtime, _ = get_mtime_and_size(path, fs, self.dvcignore) + mtime, _ = get_mtime_and_size(path, fs, self.ignore) except FileNotFoundError: return @@ -160,7 +166,7 @@ class State(StateBase): # pylint: disable=too-many-instance-attributes continue inode = get_inode(path) - mtime, _ = get_mtime_and_size(path, fs, self.dvcignore) + mtime, _ = get_mtime_and_size(path, fs, self.ignore) if ref[relative_path] == (inode, mtime): logger.debug("Removing '%s' as unused link.", path) diff --git a/dvc/objects/utils.py b/dvc/objects/utils.py index 989f84bf5..ea2da9c77 100644 --- a/dvc/objects/utils.py +++ b/dvc/objects/utils.py @@ -4,11 +4,12 @@ import json from typing import TYPE_CHECKING, Tuple if TYPE_CHECKING: + from ._ignore import Ignore from .fs.base import AnyFSPath, FileSystem def get_mtime_and_size( - path: "AnyFSPath", fs: "FileSystem", dvcignore=None + path: "AnyFSPath", fs: "FileSystem", ignore: "Ignore" = None ) -> Tuple[str, int]: import nanotime @@ -20,8 +21,8 @@ def get_mtime_and_size( size = 0 files_mtimes = {} - if dvcignore: - walk_iterator = dvcignore.find(fs, path) + if ignore: + walk_iterator = ignore.find(fs, path) else: walk_iterator = fs.find(path) for file_path in walk_iterator: diff --git a/dvc/output.py b/dvc/output.py index 54179cdfc..e355d704d 100644 --- a/dvc/output.py +++ b/dvc/output.py @@ -430,7 +430,7 @@ class Output: self.fs_path, self.fs, name, - dvcignore=self.dvcignore, + ignore=self.dvcignore, dry_run=not self.use_cache, ) return obj.hash_info @@ -552,7 +552,7 @@ class Output: self.fs_path, self.fs, self.fs.PARAM_CHECKSUM, - dvcignore=self.dvcignore, + ignore=self.dvcignore, dry_run=True, ) self.hash_info = obj.hash_info @@ -569,7 +569,7 @@ class Output: self.fs_path, self.fs, self.odb.fs.PARAM_CHECKSUM, - dvcignore=self.dvcignore, + ignore=self.dvcignore, ) self.hash_info = self.obj.hash_info @@ -597,7 +597,7 @@ class Output: filter_info or self.fs_path, self.fs, self.odb.fs.PARAM_CHECKSUM, - dvcignore=self.dvcignore, + ignore=self.dvcignore, ) otransfer( staging, @@ -612,7 +612,7 @@ class Output: obj, self.odb, relink=True, - dvcignore=self.dvcignore, + ignore=self.dvcignore, state=self.repo.state, ) self.set_exec() @@ -626,7 +626,7 @@ class Output: self.fs_path, self.fs, self.odb.fs.PARAM_CHECKSUM, - dvcignore=self.dvcignore, + ignore=self.dvcignore, ) save_obj = save_obj.filter(prefix) checkout_obj = save_obj.get(self.odb, prefix) diff --git a/dvc/repo/diff.py b/dvc/repo/diff.py index f257f3c30..071764758 100644 --- a/dvc/repo/diff.py +++ b/dvc/repo/diff.py @@ -142,7 +142,7 @@ def _output_paths(repo, targets): repo.odb.local.fs, "md5", dry_run=True, - dvcignore=output.dvcignore, + ignore=output.dvcignore, ) hash_info = obj.hash_info else: diff --git a/tests/func/test_external_repo.py b/tests/func/test_external_repo.py index 074c28baa..994109c24 100644 --- a/tests/func/test_external_repo.py +++ b/tests/func/test_external_repo.py @@ -209,7 +209,7 @@ def test_subrepos_are_ignored(tmp_dir, erepo_dir): "dir", repo.dvcfs, "md5", - dvcignore=repo.dvcignore, + ignore=repo.dvcignore, ) transfer( staging, diff --git a/tests/func/test_ignore.py b/tests/func/test_ignore.py index 073c881f5..634206ff3 100644 --- a/tests/func/test_ignore.py +++ b/tests/func/test_ignore.py @@ -4,7 +4,7 @@ from pathlib import Path import pytest -from dvc.exceptions import DvcIgnoreInCollectedDirError +from dvc.data.stage import IgnoreInCollectedDirError from dvc.ignore import DvcIgnore, DvcIgnorePatterns from dvc.objects.utils import get_mtime_and_size from dvc.output import OutputIsIgnoredError @@ -87,7 +87,7 @@ def test_remove_file(tmp_dir, dvc): def test_dvcignore_in_out_dir(tmp_dir, dvc): tmp_dir.gen({"dir": {"foo": "foo", DvcIgnore.DVCIGNORE_FILE: ""}}) - with pytest.raises(DvcIgnoreInCollectedDirError): + with pytest.raises(IgnoreInCollectedDirError): dvc.add("dir") diff --git a/tests/unit/cli/test_main.py b/tests/unit/cli/test_main.py index 95e19292d..28e95afe1 100644 --- a/tests/unit/cli/test_main.py +++ b/tests/unit/cli/test_main.py @@ -4,6 +4,7 @@ import pytest from funcy import raiser from dvc.cli import main +from dvc.data.stage import IgnoreInCollectedDirError from dvc.objects.cache import DiskError from dvc.objects.fs.base import FileSystem, RemoteMissingDepsError @@ -68,3 +69,18 @@ def test_remote_missing_deps_are_correctly_reported( "Thank you!" ) assert expected in caplog.text + + +def test_ignore_in_collected_dir_error_is_logged(tmp_dir, caplog, mocker): + error = IgnoreInCollectedDirError(".dvcignore", "dir") + mocker.patch( + "dvc.cli.parse_args", + return_value=Namespace( + func=raiser(error), + quiet=False, + verbose=True, + ), + ) + assert main() == 255 + expected = ".dvcignore file should not be in collected dir path: 'dir'" + assert expected in caplog.text