diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 74975611d..92da7b400 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -1,9 +1,19 @@ -* [ ] Have you followed the guidelines in our - [Contributing document](https://dvc.org/doc/user-guide/contributing/core)? +* [ ] ❗ Have you followed the guidelines in the + [Contributing to DVC](https://dvc.org/doc/user-guide/contributing/core) + list? -* [ ] Does your PR affect documented changes or does it add new functionality - that should be documented? If yes, have you created a PR for - [dvc.org](https://github.com/iterative/dvc.org) documenting it or at - least opened an issue for it? If so, please add a link to it. +* [ ] 📖 Check this box if this PR **does not** require + [documentation](https://dvc.org/doc) updates, or if it does **and** you + have created a separate PR in + [dvc.org](https://github.com/iterative/dvc.org) + with such updates (or at least opened an issue about it in that repo). + Please link below to your PR (or issue) in the + [dvc.org](https://github.com/iterative/dvc.org) repo. + +* [ ] ❌ Have you checked DeepSource, CodeClimate, and other sanity checks + below? We consider their findings recommendatory and don't expect + everything to be addresses. Please review them carefully and fix those + that actually improve code or fix bugs. + +Thank you for the contribution - we'll try to review it as soon as possible. 🙏 ------ diff --git a/.github/release-drafter.yml b/.github/release-drafter.yml new file mode 100644 index 000000000..abf195c69 --- /dev/null +++ b/.github/release-drafter.yml @@ -0,0 +1,5 @@ +# Config for https://github.com/apps/release-drafter +branches: + - master +template: | + $CHANGES diff --git a/.gitignore b/.gitignore index d2a206fe3..33be5cb20 100644 --- a/.gitignore +++ b/.gitignore @@ -4,6 +4,7 @@ neatlynx/__pycache__ cache *.pyc .env/ +.env2.7/ cache .dvc.conf.lock diff --git a/.restyled.yaml b/.restyled.yaml new file mode 100644 index 000000000..d5eeec715 --- /dev/null +++ b/.restyled.yaml @@ -0,0 +1,11 @@ +--- +restylers: + - name: black + image: restyled/restyler-black:v19.3b0 + command: + - black + arguments: [] + include: + - "**/*.py" + interpreters: + - python diff --git a/dvc/command/destroy.py b/dvc/command/destroy.py index ca739de4a..3dc251387 100644 --- a/dvc/command/destroy.py +++ b/dvc/command/destroy.py @@ -24,7 +24,7 @@ class CmdDestroy(CmdBase): if not self.args.force and not prompt.confirm(statement): raise DvcException( "cannot destroy without a confirmation from the user." - " Use '-f' to force." + " Use `-f` to force." ) self.repo.destroy() diff --git a/dvc/command/gc.py b/dvc/command/gc.py index 699f88304..45d774f6d 100644 --- a/dvc/command/gc.py +++ b/dvc/command/gc.py @@ -83,7 +83,7 @@ def add_parser(subparsers, parent_parser): "--all-commits", action="store_true", default=False, - help="Keep data files for all commits.", + help=argparse.SUPPRESS, ) gc_parser.add_argument( "-c", diff --git a/dvc/command/imp_url.py b/dvc/command/imp_url.py index 012ea80c0..1b0b1cf87 100644 --- a/dvc/command/imp_url.py +++ b/dvc/command/imp_url.py @@ -19,7 +19,7 @@ class CmdImportUrl(CmdBase): except DvcException: logger.exception( "failed to import {}. You could also try downloading " - "it manually and adding it with `dvc add` command.".format( + "it manually, and adding it with `dvc add`.".format( self.args.url ) ) diff --git a/dvc/command/init.py b/dvc/command/init.py index 0e9bdf83f..075e3be4c 100644 --- a/dvc/command/init.py +++ b/dvc/command/init.py @@ -53,7 +53,7 @@ def add_parser(subparsers, parent_parser): action="store_true", default=False, help=( - "Overwrite existing '.dvc' directory. " + "Overwrite existing '.dvc/' directory. " "This operation removes local cache." ), ) diff --git a/dvc/command/remove.py b/dvc/command/remove.py index 814adcfae..a00448029 100644 --- a/dvc/command/remove.py +++ b/dvc/command/remove.py @@ -28,7 +28,7 @@ class CmdRemove(CmdBase): raise DvcException( "Cannot purge without a confirmation from the user." - " Use '-f' to force." + " Use `-f` to force." ) def run(self): diff --git a/dvc/command/run.py b/dvc/command/run.py index b0761435f..c51f10bab 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -27,9 +27,9 @@ class CmdRun(CmdBase): ] ): # pragma: no cover logger.error( - "too few arguments. Specify at least one: '-d', '-o', '-O'," - " '-m', '-M', '--outs-persist', '--outs-persist-no-cache'," - " 'command'." + "too few arguments. Specify at least one: `-d`, `-o`, `-O`, " + "`-m`, `-M`, `--outs-persist`, `--outs-persist-no-cache`, " + "`command`." ) return 1 diff --git a/dvc/command/version.py b/dvc/command/version.py index 078f1fdec..7cf00c751 100644 --- a/dvc/command/version.py +++ b/dvc/command/version.py @@ -18,7 +18,7 @@ from dvc.command.base import CmdBaseNoRepo, append_doc_link from dvc.version import __version__ from dvc.exceptions import DvcException, NotDvcRepoError from dvc.system import System - +from dvc.utils.pkg import get_package_manager logger = logging.getLogger(__name__) @@ -31,17 +31,19 @@ class CmdVersion(CmdBaseNoRepo): python_version = platform.python_version() platform_type = platform.platform() binary = is_binary() - + package_manager = get_package_manager() info = ( "DVC version: {dvc_version}\n" "Python version: {python_version}\n" "Platform: {platform_type}\n" "Binary: {binary}\n" + "Package manager: {package_manager}\n" ).format( dvc_version=dvc_version, python_version=python_version, platform_type=platform_type, binary=binary, + package_manager=package_manager, ) try: diff --git a/dvc/exceptions.py b/dvc/exceptions.py index 2309bcdbf..16b8b837b 100644 --- a/dvc/exceptions.py +++ b/dvc/exceptions.py @@ -179,7 +179,7 @@ class ConfirmRemoveError(DvcException): def __init__(self, path): super(ConfirmRemoveError, self).__init__( "unable to remove '{}' without a confirmation from the user. Use " - "'-f' to force.".format(path) + "`-f` to force.".format(path) ) @@ -209,7 +209,7 @@ class NoMetricsError(DvcException): def __init__(self): super(NoMetricsError, self).__init__( "no metric files in this repository. " - "Use 'dvc metrics add' to add a metric file to track." + "Use `dvc metrics add` to add a metric file to track." ) @@ -247,7 +247,7 @@ class OverlappingOutputPathsError(DvcException): class CheckoutErrorSuggestGit(DvcException): def __init__(self, target, cause): super(CheckoutErrorSuggestGit, self).__init__( - "Did you mean 'git checkout {}'?".format(target), cause=cause + "Did you mean `git checkout {}`?".format(target), cause=cause ) diff --git a/dvc/lock.py b/dvc/lock.py index 8859ff9e7..d3bc83d19 100644 --- a/dvc/lock.py +++ b/dvc/lock.py @@ -87,6 +87,17 @@ if is_py3: self._tmp_dir, filename + ".lock" ) + # Fix for __del__ bug in flufl.lock [1] which is causing errors on + # Python shutdown [2]. + # [1] https://gitlab.com/warsaw/flufl.lock/issues/7 + # [2] https://github.com/iterative/dvc/issues/2573 + def __del__(self): + try: + if self._owned: + self.finalize() + except ImportError: + pass + else: import zc.lockfile diff --git a/dvc/logger.py b/dvc/logger.py index 71f9d9147..f26ef31ce 100644 --- a/dvc/logger.py +++ b/dvc/logger.py @@ -11,6 +11,17 @@ import logging.config import colorama +FOOTER = ( + "\n{yellow}Having any troubles?{nc}" + " Hit us up at {blue}https://dvc.org/support{nc}," + " we are always happy to help!" +).format( + blue=colorama.Fore.BLUE, + nc=colorama.Fore.RESET, + yellow=colorama.Fore.YELLOW, +) + + class LoggingException(Exception): def __init__(self, record): msg = "failed to log {}".format(str(record)) @@ -48,16 +59,6 @@ class ColorFormatter(logging.Formatter): "CRITICAL": colorama.Fore.RED, } - footer = ( - "{yellow}Having any troubles?{nc}" - " Hit us up at {blue}https://dvc.org/support{nc}," - " we are always happy to help!" - ).format( - blue=colorama.Fore.BLUE, - nc=colorama.Fore.RESET, - yellow=colorama.Fore.YELLOW, - ) - def format(self, record): if record.levelname == "INFO": return record.msg @@ -66,10 +67,7 @@ class ColorFormatter(logging.Formatter): exception, stack_trace = self._parse_exc(record.exc_info) return ( - "{color}{levelname}{nc}: {description}" - "{stack_trace}\n" - "\n" - "{footer}" + "{color}{levelname}{nc}: {description}" "{stack_trace}\n" ).format( color=self.color_code.get(record.levelname, ""), nc=colorama.Fore.RESET, @@ -77,7 +75,6 @@ class ColorFormatter(logging.Formatter): description=self._description(record.msg, exception), msg=record.msg, stack_trace=stack_trace, - footer=self.footer, ) return "{color}{levelname}{nc}: {msg}".format( diff --git a/dvc/main.py b/dvc/main.py index f8b94b8d3..5d3f01193 100644 --- a/dvc/main.py +++ b/dvc/main.py @@ -6,6 +6,7 @@ import logging from dvc.cli import parse_args from dvc.lock import LockError +from dvc.logger import FOOTER from dvc.config import ConfigError from dvc.analytics import Analytics from dvc.exceptions import NotDvcRepoError, DvcParserError @@ -75,6 +76,9 @@ def main(argv=None): # so won't be reused by any other subsequent run anyway. clean_repos() + if ret != 0: + logger.info(FOOTER) + Analytics().send_cmd(cmd, args, ret) return ret diff --git a/dvc/output/base.py b/dvc/output/base.py index f940d3f52..8fa3ad367 100644 --- a/dvc/output/base.py +++ b/dvc/output/base.py @@ -334,7 +334,7 @@ class OutputBase(object): self.remote.unprotect(self.path_info) def _collect_used_dir_cache(self, remote=None, force=False, jobs=None): - """Get a list of `info`s retaled to the given directory. + """Get a list of `info`s related to the given directory. - Pull the directory entry from the remote cache if it was changed. diff --git a/dvc/output/local.py b/dvc/output/local.py index 777ac3756..cbd9c9503 100644 --- a/dvc/output/local.py +++ b/dvc/output/local.py @@ -26,7 +26,7 @@ class OutputLOCAL(OutputBase): # so we should expect both posix and windows style paths. # PathInfo accepts both, i.e. / works everywhere, \ only on win. # - # FIXME: if we have Windows path containig / or posix one with \ + # FIXME: if we have Windows path containing / or posix one with \ # then we have #2059 bug and can't really handle that. p = self.REMOTE.path_cls(path) if not p.is_absolute(): diff --git a/dvc/path_info.py b/dvc/path_info.py index 3db44a439..6707c8d81 100644 --- a/dvc/path_info.py +++ b/dvc/path_info.py @@ -8,13 +8,12 @@ from funcy import cached_property from dvc.utils.compat import str, builtin_str, basestring, is_py2 from dvc.utils.compat import pathlib, urlparse +from dvc.utils import relpath # On Python 2.7/Windows sys.getfilesystemencoding() is set to mbcs, # which is lossy, thus we can't use that, # see https://github.com/mcmtroffaes/pathlib2/issues/56. -from dvc.utils import relpath - if is_py2: fs_encoding = "utf-8" @@ -112,52 +111,80 @@ class PosixPathInfo(PathInfo, pathlib.PurePosixPath): pass +class _URLPathInfo(PosixPathInfo): + def __str__(self): + return self.__fspath__() + + __unicode__ = __str__ + + class _URLPathParents(object): - def __init__(self, pathcls, scheme, netloc, path): - self._scheme = scheme - self._netloc = netloc - self._parents = path.parents - self._pathcls = pathcls + def __init__(self, src): + self.src = src + self._parents = self.src._path.parents def __len__(self): return len(self._parents) def __getitem__(self, idx): - return self._pathcls.from_parts( - scheme=self._scheme, - netloc=self._netloc, - path=self._parents[idx].fspath, - ) + return self.src.replace(path=self._parents[idx]) def __repr__(self): - return "<{}.parents>".format(self._pathcls.__name__) + return "<{}.parents>".format(self.src) class URLInfo(object): DEFAULT_PORTS = {"http": 80, "https": 443, "ssh": 22, "hdfs": 0} def __init__(self, url): - self.parsed = urlparse(url) - assert self.parsed.scheme != "remote" + p = urlparse(url) + assert not p.query and not p.params and not p.fragment + assert p.password is None + + self.fill_parts(p.scheme, p.hostname, p.username, p.port, p.path) @classmethod def from_parts( - cls, scheme=None, netloc=None, host=None, user=None, port=None, path="" + cls, scheme=None, host=None, user=None, port=None, path="", netloc=None ): - assert scheme and (bool(host) ^ bool(netloc)) + assert bool(host) ^ bool(netloc) - if netloc is None: - netloc = host - if user: - netloc = user + "@" + host - if port: - netloc += ":" + str(port) - return cls("{}://{}{}".format(scheme, netloc, path)) + if netloc is not None: + return cls("{}://{}{}".format(scheme, netloc, path)) + + obj = cls.__new__(cls) + obj.fill_parts(scheme, host, user, port, path) + return obj + + def fill_parts(self, scheme, host, user, port, path): + assert scheme != "remote" + assert isinstance(path, (basestring, _URLPathInfo)) + + self.scheme, self.host, self.user = scheme, host, user + self.port = int(port) if port else self.DEFAULT_PORTS.get(self.scheme) + + if isinstance(path, _URLPathInfo): + self._spath = builtin_str(path) + self._path = path + else: + if path and path[0] != "/": + path = "/" + path + self._spath = path + + @property + def _base_parts(self): + return (self.scheme, self.host, self.user, self.port) + + @property + def parts(self): + return self._base_parts + self._path.parts + + def replace(self, path=None): + return self.from_parts(*self._base_parts, path=path) @cached_property def url(self): - p = self.parsed - return "{}://{}{}".format(p.scheme, self.netloc, p.path) + return "{}://{}{}".format(self.scheme, self.netloc, self._spath) def __str__(self): return self.url @@ -170,92 +197,60 @@ class URLInfo(object): other = self.__class__(other) return ( self.__class__ == other.__class__ - and self.scheme == other.scheme - and self.netloc == other.netloc + and self._base_parts == other._base_parts and self._path == other._path ) def __hash__(self): - return hash(self.url) + return hash(self.parts) def __div__(self, other): - p = self.parsed - new_path = posixpath.join(p.path, str(other)) - if not new_path.startswith("/"): - new_path = "/" + new_path - new_url = "{}://{}{}".format(p.scheme, p.netloc, new_path) - return self.__class__(new_url) + return self.replace(path=posixpath.join(self._spath, other)) __truediv__ = __div__ - def __getattr__(self, name): - # When deepcopy is called, it creates and object without __init__, - # self.parsed is not initialized and it causes infinite recursion. - # More on this special casing here: - # https://stackoverflow.com/a/47300262/298182 - if name.startswith("__"): - raise AttributeError(name) - return getattr(self.parsed, name) - - @cached_property - def netloc(self): - p = self.parsed - netloc = p.hostname - if p.username: - netloc = p.username + "@" + netloc - if p.port and int(p.port) != self.DEFAULT_PORTS.get(p.scheme): - netloc += ":" + str(p.port) - return netloc - @property - def port(self): - return self.parsed.port or self.DEFAULT_PORTS.get(self.parsed.scheme) - - @property - def host(self): - return self.parsed.hostname - - @property - def user(self): - return self.parsed.username + def path(self): + return self._spath @cached_property def _path(self): - return PosixPathInfo(self.parsed.path) + return _URLPathInfo(self._spath) @property def name(self): return self._path.name - @property - def parts(self): - return (self.scheme, self.netloc) + self._path.parts + @cached_property + def netloc(self): + netloc = self.host + if self.user: + netloc = self.user + "@" + netloc + if self.port and int(self.port) != self.DEFAULT_PORTS.get(self.scheme): + netloc += ":" + str(self.port) + return netloc @property def bucket(self): - return self.parsed.netloc + return self.netloc @property def parent(self): - return self.from_parts( - scheme=self.scheme, - netloc=self.parsed.netloc, - path=self._path.parent.fspath, - ) + return self.replace(path=self._path.parent) @property def parents(self): - return _URLPathParents( - type(self), self.scheme, self.parsed.netloc, self._path - ) + return _URLPathParents(self) def relative_to(self, other): - if isinstance(other, str): - other = URLInfo(other) - if self.scheme != other.scheme or self.netloc != other.netloc: - raise ValueError( - "'{}' does not start with '{}'".format(self, other) - ) + if isinstance(other, basestring): + other = self.__class__(other) + if self.__class__ != other.__class__: + msg = "'{}' has incompatible class with '{}'".format(self, other) + raise ValueError(msg) + if self._base_parts != other._base_parts: + msg = "'{}' does not start with '{}'".format(self, other) + raise ValueError(msg) return self._path.relative_to(other._path) def isin(self, other): @@ -263,14 +258,12 @@ class URLInfo(object): other = self.__class__(other) elif self.__class__ != other.__class__: return False - return ( - self.scheme == other.scheme - and self.netloc == other.netloc - and self._path.isin(other._path) + return self._base_parts == other._base_parts and self._path.isin( + other._path ) class CloudURLInfo(URLInfo): @property def path(self): - return self.parsed.path.lstrip("/") + return self._spath.lstrip("/") diff --git a/dvc/progress.py b/dvc/progress.py index 7e85b6e18..68c6ddd59 100644 --- a/dvc/progress.py +++ b/dvc/progress.py @@ -3,35 +3,12 @@ from __future__ import print_function import logging import sys from tqdm import tqdm -from concurrent.futures import ThreadPoolExecutor from funcy import merge from dvc.utils import env2bool logger = logging.getLogger(__name__) -class TqdmThreadPoolExecutor(ThreadPoolExecutor): - """ - Ensure worker progressbars are cleared away properly. - """ - - def __enter__(self): - """ - Creates a blank initial dummy progress bar if needed so that workers - are forced to create "nested" bars. - """ - blank_bar = Tqdm(bar_format="Multi-Threaded:", leave=False) - if blank_bar.pos > 0: - # already nested - don't need a placeholder bar - blank_bar.close() - self.bar = blank_bar - return super(TqdmThreadPoolExecutor, self).__enter__() - - def __exit__(self, *a, **k): - super(TqdmThreadPoolExecutor, self).__exit__(*a, **k) - self.bar.close() - - class Tqdm(tqdm): """ maximum-compatibility tqdm-based progressbars diff --git a/dvc/remote/local.py b/dvc/remote/local.py index dff433756..7f950404a 100644 --- a/dvc/remote/local.py +++ b/dvc/remote/local.py @@ -4,6 +4,7 @@ import stat import errno import logging from functools import partial +from concurrent.futures import ThreadPoolExecutor from shortuuid import uuid @@ -29,7 +30,7 @@ from dvc.utils import ( ) from dvc.config import Config from dvc.exceptions import DvcException, DownloadError, UploadError -from dvc.progress import Tqdm, TqdmThreadPoolExecutor +from dvc.progress import Tqdm from dvc.path_info import PathInfo @@ -263,7 +264,7 @@ class RemoteLOCAL(RemoteBASE): # This is a performance optimization. We can safely assume that, # if the resources that we want to fetch are already cached, - # there's no need to check the remote storage for the existance of + # there's no need to check the remote storage for the existence of # those files. if download and sorted(local_exists) == sorted(md5s): remote_exists = local_exists @@ -359,7 +360,7 @@ class RemoteLOCAL(RemoteBASE): return 0 if jobs > 1: - with TqdmThreadPoolExecutor(max_workers=jobs) as executor: + with ThreadPoolExecutor(max_workers=jobs) as executor: fails = sum(executor.map(func, *plans)) else: fails = sum(map(func, *plans)) diff --git a/dvc/remote/s3.py b/dvc/remote/s3.py index 7557caecd..ded688b3c 100644 --- a/dvc/remote/s3.py +++ b/dvc/remote/s3.py @@ -4,7 +4,6 @@ from __future__ import unicode_literals import os import logging -import posixpath from funcy import cached_property from dvc.progress import Tqdm @@ -145,13 +144,13 @@ class RemoteS3(RemoteBASE): # multipart. # # If an object's etag looks like '8978c98bb5a48c2fb5f2c4c905768afa', - # then it was transfered as a single part, which means that the chunk + # then it was transferred as a single part, which means that the chunk # size used to transfer it was greater or equal to the ContentLength # of that object. So to preserve that tag over the next transfer, we # could use any value >= ContentLength. # # If an object's etag looks like '50d67013a5e1a4070bef1fc8eea4d5f9-13', - # then it was transfered as a multipart, which means that the chunk + # then it was transferred as a multipart, which means that the chunk # size used to transfer it was less than ContentLength of that object. # Unfortunately, in general, it doesn't mean that the chunk size was # the same throughout the transfer, so it means that in order to @@ -273,4 +272,4 @@ class RemoteS3(RemoteBASE): def walk_files(self, path_info, max_items=None): for fname in self._list_paths(path_info, max_items): - yield path_info / posixpath.relpath(fname, path_info.path) + yield path_info.replace(path=fname) diff --git a/dvc/remote/slow_link_detection.py b/dvc/remote/slow_link_detection.py index 0dd2285d6..bffa6e9e8 100644 --- a/dvc/remote/slow_link_detection.py +++ b/dvc/remote/slow_link_detection.py @@ -18,7 +18,7 @@ this.message = ( "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( + "`dvc config cache.slow_link_warning false`".format( blue=colorama.Fore.BLUE, reset=colorama.Fore.RESET ) ) diff --git a/dvc/remote/ssh/__init__.py b/dvc/remote/ssh/__init__.py index c6b39b5b5..7230f016a 100644 --- a/dvc/remote/ssh/__init__.py +++ b/dvc/remote/ssh/__init__.py @@ -4,7 +4,6 @@ import errno import itertools import io import os -import posixpath import getpass import logging import threading @@ -268,7 +267,7 @@ class RemoteSSH(RemoteBASE): def walk_files(self, path_info): with self.ssh(path_info) as ssh: for fname in ssh.walk_files(path_info.path): - yield path_info / posixpath.relpath(fname, path_info.path) + yield path_info.replace(path=fname) def makedirs(self, path_info): with self.ssh(path_info) as ssh: diff --git a/dvc/remote/ssh/connection.py b/dvc/remote/ssh/connection.py index dcbb1727c..b2da0b67a 100644 --- a/dvc/remote/ssh/connection.py +++ b/dvc/remote/ssh/connection.py @@ -185,8 +185,27 @@ class SSHConnection: self.sftp.get(src, dest, callback=pbar.update_to) def move(self, src, dst): + """Rename src to dst, if it is not possible (in case src and dst are + on different filesystems) and actual physical copying of data is + happening. + """ self.makedirs(posixpath.dirname(dst)) - self.sftp.rename(src, dst) + + try: + self.sftp.rename(src, dst) + except OSError: + self._atomic_copy(src, dst) + + self.remove(src) + + def _atomic_copy(self, src, dst): + tmp = tmp_fname(dst) + + try: + self.copy(src, tmp) + self.sftp.rename(tmp, dst) + finally: + self.remove(tmp) def upload(self, src, dest, no_progress_bar=False, progress_title=None): self.makedirs(posixpath.dirname(dest)) diff --git a/dvc/repo/checkout.py b/dvc/repo/checkout.py index 6f8abde42..f753c1419 100644 --- a/dvc/repo/checkout.py +++ b/dvc/repo/checkout.py @@ -50,12 +50,6 @@ def _checkout( total=total, unit="file", desc="Checkout", disable=total == 0 ) as pbar: for stage in stages: - if stage.locked: - logger.warning( - "DVC-file '{path}' is locked. Its dependencies are" - " not going to be checked out.".format(path=stage.relpath) - ) - failed.extend( stage.checkout(force=force, progress_callback=pbar.update_desc) ) diff --git a/dvc/repo/fetch.py b/dvc/repo/fetch.py index 702946163..95eba34b1 100644 --- a/dvc/repo/fetch.py +++ b/dvc/repo/fetch.py @@ -26,7 +26,7 @@ def _fetch( """Download data items from a cloud and imported repositories Returns: - int: number of succesfully downloaded files + int: number of successfully downloaded files Raises: DownloadError: thrown when there are failed downloads, either @@ -76,20 +76,20 @@ def _fetch_external(self, repo_url, repo_rev, files): cache_dir = self.cache.local.cache_dir try: with external_repo(repo_url, repo_rev, cache_dir=cache_dir) as repo: - cache = NamedCache() - for name in files: - try: - out = repo.find_out_by_relpath(name) - except OutputNotFoundError: - failed += 1 - logger.exception( - "failed to fetch data for '{}'".format(name) - ) - continue - else: - cache.update(out.get_used_cache()) - with repo.state: + cache = NamedCache() + for name in files: + try: + out = repo.find_out_by_relpath(name) + except OutputNotFoundError: + failed += 1 + logger.exception( + "failed to fetch data for '{}'".format(name) + ) + continue + else: + cache.update(out.get_used_cache()) + try: return repo.cloud.pull(cache), failed except DownloadError as exc: diff --git a/dvc/repo/get.py b/dvc/repo/get.py index 258f6d884..9096216c9 100644 --- a/dvc/repo/get.py +++ b/dvc/repo/get.py @@ -21,7 +21,6 @@ logger = logging.getLogger(__name__) @staticmethod def get(url, path, out=None, rev=None): out = resolve_output(path, out) - path = path.lstrip("/") if Stage.is_valid_filename(out): raise GetDVCFileError() diff --git a/dvc/repo/get_url.py b/dvc/repo/get_url.py index ff191e149..8ba187865 100644 --- a/dvc/repo/get_url.py +++ b/dvc/repo/get_url.py @@ -12,7 +12,8 @@ def get_url(url, out=None): if os.path.exists(url): url = os.path.abspath(url) - out = os.path.abspath(out) + + out = os.path.abspath(out) dep, = dependency.loads_from(None, [url]) out, = output.loads_from(None, [out], use_cache=False) diff --git a/dvc/repo/imp.py b/dvc/repo/imp.py index 270d4caa6..68ead389b 100644 --- a/dvc/repo/imp.py +++ b/dvc/repo/imp.py @@ -2,6 +2,5 @@ def imp(self, url, path, out=None, rev=None): erepo = {"url": url} if rev is not None: erepo["rev"] = rev - path = path.lstrip("/") return self.imp_url(path, out=out, erepo=erepo, locked=True) diff --git a/dvc/repo/init.py b/dvc/repo/init.py index 19dc20d1b..e2f6b4702 100644 --- a/dvc/repo/init.py +++ b/dvc/repo/init.py @@ -65,7 +65,7 @@ def init(root_dir=os.curdir, no_scm=False, force=False): if isinstance(scm, NoSCM) and not no_scm: raise InitError( "{repo} is not tracked by any supported scm tool (e.g. git). " - "Use '--no-scm' if you don't want to use any scm.".format( + "Use `--no-scm` if you don't want to use any scm.".format( repo=root_dir ) ) @@ -73,7 +73,7 @@ def init(root_dir=os.curdir, no_scm=False, force=False): if os.path.isdir(dvc_dir): if not force: raise InitError( - "'{repo}' exists. Use '-f' to force.".format( + "'{repo}' exists. Use `-f` to force.".format( repo=relpath(dvc_dir) ) ) diff --git a/dvc/repo/pull.py b/dvc/repo/pull.py index 7c9258824..822897e29 100644 --- a/dvc/repo/pull.py +++ b/dvc/repo/pull.py @@ -1,6 +1,11 @@ from __future__ import unicode_literals -from . import locked +import logging + +from dvc.repo import locked + + +logger = logging.getLogger(__name__) @locked diff --git a/dvc/scm/git/__init__.py b/dvc/scm/git/__init__.py index 05bb2faf1..fd4e7ad10 100644 --- a/dvc/scm/git/__init__.py +++ b/dvc/scm/git/__init__.py @@ -200,10 +200,10 @@ class Git(Base): self.repo.index.add(paths) except AssertionError: msg = ( - "failed to add '{}' to git. You can add those files" - " manually using 'git add'." - " See 'https://github.com/iterative/dvc/issues/610'" - " for more details.".format(str(paths)) + "failed to add '{}' to git. You can add those files " + "manually using `git add`. See " + "https://github.com/iterative/dvc/issues/610 for more " + "details.".format(str(paths)) ) logger.exception(msg) diff --git a/dvc/stage.py b/dvc/stage.py index 4d3b7d3d0..94ec95c21 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -242,7 +242,7 @@ class Stage(object): if self.is_callback: logger.warning( - "DVC-file '{fname}' is a 'callback' stage " + "DVC-file '{fname}' is a \"callback\" stage " "(has a command and no dependencies) and thus always " "considered as changed.".format(fname=self.relpath) ) @@ -451,8 +451,8 @@ class Stage(object): if fname is not None and os.path.basename(fname) != fname: raise StageFileBadNameError( "DVC-file name '{fname}' may not contain subdirectories" - " if '-c|--cwd' (deprecated) is specified. Use '-w|--wdir'" - " along with '-f' to specify DVC-file path with working" + " if `-c|--cwd` (deprecated) is specified. Use `-w|--wdir`" + " along with `-f` to specify DVC-file path with working" " directory.".format(fname=fname) ) wdir = cwd @@ -571,8 +571,8 @@ class Stage(object): def _check_dvc_filename(fname): if not Stage.is_valid_filename(fname): raise StageFileBadNameError( - "bad DVC-file name '{}'. DVC-files should be named" - " 'Dvcfile' or have a '.dvc' suffix (e.g. '{}.dvc').".format( + "bad DVC-file name '{}'. DVC-files should be named " + "'Dvcfile' or have a '.dvc' suffix (e.g. '{}.dvc').".format( relpath(fname), os.path.basename(fname) ) ) @@ -736,7 +736,7 @@ class Stage(object): if not force and not prompt.confirm(msg): raise StageCommitError( "unable to commit changed '{}'. Use `-f|--force` to " - "force.`".format(self.relpath) + "force.".format(self.relpath) ) self.save() diff --git a/dvc/state.py b/dvc/state.py index 61f82549d..dacc07693 100644 --- a/dvc/state.py +++ b/dvc/state.py @@ -369,14 +369,12 @@ class State(object): # pylint: disable=too-many-instance-attributes """ assert path_info.scheme == "local" assert checksum is not None - - path = fspath_py35(path_info) - assert os.path.exists(path) + assert os.path.exists(fspath_py35(path_info)) actual_mtime, actual_size = get_mtime_and_size( - path, self.repo.dvcignore + path_info, self.repo.dvcignore ) - actual_inode = get_inode(path) + actual_inode = get_inode(path_info) existing_record = self.get_state_record_for_inode(actual_inode) if not existing_record: diff --git a/dvc/updater.py b/dvc/updater.py index d2cc16fc4..8b13754e8 100644 --- a/dvc/updater.py +++ b/dvc/updater.py @@ -9,8 +9,8 @@ from packaging import version from dvc import __version__ from dvc.lock import Lock, LockError -from dvc.utils import is_binary, boxify, env2bool - +from dvc.utils import boxify, env2bool +from dvc.utils.pkg import get_package_manager logger = logging.getLogger(__name__) @@ -123,7 +123,6 @@ class Updater(object): # pragma: no cover "yum": "Run {yellow}yum{reset} update dvc", "yay": "Run {yellow}yay{reset} {blue}-S{reset} dvc", "formula": "Run {yellow}brew{reset} upgrade dvc", - "cask": "Run {yellow}brew cask{reset} upgrade dvc", "apt": ( "Run {yellow}apt-get{reset} install" " {blue}--only-upgrade{reset} dvc" @@ -142,80 +141,6 @@ class Updater(object): # pragma: no cover ), } - package_manager = self._get_package_manager() + package_manager = get_package_manager() return instructions[package_manager] - - @staticmethod - def _get_dvc_path(system): - if system in ("linux", "darwin"): - output = os.popen("which dvc") - else: - output = os.popen("where dvc") - - return output.read().lower() - - @staticmethod - def _is_conda(path): - return "conda" in path - - def _get_linux(self): - import distro - - if not is_binary(): - dvc_path = self._get_dvc_path("linux") - return "conda" if self._is_conda(dvc_path) else "pip" - - package_managers = { - "rhel": "yum", - "centos": "yum", - "fedora": "yum", - "amazon": "yum", - "opensuse": "yum", - "ubuntu": "apt", - "debian": "apt", - } - - return package_managers.get(distro.id()) - - def _get_darwin(self): - if is_binary(): - return None - - package_manager = None - - if __file__.startswith("/usr/local/Cellar"): - package_manager = "formula" - - dvc_path = self._get_dvc_path("darwin") - if self._is_conda(dvc_path): - package_manager = "conda" - - return package_manager or "pip" - - def _get_windows(self): - if is_binary(): - return None - - dvc_path = self._get_dvc_path("windows") - if self._is_conda(dvc_path): - return "conda" - - return "pip" - - def _get_package_manager(self): - import platform - from dvc.exceptions import DvcException - - m = { - "Windows": self._get_windows, - "Darwin": self._get_darwin, - "Linux": self._get_linux, - } - - system = platform.system() - func = m.get(system) - if func is None: - raise DvcException("not supported system '{}'".format(system)) - - return func() diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index 538b7a97e..b0cb6b3ff 100644 --- a/dvc/utils/__init__.py +++ b/dvc/utils/__init__.py @@ -232,7 +232,7 @@ def _to_chunks_by_chunks_number(list_to_split, num_chunks): def to_chunks(list_to_split, num_chunks=None, chunk_size=None): if (num_chunks and chunk_size) or (not num_chunks and not chunk_size): raise ValueError( - "One and only one of 'num_chunks', 'chunk_size' must be defined" + "Only one among `num_chunks` or `chunk_size` must be defined." ) if chunk_size: return _split(list_to_split, chunk_size) diff --git a/dvc/utils/fs.py b/dvc/utils/fs.py index 811b4195a..6ded4cefd 100644 --- a/dvc/utils/fs.py +++ b/dvc/utils/fs.py @@ -7,7 +7,7 @@ import logging from dvc.exceptions import DvcException from dvc.system import System -from dvc.utils import dict_md5, walk_files +from dvc.utils import dict_md5, walk_files, fspath_py35 from dvc.utils.compat import str @@ -21,7 +21,7 @@ def get_inode(path): def get_mtime_and_size(path, dvcignore): - if os.path.isdir(path): + if os.path.isdir(fspath_py35(path)): size = 0 files_mtimes = {} for file_path in walk_files(path, dvcignore): @@ -39,7 +39,7 @@ def get_mtime_and_size(path, dvcignore): # max(mtime(f) for f in non_ignored_files) mtime = dict_md5(files_mtimes) else: - base_stat = os.stat(path) + base_stat = os.stat(fspath_py35(path)) size = base_stat.st_size mtime = base_stat.st_mtime mtime = int(nanotime.timestamp(mtime)) diff --git a/dvc/utils/pkg.py b/dvc/utils/pkg.py new file mode 100644 index 000000000..a35bd7306 --- /dev/null +++ b/dvc/utils/pkg.py @@ -0,0 +1,51 @@ +from dvc.utils import is_binary + + +def get_linux(): + import distro + + if not is_binary(): + return "pip" + + package_managers = { + "rhel": "yum", + "centos": "yum", + "fedora": "yum", + "amazon": "yum", + "opensuse": "yum", + "ubuntu": "apt", + "debian": "apt", + } + + return package_managers.get(distro.id()) + + +def get_darwin(): + if not is_binary(): + if __file__.startswith("/usr/local/Cellar"): + return "formula" + else: + return "pip" + return None + + +def get_windows(): + return None if is_binary() else "pip" + + +def get_package_manager(): + import platform + from dvc.exceptions import DvcException + + m = { + "Windows": get_windows(), + "Darwin": get_darwin(), + "Linux": get_linux(), + } + + system = platform.system() + func = m.get(system) + if func is None: + raise DvcException("not supported system '{}'".format(system)) + + return func diff --git a/dvc/version.py b/dvc/version.py index 4fc71199c..6561d954c 100644 --- a/dvc/version.py +++ b/dvc/version.py @@ -7,7 +7,7 @@ import os import subprocess -_BASE_VERSION = "0.66.0" +_BASE_VERSION = "0.66.4" def _generate_version(base_version): diff --git a/setup.py b/setup.py index 47bd6c46b..6817b8dea 100644 --- a/setup.py +++ b/setup.py @@ -73,7 +73,7 @@ install_requires = [ "funcy>=1.12", "pathspec>=0.6.0", "shortuuid>=0.5.0", - "tqdm>=4.35.0,<5", + "tqdm>=4.36.1,<5", "packaging>=19.0", "win-unicode-console>=0.5; sys_platform == 'win32'", ] @@ -116,7 +116,7 @@ tests_requirements = [ "flaky>=3.5.3", "mock>=3.0.0", "xmltodict>=0.11.0", - "awscli>=1.16.125", + "awscli==1.16.266", "google-compute-engine==2.8.13", "pywin32; sys_platform == 'win32'", "Pygments", # required by collective.checkdocs, @@ -127,7 +127,7 @@ tests_requirements = [ "pydocstyle<4.0", "jaraco.windows==3.9.2", "mock-ssh-server>=0.6.0", - "moto>=1.3.14.dev55", + "moto==1.3.14.dev464", "rangehttpserver==1.2.0", ] diff --git a/tests/func/test_import.py b/tests/func/test_import.py index fc00ee7b1..4fe8857aa 100644 --- a/tests/func/test_import.py +++ b/tests/func/test_import.py @@ -2,6 +2,7 @@ from __future__ import unicode_literals import os import filecmp +import shutil import pytest from mock import patch @@ -67,6 +68,23 @@ def test_pull_imported_stage(dvc_repo, erepo): assert os.path.isfile(dst_cache) +def test_pull_imported_directory_stage(dvc_repo, erepo): + src = erepo.DATA_DIR + dst = erepo.DATA_DIR + "_imported" + stage_file = dst + ".dvc" + + dvc_repo.imp(erepo.root_dir, src, dst) + + shutil.rmtree(dst) + shutil.rmtree(dvc_repo.cache.local.cache_dir) + + dvc_repo.pull([stage_file]) + + assert os.path.exists(dst) + assert os.path.isdir(dst) + trees_equal(src, dst) + + def test_download_error_pulling_imported_stage(dvc_repo, erepo): src = erepo.FOO dst = erepo.FOO + "_imported" diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index b938f97f4..af9d041cf 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -245,5 +245,5 @@ def test_partial_push_n_pull(dvc_repo, repo_dir, caplog): def get_last_exc(caplog): - _, exc, _ = caplog.records[-1].exc_info + _, exc, _ = caplog.records[-2].exc_info return exc diff --git a/tests/func/test_repro.py b/tests/func/test_repro.py index 148d2a006..f75edce04 100644 --- a/tests/func/test_repro.py +++ b/tests/func/test_repro.py @@ -964,8 +964,8 @@ class TestReproExternalS3(TestReproExternalBase): return "aws s3 cp {} {}".format(i, o) def write(self, bucket, key, body): - s3 = boto3.resource("s3") - s3.Bucket(bucket).put_object(Key=key, Body=body) + s3 = boto3.client("s3") + s3.put_object(Bucket=bucket, Key=key, Body=body) class TestReproExternalGS(TestReproExternalBase): diff --git a/tests/unit/command/test_imp_url.py b/tests/unit/command/test_imp_url.py index 3ec0efbaa..50cb6e5bd 100644 --- a/tests/unit/command/test_imp_url.py +++ b/tests/unit/command/test_imp_url.py @@ -29,7 +29,7 @@ def test_failed_import_url(mocker, caplog, dvc_repo): assert cmd.run() == 1 expected_error = ( "failed to import http://somesite.com/file_name. " - "You could also try downloading it manually and " - "adding it with `dvc add` command." + "You could also try downloading it manually, and " + "adding it with `dvc add`." ) assert expected_error in caplog.text diff --git a/tests/unit/remote/ssh/test_connection.py b/tests/unit/remote/ssh/test_connection.py index f0c12ec21..6b1300ce3 100644 --- a/tests/unit/remote/ssh/test_connection.py +++ b/tests/unit/remote/ssh/test_connection.py @@ -111,3 +111,9 @@ def test_hardlink(repo_dir, ssh): def test_copy(repo_dir, ssh): ssh.copy("foo", "link") assert filecmp.cmp("foo", "link") + + +def test_move(repo_dir, ssh): + ssh.move("foo", "copy") + assert os.path.exists("copy") + assert not os.path.exists("foo") diff --git a/tests/unit/remote/test_local.py b/tests/unit/remote/test_local.py index c9681d50f..461e37b79 100644 --- a/tests/unit/remote/test_local.py +++ b/tests/unit/remote/test_local.py @@ -5,7 +5,7 @@ from dvc.remote.local import RemoteLOCAL def test_status_download_optimization(mocker): """When comparing the status to pull a remote cache, And the desired files to fetch are already on the local cache, - Don't check the existance of the desired files on the remote cache + Don't check the existence of the desired files on the remote cache """ remote = RemoteLOCAL(None, {}) diff --git a/tests/unit/remote/test_s3.py b/tests/unit/remote/test_s3.py index 7861fb5a9..36b9d1e23 100644 --- a/tests/unit/remote/test_s3.py +++ b/tests/unit/remote/test_s3.py @@ -83,3 +83,20 @@ def test_walk_files(remote): ] assert list(remote.walk_files(remote.path_info / "data")) == files + + +def test_copy_preserve_etag_across_buckets(remote): + s3 = remote.s3 + s3.create_bucket(Bucket="another") + + another = RemoteS3(None, {"url": "s3://another", "region": "us-east-1"}) + + from_info = remote.path_info / "foo" + to_info = another.path_info / "foo" + + remote.copy(from_info, to_info) + + from_etag = RemoteS3.get_etag(s3, "bucket", "foo") + to_etag = RemoteS3.get_etag(s3, "another", "foo") + + assert from_etag == to_etag diff --git a/tests/unit/test_logger.py b/tests/unit/test_logger.py index 5560acd4e..bcc061ff3 100644 --- a/tests/unit/test_logger.py +++ b/tests/unit/test_logger.py @@ -45,11 +45,7 @@ class TestColorFormatter: with caplog.at_level(logging.INFO, logger="dvc"): logger.error("message") - expected = ( - "{red}ERROR{nc}: message\n" - "\n" - "{footer}".format(footer=formatter.footer, **colors) - ) + expected = "{red}ERROR{nc}: message\n".format(**colors) assert expected == formatter.format(caplog.records[0]) @@ -60,11 +56,7 @@ class TestColorFormatter: except Exception: logger.exception("message") - expected = ( - "{red}ERROR{nc}: message\n" - "\n" - "{footer}".format(footer=formatter.footer, **colors) - ) + expected = "{red}ERROR{nc}: message\n".format(**colors) assert expected == formatter.format(caplog.records[0]) @@ -75,11 +67,7 @@ class TestColorFormatter: except Exception: logger.exception("") - expected = ( - "{red}ERROR{nc}: description\n" - "\n" - "{footer}".format(footer=formatter.footer, **colors) - ) + expected = "{red}ERROR{nc}: description\n".format(**colors) assert expected == formatter.format(caplog.records[0]) @@ -90,10 +78,8 @@ class TestColorFormatter: except Exception: logger.exception("message") - expected = ( - "{red}ERROR{nc}: message - description\n" - "\n" - "{footer}".format(footer=formatter.footer, **colors) + expected = "{red}ERROR{nc}: message - description\n".format( + **colors ) assert expected == formatter.format(caplog.records[0]) @@ -110,13 +96,8 @@ class TestColorFormatter: "{red}ERROR{nc}: description\n" "{red}{line}{nc}\n" "{stack_trace}" - "{red}{line}{nc}\n" - "\n" - "{footer}".format( - footer=formatter.footer, - line="-" * 60, - stack_trace=stack_trace, - **colors + "{red}{line}{nc}\n".format( + line="-" * 60, stack_trace=stack_trace, **colors ) ) @@ -138,10 +119,7 @@ class TestColorFormatter: "{red}ERROR{nc}: message - second: first\n" "{red}{line}{nc}\n" "{stack_trace}" - "{red}{line}{nc}\n" - "\n" - "{footer}".format( - footer=formatter.footer, + "{red}{line}{nc}\n".format( line="-" * 60, stack_trace="\n".join([first_traceback, second_traceback]), **colors diff --git a/tests/unit/utils/test_fs.py b/tests/unit/utils/test_fs.py index 30a96b63f..d479b10d9 100644 --- a/tests/unit/utils/test_fs.py +++ b/tests/unit/utils/test_fs.py @@ -14,6 +14,7 @@ from dvc.utils.fs import ( contains_symlink_up_to, BasePathNotInCheckedPathException, get_parent_dirs_up_to, + get_inode, ) from mock import patch from tests.basic_env import TestDir @@ -149,3 +150,20 @@ def test_relpath_windows_different_drives(): rel_info = relpath(info1, info2) assert isinstance(rel_info, str) assert rel_info == path1 + + +def test_get_inode(repo_dir): + path = repo_dir.FOO + path_info = PathInfo(path) + assert get_inode(path) == get_inode(path_info) + + +@pytest.mark.parametrize("path", [TestDir.DATA, TestDir.DATA_DIR]) +def test_path_object_and_str_are_valid_types_get_mtime_and_size( + path, repo_dir +): + dvcignore = DvcIgnoreFilter(repo_dir.root_dir) + time, size = get_mtime_and_size(path, dvcignore) + object_time, object_size = get_mtime_and_size(PathInfo(path), dvcignore) + assert time == object_time + assert size == object_size