From 0b1d1f5ce4c6cbbe13c7c3bf206cbc45ec0a5190 Mon Sep 17 00:00:00 2001 From: pawel Date: Mon, 20 Jan 2020 15:44:04 +0100 Subject: [PATCH 1/7] data cloud: pull: trust remotes md5 on proper flag --- dvc/command/data_sync.py | 7 ++++ dvc/data_cloud.py | 19 +++++++++-- dvc/output/base.py | 8 ++++- dvc/repo/__init__.py | 2 ++ dvc/repo/fetch.py | 8 ++++- dvc/repo/pull.py | 2 ++ tests/func/test_remote.py | 71 +++++++++++++++++++++++++++++++++++++++ 7 files changed, 113 insertions(+), 4 deletions(-) diff --git a/dvc/command/data_sync.py b/dvc/command/data_sync.py index 9442e2bbb..0b8b9551d 100644 --- a/dvc/command/data_sync.py +++ b/dvc/command/data_sync.py @@ -66,6 +66,7 @@ class CmdDataFetch(CmdDataBase): all_tags=self.args.all_tags, with_deps=self.args.with_deps, recursive=self.args.recursive, + trust_remote=self.args.trust_remote, ) except DvcException: logger.exception("failed to fetch data from the cloud") @@ -231,6 +232,12 @@ def add_parser(subparsers, _parent_parser): default=False, help="Fetch cache for subdirectories of specified directory.", ) + fetch_parser.add_argument( + "--trust-remote", + action="store_true", + default=False, + help="Trust remote cache checksums upon fetching.", + ) fetch_parser.set_defaults(func=CmdDataFetch) # Status diff --git a/dvc/data_cloud.py b/dvc/data_cloud.py index fadd536bd..d83275327 100644 --- a/dvc/data_cloud.py +++ b/dvc/data_cloud.py @@ -62,7 +62,14 @@ class DataCloud(object): show_checksums=show_checksums, ) - def pull(self, cache, jobs=None, remote=None, show_checksums=False): + def pull( + self, + cache, + jobs=None, + remote=None, + show_checksums=False, + trust_remote=False, + ): """Pull data items in a cloud-agnostic way. Args: @@ -73,12 +80,20 @@ class DataCloud(object): show_checksums (bool): show checksums instead of file names in information messages. """ - return self.repo.cache.local.pull( + downloaded_items_num = self.repo.cache.local.pull( cache, jobs=jobs, remote=self.get_remote(remote, "pull"), show_checksums=show_checksums, ) + if trust_remote: + for checksum in cache["local"].keys(): + cache_file = self.repo.cache.local.checksum_to_path_info( + checksum + ) + if self.repo.cache.local.exists(cache_file): + self.repo.state.save(cache_file, checksum) + return downloaded_items_num def status(self, cache, jobs=None, remote=None, show_checksums=False): """Check status of data items in a cloud-agnostic way. diff --git a/dvc/output/base.py b/dvc/output/base.py index 0fca876c4..beb175b80 100644 --- a/dvc/output/base.py +++ b/dvc/output/base.py @@ -340,7 +340,12 @@ class OutputBase(object): self.remote.unprotect(self.path_info) def _collect_used_dir_cache( - self, remote=None, force=False, jobs=None, filter_info=None + self, + remote=None, + force=False, + jobs=None, + filter_info=None, + trust_remote=False, ): """Get a list of `info`s related to the given directory. @@ -370,6 +375,7 @@ class OutputBase(object): jobs=jobs, remote=remote, show_checksums=False, + trust_remote=trust_remote, ) except DvcException: logger.debug("failed to pull cache for '{}'".format(self)) diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py index 9d0a506a8..895928cd6 100644 --- a/dvc/repo/__init__.py +++ b/dvc/repo/__init__.py @@ -223,6 +223,7 @@ class Repo(object): force=False, jobs=None, recursive=False, + trust_remote=False, ): """Get the stages related to the given target and collect the `info` of its outputs. @@ -262,6 +263,7 @@ class Repo(object): force=force, jobs=jobs, filter_info=filter_info, + trust_remote=trust_remote, ) cache.update(used_cache, suffix=suffix) diff --git a/dvc/repo/fetch.py b/dvc/repo/fetch.py index e995630e4..1064b4915 100644 --- a/dvc/repo/fetch.py +++ b/dvc/repo/fetch.py @@ -20,6 +20,7 @@ def _fetch( with_deps=False, all_tags=False, recursive=False, + trust_remote=False, ): """Download data items from a cloud and imported repositories @@ -42,6 +43,7 @@ def _fetch( remote=remote, jobs=jobs, recursive=recursive, + trust_remote=trust_remote, ) downloaded = 0 @@ -49,7 +51,11 @@ def _fetch( try: downloaded += self.cloud.pull( - used, jobs, remote=remote, show_checksums=show_checksums + used, + jobs, + remote=remote, + show_checksums=show_checksums, + trust_remote=trust_remote, ) except NoRemoteError: if not used.external and used["local"]: diff --git a/dvc/repo/pull.py b/dvc/repo/pull.py index fadd34d3d..474cecda7 100644 --- a/dvc/repo/pull.py +++ b/dvc/repo/pull.py @@ -17,6 +17,7 @@ def pull( all_tags=False, force=False, recursive=False, + trust_remote=False, ): processed_files_count = self._fetch( targets, @@ -26,6 +27,7 @@ def pull( all_tags=all_tags, with_deps=with_deps, recursive=recursive, + trust_remote=trust_remote, ) self._checkout( targets=targets, with_deps=with_deps, force=force, recursive=recursive diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index c6d1cffe9..07aadf071 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -6,6 +6,7 @@ import configobj import pytest from mock import patch +from dvc import remote from dvc.config import Config, ConfigError from dvc.exceptions import DownloadError, UploadError from dvc.main import main @@ -13,6 +14,8 @@ from dvc.path_info import PathInfo from dvc.remote import RemoteLOCAL, RemoteConfig from dvc.remote.base import RemoteBASE from dvc.compat import fspath +from dvc.scm import Git +from dvc.utils import file_md5 from tests.basic_env import TestDvc from tests.remotes import Local @@ -257,3 +260,71 @@ def test_modify_missing_remote(dvc): with pytest.raises(ConfigError, match=r"unable to find remote section"): remote_config.modify("myremote", "gdrive_client_id", "xxx") + + +def test_trust_remote_checksums(tmp_dir, mocker, tmp_path_factory, erepo_dir): + with erepo_dir.chdir(): + + erepo_dir.dvc_gen({"file": "file content"}, commit="add dir") + + Git.clone(fspath(erepo_dir), fspath(tmp_dir)) + + from dvc.repo import Repo + + dvc = Repo(fspath(tmp_dir)) + + from tests.utils import spy + + file_md5_spy = spy(file_md5) + mocker.patch.object(remote.local, "file_md5", file_md5_spy) + + dvc.pull(trust_remote=True) + + assert len(file_md5_spy.mock.call_args_list) == 1 + + +def test_trust_remote_checksums_dir( + tmp_dir, mocker, tmp_path_factory, erepo_dir +): + with erepo_dir.chdir(): + erepo_dir.dvc_gen({"dir": {"file": "file content"}}, commit="add dir") + + Git.clone(fspath(erepo_dir), fspath(tmp_dir)) + + from dvc.repo import Repo + + dvc = Repo(fspath(tmp_dir)) + + from tests.utils import spy + + file_md5_spy = spy(file_md5) + mocker.patch.object(remote.local, "file_md5", file_md5_spy) + + dvc.pull(trust_remote=True) + + assert len(file_md5_spy.mock.call_args_list) == 1 + + +def test_trust_remote_checksums_external_dep( + tmp_dir, mocker, tmp_path_factory, erepo_dir +): + external_path = tmp_path_factory.mktemp("external_dep") / "file" + external_path.write_text("file content") + + with erepo_dir.chdir(): + erepo_dir.dvc_add(fspath(external_path), commit="add external file") + + Git.clone(fspath(erepo_dir), fspath(tmp_dir)) + + from dvc.repo import Repo + + dvc = Repo(fspath(tmp_dir)) + + from tests.utils import spy + + file_md5_spy = spy(file_md5) + mocker.patch.object(remote.local, "file_md5", file_md5_spy) + + dvc.pull(trust_remote=True) + + assert len(file_md5_spy.mock.call_args_list) == 1 From ef1b06ddf9911acef658bbbe5c80a15a998192f7 Mon Sep 17 00:00:00 2001 From: pawel Date: Mon, 20 Jan 2020 17:24:50 +0100 Subject: [PATCH 2/7] test_remote: use mocker.spy instead of spy --- tests/func/test_remote.py | 27 ++++++++------------------- 1 file changed, 8 insertions(+), 19 deletions(-) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 07aadf071..fd9348cd9 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -6,7 +6,6 @@ import configobj import pytest from mock import patch -from dvc import remote from dvc.config import Config, ConfigError from dvc.exceptions import DownloadError, UploadError from dvc.main import main @@ -15,7 +14,6 @@ from dvc.remote import RemoteLOCAL, RemoteConfig from dvc.remote.base import RemoteBASE from dvc.compat import fspath from dvc.scm import Git -from dvc.utils import file_md5 from tests.basic_env import TestDvc from tests.remotes import Local @@ -262,7 +260,7 @@ def test_modify_missing_remote(dvc): remote_config.modify("myremote", "gdrive_client_id", "xxx") -def test_trust_remote_checksums(tmp_dir, mocker, tmp_path_factory, erepo_dir): +def test_trust_remote_checksums(tmp_dir, tmp_path_factory, erepo_dir, mocker): with erepo_dir.chdir(): erepo_dir.dvc_gen({"file": "file content"}, commit="add dir") @@ -273,14 +271,11 @@ def test_trust_remote_checksums(tmp_dir, mocker, tmp_path_factory, erepo_dir): dvc = Repo(fspath(tmp_dir)) - from tests.utils import spy - - file_md5_spy = spy(file_md5) - mocker.patch.object(remote.local, "file_md5", file_md5_spy) + md5_spy = mocker.spy(dvc.cache.local, "get_file_checksum") dvc.pull(trust_remote=True) - assert len(file_md5_spy.mock.call_args_list) == 1 + assert md5_spy.call_count == 0 def test_trust_remote_checksums_dir( @@ -295,14 +290,11 @@ def test_trust_remote_checksums_dir( dvc = Repo(fspath(tmp_dir)) - from tests.utils import spy + md5_spy = mocker.spy(dvc.cache.local, "get_file_checksum") - file_md5_spy = spy(file_md5) - mocker.patch.object(remote.local, "file_md5", file_md5_spy) + dvc.pull(trust_remote=False) - dvc.pull(trust_remote=True) - - assert len(file_md5_spy.mock.call_args_list) == 1 + assert md5_spy.call_count == 0 def test_trust_remote_checksums_external_dep( @@ -320,11 +312,8 @@ def test_trust_remote_checksums_external_dep( dvc = Repo(fspath(tmp_dir)) - from tests.utils import spy - - file_md5_spy = spy(file_md5) - mocker.patch.object(remote.local, "file_md5", file_md5_spy) + md5_spy = mocker.spy(dvc.cache.local, "get_file_checksum") dvc.pull(trust_remote=True) - assert len(file_md5_spy.mock.call_args_list) == 1 + assert md5_spy.call_count == 0 From b451de54960bb154b2f59223a8d6bf3fb81f3079 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 21 Jan 2020 12:00:20 +0100 Subject: [PATCH 3/7] remote: tests: refactor verify test --- dvc/command/data_sync.py | 13 +++++++-- dvc/data_cloud.py | 24 ++++++++-------- dvc/output/base.py | 4 +-- dvc/repo/__init__.py | 4 +-- dvc/repo/fetch.py | 6 ++-- dvc/repo/pull.py | 4 +-- tests/func/test_remote.py | 59 +++++++++------------------------------ 7 files changed, 43 insertions(+), 71 deletions(-) diff --git a/dvc/command/data_sync.py b/dvc/command/data_sync.py index 0b8b9551d..5c274cfee 100644 --- a/dvc/command/data_sync.py +++ b/dvc/command/data_sync.py @@ -28,6 +28,7 @@ class CmdDataPull(CmdDataBase): with_deps=self.args.with_deps, force=self.args.force, recursive=self.args.recursive, + verify=self.args.verify, ) except DvcException: logger.exception("failed to pull data from the cloud") @@ -66,7 +67,7 @@ class CmdDataFetch(CmdDataBase): all_tags=self.args.all_tags, with_deps=self.args.with_deps, recursive=self.args.recursive, - trust_remote=self.args.trust_remote, + verify=self.args.verify, ) except DvcException: logger.exception("failed to fetch data from the cloud") @@ -146,6 +147,12 @@ def add_parser(subparsers, _parent_parser): default=False, help="Pull cache for subdirectories of the specified directory.", ) + pull_parser.add_argument( + "--verify", + action="store_true", + default=False, + help="Verify downloaded files checksums.", + ) pull_parser.set_defaults(func=CmdDataPull) # Push @@ -233,10 +240,10 @@ def add_parser(subparsers, _parent_parser): help="Fetch cache for subdirectories of specified directory.", ) fetch_parser.add_argument( - "--trust-remote", + "--verify", action="store_true", default=False, - help="Trust remote cache checksums upon fetching.", + help="Verify downloaded files checksums.", ) fetch_parser.set_defaults(func=CmdDataFetch) diff --git a/dvc/data_cloud.py b/dvc/data_cloud.py index d83275327..fc4a53f33 100644 --- a/dvc/data_cloud.py +++ b/dvc/data_cloud.py @@ -63,12 +63,7 @@ class DataCloud(object): ) def pull( - self, - cache, - jobs=None, - remote=None, - show_checksums=False, - trust_remote=False, + self, cache, jobs=None, remote=None, show_checksums=False, verify=False ): """Pull data items in a cloud-agnostic way. @@ -86,15 +81,18 @@ class DataCloud(object): remote=self.get_remote(remote, "pull"), show_checksums=show_checksums, ) - if trust_remote: - for checksum in cache["local"].keys(): - cache_file = self.repo.cache.local.checksum_to_path_info( - checksum - ) - if self.repo.cache.local.exists(cache_file): - self.repo.state.save(cache_file, checksum) + + if not verify: + self._save_pulled_checksums(cache) + return downloaded_items_num + def _save_pulled_checksums(self, cache): + for checksum in cache["local"].keys(): + cache_file = self.repo.cache.local.checksum_to_path_info(checksum) + if self.repo.cache.local.exists(cache_file): + self.repo.state.save(cache_file, checksum) + def status(self, cache, jobs=None, remote=None, show_checksums=False): """Check status of data items in a cloud-agnostic way. diff --git a/dvc/output/base.py b/dvc/output/base.py index beb175b80..04d9ed461 100644 --- a/dvc/output/base.py +++ b/dvc/output/base.py @@ -345,7 +345,7 @@ class OutputBase(object): force=False, jobs=None, filter_info=None, - trust_remote=False, + verify=False, ): """Get a list of `info`s related to the given directory. @@ -375,7 +375,7 @@ class OutputBase(object): jobs=jobs, remote=remote, show_checksums=False, - trust_remote=trust_remote, + verify=verify, ) except DvcException: logger.debug("failed to pull cache for '{}'".format(self)) diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py index 895928cd6..96663f67f 100644 --- a/dvc/repo/__init__.py +++ b/dvc/repo/__init__.py @@ -223,7 +223,7 @@ class Repo(object): force=False, jobs=None, recursive=False, - trust_remote=False, + verify=False, ): """Get the stages related to the given target and collect the `info` of its outputs. @@ -263,7 +263,7 @@ class Repo(object): force=force, jobs=jobs, filter_info=filter_info, - trust_remote=trust_remote, + verify=verify, ) cache.update(used_cache, suffix=suffix) diff --git a/dvc/repo/fetch.py b/dvc/repo/fetch.py index 1064b4915..37fb0a56b 100644 --- a/dvc/repo/fetch.py +++ b/dvc/repo/fetch.py @@ -20,7 +20,7 @@ def _fetch( with_deps=False, all_tags=False, recursive=False, - trust_remote=False, + verify=False, ): """Download data items from a cloud and imported repositories @@ -43,7 +43,7 @@ def _fetch( remote=remote, jobs=jobs, recursive=recursive, - trust_remote=trust_remote, + verify=verify, ) downloaded = 0 @@ -55,7 +55,7 @@ def _fetch( jobs, remote=remote, show_checksums=show_checksums, - trust_remote=trust_remote, + verify=verify, ) except NoRemoteError: if not used.external and used["local"]: diff --git a/dvc/repo/pull.py b/dvc/repo/pull.py index 474cecda7..52779376d 100644 --- a/dvc/repo/pull.py +++ b/dvc/repo/pull.py @@ -17,7 +17,7 @@ def pull( all_tags=False, force=False, recursive=False, - trust_remote=False, + verify=False, ): processed_files_count = self._fetch( targets, @@ -27,7 +27,7 @@ def pull( all_tags=all_tags, with_deps=with_deps, recursive=recursive, - trust_remote=trust_remote, + verify=verify, ) self._checkout( targets=targets, with_deps=with_deps, force=force, recursive=recursive diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index fd9348cd9..8f9d12b9f 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -260,10 +260,12 @@ def test_modify_missing_remote(dvc): remote_config.modify("myremote", "gdrive_client_id", "xxx") -def test_trust_remote_checksums(tmp_dir, tmp_path_factory, erepo_dir, mocker): +def test_verify_checksums(tmp_dir, tmp_path_factory, erepo_dir, mocker): with erepo_dir.chdir(): - - erepo_dir.dvc_gen({"file": "file content"}, commit="add dir") + erepo_dir.dvc_gen({"file": "file1 content"}, commit="add file") + erepo_dir.dvc_gen( + {"dir": {"subfile": "file2 content"}}, commit="add " "dir" + ) Git.clone(fspath(erepo_dir), fspath(tmp_dir)) @@ -271,49 +273,14 @@ def test_trust_remote_checksums(tmp_dir, tmp_path_factory, erepo_dir, mocker): dvc = Repo(fspath(tmp_dir)) - md5_spy = mocker.spy(dvc.cache.local, "get_file_checksum") + file_md5_spy = mocker.spy(dvc.cache.local, "get_file_checksum") - dvc.pull(trust_remote=True) + dvc.pull() + assert file_md5_spy.call_count == 0 - assert md5_spy.call_count == 0 + # Removing cache will invalidate existing state entries + shutil.rmtree(dvc.cache.local.cache_dir) - -def test_trust_remote_checksums_dir( - tmp_dir, mocker, tmp_path_factory, erepo_dir -): - with erepo_dir.chdir(): - erepo_dir.dvc_gen({"dir": {"file": "file content"}}, commit="add dir") - - Git.clone(fspath(erepo_dir), fspath(tmp_dir)) - - from dvc.repo import Repo - - dvc = Repo(fspath(tmp_dir)) - - md5_spy = mocker.spy(dvc.cache.local, "get_file_checksum") - - dvc.pull(trust_remote=False) - - assert md5_spy.call_count == 0 - - -def test_trust_remote_checksums_external_dep( - tmp_dir, mocker, tmp_path_factory, erepo_dir -): - external_path = tmp_path_factory.mktemp("external_dep") / "file" - external_path.write_text("file content") - - with erepo_dir.chdir(): - erepo_dir.dvc_add(fspath(external_path), commit="add external file") - - Git.clone(fspath(erepo_dir), fspath(tmp_dir)) - - from dvc.repo import Repo - - dvc = Repo(fspath(tmp_dir)) - - md5_spy = mocker.spy(dvc.cache.local, "get_file_checksum") - - dvc.pull(trust_remote=True) - - assert md5_spy.call_count == 0 + dvc.pull(verify=True) + assert file_md5_spy.call_count == 3 + pass From f4e0079ec1415a4422d876104d9bb780e5d48641 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 21 Jan 2020 12:00:39 +0100 Subject: [PATCH 4/7] data sync: add command unit test --- tests/func/test_remote.py | 3 +- tests/unit/command/test_data_sync.py | 74 ++++++++++++++++++++++++++++ 2 files changed, 75 insertions(+), 2 deletions(-) create mode 100644 tests/unit/command/test_data_sync.py diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 8f9d12b9f..7639d6b88 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -260,7 +260,7 @@ def test_modify_missing_remote(dvc): remote_config.modify("myremote", "gdrive_client_id", "xxx") -def test_verify_checksums(tmp_dir, tmp_path_factory, erepo_dir, mocker): +def test_verify_checksums(tmp_dir, erepo_dir, mocker): with erepo_dir.chdir(): erepo_dir.dvc_gen({"file": "file1 content"}, commit="add file") erepo_dir.dvc_gen( @@ -283,4 +283,3 @@ def test_verify_checksums(tmp_dir, tmp_path_factory, erepo_dir, mocker): dvc.pull(verify=True) assert file_md5_spy.call_count == 3 - pass diff --git a/tests/unit/command/test_data_sync.py b/tests/unit/command/test_data_sync.py new file mode 100644 index 000000000..8565c2e83 --- /dev/null +++ b/tests/unit/command/test_data_sync.py @@ -0,0 +1,74 @@ +from dvc.cli import parse_args +from dvc.command.data_sync import CmdDataFetch, CmdDataPull + + +def test_fetch(mocker): + cli_args = parse_args( + [ + "fetch", + "target.dvc", + "-r", + "remote", + "-a", + "-T", + "-d", + "-R", + "--verify", + "-j", + "2", + ] + ) + assert cli_args.func == CmdDataFetch + + cmd = cli_args.func(cli_args) + m = mocker.patch("dvc.repo.Repo.fetch") + + assert cmd.run() == 0 + + m.assert_called_once_with( + targets=["target.dvc"], + remote="remote", + all_branches=True, + all_tags=True, + with_deps=True, + recursive=True, + verify=True, + jobs=2, + ) + + +def test_pull(mocker): + cli_args = parse_args( + [ + "pull", + "target.dvc", + "-r", + "remote", + "-a", + "-T", + "-d", + "-R", + "-f", + "--verify", + "-j", + "2", + ] + ) + assert cli_args.func == CmdDataPull + + cmd = cli_args.func(cli_args) + m = mocker.patch("dvc.repo.Repo.pull") + + assert cmd.run() == 0 + + m.assert_called_once_with( + targets=["target.dvc"], + remote="remote", + all_branches=True, + all_tags=True, + with_deps=True, + recursive=True, + force=True, + verify=True, + jobs=2, + ) From a148d16a9550cf22abdcb4e6093452c0bf7acfa0 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 21 Jan 2020 13:35:25 +0100 Subject: [PATCH 5/7] data cloud: verify is config option now --- dvc/command/data_sync.py | 14 ------ dvc/config.py | 2 + dvc/data_cloud.py | 12 ++--- dvc/output/base.py | 8 +-- dvc/remote/base.py | 4 ++ dvc/remote/gdrive.py | 1 + dvc/repo/__init__.py | 2 - dvc/repo/fetch.py | 8 +-- dvc/repo/pull.py | 2 - tests/func/test_remote.py | 7 ++- tests/unit/command/test_data_sync.py | 74 ---------------------------- 11 files changed, 19 insertions(+), 115 deletions(-) delete mode 100644 tests/unit/command/test_data_sync.py diff --git a/dvc/command/data_sync.py b/dvc/command/data_sync.py index 5c274cfee..9442e2bbb 100644 --- a/dvc/command/data_sync.py +++ b/dvc/command/data_sync.py @@ -28,7 +28,6 @@ class CmdDataPull(CmdDataBase): with_deps=self.args.with_deps, force=self.args.force, recursive=self.args.recursive, - verify=self.args.verify, ) except DvcException: logger.exception("failed to pull data from the cloud") @@ -67,7 +66,6 @@ class CmdDataFetch(CmdDataBase): all_tags=self.args.all_tags, with_deps=self.args.with_deps, recursive=self.args.recursive, - verify=self.args.verify, ) except DvcException: logger.exception("failed to fetch data from the cloud") @@ -147,12 +145,6 @@ def add_parser(subparsers, _parent_parser): default=False, help="Pull cache for subdirectories of the specified directory.", ) - pull_parser.add_argument( - "--verify", - action="store_true", - default=False, - help="Verify downloaded files checksums.", - ) pull_parser.set_defaults(func=CmdDataPull) # Push @@ -239,12 +231,6 @@ def add_parser(subparsers, _parent_parser): default=False, help="Fetch cache for subdirectories of specified directory.", ) - fetch_parser.add_argument( - "--verify", - action="store_true", - default=False, - help="Verify downloaded files checksums.", - ) fetch_parser.set_defaults(func=CmdDataFetch) # Status diff --git a/dvc/config.py b/dvc/config.py index 65250c76d..0759395d0 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -198,6 +198,7 @@ class Config(object): # pylint: disable=too-many-instance-attributes SECTION_REMOTE_ASK_PASSWORD = "ask_password" SECTION_REMOTE_GSS_AUTH = "gss_auth" SECTION_REMOTE_NO_TRAVERSE = "no_traverse" + SECTION_REMOTE_VERIFY = "verify" SECTION_REMOTE_SCHEMA = { Required(SECTION_REMOTE_URL): str, SECTION_AWS_REGION: str, @@ -232,6 +233,7 @@ class Config(object): # pylint: disable=too-many-instance-attributes SECTION_GDRIVE_USER_CREDENTIALS_FILE: str, PRIVATE_CWD: str, SECTION_REMOTE_NO_TRAVERSE: Bool, + SECTION_REMOTE_VERIFY: Bool, } SECTION_STATE = "state" diff --git a/dvc/data_cloud.py b/dvc/data_cloud.py index fc4a53f33..a4d67db35 100644 --- a/dvc/data_cloud.py +++ b/dvc/data_cloud.py @@ -62,9 +62,7 @@ class DataCloud(object): show_checksums=show_checksums, ) - def pull( - self, cache, jobs=None, remote=None, show_checksums=False, verify=False - ): + def pull(self, cache, jobs=None, remote=None, show_checksums=False): """Pull data items in a cloud-agnostic way. Args: @@ -75,14 +73,12 @@ class DataCloud(object): show_checksums (bool): show checksums instead of file names in information messages. """ + remote = self.get_remote(remote, "pull") downloaded_items_num = self.repo.cache.local.pull( - cache, - jobs=jobs, - remote=self.get_remote(remote, "pull"), - show_checksums=show_checksums, + cache, jobs=jobs, remote=remote, show_checksums=show_checksums ) - if not verify: + if not remote.verify: self._save_pulled_checksums(cache) return downloaded_items_num diff --git a/dvc/output/base.py b/dvc/output/base.py index 04d9ed461..0fca876c4 100644 --- a/dvc/output/base.py +++ b/dvc/output/base.py @@ -340,12 +340,7 @@ class OutputBase(object): self.remote.unprotect(self.path_info) def _collect_used_dir_cache( - self, - remote=None, - force=False, - jobs=None, - filter_info=None, - verify=False, + self, remote=None, force=False, jobs=None, filter_info=None ): """Get a list of `info`s related to the given directory. @@ -375,7 +370,6 @@ class OutputBase(object): jobs=jobs, remote=remote, show_checksums=False, - verify=verify, ) except DvcException: logger.debug("failed to pull cache for '{}'".format(self)) diff --git a/dvc/remote/base.py b/dvc/remote/base.py index 788742c8c..7262ba1a9 100644 --- a/dvc/remote/base.py +++ b/dvc/remote/base.py @@ -80,6 +80,7 @@ class RemoteBASE(object): CHECKSUM_JOBS = max(1, min(4, cpu_count() // 2)) DEFAULT_CACHE_TYPES = ["copy"] DEFAULT_NO_TRAVERSE = True + DEFAULT_VERIFY = False state = StateNoop() @@ -93,6 +94,9 @@ class RemoteBASE(object): self.no_traverse = config.get( Config.SECTION_REMOTE_NO_TRAVERSE, self.DEFAULT_NO_TRAVERSE ) + self.verify = config.get( + Config.SECTION_REMOTE_VERIFY, self.DEFAULT_VERIFY + ) self._dir_info = {} types = config.get(Config.SECTION_CACHE_TYPE, None) diff --git a/dvc/remote/gdrive.py b/dvc/remote/gdrive.py index a38b2a1a2..24d65aa25 100644 --- a/dvc/remote/gdrive.py +++ b/dvc/remote/gdrive.py @@ -63,6 +63,7 @@ class RemoteGDrive(RemoteBASE): path_cls = CloudURLInfo REQUIRES = {"pydrive2": "pydrive2"} DEFAULT_NO_TRAVERSE = False + DEFAULT_VERIFY = True GDRIVE_USER_CREDENTIALS_DATA = "GDRIVE_USER_CREDENTIALS_DATA" DEFAULT_USER_CREDENTIALS_FILE = "gdrive-user-credentials.json" diff --git a/dvc/repo/__init__.py b/dvc/repo/__init__.py index 96663f67f..9d0a506a8 100644 --- a/dvc/repo/__init__.py +++ b/dvc/repo/__init__.py @@ -223,7 +223,6 @@ class Repo(object): force=False, jobs=None, recursive=False, - verify=False, ): """Get the stages related to the given target and collect the `info` of its outputs. @@ -263,7 +262,6 @@ class Repo(object): force=force, jobs=jobs, filter_info=filter_info, - verify=verify, ) cache.update(used_cache, suffix=suffix) diff --git a/dvc/repo/fetch.py b/dvc/repo/fetch.py index 37fb0a56b..e995630e4 100644 --- a/dvc/repo/fetch.py +++ b/dvc/repo/fetch.py @@ -20,7 +20,6 @@ def _fetch( with_deps=False, all_tags=False, recursive=False, - verify=False, ): """Download data items from a cloud and imported repositories @@ -43,7 +42,6 @@ def _fetch( remote=remote, jobs=jobs, recursive=recursive, - verify=verify, ) downloaded = 0 @@ -51,11 +49,7 @@ def _fetch( try: downloaded += self.cloud.pull( - used, - jobs, - remote=remote, - show_checksums=show_checksums, - verify=verify, + used, jobs, remote=remote, show_checksums=show_checksums ) except NoRemoteError: if not used.external and used["local"]: diff --git a/dvc/repo/pull.py b/dvc/repo/pull.py index 52779376d..fadd34d3d 100644 --- a/dvc/repo/pull.py +++ b/dvc/repo/pull.py @@ -17,7 +17,6 @@ def pull( all_tags=False, force=False, recursive=False, - verify=False, ): processed_files_count = self._fetch( targets, @@ -27,7 +26,6 @@ def pull( all_tags=all_tags, with_deps=with_deps, recursive=recursive, - verify=verify, ) self._checkout( targets=targets, with_deps=with_deps, force=force, recursive=recursive diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 7639d6b88..44580e571 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -280,6 +280,11 @@ def test_verify_checksums(tmp_dir, erepo_dir, mocker): # Removing cache will invalidate existing state entries shutil.rmtree(dvc.cache.local.cache_dir) + dvc.config.set( + Config.SECTION_REMOTE_FMT.format("upstream"), + Config.SECTION_REMOTE_VERIFY, + "True", + ) - dvc.pull(verify=True) + dvc.pull() assert file_md5_spy.call_count == 3 diff --git a/tests/unit/command/test_data_sync.py b/tests/unit/command/test_data_sync.py deleted file mode 100644 index 8565c2e83..000000000 --- a/tests/unit/command/test_data_sync.py +++ /dev/null @@ -1,74 +0,0 @@ -from dvc.cli import parse_args -from dvc.command.data_sync import CmdDataFetch, CmdDataPull - - -def test_fetch(mocker): - cli_args = parse_args( - [ - "fetch", - "target.dvc", - "-r", - "remote", - "-a", - "-T", - "-d", - "-R", - "--verify", - "-j", - "2", - ] - ) - assert cli_args.func == CmdDataFetch - - cmd = cli_args.func(cli_args) - m = mocker.patch("dvc.repo.Repo.fetch") - - assert cmd.run() == 0 - - m.assert_called_once_with( - targets=["target.dvc"], - remote="remote", - all_branches=True, - all_tags=True, - with_deps=True, - recursive=True, - verify=True, - jobs=2, - ) - - -def test_pull(mocker): - cli_args = parse_args( - [ - "pull", - "target.dvc", - "-r", - "remote", - "-a", - "-T", - "-d", - "-R", - "-f", - "--verify", - "-j", - "2", - ] - ) - assert cli_args.func == CmdDataPull - - cmd = cli_args.func(cli_args) - m = mocker.patch("dvc.repo.Repo.pull") - - assert cmd.run() == 0 - - m.assert_called_once_with( - targets=["target.dvc"], - remote="remote", - all_branches=True, - all_tags=True, - with_deps=True, - recursive=True, - force=True, - verify=True, - jobs=2, - ) From e36cc8c9d25e9856aa601a03aaf92e51fae7ba55 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 21 Jan 2020 13:43:37 +0100 Subject: [PATCH 6/7] data cloud: add explanaiton to pulled checksums save --- dvc/data_cloud.py | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/dvc/data_cloud.py b/dvc/data_cloud.py index a4d67db35..bf4af73f1 100644 --- a/dvc/data_cloud.py +++ b/dvc/data_cloud.py @@ -87,6 +87,10 @@ class DataCloud(object): for checksum in cache["local"].keys(): cache_file = self.repo.cache.local.checksum_to_path_info(checksum) if self.repo.cache.local.exists(cache_file): + # We can safely save here, as existing corrupted files will be + # removed upon status, while files corrupted during download + # will not be copied from tmp_file (see download + # implementation in `remote/base`) self.repo.state.save(cache_file, checksum) def status(self, cache, jobs=None, remote=None, show_checksums=False): From 482dfaf9986ca7a2facc0524b3bec83aea5aeef3 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Redzy=C5=84ski?= Date: Wed, 22 Jan 2020 10:53:26 +0100 Subject: [PATCH 7/7] data cloud: refactor verify checksums test --- dvc/data_cloud.py | 3 +-- tests/func/test_data_cloud.py | 37 ++++++++++++++++++++++++++++++++++- tests/func/test_remote.py | 31 ----------------------------- 3 files changed, 37 insertions(+), 34 deletions(-) diff --git a/dvc/data_cloud.py b/dvc/data_cloud.py index bf4af73f1..e85c8b280 100644 --- a/dvc/data_cloud.py +++ b/dvc/data_cloud.py @@ -89,8 +89,7 @@ class DataCloud(object): if self.repo.cache.local.exists(cache_file): # We can safely save here, as existing corrupted files will be # removed upon status, while files corrupted during download - # will not be copied from tmp_file (see download - # implementation in `remote/base`) + # will not be moved from tmp_file (see `RemoteBASE.download()`) self.repo.state.save(cache_file, checksum) def status(self, cache, jobs=None, remote=None, show_checksums=False): diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index 748a7fe63..137fd674c 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -9,10 +9,11 @@ import pytest from mock import patch from dvc.cache import NamedCache +from dvc.compat import fspath from dvc.config import Config from dvc.data_cloud import DataCloud from dvc.main import main -from dvc.remote import RemoteAZURE +from dvc.remote import RemoteAZURE, RemoteConfig from dvc.remote import RemoteGDrive from dvc.remote import RemoteGS from dvc.remote import RemoteHDFS @@ -653,3 +654,37 @@ class TestShouldWarnOnNoChecksumInLocalAndRemoteCache(TestDvc): assert self.message_header in self._caplog.text assert self.message_foo_part in self._caplog.text assert self.message_bar_part in self._caplog.text + + +def test_verify_checksums(tmp_dir, scm, dvc, mocker, tmp_path_factory): + tmp_dir.dvc_gen({"file": "file1 content"}, commit="add file") + tmp_dir.dvc_gen({"dir": {"subfile": "file2 content"}}, commit="add dir") + + RemoteConfig(dvc.config).add( + "local_remote", + fspath(tmp_path_factory.mktemp("local_remote")), + default=True, + ) + dvc.push() + + # remove artifacts and cache to trigger fetching + os.remove("file") + shutil.rmtree("dir") + shutil.rmtree(dvc.cache.local.cache_dir) + + checksum_spy = mocker.spy(dvc.cache.local, "get_file_checksum") + + dvc.pull() + assert checksum_spy.call_count == 0 + + # Removing cache will invalidate existing state entries + shutil.rmtree(dvc.cache.local.cache_dir) + + dvc.config.set( + Config.SECTION_REMOTE_FMT.format("local_remote"), + Config.SECTION_REMOTE_VERIFY, + "True", + ) + + dvc.pull() + assert checksum_spy.call_count == 3 diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 44580e571..c6d1cffe9 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -13,7 +13,6 @@ from dvc.path_info import PathInfo from dvc.remote import RemoteLOCAL, RemoteConfig from dvc.remote.base import RemoteBASE from dvc.compat import fspath -from dvc.scm import Git from tests.basic_env import TestDvc from tests.remotes import Local @@ -258,33 +257,3 @@ def test_modify_missing_remote(dvc): with pytest.raises(ConfigError, match=r"unable to find remote section"): remote_config.modify("myremote", "gdrive_client_id", "xxx") - - -def test_verify_checksums(tmp_dir, erepo_dir, mocker): - with erepo_dir.chdir(): - erepo_dir.dvc_gen({"file": "file1 content"}, commit="add file") - erepo_dir.dvc_gen( - {"dir": {"subfile": "file2 content"}}, commit="add " "dir" - ) - - Git.clone(fspath(erepo_dir), fspath(tmp_dir)) - - from dvc.repo import Repo - - dvc = Repo(fspath(tmp_dir)) - - file_md5_spy = mocker.spy(dvc.cache.local, "get_file_checksum") - - dvc.pull() - assert file_md5_spy.call_count == 0 - - # Removing cache will invalidate existing state entries - shutil.rmtree(dvc.cache.local.cache_dir) - dvc.config.set( - Config.SECTION_REMOTE_FMT.format("upstream"), - Config.SECTION_REMOTE_VERIFY, - "True", - ) - - dvc.pull() - assert file_md5_spy.call_count == 3