From e393e29ef5c1508f2db7d0e06bfb424688edee7e Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Fri, 3 Jan 2020 04:00:02 +0200 Subject: [PATCH 01/78] metrics: introduce `diff` This first implementation is based on existing `dvc metrics show` functionality, but is also able to auto-detect json to properly show diff for it. --- dvc/command/metrics.py | 122 ++++++++++++++++++++++++++++- dvc/repo/brancher.py | 6 +- dvc/repo/metrics/__init__.py | 5 ++ dvc/repo/metrics/diff.py | 105 +++++++++++++++++++++++++ dvc/repo/metrics/show.py | 7 +- setup.py | 2 + tests/func/test_metrics.py | 95 +++++++++++++++++++--- tests/unit/command/test_metrics.py | 67 ++++++++++++++++ 8 files changed, 390 insertions(+), 19 deletions(-) create mode 100644 dvc/repo/metrics/diff.py create mode 100644 tests/unit/command/test_metrics.py diff --git a/dvc/command/metrics.py b/dvc/command/metrics.py index a2bc2587a..ef79392fb 100644 --- a/dvc/command/metrics.py +++ b/dvc/command/metrics.py @@ -26,7 +26,12 @@ def show_metrics(metrics, all_branches=False, all_tags=False): logger.info("{branch}:".format(branch=branch)) for fname, metric in val.items(): - lines = metric if type(metric) is list else metric.splitlines() + if isinstance(metric, dict): + lines = list(metric.values()) + elif isinstance(metric, list): + lines = metric + else: + lines = metric.splitlines() if len(lines) > 1: logger.info("\t{fname}:".format(fname=fname)) @@ -100,6 +105,59 @@ class CmdMetricsRemove(CmdBase): return 0 +def _show_diff(diff): + from texttable import Texttable + + if not diff: + return "No changes." + + table = Texttable() + + # remove borders to make it easier for users to copy stuff + table.set_chars(("", "", "", "")) + table.set_deco(0) + + rows = [["Path", "Metric", "Value", "Change"]] + for fname, mdiff in diff.items(): + for metric, change in mdiff.items(): + rows.append( + [ + fname, + metric, + change["new"], + change.get("diff", "diff not supported"), + ] + ) + table.add_rows(rows) + return table.draw() + + +class CmdMetricsDiff(CmdBase): + def run(self): + try: + diff = self.repo.metrics.diff( + a_ref=self.args.a_ref, + b_ref=self.args.b_ref, + targets=self.args.targets, + typ=self.args.type, + xpath=self.args.xpath, + recursive=self.args.recursive, + ) + + if self.args.show_json: + import json + + logger.info(json.dumps(diff)) + else: + logger.info(_show_diff(diff)) + + except DvcException: + logger.exception("failed to show metrics diff") + return 1 + + return 0 + + def add_parser(subparsers, parent_parser): METRICS_HELP = "Commands to add, manage, collect and display metrics." @@ -214,3 +272,65 @@ def add_parser(subparsers, parent_parser): ) metrics_remove_parser.add_argument("path", help="Path to a metric file.") metrics_remove_parser.set_defaults(func=CmdMetricsRemove) + + METRICS_DIFF_HELP = "Output metric values." + metrics_diff_parser = metrics_subparsers.add_parser( + "diff", + parents=[parent_parser], + description=append_doc_link(METRICS_DIFF_HELP, "metrics/diff"), + help=METRICS_DIFF_HELP, + formatter_class=argparse.RawDescriptionHelpFormatter, + ) + metrics_diff_parser.add_argument( + "a_ref", + nargs="?", + help=( + "Git reference from which diff is calculated. " + "If omitted `HEAD`(latest commit) is used." + ), + ) + metrics_diff_parser.add_argument( + "b_ref", + nargs="?", + help=( + "Git reference to which diff is calculated. " + "If omitted current working tree is used." + ), + ) + metrics_diff_parser.add_argument( + "--targets", + nargs="*", + help=( + "Metric files or directories (see -R) to show diff for. " + "Shows diff for all metric files by default." + ), + ) + metrics_diff_parser.add_argument( + "-t", + "--type", + help=( + "Type of metrics (json/tsv/htsv/csv/hcsv). " + "It can be detected by the file extension automatically. " + "Unsupported types will be treated as raw." + ), + ) + metrics_diff_parser.add_argument( + "-x", "--xpath", help="json/tsv/htsv/csv/hcsv path." + ) + metrics_diff_parser.add_argument( + "-R", + "--recursive", + action="store_true", + default=False, + help=( + "If any target is a directory, recursively search and process " + "metric files." + ), + ) + metrics_diff_parser.add_argument( + "--show-json", + action="store_true", + default=False, + help="Show output in JSON format.", + ) + metrics_diff_parser.set_defaults(func=CmdMetricsDiff) diff --git a/dvc/repo/brancher.py b/dvc/repo/brancher.py index 8b096726c..f64bf41b7 100644 --- a/dvc/repo/brancher.py +++ b/dvc/repo/brancher.py @@ -4,7 +4,7 @@ from dvc.scm.tree import WorkingTree def brancher( # noqa: E302 - self, all_branches=False, all_tags=False, all_commits=False + self, revs=None, all_branches=False, all_tags=False, all_commits=False ): """Generator that iterates over specified revisions. @@ -20,12 +20,12 @@ def brancher( # noqa: E302 - empty string it there is no branches to iterate over - "Working Tree" if there are uncommitted changes in the SCM repo """ - if not any([all_branches, all_tags, all_commits]): + if not any([revs, all_branches, all_tags, all_commits]): yield "" return saved_tree = self.tree - revs = [] + revs = revs or [] scm = self.scm diff --git a/dvc/repo/metrics/__init__.py b/dvc/repo/metrics/__init__.py index 8904a00b9..7fce413da 100644 --- a/dvc/repo/metrics/__init__.py +++ b/dvc/repo/metrics/__init__.py @@ -21,3 +21,8 @@ class Metrics(object): from dvc.repo.metrics.remove import remove return remove(self.repo, *args, **kwargs) + + def diff(self, *args, **kwargs): + from .diff import diff + + return diff(self.repo, *args, **kwargs) diff --git a/dvc/repo/metrics/diff.py b/dvc/repo/metrics/diff.py new file mode 100644 index 000000000..aa056cadc --- /dev/null +++ b/dvc/repo/metrics/diff.py @@ -0,0 +1,105 @@ +import json +from collections import defaultdict + +from flatten_dict import flatten + +from dvc.exceptions import NoMetricsError + + +def _parse(raw): + if isinstance(raw, (dict, list, int, float)): + return raw + + assert isinstance(raw, str) + try: + return json.loads(raw) + except json.JSONDecodeError: + return raw + + +def _diff_vals(old, new): + if ( + isinstance(new, list) + and isinstance(old, list) + and len(old) == len(new) == 1 + ): + return _diff_vals(old[0], new[0]) + + if old == new: + return {} + + res = {"old": old, "new": new} + if isinstance(new, (int, float)) and isinstance(old, (int, float)): + res["diff"] = new - old + return res + + +# dot_reducer is not released yet (flatten-dict > 0.2.0) +def _dot(k1, k2): + if k1 is None: + return k2 + return "{0}.{1}".format(k1, k2) + + +def _diff_dicts(old_dict, new_dict): + old_default = None + new_default = None + + if isinstance(new_dict, dict): + new = flatten(new_dict, reducer=_dot) + else: + new = defaultdict(lambda: "not a dict") + new_default = "unable to parse" + + if isinstance(old_dict, dict): + old = flatten(old_dict, reducer=_dot) + else: + old = defaultdict(lambda: "not a dict") + old_default = "unable to parse" + + res = defaultdict(dict) + + xpaths = set(old.keys()) + xpaths.update(set(new.keys())) + for xpath in xpaths: + old_val = old.get(xpath, old_default) + new_val = new.get(xpath, new_default) + val_diff = _diff_vals(old_val, new_val) + if val_diff: + res[xpath] = val_diff + return dict(res) + + +def _diff(old_raw, new_raw): + old = _parse(old_raw) + new = _parse(new_raw) + + if isinstance(new, dict) or isinstance(old, dict): + return _diff_dicts(old, new) + + return {"": _diff_vals(old, new)} + + +def _get_metrics(repo, *args, rev=None, **kwargs): + try: + metrics = repo.metrics.show( + *args, **kwargs, revs=[rev] if rev else None + ) + return metrics[rev or ""] + except NoMetricsError: + return {} + + +def diff(repo, *args, a_ref=None, b_ref=None, **kwargs): + old = _get_metrics(repo, *args, **kwargs, rev=(a_ref or "HEAD")) + new = _get_metrics(repo, *args, **kwargs, rev=b_ref) + + paths = set(old.keys()) + paths.update(set(new.keys())) + + res = defaultdict(dict) + for path in paths: + path_diff = _diff(old[path], new[path]) + if path_diff: + res[path] = path_diff + return dict(res) diff --git a/dvc/repo/metrics/show.py b/dvc/repo/metrics/show.py index 01df0e86d..9f0f3ef13 100644 --- a/dvc/repo/metrics/show.py +++ b/dvc/repo/metrics/show.py @@ -20,7 +20,7 @@ logger = logging.getLogger(__name__) def _read_metric_json(fd, json_path): parser = parse(json_path) - return [x.value for x in parser.find(json.load(fd))] + return {str(x.full_path): x.value for x in parser.find(json.load(fd))} def _get_values(row): @@ -266,6 +266,7 @@ def show( all_branches=False, all_tags=False, recursive=False, + revs=None, ): res = {} found = set() @@ -274,7 +275,9 @@ def show( # Iterate once to call `_collect_metrics` on all the stages targets = [None] - for branch in repo.brancher(all_branches=all_branches, all_tags=all_tags): + for branch in repo.brancher( + revs=revs, all_branches=all_branches, all_tags=all_tags + ): metrics = {} for target in targets: diff --git a/setup.py b/setup.py index f8c4fda59..592ef584c 100644 --- a/setup.py +++ b/setup.py @@ -75,6 +75,8 @@ install_requires = [ "win-unicode-console>=0.5; sys_platform == 'win32'", "pywin32>=225; sys_platform == 'win32'", "networkx>=2.1,<2.4", + "flatten-dict>=0.2.0", + "texttable>=0.5.2", ] diff --git a/tests/func/test_metrics.py b/tests/func/test_metrics.py index 8f69ec315..42043dbd8 100644 --- a/tests/func/test_metrics.py +++ b/tests/func/test_metrics.py @@ -4,6 +4,8 @@ import json import logging import os +import pytest + from dvc.exceptions import DvcException from dvc.exceptions import NoMetricsError from dvc.main import main @@ -113,9 +115,9 @@ class TestMetrics(TestMetricsBase): ["metric_json"], typ="json", xpath="branch", all_branches=True ) self.assertEqual(len(ret), 3) - self.assertSequenceEqual(ret["foo"]["metric_json"], ["foo"]) - self.assertSequenceEqual(ret["bar"]["metric_json"], ["bar"]) - self.assertSequenceEqual(ret["baz"]["metric_json"], ["baz"]) + self.assertSequenceEqual(ret["foo"]["metric_json"], {"branch": "foo"}) + self.assertSequenceEqual(ret["bar"]["metric_json"], {"branch": "bar"}) + self.assertSequenceEqual(ret["baz"]["metric_json"], {"branch": "baz"}) ret = self.dvc.metrics.show( ["metric_tsv"], typ="tsv", xpath="0,0", all_branches=True @@ -158,13 +160,13 @@ class TestMetrics(TestMetricsBase): self.assertEqual(len(ret), 1) self.assertSequenceEqual( ret["foo"]["metric_json_ext"], - [ - { + { + "metrics.[0]": { "dataset": "train", "deviation_mse": 0.173461, "value_mse": 0.421601, } - ], + }, ) self.assertRaises(KeyError, lambda: ret["bar"]) self.assertRaises(KeyError, lambda: ret["baz"]) @@ -723,9 +725,9 @@ class TestCachedMetrics(TestDvcGit): self.assertEqual( res, { - "master": {"metrics.json": ["master"]}, - "one": {"metrics.json": ["one"]}, - "two": {"metrics.json": ["two"]}, + "master": {"metrics.json": {"metrics": "master"}}, + "one": {"metrics.json": {"metrics": "one"}}, + "two": {"metrics.json": {"metrics": "two"}}, }, ) @@ -736,9 +738,9 @@ class TestCachedMetrics(TestDvcGit): self.assertEqual( res, { - "master": {"metrics.json": ["master"]}, - "one": {"metrics.json": ["one"]}, - "two": {"metrics.json": ["two"]}, + "master": {"metrics.json": {"metrics": "master"}}, + "one": {"metrics.json": {"metrics": "one"}}, + "two": {"metrics.json": {"metrics": "two"}}, }, ) @@ -802,6 +804,10 @@ class TestMetricsType(TestDvcGit): for branch in self.branches: if isinstance(ret[branch][file_name], list): self.assertSequenceEqual(ret[branch][file_name], [branch]) + elif isinstance(ret[branch][file_name], dict): + self.assertSequenceEqual( + ret[branch][file_name], {"branch": branch} + ) else: self.assertSequenceEqual(ret[branch][file_name], branch) @@ -834,7 +840,7 @@ def test_show_xpath_should_override_stage_xpath(tmp_dir, dvc): dvc.run(cmd="", overwrite=True, metrics=["metric"]) dvc.metrics.modify("metric", typ="json", xpath="m2") - assert dvc.metrics.show(xpath="m1") == {"": {"metric": [0.1]}} + assert dvc.metrics.show(xpath="m1") == {"": {"metric": {"m1": 0.1}}} def test_show_multiple_outputs(tmp_dir, dvc, caplog): @@ -871,3 +877,66 @@ def test_show_multiple_outputs(tmp_dir, dvc, caplog): "the following metrics do not exist, " "are not metric files or are malformed: 'not-found'" ) in caplog.text + + +def test_metrics_diff_raw(tmp_dir, scm, dvc): + def _gen(val): + tmp_dir.gen({"metrics": val}) + dvc.run(cmd="", metrics=["metrics"]) + dvc.scm.add(["metrics.dvc"]) + dvc.scm.commit(str(val)) + + _gen("raw 1") + _gen("raw 2") + _gen("raw 3") + + assert dvc.metrics.diff(a_ref="HEAD~2") == { + "metrics": {"": {"old": "raw 1", "new": "raw 3"}} + } + + +@pytest.mark.parametrize("xpath", [True, False]) +def test_metrics_diff_json(tmp_dir, scm, dvc, xpath): + def _gen(val): + metrics = {"a": {"b": {"c": val, "d": 1, "e": str(val)}}} + tmp_dir.gen({"m.json": json.dumps(metrics)}) + dvc.run(cmd="", metrics=["m.json"]) + dvc.metrics.modify("m.json", typ="json") + if xpath: + dvc.metrics.modify("m.json", xpath="a.b.c") + dvc.scm.add(["m.json.dvc"]) + dvc.scm.commit(str(val)) + + _gen(1) + _gen(2) + _gen(3) + + expected = {"m.json": {"a.b.c": {"old": 1, "new": 3, "diff": 2}}} + + if not xpath: + expected["m.json"]["a.b.e"] = {"old": "1", "new": "3"} + + assert expected == dvc.metrics.diff(a_ref="HEAD~2") + + +def test_metrics_diff_broken_json(tmp_dir, scm, dvc): + metrics = {"a": {"b": {"c": 1, "d": 1, "e": "3"}}} + tmp_dir.gen({"m.json": json.dumps(metrics)}) + dvc.run(cmd="", metrics_no_cache=["m.json"]) + dvc.scm.add(["m.json.dvc", "m.json"]) + dvc.scm.commit("add metrics") + + (tmp_dir / "m.json").write_text(json.dumps(metrics) + "ma\nlformed\n") + + assert dvc.metrics.diff() == { + "m.json": { + "a.b.c": {"old": 1, "new": "unable to parse"}, + "a.b.d": {"old": 1, "new": "unable to parse"}, + "a.b.e": {"old": "3", "new": "unable to parse"}, + } + } + + +def test_metrics_diff_no_metrics(tmp_dir, scm, dvc): + tmp_dir.scm_gen({"foo": "foo"}, commit="add foo") + assert dvc.metrics.diff(a_ref="HEAD~1") == {} diff --git a/tests/unit/command/test_metrics.py b/tests/unit/command/test_metrics.py new file mode 100644 index 000000000..fb2394bd2 --- /dev/null +++ b/tests/unit/command/test_metrics.py @@ -0,0 +1,67 @@ +from dvc.cli import parse_args +from dvc.command.metrics import CmdMetricsDiff, _show_diff + + +def test_metrics_diff(dvc, mocker): + cli_args = parse_args( + [ + "metrics", + "diff", + "HEAD~10", + "HEAD~1", + "-t", + "json", + "-x", + "x.path", + "-R", + "--show-json", + "--targets", + "target1", + "target2", + ] + ) + assert cli_args.func == CmdMetricsDiff + + cmd = cli_args.func(cli_args) + m = mocker.patch("dvc.repo.metrics.diff.diff", return_value={}) + + assert cmd.run() == 0 + + m.assert_called_once_with( + cmd.repo, + targets=["target1", "target2"], + a_ref="HEAD~10", + b_ref="HEAD~1", + typ="json", + xpath="x.path", + recursive=True, + ) + + +def test_metrics_show_json_diff(): + assert _show_diff( + {"metrics.json": {"a.b.c": {"old": 1, "new": 2, "diff": 3}}} + ) == ( + " Path Metric Value Change\n" + "metrics.json a.b.c 2 3 " + ) + + +def test_metrics_show_raw_diff(): + assert _show_diff({"metrics": {"": {"old": "1", "new": "2"}}}) == ( + " Path Metric Value Change \n" + "metrics 2 diff not supported" + ) + + +def test_metrics_diff_no_diff(): + assert _show_diff( + {"other.json": {"a.b.d": {"old": "old", "new": "new"}}} + ) == ( + " Path Metric Value Change \n" + "other.json a.b.d new diff not supported" + ) + + +def test_metrics_diff_no_changes(): + assert _show_diff({}) == "No changes." From d55427dbd9630969bf47ab278642354fe0931f51 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Mon, 13 Jan 2020 16:30:15 +0545 Subject: [PATCH 02/78] test: refactor hdfs related functions in HDFS test helpers --- tests/func/test_data_cloud.py | 11 +++---- tests/func/test_repro.py | 4 +-- tests/remotes.py | 61 +++++++++++++++++------------------ 3 files changed, 37 insertions(+), 39 deletions(-) diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index 10fee7da3..b0fb40abd 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -32,9 +32,9 @@ from tests.utils import spy from tests.remotes import ( _should_test_gcp, - _should_test_hdfs, Azure, GDrive, + HDFS, S3, SSHMocked, OSS, @@ -45,7 +45,6 @@ from tests.remotes import ( TEST_GDRIVE_CLIENT_SECRET, TEST_REMOTE, get_gcp_url, - get_hdfs_url, get_local_url, ) @@ -332,10 +331,10 @@ class TestRemoteSSHMocked(TestDataCloudBase): class TestRemoteHDFS(TestDataCloudBase): def _should_test(self): - return _should_test_hdfs() + return HDFS.should_test() def _get_url(self): - return get_hdfs_url() + return HDFS.get_url() def _get_cloud_class(self): return RemoteHDFS @@ -431,10 +430,10 @@ class TestRemoteLOCALCLI(TestDataCloudCLIBase): class TestRemoteHDFSCLI(TestDataCloudCLIBase): def _should_test(self): - return _should_test_hdfs() + return HDFS.should_test() def _test(self): - url = get_hdfs_url() + url = HDFS.get_url() self.main(["remote", "add", TEST_REMOTE, url]) diff --git a/tests/func/test_repro.py b/tests/func/test_repro.py index 4a8435c5b..4c3f6a1b5 100644 --- a/tests/func/test_repro.py +++ b/tests/func/test_repro.py @@ -36,7 +36,7 @@ from dvc.utils.stage import load_stage_file from tests.basic_env import TestDvc from tests.remotes import ( _should_test_gcp, - _should_test_hdfs, + HDFS, S3, SSH, SSHMocked, @@ -996,7 +996,7 @@ class TestReproExternalGS(TestReproExternalBase): class TestReproExternalHDFS(TestReproExternalBase): def should_test(self): - return _should_test_hdfs() + return HDFS.should_test() @property def scheme(self): diff --git a/tests/remotes.py b/tests/remotes.py index d4c61ebc6..daa206a17 100644 --- a/tests/remotes.py +++ b/tests/remotes.py @@ -66,29 +66,6 @@ def _should_test_gcp(): return True -def _should_test_hdfs(): - if platform.system() != "Linux": - return False - - try: - check_output( - ["hadoop", "version"], shell=True, executable=os.getenv("SHELL") - ) - except (CalledProcessError, IOError): - return False - - p = Popen( - "hadoop fs -ls hdfs://127.0.0.1/", - shell=True, - executable=os.getenv("SHELL"), - ) - p.communicate() - if p.returncode != 0: - return False - - return True - - def get_local_storagepath(): return TestDvc.mkdtemp() @@ -97,12 +74,6 @@ def get_local_url(): return get_local_storagepath() -def get_hdfs_url(): - return "hdfs://{}@127.0.0.1{}".format( - getpass.getuser(), get_local_storagepath() - ) - - def get_gcp_storagepath(): return TEST_GCP_REPO_BUCKET + "/" + str(uuid.uuid4()) @@ -277,5 +248,33 @@ class SSHMocked: class HDFS: - should_test = _should_test_hdfs - get_url = get_hdfs_url + @staticmethod + def should_test(): + if platform.system() != "Linux": + return False + + try: + check_output( + ["hadoop", "version"], + shell=True, + executable=os.getenv("SHELL"), + ) + except (CalledProcessError, IOError): + return False + + p = Popen( + "hadoop fs -ls hdfs://127.0.0.1/", + shell=True, + executable=os.getenv("SHELL"), + ) + p.communicate() + if p.returncode != 0: + return False + + return True + + @staticmethod + def get_url(): + return "hdfs://{}@127.0.0.1{}".format( + getpass.getuser(), get_local_storagepath() + ) From 26d85f1a528a4ccc4dc51640e7019ab06ead4b15 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Mon, 13 Jan 2020 23:49:22 +0545 Subject: [PATCH 03/78] test: merge gcp related functions to GCP helper class --- tests/func/test_data_cloud.py | 11 +++--- tests/func/test_repro.py | 9 +++-- tests/remotes.py | 63 +++++++++++++++++------------------ 3 files changed, 41 insertions(+), 42 deletions(-) diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index b0fb40abd..7b2e349c8 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -31,8 +31,8 @@ from tests.basic_env import TestDvc from tests.utils import spy from tests.remotes import ( - _should_test_gcp, Azure, + GCP, GDrive, HDFS, S3, @@ -44,7 +44,6 @@ from tests.remotes import ( TEST_GDRIVE_CLIENT_ID, TEST_GDRIVE_CLIENT_SECRET, TEST_REMOTE, - get_gcp_url, get_local_url, ) @@ -231,7 +230,7 @@ class TestRemoteGDrive(TestDataCloudBase): class TestRemoteGS(TestDataCloudBase): def _should_test(self): - return _should_test_gcp() + return GCP.should_test() def _setup_cloud(self): self._ensure_should_run() @@ -249,7 +248,7 @@ class TestRemoteGS(TestDataCloudBase): self.assertIsInstance(self.cloud.get_remote(), self._get_cloud_class()) def _get_url(self): - return get_gcp_url() + return GCP.get_url() def _get_cloud_class(self): return RemoteGS @@ -484,10 +483,10 @@ class TestRemoteGDriveCLI(TestDataCloudCLIBase): class TestRemoteGSCLI(TestDataCloudCLIBase): def _should_test(self): - return _should_test_gcp() + return GCP.should_test() def _test(self): - url = get_gcp_url() + url = GCP.get_url() self.main(["remote", "add", TEST_REMOTE, url]) self.main( diff --git a/tests/func/test_repro.py b/tests/func/test_repro.py index 4c3f6a1b5..3e3be2349 100644 --- a/tests/func/test_repro.py +++ b/tests/func/test_repro.py @@ -9,6 +9,7 @@ from pathlib import Path from subprocess import PIPE from subprocess import Popen from urllib.parse import urljoin +from unittest import SkipTest import boto3 import paramiko @@ -35,7 +36,7 @@ from dvc.utils.stage import dump_stage_file from dvc.utils.stage import load_stage_file from tests.basic_env import TestDvc from tests.remotes import ( - _should_test_gcp, + GCP, HDFS, S3, SSH, @@ -857,7 +858,9 @@ class TestReproExternalBase(TestDvc): @patch("dvc.prompt.confirm", return_value=True) def test(self, mock_prompt): if not self.should_test(): - return + raise SkipTest( + "Test {} is disabled".format(self.__class__.__name__) + ) cache = ( self.scheme @@ -975,7 +978,7 @@ class TestReproExternalS3(TestReproExternalBase): class TestReproExternalGS(TestReproExternalBase): def should_test(self): - return _should_test_gcp() + return GCP.should_test() @property def scheme(self): diff --git a/tests/remotes.py b/tests/remotes.py index daa206a17..4623c7606 100644 --- a/tests/remotes.py +++ b/tests/remotes.py @@ -43,29 +43,6 @@ TEST_GDRIVE_CLIENT_ID = ( TEST_GDRIVE_CLIENT_SECRET = "2fy_HyzSwkxkGzEken7hThXb" -def _should_test_gcp(): - do_test = env2bool("DVC_TEST_GCP", undefined=None) - if do_test is not None: - return do_test - - if not os.path.exists(TEST_GCP_CREDS_FILE): - return False - - try: - check_output( - [ - "gcloud", - "auth", - "activate-service-account", - "--key-file", - TEST_GCP_CREDS_FILE, - ] - ) - except (CalledProcessError, OSError): - return False - return True - - def get_local_storagepath(): return TestDvc.mkdtemp() @@ -74,14 +51,6 @@ def get_local_url(): return get_local_storagepath() -def get_gcp_storagepath(): - return TEST_GCP_REPO_BUCKET + "/" + str(uuid.uuid4()) - - -def get_gcp_url(): - return "gs://" + get_gcp_storagepath() - - class Local: should_test = lambda: True # noqa: E731 get_url = get_local_url @@ -131,8 +100,36 @@ class S3Mocked(S3): class GCP: - should_test = _should_test_gcp - get_url = get_gcp_url + @staticmethod + def should_test(): + do_test = env2bool("DVC_TEST_GCP", undefined=None) + if do_test is not None: + return do_test + + if not os.path.exists(TEST_GCP_CREDS_FILE): + return False + + try: + check_output( + [ + "gcloud", + "auth", + "activate-service-account", + "--key-file", + TEST_GCP_CREDS_FILE, + ] + ) + except (CalledProcessError, OSError): + return False + return True + + @staticmethod + def get_storagepath(): + return TEST_GCP_REPO_BUCKET + "/" + str(uuid.uuid4()) + + @staticmethod + def get_url(): + return "gs://" + GCP.get_storagepath() @classmethod @contextmanager From 2c9b763906b6a4f4144626bc3f61ddfc9b516cf7 Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Mon, 13 Jan 2020 12:36:20 -0600 Subject: [PATCH 04/78] version: decode correctly commands output --- dvc/version.py | 2 +- tests/unit/test_version.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/dvc/version.py b/dvc/version.py index a1ebd240b..411510f57 100644 --- a/dvc/version.py +++ b/dvc/version.py @@ -49,7 +49,7 @@ def _is_release(dir_path, base_version): ["git", "describe", "--tags", "--exact-match"], cwd=dir_path, stderr=subprocess.STDOUT, - ) + ).decode("utf-8") tag = output.strip() return tag == base_version except subprocess.CalledProcessError: diff --git a/tests/unit/test_version.py b/tests/unit/test_version.py index 1d21c7205..5cd4f8ca5 100644 --- a/tests/unit/test_version.py +++ b/tests/unit/test_version.py @@ -12,10 +12,10 @@ def test_is_release(): assert ret is False m.side_effect = None - m.return_value = dvc.version._BASE_VERSION + m.return_value = dvc.version._BASE_VERSION.encode("ascii") ret = dvc.version._is_release(None, dvc.version._BASE_VERSION) assert ret - m.return_value = "630d1741c2d5dd89a3176bd15b63121b905d35c9" + m.return_value = b"630d1741c2d5dd89a3176bd15b63121b905d35c9" ret = dvc.version._is_release(None, dvc.version._BASE_VERSION) assert ret is False From ff094b25607feebd92ffb7d8fa8c69861cf74656 Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Mon, 13 Jan 2020 17:51:20 -0600 Subject: [PATCH 05/78] remote: error out when modifying a non-existing remote Close #3131 --- dvc/remote/config.py | 5 +++-- tests/func/test_remote.py | 11 +++++++++-- 2 files changed, 12 insertions(+), 4 deletions(-) diff --git a/dvc/remote/config.py b/dvc/remote/config.py index 87572bd1d..b409e7eab 100644 --- a/dvc/remote/config.py +++ b/dvc/remote/config.py @@ -3,8 +3,7 @@ import os import posixpath from urllib.parse import urlparse -from dvc.config import Config -from dvc.config import ConfigError +from dvc.config import Config, ConfigError from dvc.utils import relpath @@ -134,6 +133,8 @@ class RemoteConfig(object): break def modify(self, name, option, value, level=None): + self.get_settings(name) + self.config.set( Config.SECTION_REMOTE_FMT.format(name), option, value, level=level ) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 777666bd9..09674eff3 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -6,7 +6,7 @@ import configobj import pytest from mock import patch -from dvc.config import Config +from dvc.config import Config, ConfigError from dvc.exceptions import DownloadError, UploadError from dvc.main import main from dvc.path_info import PathInfo @@ -31,10 +31,10 @@ class TestRemote(TestDvc): self.assertEqual(main(["remote", "list"]), 0) - self.assertEqual(main(["remote", "remove", remotes[0]]), 0) self.assertEqual( main(["remote", "modify", remotes[0], "option", "value"]), 0 ) + self.assertEqual(main(["remote", "remove", remotes[0]]), 0) self.assertEqual(main(["remote", "list"]), 0) @@ -250,3 +250,10 @@ def test_raise_on_too_many_open_files(tmp_dir, dvc, tmp_path_factory, mocker): with pytest.raises(OSError) as e: dvc.push() assert e.errno == errno.EMFILE + + +def test_modify_missing_remote(dvc): + remote_config = RemoteConfig(dvc.config) + + with pytest.raises(ConfigError, match=r"unable to find remote section"): + remote_config.modify("myremote", "gdrive_client_id", "xxx") From a27910d71d655d6f803763f4f070c9bac8f6434c Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Tue, 14 Jan 2020 00:17:45 +0545 Subject: [PATCH 06/78] test: merge local related function in Local helper class --- tests/func/test_data_cloud.py | 16 ++++++++-------- tests/func/test_remote.py | 4 ++-- tests/remotes.py | 25 ++++++++++++------------- 3 files changed, 22 insertions(+), 23 deletions(-) diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index 7b2e349c8..cfe0febfc 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -35,6 +35,7 @@ from tests.remotes import ( GCP, GDrive, HDFS, + Local, S3, SSHMocked, OSS, @@ -44,7 +45,6 @@ from tests.remotes import ( TEST_GDRIVE_CLIENT_ID, TEST_GDRIVE_CLIENT_SECRET, TEST_REMOTE, - get_local_url, ) @@ -278,10 +278,10 @@ class TestRemoteOSS(TestDataCloudBase): class TestRemoteLOCAL(TestDataCloudBase): def _should_test(self): - return True + return Local.should_test() def _get_url(self): - self.dname = get_local_url() + self.dname = Local.get_url() return self.dname def _get_cloud_class(self): @@ -420,7 +420,7 @@ class TestDataCloudCLIBase(TestDvc): class TestRemoteLOCALCLI(TestDataCloudCLIBase): def _test(self): - url = get_local_url() + url = Local.get_url() self.main(["remote", "add", TEST_REMOTE, url]) @@ -545,7 +545,7 @@ class TestWarnOnOutdatedStage(TestDvc): self.assertEqual(ret, 0) def _test(self): - url = get_local_url() + url = Local.get_url() self.main(["remote", "add", "-d", TEST_REMOTE, url]) stage = self.dvc.run(outs=["bar"], cmd="echo bar > bar") @@ -577,11 +577,11 @@ class TestRecursiveSyncOperations(TestDataCloudBase): self.assertEqual(ret, 0) def _get_url(self): - self.dname = get_local_url() + self.dname = Local.get_url() return self.dname def _should_test(self): - return True + return Local.should_test() def _get_cloud_class(self): return RemoteLOCAL @@ -659,7 +659,7 @@ class TestCheckSumRecalculation(TestDvc): with patch.object( RemoteLOCAL, "get_file_checksum", test_get_file_checksum ): - url = get_local_url() + url = Local.get_url() ret = main(["remote", "add", "-d", TEST_REMOTE, url]) self.assertEqual(ret, 0) ret = main(["config", "cache.type", "hardlink"]) diff --git a/tests/func/test_remote.py b/tests/func/test_remote.py index 777666bd9..99e8e248c 100644 --- a/tests/func/test_remote.py +++ b/tests/func/test_remote.py @@ -14,7 +14,7 @@ from dvc.remote import RemoteLOCAL, RemoteConfig from dvc.remote.base import RemoteBASE from dvc.compat import fspath from tests.basic_env import TestDvc -from tests.remotes import get_local_storagepath +from tests.remotes import Local class TestRemote(TestDvc): @@ -159,7 +159,7 @@ class TestRemoteShouldHandleUppercaseRemoteName(TestDvc): upper_case_remote_name = "UPPERCASEREMOTE" def test(self): - remote_url = get_local_storagepath() + remote_url = Local.get_storagepath() ret = main(["remote", "add", self.upper_case_remote_name, remote_url]) self.assertEqual(ret, 0) diff --git a/tests/remotes.py b/tests/remotes.py index 4623c7606..cf80fd160 100644 --- a/tests/remotes.py +++ b/tests/remotes.py @@ -43,17 +43,16 @@ TEST_GDRIVE_CLIENT_ID = ( TEST_GDRIVE_CLIENT_SECRET = "2fy_HyzSwkxkGzEken7hThXb" -def get_local_storagepath(): - return TestDvc.mkdtemp() - - -def get_local_url(): - return get_local_storagepath() - - class Local: should_test = lambda: True # noqa: E731 - get_url = get_local_url + + @staticmethod + def get_storagepath(): + return TestDvc.mkdtemp() + + @staticmethod + def get_url(): + return Local.get_storagepath() class S3: @@ -215,7 +214,7 @@ class SSH: @staticmethod def get_url(): return "ssh://{}@127.0.0.1:22{}".format( - getpass.getuser(), get_local_storagepath() + getpass.getuser(), Local.get_storagepath() ) @@ -224,9 +223,9 @@ class SSHMocked: @staticmethod def get_url(user, port): - path = get_local_storagepath() + path = Local.get_storagepath() if os.name == "nt": - # NOTE: On Windows get_local_storagepath() will return an + # NOTE: On Windows Local.get_storagepath() will return an # ntpath that looks something like `C:\some\path`, which is not # compatible with SFTP paths [1], so we need to convert it to # a proper posixpath. @@ -273,5 +272,5 @@ class HDFS: @staticmethod def get_url(): return "hdfs://{}@127.0.0.1{}".format( - getpass.getuser(), get_local_storagepath() + getpass.getuser(), Local.get_storagepath() ) From 876b659c2cd84411b2090fc1694aae0ea6566bb2 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 14 Jan 2020 10:27:29 +0200 Subject: [PATCH 07/78] remote: config: add a comment about section existence check https://github.com/iterative/dvc/pull/3132#discussion_r366202037 --- dvc/remote/config.py | 1 + 1 file changed, 1 insertion(+) diff --git a/dvc/remote/config.py b/dvc/remote/config.py index b409e7eab..3401d3e82 100644 --- a/dvc/remote/config.py +++ b/dvc/remote/config.py @@ -133,6 +133,7 @@ class RemoteConfig(object): break def modify(self, name, option, value, level=None): + # try to get remote settings to verify that section exists self.get_settings(name) self.config.set( From a09bfaf9db4a7cbf02f80913edbb3a57dd78aa80 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 14 Jan 2020 10:29:01 +0200 Subject: [PATCH 08/78] dvc: bump to 0.81.1 --- dvc/version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/version.py b/dvc/version.py index 411510f57..7e0719fc0 100644 --- a/dvc/version.py +++ b/dvc/version.py @@ -7,7 +7,7 @@ import os import subprocess -_BASE_VERSION = "0.81.0" +_BASE_VERSION = "0.81.1" def _generate_version(base_version): From 09223442f572695c22bdf2959206411e764aa85c Mon Sep 17 00:00:00 2001 From: rxxg Date: Wed, 8 Jan 2020 16:16:21 +0100 Subject: [PATCH 09/78] Use speedcopy module for faster copies across network boundaries --- dvc/system.py | 4 ++-- setup.py | 1 + 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/dvc/system.py b/dvc/system.py index d32663b4a..a6fc77bf4 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -1,7 +1,7 @@ import errno import logging import os -import shutil +import speedcopy from dvc.compat import fspath from dvc.exceptions import DvcException @@ -18,7 +18,7 @@ class System(object): @staticmethod def copy(src, dest): src, dest = fspath(src), fspath(dest) - return shutil.copyfile(src, dest) + return speedcopy.copyfile(src, dest) @staticmethod def hardlink(source, link_name): diff --git a/setup.py b/setup.py index f8c4fda59..4aaf0b7b1 100644 --- a/setup.py +++ b/setup.py @@ -75,6 +75,7 @@ install_requires = [ "win-unicode-console>=0.5; sys_platform == 'win32'", "pywin32>=225; sys_platform == 'win32'", "networkx>=2.1,<2.4", + "speedcopy>=2", ] From 811fc5317c4e18f402aa0631a44faabb38eea576 Mon Sep 17 00:00:00 2001 From: rxxg Date: Thu, 9 Jan 2020 14:25:01 +0100 Subject: [PATCH 10/78] Use speedcopy module with bugfix --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 4aaf0b7b1..5008f9ce3 100644 --- a/setup.py +++ b/setup.py @@ -75,7 +75,7 @@ install_requires = [ "win-unicode-console>=0.5; sys_platform == 'win32'", "pywin32>=225; sys_platform == 'win32'", "networkx>=2.1,<2.4", - "speedcopy>=2", + "speedcopy @ git+https://github.com/rxxg/speedcopy@5", ] From 674541f9225ee96acd237bfb8791ace3738ffa49 Mon Sep 17 00:00:00 2001 From: rxxg Date: Thu, 9 Jan 2020 17:57:15 +0100 Subject: [PATCH 11/78] Use pyfastcopy for non-Windows systems using Python < 3.8 --- dvc/system.py | 19 +++++++++++++++---- setup.py | 1 + 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/dvc/system.py b/dvc/system.py index a6fc77bf4..7a6a70250 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -1,7 +1,7 @@ import errno import logging import os -import speedcopy +import platform from dvc.compat import fspath from dvc.exceptions import DvcException @@ -18,7 +18,20 @@ class System(object): @staticmethod def copy(src, dest): src, dest = fspath(src), fspath(dest) - return speedcopy.copyfile(src, dest) + system = platform.system() + if system == "Windows": + import speedcopy + + return speedcopy.copyfile(src, dest) + else: + import shutil + import sys + + if sys.version_info < (3, 8): + # Importing the module monkey-patches shutil.copyfile + import pyfastcopy # noqa: F401 + + return shutil.copyfile(src, dest) @staticmethod def hardlink(source, link_name): @@ -90,8 +103,6 @@ class System(object): @staticmethod def reflink(source, link_name): - import platform - source, link_name = fspath(source), fspath(link_name) system = platform.system() diff --git a/setup.py b/setup.py index 5008f9ce3..13792fbc4 100644 --- a/setup.py +++ b/setup.py @@ -76,6 +76,7 @@ install_requires = [ "pywin32>=225; sys_platform == 'win32'", "networkx>=2.1,<2.4", "speedcopy @ git+https://github.com/rxxg/speedcopy@5", + "pyfastcopy>=1.0.3", ] From ebb0243e8748fff61dce7eafd184585997a6d808 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 14 Jan 2020 11:15:11 +0200 Subject: [PATCH 12/78] setup: rollback pydrive dep pypi doesn't allow direct github deps https://travis-ci.com/iterative/dvc/jobs/275282583 --- setup.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/setup.py b/setup.py index f8c4fda59..cb5abd6cc 100644 --- a/setup.py +++ b/setup.py @@ -84,10 +84,7 @@ gs = ["google-cloud-storage==1.19.0"] # google-api-python-client is internal dependency of pydrive. After merge of # https://github.com/gsuitedevs/PyDrive/pull/180 into pydrive's master, # usage of google-api-python-client can be removed from DVC. -gdrive = [ - "pydrive @ git+https://github.com/gsuitedevs/PyDrive@master#egg=pydrive", - "google-api-python-client>=1.2", -] +gdrive = ["pydrive>=1.3.1, "google-api-python-client>=1.2"] s3 = ["boto3>=1.9.201"] azure = ["azure-storage-blob==2.1.0"] oss = ["oss2==2.6.1"] From 8da898d483b79cb1c98479d640272fd1e26b5b8e Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 14 Jan 2020 11:15:36 +0200 Subject: [PATCH 13/78] dvc: bump to 0.81.2 --- dvc/version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/version.py b/dvc/version.py index 7e0719fc0..4c0ccdb01 100644 --- a/dvc/version.py +++ b/dvc/version.py @@ -7,7 +7,7 @@ import os import subprocess -_BASE_VERSION = "0.81.1" +_BASE_VERSION = "0.81.2" def _generate_version(base_version): From f18bfdc674c5d2b9261a579a2742b76a8cb79711 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 14 Jan 2020 11:22:29 +0200 Subject: [PATCH 14/78] setup: fix typo --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index cb5abd6cc..d5fa6fe63 100644 --- a/setup.py +++ b/setup.py @@ -84,7 +84,7 @@ gs = ["google-cloud-storage==1.19.0"] # google-api-python-client is internal dependency of pydrive. After merge of # https://github.com/gsuitedevs/PyDrive/pull/180 into pydrive's master, # usage of google-api-python-client can be removed from DVC. -gdrive = ["pydrive>=1.3.1, "google-api-python-client>=1.2"] +gdrive = ["pydrive>=1.3.1", "google-api-python-client>=1.2"] s3 = ["boto3>=1.9.201"] azure = ["azure-storage-blob==2.1.0"] oss = ["oss2==2.6.1"] From 482473aa69fd97ebde8bdd2ae23b0c703158f765 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 14 Jan 2020 11:23:10 +0200 Subject: [PATCH 15/78] dvc: bump to 0.81.3 --- dvc/version.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/version.py b/dvc/version.py index 4c0ccdb01..a24e36253 100644 --- a/dvc/version.py +++ b/dvc/version.py @@ -7,7 +7,7 @@ import os import subprocess -_BASE_VERSION = "0.81.2" +_BASE_VERSION = "0.81.3" def _generate_version(base_version): From de9ff82b9c74cdb58577f6d428ad0366603cde6f Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 13:40:44 +0100 Subject: [PATCH 16/78] tests: local: unit: migrate to dir helpers --- tests/unit/output/test_local.py | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/tests/unit/output/test_local.py b/tests/unit/output/test_local.py index 1e64d32ac..50059e6ea 100644 --- a/tests/unit/output/test_local.py +++ b/tests/unit/output/test_local.py @@ -23,27 +23,27 @@ class TestOutputLOCAL(TestDvc): o.save() -def test_str_workdir_outside_repo(erepo): - stage = Stage(erepo.dvc) +def test_str_workdir_outside_repo(erepo_dir): + stage = Stage(erepo_dir.dvc) output = OutputLOCAL(stage, "path", cache=False) - assert relpath("path", erepo.dvc.root_dir) == str(output) + assert relpath("path", erepo_dir.dvc.root_dir) == str(output) -def test_str_workdir_inside_repo(dvc_repo): - stage = Stage(dvc_repo) +def test_str_workdir_inside_repo(dvc): + stage = Stage(dvc) output = OutputLOCAL(stage, "path", cache=False) assert "path" == str(output) - stage = Stage(dvc_repo, wdir="some_folder") + stage = Stage(dvc, wdir="some_folder") output = OutputLOCAL(stage, "path", cache=False) assert os.path.join("some_folder", "path") == str(output) -def test_str_on_absolute_path(dvc_repo): - stage = Stage(dvc_repo) +def test_str_on_absolute_path(dvc): + stage = Stage(dvc) path = os.path.abspath(os.path.join("path", "to", "file")) output = OutputLOCAL(stage, path, cache=False) From 8e756f04521c0aedd54a4f0f4767b6a01baeae90 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 13:51:40 +0100 Subject: [PATCH 17/78] stage: tests: unit: migrate to dir helpers --- tests/unit/test_stage.py | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/test_stage.py b/tests/unit/test_stage.py index ce23fad52..298f3fd51 100644 --- a/tests/unit/test_stage.py +++ b/tests/unit/test_stage.py @@ -93,14 +93,14 @@ def test_stage_update(mocker): not isinstance(threading.current_thread(), threading._MainThread), reason="Not running in the main thread.", ) -def test_stage_run_ignore_sigint(dvc_repo, mocker): +def test_stage_run_ignore_sigint(dvc, mocker): proc = mocker.Mock() communicate = mocker.Mock() proc.configure_mock(returncode=0, communicate=communicate) popen = mocker.patch.object(subprocess, "Popen", return_value=proc) signal_mock = mocker.patch("signal.signal") - dvc_repo.run(cmd="path") + dvc.run(cmd="path") assert popen.called_once() assert communicate.called_once_with() @@ -108,9 +108,9 @@ def test_stage_run_ignore_sigint(dvc_repo, mocker): assert signal.getsignal(signal.SIGINT) == signal.default_int_handler -def test_always_changed(dvc_repo): - stage = Stage(dvc_repo, "path", always_changed=True) +def test_always_changed(dvc): + stage = Stage(dvc, "path", always_changed=True) stage.save() - with dvc_repo.lock: + with dvc.lock: assert stage.changed() assert stage.status()["path"] == ["always changed"] From cabe619d97c60f8c5a9d7c34f6da645958f0ddcb Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 13:53:25 +0100 Subject: [PATCH 18/78] update: tests: unit: migrate to dir helpers --- tests/unit/command/test_update.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/unit/command/test_update.py b/tests/unit/command/test_update.py index c44def6fa..677b24a5a 100644 --- a/tests/unit/command/test_update.py +++ b/tests/unit/command/test_update.py @@ -2,7 +2,7 @@ from dvc.cli import parse_args from dvc.command.update import CmdUpdate -def test_update(dvc_repo, mocker): +def test_update(dvc, mocker): targets = ["target1", "target2", "target3"] cli_args = parse_args(["update"] + targets) assert cli_args.func == CmdUpdate From ba63900b29e2c766b749beebc400aced317090e5 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 13:43:11 +0100 Subject: [PATCH 19/78] tests: repro: unit: migrate to dir helpers --- tests/unit/command/test_repro.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/unit/command/test_repro.py b/tests/unit/command/test_repro.py index 9dc66dc4f..dcdfdbe74 100644 --- a/tests/unit/command/test_repro.py +++ b/tests/unit/command/test_repro.py @@ -15,14 +15,14 @@ default_arguments = { } -def test_default_arguments(dvc_repo, mocker): +def test_default_arguments(dvc, mocker): cmd = CmdRepro(parse_args(["repro"])) mocker.patch.object(cmd.repo, "reproduce") cmd.run() cmd.repo.reproduce.assert_called_with("Dvcfile", **default_arguments) -def test_downstream(dvc_repo, mocker): +def test_downstream(dvc, mocker): cmd = CmdRepro(parse_args(["repro", "--downstream"])) mocker.patch.object(cmd.repo, "reproduce") cmd.run() From 26c484515788c4d0d083085a9e34070b7a6a3d71 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 13:45:35 +0100 Subject: [PATCH 20/78] tests: run: unit: migrate to dir helpers --- tests/unit/command/test_run.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/tests/unit/command/test_run.py b/tests/unit/command/test_run.py index 6e1f3ba77..f11b8e7a1 100644 --- a/tests/unit/command/test_run.py +++ b/tests/unit/command/test_run.py @@ -2,7 +2,7 @@ from dvc.cli import parse_args from dvc.command.run import CmdRun -def test_run(mocker, dvc_repo): +def test_run(mocker, dvc): cli_args = parse_args( [ "run", @@ -64,7 +64,7 @@ def test_run(mocker, dvc_repo): ) -def test_run_args_from_cli(mocker, dvc_repo): +def test_run_args_from_cli(mocker, dvc): args = parse_args(["run", "echo", "foo"]) cmd = args.func(args) m = mocker.patch.object(cmd.repo, "run", autospec=True) @@ -90,7 +90,7 @@ def test_run_args_from_cli(mocker, dvc_repo): ) -def test_run_args_with_spaces(mocker, dvc_repo): +def test_run_args_with_spaces(mocker, dvc): args = parse_args(["run", "echo", "foo bar"]) cmd = args.func(args) m = mocker.patch.object(cmd.repo, "run", autospec=True) From 37b9d32e04fb5d2228d71f135860a73ac5f03ad3 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Tue, 14 Jan 2020 15:09:43 +0545 Subject: [PATCH 21/78] test: refactor: use helper remotes in TestCases --- tests/func/test_data_cloud.py | 97 ++++++++--------------------------- tests/remotes.py | 8 +-- 2 files changed, 26 insertions(+), 79 deletions(-) diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index cfe0febfc..9da9b1189 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -77,17 +77,19 @@ class TestDataCloudBase(TestDvc): def _get_cloud_class(self): return None - def _should_test(self): + @staticmethod + def should_test(): return False - def _get_url(self): - return "" + @staticmethod + def get_url(): + return NotImplementedError def _get_keyfile(self): return None def _ensure_should_run(self): - if not self._should_test(): + if not self.should_test(): raise SkipTest( "Test {} is disabled".format(self.__class__.__name__) ) @@ -95,7 +97,7 @@ class TestDataCloudBase(TestDvc): def _setup_cloud(self): self._ensure_should_run() - repo = self._get_url() + repo = self.get_url() keyfile = self._get_keyfile() config = copy.deepcopy(TEST_CONFIG) @@ -188,25 +190,16 @@ class TestDataCloudBase(TestDvc): self._test_cloud() -class TestRemoteS3(TestDataCloudBase): - def _should_test(self): - return S3.should_test() - - def _get_url(self): - return S3.get_url() - +class TestRemoteS3(S3, TestDataCloudBase): def _get_cloud_class(self): return RemoteS3 -class TestRemoteGDrive(TestDataCloudBase): - def _should_test(self): - return GDrive.should_test() - +class TestRemoteGDrive(GDrive, TestDataCloudBase): def _setup_cloud(self): self._ensure_should_run() - repo = self._get_url() + repo = self.get_url() config = copy.deepcopy(TEST_CONFIG) config[TEST_SECTION][Config.SECTION_REMOTE_URL] = repo @@ -221,21 +214,15 @@ class TestRemoteGDrive(TestDataCloudBase): self.assertIsInstance(self.cloud.get_remote(), self._get_cloud_class()) - def _get_url(self): - return GDrive.get_url() - def _get_cloud_class(self): return RemoteGDrive -class TestRemoteGS(TestDataCloudBase): - def _should_test(self): - return GCP.should_test() - +class TestRemoteGS(GCP, TestDataCloudBase): def _setup_cloud(self): self._ensure_should_run() - repo = self._get_url() + repo = self.get_url() config = copy.deepcopy(TEST_CONFIG) config[TEST_SECTION][Config.SECTION_REMOTE_URL] = repo @@ -247,53 +234,27 @@ class TestRemoteGS(TestDataCloudBase): self.assertIsInstance(self.cloud.get_remote(), self._get_cloud_class()) - def _get_url(self): - return GCP.get_url() - def _get_cloud_class(self): return RemoteGS -class TestRemoteAZURE(TestDataCloudBase): - def _should_test(self): - return Azure.should_test() - - def _get_url(self): - return Azure.get_url() - +class TestRemoteAZURE(Azure, TestDataCloudBase): def _get_cloud_class(self): return RemoteAZURE -class TestRemoteOSS(TestDataCloudBase): - def _should_test(self): - return OSS.should_test() - - def _get_url(self): - return OSS.get_url() - +class TestRemoteOSS(OSS, TestDataCloudBase): def _get_cloud_class(self): return RemoteOSS -class TestRemoteLOCAL(TestDataCloudBase): - def _should_test(self): - return Local.should_test() - - def _get_url(self): - self.dname = Local.get_url() - return self.dname - +class TestRemoteLOCAL(Local, TestDataCloudBase): def _get_cloud_class(self): return RemoteLOCAL - def test(self): - super().test() - self.assertTrue(os.path.isdir(self.dname)) - @pytest.mark.usefixtures("ssh_server") -class TestRemoteSSHMocked(TestDataCloudBase): +class TestRemoteSSHMocked(SSHMocked, TestDataCloudBase): @pytest.fixture(autouse=True) def setup_method_fixture(self, request, ssh_server): self.ssh_server = ssh_server @@ -302,7 +263,7 @@ class TestRemoteSSHMocked(TestDataCloudBase): def _setup_cloud(self): self._ensure_should_run() - repo = self._get_url() + repo = self.get_url() keyfile = self._get_keyfile() config = copy.deepcopy(TEST_CONFIG) @@ -314,27 +275,18 @@ class TestRemoteSSHMocked(TestDataCloudBase): self.assertIsInstance(self.cloud.get_remote(), self._get_cloud_class()) - def _get_url(self): + def get_url(self): user = self.ssh_server.test_creds["username"] - return SSHMocked.get_url(user, self.ssh_server.port) + return super().get_url(user, self.ssh_server.port) def _get_keyfile(self): return self.ssh_server.test_creds["key_filename"] - def _should_test(self): - return SSHMocked.should_test() - def _get_cloud_class(self): return RemoteSSH -class TestRemoteHDFS(TestDataCloudBase): - def _should_test(self): - return HDFS.should_test() - - def _get_url(self): - return HDFS.get_url() - +class TestRemoteHDFS(HDFS, TestDataCloudBase): def _get_cloud_class(self): return RemoteHDFS @@ -571,18 +523,11 @@ class TestWarnOnOutdatedStage(TestDvc): self._test() -class TestRecursiveSyncOperations(TestDataCloudBase): +class TestRecursiveSyncOperations(Local, TestDataCloudBase): def main(self, args): ret = main(args) self.assertEqual(ret, 0) - def _get_url(self): - self.dname = Local.get_url() - return self.dname - - def _should_test(self): - return Local.should_test() - def _get_cloud_class(self): return RemoteLOCAL diff --git a/tests/remotes.py b/tests/remotes.py index cf80fd160..2cf1c19b1 100644 --- a/tests/remotes.py +++ b/tests/remotes.py @@ -42,9 +42,11 @@ TEST_GDRIVE_CLIENT_ID = ( ) TEST_GDRIVE_CLIENT_SECRET = "2fy_HyzSwkxkGzEken7hThXb" +always_test = staticmethod(lambda: True) + class Local: - should_test = lambda: True # noqa: E731 + should_test = always_test @staticmethod def get_storagepath(): @@ -79,7 +81,7 @@ class S3: class S3Mocked(S3): - should_test = lambda: True # noqa: E731 + should_test = always_test @classmethod @contextmanager @@ -219,7 +221,7 @@ class SSH: class SSHMocked: - should_test = lambda: True # noqa: E731 + should_test = always_test @staticmethod def get_url(user, port): From acd60ac1cff2488ab0df9ce823a8b5fe65640b78 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 14:52:31 +0100 Subject: [PATCH 22/78] connection: test: unit: remove repo_dir --- tests/unit/remote/ssh/test_connection.py | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/tests/unit/remote/ssh/test_connection.py b/tests/unit/remote/ssh/test_connection.py index 3f79d9a49..adfada2aa 100644 --- a/tests/unit/remote/ssh/test_connection.py +++ b/tests/unit/remote/ssh/test_connection.py @@ -88,7 +88,8 @@ def test_walk(tmp_path, ssh): not in ["xfs", "apfs", "btrfs"], reason="Reflinks only work in specified file systems", ) -def test_reflink(repo_dir, ssh): +def test_reflink(tmp_dir, ssh): + tmp_dir.gen("foo", "foo content") ssh.reflink("foo", "link") assert filecmp.cmp("foo", "link") assert not System.is_symlink("link") @@ -99,7 +100,8 @@ def test_reflink(repo_dir, ssh): platform.system() == "Windows", reason="sftp symlink is not supported on Windows", ) -def test_symlink(repo_dir, ssh): +def test_symlink(tmp_dir, ssh): + tmp_dir.gen("foo", "foo content") ssh.symlink("foo", "link") assert System.is_symlink("link") @@ -108,7 +110,8 @@ def test_symlink(repo_dir, ssh): platform.system() == "Windows", reason="hardlink is temporarily not supported on Windows", ) -def test_hardlink(repo_dir, ssh): +def test_hardlink(tmp_dir, ssh): + tmp_dir.gen("foo", "foo content") ssh.hardlink("foo", "link") assert System.is_hardlink("link") @@ -117,12 +120,14 @@ def test_hardlink(repo_dir, ssh): platform.system() == "Windows", reason="copy is temporarily not supported on Windows", ) -def test_copy(repo_dir, ssh): +def test_copy(tmp_dir, ssh): + tmp_dir.gen("foo", "foo content") ssh.copy("foo", "link") assert filecmp.cmp("foo", "link") -def test_move(repo_dir, ssh): +def test_move(tmp_dir, ssh): + tmp_dir.gen("foo", "foo content") ssh.move("foo", "copy") assert os.path.exists("copy") assert not os.path.exists("foo") From d5290e2a5c507632068439662ef223ad8673ceb2 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 14 Jan 2020 16:21:05 +0200 Subject: [PATCH 23/78] travis: build snap in parallel with other packages --- .travis.yml | 20 ++++++++++++++++++++ 1 file changed, 20 insertions(+) diff --git a/.travis.yml b/.travis.yml index 66be424aa..586829929 100644 --- a/.travis.yml +++ b/.travis.yml @@ -75,6 +75,26 @@ jobs: language: python python: 3.7 script: ./scripts/build_package.sh + # build snap in parallel with other pkgs + - name: Snapcraft snap + addons: + snaps: + - name: snapcraft + channel: stable + confinement: classic + - name: lxd + channel: stable + env: + - SNAPCRAFT_IMAGE_INFO: '{"build_url": "$TRAVIS_BUILD_URL"}' + before_install: + - sudo /snap/bin/lxd.migrate -yes + - sudo /snap/bin/lxd waitready + - sudo /snap/bin/lxd init --auto + install: + script: + - sudo snapcraft --use-lxd + after_failure: + - sudo journalctl -u snapd # deploy jobs - name: "Pypi pkgs" if: tag is present From 144f10d8cb19daf68b67a15259fc8388d61aaf3b Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 15:25:55 +0100 Subject: [PATCH 24/78] fs: tests: remove repo_dir --- tests/unit/utils/test_fs.py | 67 +++++++++++++++++++++++-------------- 1 file changed, 41 insertions(+), 26 deletions(-) diff --git a/tests/unit/utils/test_fs.py b/tests/unit/utils/test_fs.py index fbde5fcdf..0c0e19a0b 100644 --- a/tests/unit/utils/test_fs.py +++ b/tests/unit/utils/test_fs.py @@ -6,6 +6,7 @@ import pytest from mock import patch import dvc +from dvc.compat import fspath from dvc.ignore import CleanTree from dvc.path_info import PathInfo from dvc.scm.tree import WorkingTree @@ -120,26 +121,33 @@ def test_relpath_windows_different_drives(): 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) +def test_get_inode(tmp_dir): + tmp_dir.gen("foo", "foo content") + + assert get_inode("foo") == get_inode(PathInfo("foo")) -@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 -): - tree = CleanTree(WorkingTree(repo_dir.root_dir)) - time, size = get_mtime_and_size(path, tree) - object_time, object_size = get_mtime_and_size(PathInfo(path), tree) +def test_path_object_and_str_are_valid_types_get_mtime_and_size(tmp_dir): + tmp_dir.gen( + {"dir": {"dir_file": "dir file content"}, "file": "file_content"} + ) + tree = CleanTree(WorkingTree(tmp_dir)) + + time, size = get_mtime_and_size("dir", tree) + object_time, object_size = get_mtime_and_size(PathInfo("dir"), tree) + assert time == object_time + assert size == object_size + + time, size = get_mtime_and_size("file", tree) + object_time, object_size = get_mtime_and_size(PathInfo("file"), tree) assert time == object_time assert size == object_size -def test_move(repo_dir): - src = repo_dir.FOO - src_info = PathInfo(repo_dir.BAR) +def test_move(tmp_dir): + tmp_dir.gen({"foo": "foo content", "bar": "bar content"}) + src = "foo" + src_info = PathInfo("bar") dest = os.path.join("some", "directory") dest_info = PathInfo(os.path.join("some", "path-like", "directory")) @@ -156,9 +164,10 @@ def test_move(repo_dir): assert len(os.listdir(dest_info.fspath)) == 1 -def test_remove(repo_dir): - path = repo_dir.FOO - path_info = PathInfo(repo_dir.BAR) +def test_remove(tmp_dir): + tmp_dir.gen({"foo": "foo content", "bar": "bar content"}) + path = "foo" + path_info = PathInfo("bar") remove(path) assert not os.path.isfile(path) @@ -208,11 +217,9 @@ def test_path_isin_with_absolute_path(): assert path_isin(child, parent) -def test_makedirs(repo_dir): - path = os.path.join(repo_dir.root_dir, "directory") - path_info = PathInfo( - os.path.join(repo_dir.root_dir, "another", "directory") - ) +def test_makedirs(tmp_dir): + path = os.path.join(fspath(tmp_dir), "directory") + path_info = PathInfo(os.path.join(fspath(tmp_dir), "another", "directory")) makedirs(path) assert os.path.isdir(path) @@ -221,11 +228,19 @@ def test_makedirs(repo_dir): assert os.path.isdir(path_info.fspath) -@pytest.mark.parametrize("path", [TestDir.DATA, TestDir.DATA_DIR]) -def test_copyfile(path, repo_dir): - src = repo_dir.FOO +@pytest.mark.parametrize("path", ["file", "dir"]) +def test_copyfile(path, tmp_dir): + tmp_dir.gen( + { + "foo": "foo content", + "bar": "bar content", + "file": "file content", + "dir": {}, + } + ) + src = "foo" dest = path - src_info = PathInfo(repo_dir.BAR) + src_info = PathInfo("bar") dest_info = PathInfo(path) copyfile(src, dest) From b0f83f1486b6b338e6251afdfc977f2279afbf83 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 14 Jan 2020 16:28:03 +0200 Subject: [PATCH 25/78] travis: deploy snap on tag or master --- .travis.yml | 1 + 1 file changed, 1 insertion(+) diff --git a/.travis.yml b/.travis.yml index 586829929..ac0a017f6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -118,6 +118,7 @@ jobs: tags: true repo: iterative/dvc - name: Snapcraft snap + if: tag is present or branch = master stage: deploy addons: snaps: From cb5b5ed334774ef245778cf92b30bb31c8d250c3 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 15:32:48 +0100 Subject: [PATCH 26/78] ssh: test: unit, utils: test: unit: remove repo_dir --- tests/unit/remote/ssh/test_ssh.py | 2 +- tests/unit/utils/test_utils.py | 8 ++++---- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tests/unit/remote/ssh/test_ssh.py b/tests/unit/remote/ssh/test_ssh.py index 3c438bfb1..ce25665eb 100644 --- a/tests/unit/remote/ssh/test_ssh.py +++ b/tests/unit/remote/ssh/test_ssh.py @@ -186,7 +186,7 @@ def test_ssh_gss_auth(mock_file, mock_exists, config, expected_gss_auth): assert remote.gss_auth == expected_gss_auth -def test_hardlink_optimization(repo_dir, ssh_server): +def test_hardlink_optimization(tmp_dir, ssh_server): port = ssh_server.test_creds["port"] user = ssh_server.test_creds["username"] diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index c4849ae30..e53c8df21 100644 --- a/tests/unit/utils/test_utils.py +++ b/tests/unit/utils/test_utils.py @@ -78,10 +78,10 @@ def test_fix_env_pyenv(path, orig): assert fix_env(env)["PATH"] == orig -def test_file_md5(repo_dir): - fname = repo_dir.FOO - fname_object = PathInfo(fname) - assert file_md5(fname) == file_md5(fname_object) +def test_file_md5(tmp_dir): + tmp_dir.gen("foo", "foo content") + + assert file_md5("foo") == file_md5(PathInfo("foo")) def test_tmp_fname(): From 45b97b14c473a71de6547601f3fd985a61bcbff2 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Tue, 14 Jan 2020 20:29:29 +0545 Subject: [PATCH 27/78] test: use helper remotes in child of TestDataCloudCLIBase --- tests/func/test_data_cloud.py | 59 ++++++++++++++--------------------- 1 file changed, 23 insertions(+), 36 deletions(-) diff --git a/tests/func/test_data_cloud.py b/tests/func/test_data_cloud.py index 9da9b1189..748a7fe63 100644 --- a/tests/func/test_data_cloud.py +++ b/tests/func/test_data_cloud.py @@ -296,6 +296,14 @@ class TestDataCloudCLIBase(TestDvc): ret = main(args) self.assertEqual(ret, 0) + @staticmethod + def should_test(): + return False + + @staticmethod + def get_url(): + raise NotImplementedError + def _test_cloud(self, remote=None): args = ["-v", "-j", "2"] if remote: @@ -356,59 +364,47 @@ class TestDataCloudCLIBase(TestDvc): self.assertTrue(os.path.isfile(self.FOO)) self.assertTrue(os.path.isdir(self.DATA_DIR)) - def _should_test(self): - return True - def _test(self): pass def test(self): - if not self._should_test(): + if not self.should_test(): raise SkipTest( "Test {} is disabled".format(self.__class__.__name__) ) self._test() -class TestRemoteLOCALCLI(TestDataCloudCLIBase): +class TestRemoteLOCALCLI(Local, TestDataCloudCLIBase): def _test(self): - url = Local.get_url() + url = self.get_url() self.main(["remote", "add", TEST_REMOTE, url]) self._test_cloud(TEST_REMOTE) -class TestRemoteHDFSCLI(TestDataCloudCLIBase): - def _should_test(self): - return HDFS.should_test() - +class TestRemoteHDFSCLI(HDFS, TestDataCloudCLIBase): def _test(self): - url = HDFS.get_url() + url = self.get_url() self.main(["remote", "add", TEST_REMOTE, url]) self._test_cloud(TEST_REMOTE) -class TestRemoteS3CLI(TestDataCloudCLIBase): - def _should_test(self): - return S3.should_test() - +class TestRemoteS3CLI(S3, TestDataCloudCLIBase): def _test(self): - url = S3.get_url() + url = self.get_url() self.main(["remote", "add", TEST_REMOTE, url]) self._test_cloud(TEST_REMOTE) -class TestRemoteGDriveCLI(TestDataCloudCLIBase): - def _should_test(self): - return GDrive.should_test() - +class TestRemoteGDriveCLI(GDrive, TestDataCloudCLIBase): def _test(self): - url = GDrive.get_url() + url = self.get_url() self.main(["remote", "add", TEST_REMOTE, url]) self.main( @@ -433,12 +429,9 @@ class TestRemoteGDriveCLI(TestDataCloudCLIBase): self._test_cloud(TEST_REMOTE) -class TestRemoteGSCLI(TestDataCloudCLIBase): - def _should_test(self): - return GCP.should_test() - +class TestRemoteGSCLI(GCP, TestDataCloudCLIBase): def _test(self): - url = GCP.get_url() + url = self.get_url() self.main(["remote", "add", TEST_REMOTE, url]) self.main( @@ -454,24 +447,18 @@ class TestRemoteGSCLI(TestDataCloudCLIBase): self._test_cloud(TEST_REMOTE) -class TestRemoteAZURECLI(TestDataCloudCLIBase): - def _should_test(self): - return Azure.should_test() - +class TestRemoteAZURECLI(Azure, TestDataCloudCLIBase): def _test(self): - url = Azure.get_url() + url = self.get_url() self.main(["remote", "add", TEST_REMOTE, url]) self._test_cloud(TEST_REMOTE) -class TestRemoteOSSCLI(TestDataCloudCLIBase): - def _should_test(self): - return OSS.should_test() - +class TestRemoteOSSCLI(OSS, TestDataCloudCLIBase): def _test(self): - url = OSS.get_url() + url = self.get_url() self.main(["remote", "add", TEST_REMOTE, url]) From 79936000191a7d4b87e484e16bff9719a6ae0f06 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Tue, 14 Jan 2020 23:31:44 +0545 Subject: [PATCH 28/78] test: use helper remotes in external repro --- tests/func/test_repro.py | 33 ++++++++++----------------------- 1 file changed, 10 insertions(+), 23 deletions(-) diff --git a/tests/func/test_repro.py b/tests/func/test_repro.py index 3e3be2349..797e975e2 100644 --- a/tests/func/test_repro.py +++ b/tests/func/test_repro.py @@ -38,6 +38,7 @@ from tests.basic_env import TestDvc from tests.remotes import ( GCP, HDFS, + Local, S3, SSH, SSHMocked, @@ -807,7 +808,8 @@ class TestCmdReproChdir(TestDvc): class TestReproExternalBase(TestDvc): - def should_test(self): + @staticmethod + def should_test(): return False @property @@ -956,10 +958,7 @@ class TestReproExternalBase(TestDvc): @pytest.mark.skipif(os.name == "nt", reason="temporarily disabled on windows") -class TestReproExternalS3(TestReproExternalBase): - def should_test(self): - return S3.should_test() - +class TestReproExternalS3(S3, TestReproExternalBase): @property def scheme(self): return "s3" @@ -976,10 +975,7 @@ class TestReproExternalS3(TestReproExternalBase): s3.put_object(Bucket=bucket, Key=key, Body=body) -class TestReproExternalGS(TestReproExternalBase): - def should_test(self): - return GCP.should_test() - +class TestReproExternalGS(GCP, TestReproExternalBase): @property def scheme(self): return "gs" @@ -997,10 +993,7 @@ class TestReproExternalGS(TestReproExternalBase): bucket.blob(key).upload_from_string(body) -class TestReproExternalHDFS(TestReproExternalBase): - def should_test(self): - return HDFS.should_test() - +class TestReproExternalHDFS(HDFS, TestReproExternalBase): @property def scheme(self): return "hdfs" @@ -1057,12 +1050,9 @@ class TestReproExternalHDFS(TestReproExternalBase): @flaky(max_runs=3, min_passes=1) -class TestReproExternalSSH(TestReproExternalBase): +class TestReproExternalSSH(SSH, TestReproExternalBase): _dir = None - def should_test(self): - return SSH.should_test() - @property def scheme(self): return "ssh" @@ -1070,7 +1060,7 @@ class TestReproExternalSSH(TestReproExternalBase): @property def bucket(self): if not self._dir: - self._dir = TestDvc.mkdtemp() + self._dir = self.mkdtemp() return "{}@127.0.0.1:{}".format(getpass.getuser(), self._dir) def cmd(self, i, o): @@ -1104,10 +1094,10 @@ class TestReproExternalSSH(TestReproExternalBase): fobj.write(body) -class TestReproExternalLOCAL(TestReproExternalBase): +class TestReproExternalLOCAL(Local, TestReproExternalBase): def setUp(self): super().setUp() - self.tmpdir = TestDvc.mkdtemp() + self.tmpdir = self.mkdtemp() ret = main(["config", "cache.type", "hardlink"]) self.assertEqual(ret, 0) self.dvc = DvcRepo(".") @@ -1116,9 +1106,6 @@ class TestReproExternalLOCAL(TestReproExternalBase): def cache_type(self): return "hardlink" - def should_test(self): - return True - @property def cache_scheme(self): return "local" From f949b319762468c38a8611b7a3b3e128d6a4e52d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Tue, 14 Jan 2020 17:54:20 +0000 Subject: [PATCH 29/78] status: implement support for imported files Fixes #2959 --- dvc/repo/status.py | 4 ++-- dvc/stage.py | 6 ++++-- 2 files changed, 6 insertions(+), 4 deletions(-) diff --git a/dvc/repo/status.py b/dvc/repo/status.py index 2e9a7a948..8f1672565 100644 --- a/dvc/repo/status.py +++ b/dvc/repo/status.py @@ -19,7 +19,7 @@ def _local_status(self, targets=None, with_deps=False): stages = self.collect(None, with_deps=with_deps) for stage in stages: - if stage.locked: + if stage.locked and not stage.is_repo_import: logger.warning( "DVC-file '{path}' is locked. Its dependencies are" " not going to be shown in the status output.".format( @@ -27,7 +27,7 @@ def _local_status(self, targets=None, with_deps=False): ) ) - status.update(stage.status()) + status.update(stage.status(for_status_command=True)) return status diff --git a/dvc/stage.py b/dvc/stage.py index 449cb261f..85832720b 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -1006,10 +1006,12 @@ class Stage(object): return ret @rwlocked(read=["deps", "outs"]) - def status(self): + def status(self, for_status_command=False): ret = [] - if not self.locked: + show_import = self.is_repo_import and for_status_command + + if not self.locked or show_import: deps_status = self._status(self.deps) if deps_status: ret.append({"changed deps": deps_status}) From c9b0593220bab3358d76f7f11f682947a1d1b26c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Tue, 14 Jan 2020 18:51:56 +0000 Subject: [PATCH 30/78] status: enable status of files imported from remote git repos --- dvc/dependency/repo.py | 29 +++++++++++++++++++++++------ 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index 57244bc4f..8b1840b7c 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -1,6 +1,7 @@ import copy import os from contextlib import contextmanager +import filecmp from funcy import merge @@ -10,6 +11,7 @@ from dvc.external_repo import external_repo from dvc.exceptions import NotDvcRepoError from dvc.exceptions import OutputNotFoundError from dvc.exceptions import PathMissingError +from dvc.exceptions import NoOutputInExternalRepoError from dvc.utils.fs import fs_copy @@ -46,14 +48,29 @@ class DependencyREPO(DependencyLOCAL): yield repo def status(self): - with self._make_repo() as repo: - current = repo.find_out_by_relpath(self.def_path).info + try: + with self._make_repo() as repo: + current = repo.find_out_by_relpath(self.def_path).info - with self._make_repo(rev_lock=None) as repo: - updated = repo.find_out_by_relpath(self.def_path).info + with self._make_repo(rev_lock=None) as repo: + updated = repo.find_out_by_relpath(self.def_path).info - if current != updated: - return {str(self): "update available"} + if current != updated: + return {str(self): "update available"} + + except NoOutputInExternalRepoError: + url = self.def_repo["url"] + old_clone = cached_clone(url, rev=self.def_repo.get("rev_lock")) + new_clone = cached_clone(url) + old_file = os.path.join(old_clone, self.def_path) + new_file = os.path.join(new_clone, self.def_path) + + if not os.path.exists(old_file) or not os.path.exists(new_file): + raise + + files_differ = not filecmp.cmp(old_file, new_file) + if files_differ: + return {str(self): "update available"} return {} From 8f464c2a071c8b9105957483a91a259f466ee0a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Tue, 14 Jan 2020 19:00:46 +0000 Subject: [PATCH 31/78] status: don't clone more than once for plain files --- dvc/dependency/repo.py | 35 ++++++++++++++--------------------- 1 file changed, 14 insertions(+), 21 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index 8b1840b7c..63553b4a4 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -11,7 +11,6 @@ from dvc.external_repo import external_repo from dvc.exceptions import NotDvcRepoError from dvc.exceptions import OutputNotFoundError from dvc.exceptions import PathMissingError -from dvc.exceptions import NoOutputInExternalRepoError from dvc.utils.fs import fs_copy @@ -48,29 +47,23 @@ class DependencyREPO(DependencyLOCAL): yield repo def status(self): - try: - with self._make_repo() as repo: + with self._make_repo() as repo, self._make_repo( + rev_lock=None + ) as updated_repo: + try: current = repo.find_out_by_relpath(self.def_path).info + updated = updated_repo.find_out_by_relpath(self.def_path).info - with self._make_repo(rev_lock=None) as repo: - updated = repo.find_out_by_relpath(self.def_path).info + has_changed = current != updated + except OutputNotFoundError: + current_path = os.path.join(repo.root_dir, self.def_path) + updated_path = os.path.join( + updated_repo.root_dir, self.def_path + ) + has_changed = not filecmp.cmp(current_path, updated_path) - if current != updated: - return {str(self): "update available"} - - except NoOutputInExternalRepoError: - url = self.def_repo["url"] - old_clone = cached_clone(url, rev=self.def_repo.get("rev_lock")) - new_clone = cached_clone(url) - old_file = os.path.join(old_clone, self.def_path) - new_file = os.path.join(new_clone, self.def_path) - - if not os.path.exists(old_file) or not os.path.exists(new_file): - raise - - files_differ = not filecmp.cmp(old_file, new_file) - if files_differ: - return {str(self): "update available"} + if has_changed: + return {str(self): "update available"} return {} From cfbdccdbede9d3c8f4f0418565ed37efc4cc562b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Tue, 14 Jan 2020 19:49:21 +0000 Subject: [PATCH 32/78] update test --- tests/func/test_update.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/tests/func/test_update.py b/tests/func/test_update.py index 108e32b82..96f70d8f3 100644 --- a/tests/func/test_update.py +++ b/tests/func/test_update.py @@ -19,8 +19,13 @@ def test_update_import(tmp_dir, dvc, erepo_dir): # cli call, so we need to clean the caches to see the changes. clean_repos() - assert dvc.status([stage.path]) == {} + status, = dvc.status([stage.path])["version.dvc"] + changed_dep, = list(status["changed deps"].items()) + assert changed_dep[0].startswith("version ") + assert changed_dep[1] == "update available" + dvc.update(stage.path) + assert dvc.status([stage.path]) == {} assert imported.is_file() From e5d35cb3ce20ab1ea7d0b18ffec8df3417a0f375 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Tue, 14 Jan 2020 20:32:46 +0000 Subject: [PATCH 33/78] reduce cognitive complexity of _local_status --- dvc/repo/status.py | 16 ++++++++++------ 1 file changed, 10 insertions(+), 6 deletions(-) diff --git a/dvc/repo/status.py b/dvc/repo/status.py index 8f1672565..85299d8ff 100644 --- a/dvc/repo/status.py +++ b/dvc/repo/status.py @@ -10,14 +10,9 @@ from . import locked logger = logging.getLogger(__name__) -def _local_status(self, targets=None, with_deps=False): +def _joint_status(stages): status = {} - if targets: - stages = cat(self.collect(t, with_deps=with_deps) for t in targets) - else: - stages = self.collect(None, with_deps=with_deps) - for stage in stages: if stage.locked and not stage.is_repo_import: logger.warning( @@ -32,6 +27,15 @@ def _local_status(self, targets=None, with_deps=False): return status +def _local_status(self, targets=None, with_deps=False): + if targets: + stages = cat(self.collect(t, with_deps=with_deps) for t in targets) + else: + stages = self.collect(None, with_deps=with_deps) + + return _joint_status(stages) + + def _cloud_status( self, targets=None, From 7d6ecd1ccb3580149682d76698d84763810282bd Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Wed, 15 Jan 2020 00:37:47 +0200 Subject: [PATCH 34/78] import: don't forget to set rev_lock for git files Related to #3087 --- dvc/dependency/repo.py | 2 ++ tests/func/test_import.py | 39 +++++++++++++++++++++++++++++++++------ tests/func/test_update.py | 30 +++++++++++++++++++++++++++--- 3 files changed, 62 insertions(+), 9 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index 57244bc4f..3e1878f9c 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -11,6 +11,7 @@ from dvc.exceptions import NotDvcRepoError from dvc.exceptions import OutputNotFoundError from dvc.exceptions import PathMissingError from dvc.utils.fs import fs_copy +from dvc.scm import SCM class DependencyREPO(DependencyLOCAL): @@ -105,6 +106,7 @@ class DependencyREPO(DependencyLOCAL): src_full_path = os.path.join(repo_dir, src_path) dst_full_path = os.path.abspath(to_path) fs_copy(src_full_path, dst_full_path) + self.def_repo[self.PARAM_REV_LOCK] = SCM(repo_dir).get_rev() return True def download(self, to): diff --git a/tests/func/test_import.py b/tests/func/test_import.py index c3a2c7b00..380eaeb88 100644 --- a/tests/func/test_import.py +++ b/tests/func/test_import.py @@ -21,11 +21,15 @@ def test_import(tmp_dir, scm, dvc, erepo_dir): with erepo_dir.chdir(): erepo_dir.dvc_gen("foo", "foo content", commit="create foo") - dvc.imp(fspath(erepo_dir), "foo", "foo_imported") + stage = dvc.imp(fspath(erepo_dir), "foo", "foo_imported") assert os.path.isfile("foo_imported") assert (tmp_dir / "foo_imported").read_text() == "foo content" assert scm.repo.git.check_ignore("foo_imported") + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev_lock": erepo_dir.scm.get_rev(), + } @pytest.mark.parametrize("src_is_dvc", [True, False]) @@ -39,13 +43,17 @@ def test_import_git_file(erepo_dir, tmp_dir, dvc, scm, src_is_dvc): erepo_dir.scm_gen({src: "hello"}, commit="add a regular file") - tmp_dir.dvc.imp(fspath(erepo_dir), src, dst) + stage = tmp_dir.dvc.imp(fspath(erepo_dir), src, dst) assert (tmp_dir / dst).is_file() assert filecmp.cmp( fspath(erepo_dir / src), fspath(tmp_dir / dst), shallow=False ) assert tmp_dir.scm.repo.git.check_ignore(fspath(tmp_dir / dst)) + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev_lock": erepo_dir.scm.get_rev(), + } @pytest.mark.parametrize("src_is_dvc", [True, False]) @@ -59,22 +67,30 @@ def test_import_git_dir(erepo_dir, tmp_dir, dvc, scm, src_is_dvc): erepo_dir.scm_gen({src: {"file.txt": "hello"}}, commit="add a dir") - tmp_dir.dvc.imp(fspath(erepo_dir), src, dst) + stage = dvc.imp(fspath(erepo_dir), src, dst) assert (tmp_dir / dst).is_dir() trees_equal(fspath(erepo_dir / src), fspath(tmp_dir / dst)) assert tmp_dir.scm.repo.git.check_ignore(fspath(tmp_dir / dst)) + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev_lock": erepo_dir.scm.get_rev(), + } def test_import_dir(tmp_dir, scm, dvc, erepo_dir): with erepo_dir.chdir(): erepo_dir.dvc_gen({"dir": {"foo": "foo content"}}, commit="create dir") - dvc.imp(fspath(erepo_dir), "dir", "dir_imported") + stage = dvc.imp(fspath(erepo_dir), "dir", "dir_imported") assert os.path.isdir("dir_imported") trees_equal(fspath(erepo_dir / "dir"), "dir_imported") assert scm.repo.git.check_ignore("dir_imported") + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev_lock": erepo_dir.scm.get_rev(), + } def test_import_non_cached(erepo_dir, tmp_dir, dvc, scm): @@ -90,23 +106,34 @@ def test_import_non_cached(erepo_dir, tmp_dir, dvc, scm): erepo_dir.scm.add([fspath(erepo_dir / src)]) erepo_dir.scm.commit("add a non-cached output") - tmp_dir.dvc.imp(fspath(erepo_dir), src, dst) + stage = tmp_dir.dvc.imp(fspath(erepo_dir), src, dst) assert (tmp_dir / dst).is_file() assert filecmp.cmp( fspath(erepo_dir / src), fspath(tmp_dir / dst), shallow=False ) assert tmp_dir.scm.repo.git.check_ignore(dst) + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev_lock": erepo_dir.scm.get_rev(), + } def test_import_rev(tmp_dir, scm, dvc, erepo_dir): + rev = None with erepo_dir.chdir(), erepo_dir.branch("branch", new=True): erepo_dir.dvc_gen("foo", "foo content", commit="create foo on branch") + rev = erepo_dir.scm.get_rev() - dvc.imp(fspath(erepo_dir), "foo", "foo_imported", rev="branch") + stage = dvc.imp(fspath(erepo_dir), "foo", "foo_imported", rev="branch") assert (tmp_dir / "foo_imported").read_text() == "foo content" assert scm.repo.git.check_ignore("foo_imported") + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev": "branch", + "rev_lock": rev, + } def test_pull_imported_stage(tmp_dir, dvc, erepo_dir): diff --git a/tests/func/test_update.py b/tests/func/test_update.py index 108e32b82..5afbac048 100644 --- a/tests/func/test_update.py +++ b/tests/func/test_update.py @@ -1,19 +1,36 @@ +import pytest + +from dvc.stage import Stage from dvc.compat import fspath from dvc.external_repo import clean_repos -def test_update_import(tmp_dir, dvc, erepo_dir): +@pytest.mark.parametrize("cached", [True, False]) +def test_update_import(tmp_dir, dvc, erepo_dir, cached): + old_rev = None with erepo_dir.branch("branch", new=True), erepo_dir.chdir(): - erepo_dir.dvc_gen("version", "branch", "add version file") + gen = erepo_dir.dvc_gen if cached else erepo_dir.scm_gen + gen("version", "branch", "add version file") + old_rev = erepo_dir.scm.get_rev() stage = dvc.imp(fspath(erepo_dir), "version", "version", rev="branch") imported = tmp_dir / "version" assert imported.is_file() assert imported.read_text() == "branch" + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev": "branch", + "rev_lock": old_rev, + } + new_rev = None with erepo_dir.branch("branch", new=False), erepo_dir.chdir(): - erepo_dir.dvc_gen("version", "updated", "update version content") + gen = erepo_dir.dvc_gen if cached else erepo_dir.scm_gen + gen("version", "updated", "update version content") + new_rev = erepo_dir.scm.get_rev() + + assert old_rev != new_rev # Caching in external repos doesn't see upstream updates within single # cli call, so we need to clean the caches to see the changes. @@ -26,6 +43,13 @@ def test_update_import(tmp_dir, dvc, erepo_dir): assert imported.is_file() assert imported.read_text() == "updated" + stage = Stage.load(dvc, stage.path) + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev": "branch", + "rev_lock": new_rev, + } + def test_update_import_url(tmp_dir, dvc, tmp_path_factory): import_src = tmp_path_factory.mktemp("import_url_source") From 410a8f8aebb72d98ab41090a4f7955d5f0b01cc0 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Wed, 15 Jan 2020 00:55:34 +0200 Subject: [PATCH 35/78] travis: don't deploy snap on PRs --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index ac0a017f6..4bddf32e6 100644 --- a/.travis.yml +++ b/.travis.yml @@ -118,7 +118,7 @@ jobs: tags: true repo: iterative/dvc - name: Snapcraft snap - if: tag is present or branch = master + if: type != pull_request stage: deploy addons: snaps: From 3228b476fab96f16f00bedd5eea83ca455b6c633 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Tue, 14 Jan 2020 22:58:11 +0000 Subject: [PATCH 36/78] compare checksums instead of file bytes --- dvc/dependency/repo.py | 19 +++++++++++++------ 1 file changed, 13 insertions(+), 6 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index 63553b4a4..ec8eb0e84 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -1,7 +1,6 @@ import copy import os from contextlib import contextmanager -import filecmp from funcy import merge @@ -12,6 +11,7 @@ from dvc.exceptions import NotDvcRepoError from dvc.exceptions import OutputNotFoundError from dvc.exceptions import PathMissingError from dvc.utils.fs import fs_copy +from dvc.path_info import PathInfo class DependencyREPO(DependencyLOCAL): @@ -46,6 +46,11 @@ class DependencyREPO(DependencyLOCAL): with external_repo(**merge(self.def_repo, overrides)) as repo: yield repo + def _get_checksum_in_repo(self, repo): + return repo.cache.local.get_checksum( + PathInfo(os.path.join(repo.root_dir, self.def_path)) + ) + def status(self): with self._make_repo() as repo, self._make_repo( rev_lock=None @@ -56,11 +61,13 @@ class DependencyREPO(DependencyLOCAL): has_changed = current != updated except OutputNotFoundError: - current_path = os.path.join(repo.root_dir, self.def_path) - updated_path = os.path.join( - updated_repo.root_dir, self.def_path - ) - has_changed = not filecmp.cmp(current_path, updated_path) + # Need to load the state before calculating checksums + repo.state.load() + updated_repo.state.load() + + has_changed = self._get_checksum_in_repo( + repo + ) != self._get_checksum_in_repo(updated_repo) if has_changed: return {str(self): "update available"} From 799d902c98314367eeac238c8cde864c0142b912 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Tue, 14 Jan 2020 23:41:48 +0000 Subject: [PATCH 37/78] add test that checks that a file can be upgraded from git tracking to DVC tracking --- tests/func/test_update.py | 51 +++++++++++++++++++++++++++++++++++++++ 1 file changed, 51 insertions(+) diff --git a/tests/func/test_update.py b/tests/func/test_update.py index 8d6f71cc6..dd467bc30 100644 --- a/tests/func/test_update.py +++ b/tests/func/test_update.py @@ -56,6 +56,57 @@ def test_update_import(tmp_dir, dvc, erepo_dir, cached): } +def test_update_import_after_remote_updates_to_dvc(tmp_dir, dvc, erepo_dir): + old_rev = None + with erepo_dir.branch("branch", new=True), erepo_dir.chdir(): + erepo_dir.scm_gen("version", "branch", commit="add version file") + old_rev = erepo_dir.scm.get_rev() + + stage = dvc.imp(fspath(erepo_dir), "version", "version", rev="branch") + + imported = tmp_dir / "version" + assert imported.is_file() + assert imported.read_text() == "branch" + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev": "branch", + "rev_lock": old_rev, + } + + new_rev = None + with erepo_dir.branch("branch", new=False), erepo_dir.chdir(): + erepo_dir.scm.repo.index.remove("version") + erepo_dir.dvc_gen("version", "updated") + erepo_dir.scm.add(["version", "version.dvc"]) + erepo_dir.scm.commit("upgrade to DVC tracking") + new_rev = erepo_dir.scm.get_rev() + + assert old_rev != new_rev + + # Caching in external repos doesn't see upstream updates within single + # cli call, so we need to clean the caches to see the changes. + clean_repos() + + status, = dvc.status([stage.path])["version.dvc"] + changed_dep, = list(status["changed deps"].items()) + assert changed_dep[0].startswith("version ") + assert changed_dep[1] == "update available" + + dvc.update(stage.path) + + assert dvc.status([stage.path]) == {} + + assert imported.is_file() + assert imported.read_text() == "updated" + + stage = Stage.load(dvc, stage.path) + assert stage.deps[0].def_repo == { + "url": fspath(erepo_dir), + "rev": "branch", + "rev_lock": new_rev, + } + + def test_update_import_url(tmp_dir, dvc, tmp_path_factory): import_src = tmp_path_factory.mktemp("import_url_source") src = import_src / "file" From 289797e7eb4cc648ed4eaa9aa15f0c8c524d914c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Wed, 15 Jan 2020 11:33:26 +0000 Subject: [PATCH 38/78] can now get whether an item changed, but not when an item changes from non-DVC to DVC --- dvc/dependency/repo.py | 44 ++++++++++++++++++++++++++++++++++++--- tests/func/test_status.py | 35 +++++++++++++++++++++++++++++++ 2 files changed, 76 insertions(+), 3 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index 875544a00..b273e34e3 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -1,6 +1,7 @@ import copy import os from contextlib import contextmanager +import filecmp from funcy import merge @@ -52,7 +53,7 @@ class DependencyREPO(DependencyLOCAL): PathInfo(os.path.join(repo.root_dir, self.def_path)) ) - def status(self): + def _has_changed(self): with self._make_repo() as repo, self._make_repo( rev_lock=None ) as updated_repo: @@ -60,16 +61,53 @@ class DependencyREPO(DependencyLOCAL): current = repo.find_out_by_relpath(self.def_path).info updated = updated_repo.find_out_by_relpath(self.def_path).info - has_changed = current != updated + return current != updated except OutputNotFoundError: # Need to load the state before calculating checksums repo.state.load() updated_repo.state.load() - has_changed = self._get_checksum_in_repo( + return self._get_checksum_in_repo( repo ) != self._get_checksum_in_repo(updated_repo) + def _has_changed_non_dvc(self): + url = self.def_repo[self.PARAM_URL] + rev = self.def_repo.get(self.PARAM_REV) + rev_lock = self.def_repo.get(self.PARAM_REV_LOCK, rev) + + current_repo = cached_clone(url, rev=rev_lock or rev) + updated_repo = cached_clone(url, rev=rev) + + current_path = os.path.join(current_repo, self.def_path) + updated_path = os.path.join(updated_repo, self.def_path) + + if not os.path.exists(current_path): + raise FileNotFoundError(current_path) + + if not os.path.exists(updated_path): + raise FileNotFoundError(updated_path) + + is_dir = os.path.isdir(current_path) + + assert is_dir == os.path.isdir(updated_path) + + if is_dir: + comparison = filecmp.dircmp(current_path, updated_path) + return not ( + comparison.left_only + or comparison.right_only + or comparison.diff_files + ) + + return not filecmp.cmp(current_path, updated_path, shallow=False) + + def status(self): + try: + has_changed = self._has_changed() + except NotDvcRepoError: + has_changed = self._has_changed_non_dvc() + if has_changed: return {str(self): "update available"} diff --git a/tests/func/test_status.py b/tests/func/test_status.py index e4de03852..b122a9e7a 100644 --- a/tests/func/test_status.py +++ b/tests/func/test_status.py @@ -1,9 +1,12 @@ import os +import shutil from mock import patch from dvc.main import main +from dvc.compat import fspath from tests.basic_env import TestDvc +from dvc.external_repo import clean_repos class TestStatus(TestDvc): @@ -23,3 +26,35 @@ class TestStatus(TestDvc): def test_implied_cloud(self, mock_status): main(["status", "--remote", "something"]) mock_status.assert_called() + + +def test_status_non_dvc_repo_import(tmp_dir, dvc, erepo_dir): + with erepo_dir.branch("branch", new=True), erepo_dir.chdir(): + erepo_dir.scm.repo.index.remove([".dvc"], r=True) + shutil.rmtree(".dvc") + erepo_dir.scm_gen("file", "first version") + erepo_dir.scm.add(["file"]) + erepo_dir.scm.commit("first version") + + dvc.imp(fspath(erepo_dir), "file", "file", rev="branch") + + status = dvc.status(["file.dvc"]) + + assert status == {} + + # Caching in external repos doesn't see upstream updates within single + # cli call, so we need to clean the caches to see the changes. + clean_repos() + + with erepo_dir.branch("branch", new=False), erepo_dir.chdir(): + erepo_dir.scm_gen("file", "second_version", commit="update file") + erepo_dir.scm.add(["file"]) + erepo_dir.scm.commit("first version") + + status, = dvc.status(["file.dvc"])["file.dvc"] + + assert status == { + "changed deps": { + "file ({})".format(fspath(erepo_dir)): "update available" + } + } From 8fca139246cc15e7ca0ce772d9e76ca8862b8e56 Mon Sep 17 00:00:00 2001 From: rxxg Date: Wed, 15 Jan 2020 12:50:43 +0100 Subject: [PATCH 39/78] Use released version of speedcopy library --- setup.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/setup.py b/setup.py index 13792fbc4..a3fe8421f 100644 --- a/setup.py +++ b/setup.py @@ -75,7 +75,7 @@ install_requires = [ "win-unicode-console>=0.5; sys_platform == 'win32'", "pywin32>=225; sys_platform == 'win32'", "networkx>=2.1,<2.4", - "speedcopy @ git+https://github.com/rxxg/speedcopy@5", + "speedcopy>=2.0.1", "pyfastcopy>=1.0.3", ] From 008a531535babc4d0a44716cfc9991c8f2677343 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Wed, 15 Jan 2020 12:39:15 +0000 Subject: [PATCH 40/78] support non-DVC repositories --- dvc/dependency/repo.py | 76 ++++++++++++++++++++++++--------------- tests/func/test_status.py | 34 ++++++++++++++++++ tests/func/test_update.py | 45 +++++++++++++++++++++++ 3 files changed, 127 insertions(+), 28 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index b273e34e3..424269d60 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -53,35 +53,37 @@ class DependencyREPO(DependencyLOCAL): PathInfo(os.path.join(repo.root_dir, self.def_path)) ) - def _has_changed(self): - with self._make_repo() as repo, self._make_repo( - rev_lock=None - ) as updated_repo: - try: - current = repo.find_out_by_relpath(self.def_path).info - updated = updated_repo.find_out_by_relpath(self.def_path).info + # fileinfo is a dictionary containing "checksum" and "path" + def _get_fileinfo(self, updated=False): + rev_lock = None + if not updated: + rev_lock = self.def_repo.get(self.PARAM_REV_LOCK) - return current != updated - except OutputNotFoundError: - # Need to load the state before calculating checksums - repo.state.load() - updated_repo.state.load() + try: + with self._make_repo(rev_lock=rev_lock) as repo: + path = os.path.join(repo.root_dir, self.def_path) + try: + checksum = repo.find_out_by_relpath(self.def_path).info[ + "md5" + ] + except OutputNotFoundError: + repo.state.load() + checksum = repo.cache.local.get_checksum(PathInfo(path)) - return self._get_checksum_in_repo( - repo - ) != self._get_checksum_in_repo(updated_repo) + assert checksum - def _has_changed_non_dvc(self): - url = self.def_repo[self.PARAM_URL] - rev = self.def_repo.get(self.PARAM_REV) - rev_lock = self.def_repo.get(self.PARAM_REV_LOCK, rev) + return {"checksum": checksum, "path": path, "repo": repo} - current_repo = cached_clone(url, rev=rev_lock or rev) - updated_repo = cached_clone(url, rev=rev) - - current_path = os.path.join(current_repo, self.def_path) - updated_path = os.path.join(updated_repo, self.def_path) + except NotDvcRepoError: + repo_path = cached_clone( + self.def_repo[self.PARAM_URL], + rev=rev_lock or self.def_repo.get(self.PARAM_REV), + ) + path = os.path.join(repo_path, self.def_path) + return {"checksum": None, "path": path, "repo": None} + @staticmethod + def _paths_changed(current_path, updated_path): if not os.path.exists(current_path): raise FileNotFoundError(current_path) @@ -102,11 +104,29 @@ class DependencyREPO(DependencyLOCAL): return not filecmp.cmp(current_path, updated_path, shallow=False) + def _checkout_if_needed(self, fileinfo): + if os.path.exists(fileinfo["path"]): + return + if "repo" not in fileinfo: + return + + fileinfo["repo"].checkout([fileinfo["path"] + ".dvc"], recursive=True) + def status(self): - try: - has_changed = self._has_changed() - except NotDvcRepoError: - has_changed = self._has_changed_non_dvc() + current_fileinfo = self._get_fileinfo(updated=False) + updated_fileinfo = self._get_fileinfo(updated=True) + print("CURRENT", current_fileinfo, "UPDATED", updated_fileinfo) + + if current_fileinfo["checksum"] and updated_fileinfo["checksum"]: + has_changed = ( + current_fileinfo["checksum"] != updated_fileinfo["checksum"] + ) + else: + self._checkout_if_needed(current_fileinfo) + self._checkout_if_needed(updated_fileinfo) + has_changed = DependencyREPO._paths_changed( + current_fileinfo["path"], updated_fileinfo["path"] + ) if has_changed: return {str(self): "update available"} diff --git a/tests/func/test_status.py b/tests/func/test_status.py index b122a9e7a..1b688803a 100644 --- a/tests/func/test_status.py +++ b/tests/func/test_status.py @@ -3,6 +3,7 @@ import shutil from mock import patch +from dvc.repo import Repo from dvc.main import main from dvc.compat import fspath from tests.basic_env import TestDvc @@ -58,3 +59,36 @@ def test_status_non_dvc_repo_import(tmp_dir, dvc, erepo_dir): "file ({})".format(fspath(erepo_dir)): "update available" } } + + +def test_status_before_and_after_dvc_init(tmp_dir, dvc, erepo_dir): + with erepo_dir.chdir(): + erepo_dir.scm.repo.index.remove([".dvc"], r=True) + shutil.rmtree(".dvc") + erepo_dir.scm_gen("file", "first version") + erepo_dir.scm.add(["file"]) + erepo_dir.scm.commit("first version") + old_rev = erepo_dir.scm.get_rev() + + dvc.imp(fspath(erepo_dir), "file", "file") + + assert dvc.status(["file.dvc"]) == {} + + with erepo_dir.chdir(): + Repo.init() + erepo_dir.scm.repo.index.remove(["file"]) + os.remove("file") + erepo_dir.dvc_gen("file", "second version") + erepo_dir.scm.add([".dvc", "file.dvc"]) + erepo_dir.scm.commit("version with dvc") + new_rev = erepo_dir.scm.get_rev() + + assert old_rev != new_rev + + # Caching in external repos doesn't see upstream updates within single + # cli call, so we need to clean the caches to see the changes. + clean_repos() + + status = dvc.status(["file.dvc"]) + + print("STATUS", status) diff --git a/tests/func/test_update.py b/tests/func/test_update.py index dd467bc30..6f483fff0 100644 --- a/tests/func/test_update.py +++ b/tests/func/test_update.py @@ -1,5 +1,8 @@ import pytest +import os +import shutil +from dvc.repo import Repo from dvc.stage import Stage from dvc.compat import fspath from dvc.external_repo import clean_repos @@ -107,6 +110,48 @@ def test_update_import_after_remote_updates_to_dvc(tmp_dir, dvc, erepo_dir): } +def test_update_before_and_after_dvc_init(tmp_dir, dvc, erepo_dir): + with erepo_dir.chdir(): + erepo_dir.scm.repo.index.remove([".dvc"], r=True) + shutil.rmtree(".dvc") + erepo_dir.scm_gen("file", "first version") + erepo_dir.scm.add(["file"]) + erepo_dir.scm.commit("first version") + old_rev = erepo_dir.scm.get_rev() + + stage = dvc.imp(fspath(erepo_dir), "file", "file") + + with erepo_dir.chdir(): + Repo.init() + erepo_dir.scm.repo.index.remove(["file"]) + os.remove("file") + erepo_dir.dvc_gen("file", "second version") + erepo_dir.scm.add([".dvc", "file.dvc"]) + erepo_dir.scm.commit("version with dvc") + new_rev = erepo_dir.scm.get_rev() + + assert old_rev != new_rev + + # Caching in external repos doesn't see upstream updates within single + # cli call, so we need to clean the caches to see the changes. + clean_repos() + + assert dvc.status([stage.path]) == { + "file.dvc": [ + { + "changed deps": { + "file ({})".format(fspath(erepo_dir)): "update available" + } + } + ] + } + + dvc.update(stage.path) + + assert (tmp_dir / "file").read_text() == "second version" + assert dvc.status([stage.path]) == {} + + def test_update_import_url(tmp_dir, dvc, tmp_path_factory): import_src = tmp_path_factory.mktemp("import_url_source") src = import_src / "file" From 4ada384fcd07c69737608eb0918f1080f668e3d7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Wed, 15 Jan 2020 12:43:14 +0000 Subject: [PATCH 41/78] remove trace statement --- dvc/dependency/repo.py | 1 - tests/func/test_status.py | 8 ++++++-- 2 files changed, 6 insertions(+), 3 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index 424269d60..f8ffeb9f6 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -115,7 +115,6 @@ class DependencyREPO(DependencyLOCAL): def status(self): current_fileinfo = self._get_fileinfo(updated=False) updated_fileinfo = self._get_fileinfo(updated=True) - print("CURRENT", current_fileinfo, "UPDATED", updated_fileinfo) if current_fileinfo["checksum"] and updated_fileinfo["checksum"]: has_changed = ( diff --git a/tests/func/test_status.py b/tests/func/test_status.py index 1b688803a..d8a65b9c7 100644 --- a/tests/func/test_status.py +++ b/tests/func/test_status.py @@ -89,6 +89,10 @@ def test_status_before_and_after_dvc_init(tmp_dir, dvc, erepo_dir): # cli call, so we need to clean the caches to see the changes. clean_repos() - status = dvc.status(["file.dvc"]) + status, = dvc.status(["file.dvc"])["file.dvc"] - print("STATUS", status) + assert status == { + "changed deps": { + "file ({})".format(fspath(erepo_dir)): "update available" + } + } From bc9a94b5ee75d6d4d0d4005b70bbdda2c4a676ff Mon Sep 17 00:00:00 2001 From: pawel Date: Mon, 30 Dec 2019 12:49:00 +0100 Subject: [PATCH 42/78] point to troubleshooting guide on too many open files error --- dvc/main.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dvc/main.py b/dvc/main.py index 1a29f5874..991287389 100644 --- a/dvc/main.py +++ b/dvc/main.py @@ -59,7 +59,9 @@ def main(argv=None): except Exception as exc: # pylint: disable=broad-except if isinstance(exc, OSError) and exc.errno == errno.EMFILE: logger.exception( - "too many open files, please increase your `ulimit`", + "too many open files, please visit " + "https://error.dvc.org/many-files to see how to handle this " + "problem", extra={"tb_only": True}, ) else: From d386237d86f6a7cc697bc1c4c351ba4161377923 Mon Sep 17 00:00:00 2001 From: pawel Date: Fri, 3 Jan 2020 09:43:17 +0100 Subject: [PATCH 43/78] main: format link to troubleshooting guide --- dvc/main.py | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/dvc/main.py b/dvc/main.py index 991287389..5c8dc10de 100644 --- a/dvc/main.py +++ b/dvc/main.py @@ -10,11 +10,12 @@ from dvc.exceptions import DvcParserError from dvc.exceptions import NotDvcRepoError from dvc.external_repo import clean_repos from dvc.logger import FOOTER - +from dvc.utils import format_link # 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 + "".encode("idna") @@ -60,8 +61,8 @@ def main(argv=None): if isinstance(exc, OSError) and exc.errno == errno.EMFILE: logger.exception( "too many open files, please visit " - "https://error.dvc.org/many-files to see how to handle this " - "problem", + "{} to see how to handle this " + "problem".format(format_link("https://error.dvc.org/many-files")), extra={"tb_only": True}, ) else: From 0fcbfd04cece4d6adb5160ad3f39315471369dba Mon Sep 17 00:00:00 2001 From: pawel Date: Mon, 13 Jan 2020 16:38:24 +0100 Subject: [PATCH 44/78] main: point to error.dvc.org after too many open files error --- dvc/main.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/dvc/main.py b/dvc/main.py index 5c8dc10de..829908af6 100644 --- a/dvc/main.py +++ b/dvc/main.py @@ -62,7 +62,9 @@ def main(argv=None): logger.exception( "too many open files, please visit " "{} to see how to handle this " - "problem".format(format_link("https://error.dvc.org/many-files")), + "problem".format( + format_link("https://error.dvc.org/many-files") + ), extra={"tb_only": True}, ) else: From 4918c56ca38010193cf7d5bc570ad1ce685ce869 Mon Sep 17 00:00:00 2001 From: Guro Bokum Date: Tue, 14 Jan 2020 10:23:53 +0800 Subject: [PATCH 45/78] remote: config checksum_jobs Close #2920 --- dvc/config.py | 2 ++ dvc/remote/base.py | 19 +++++++++++---- tests/unit/remote/test_gdrive.py | 2 ++ tests/unit/remote/test_remote.py | 41 ++++++++++++++++++++++++++++++++ 4 files changed, 59 insertions(+), 5 deletions(-) create mode 100644 tests/unit/remote/test_remote.py diff --git a/dvc/config.py b/dvc/config.py index ad2070939..0fd312aa1 100644 --- a/dvc/config.py +++ b/dvc/config.py @@ -186,6 +186,7 @@ class Config(object): # pylint: disable=too-many-instance-attributes SECTION_GDRIVE_CLIENT_SECRET = "gdrive_client_secret" SECTION_GDRIVE_USER_CREDENTIALS_FILE = "gdrive_user_credentials_file" + SECTION_REMOTE_CHECKSUM_JOBS = "checksum_jobs" SECTION_REMOTE_REGEX = r'^\s*remote\s*"(?P.*)"\s*$' SECTION_REMOTE_FMT = 'remote "{}"' SECTION_REMOTE_URL = "url" @@ -214,6 +215,7 @@ class Config(object): # pylint: disable=too-many-instance-attributes SECTION_GCP_PROJECTNAME: str, SECTION_CACHE_TYPE: supported_cache_type, Optional(SECTION_CACHE_PROTECTED, default=False): Bool, + SECTION_REMOTE_CHECKSUM_JOBS: All(Coerce(int), Range(1)), SECTION_REMOTE_USER: str, SECTION_REMOTE_PORT: Coerce(int), SECTION_REMOTE_KEY_FILE: str, diff --git a/dvc/remote/base.py b/dvc/remote/base.py index 36409645e..9f47779c8 100644 --- a/dvc/remote/base.py +++ b/dvc/remote/base.py @@ -86,11 +86,7 @@ class RemoteBASE(object): self.repo = repo self._check_requires(config) - - core = config.get(Config.SECTION_CORE, {}) - self.checksum_jobs = core.get( - Config.SECTION_CORE_CHECKSUM_JOBS, self.CHECKSUM_JOBS - ) + self.checksum_jobs = self._get_checksum_jobs(config) self.protected = False self.no_traverse = config.get(Config.SECTION_REMOTE_NO_TRAVERSE, True) @@ -142,6 +138,19 @@ class RemoteBASE(object): ).format(url, missing, " ".join(missing), self.scheme) raise RemoteMissingDepsError(msg) + def _get_checksum_jobs(self, config): + checksum_jobs = config.get(Config.SECTION_REMOTE_CHECKSUM_JOBS) + if checksum_jobs: + return checksum_jobs + + if self.repo: + core = self.repo.config.config.get(Config.SECTION_CORE, {}) + return core.get( + Config.SECTION_CORE_CHECKSUM_JOBS, self.CHECKSUM_JOBS + ) + + return self.CHECKSUM_JOBS + def __repr__(self): return "{class_name}: '{path_info}'".format( class_name=type(self).__name__, diff --git a/tests/unit/remote/test_gdrive.py b/tests/unit/remote/test_gdrive.py index bdc895107..09dc7e62b 100644 --- a/tests/unit/remote/test_gdrive.py +++ b/tests/unit/remote/test_gdrive.py @@ -1,6 +1,7 @@ import pytest import os +from dvc.config import Config from dvc.remote.gdrive import ( RemoteGDrive, GDriveAccessTokenRefreshError, @@ -14,6 +15,7 @@ USER_CREDS_MISSED_KEY_ERROR = "{}" class Repo(object): tmp_dir = "" + config = Config() class TestRemoteGDrive(object): diff --git a/tests/unit/remote/test_remote.py b/tests/unit/remote/test_remote.py new file mode 100644 index 000000000..b2fa73aff --- /dev/null +++ b/tests/unit/remote/test_remote.py @@ -0,0 +1,41 @@ +from dvc.remote import Remote + + +def set_config_opts(dvc, commands): + list(map(lambda args: dvc.config.set(*args), commands)) + + +def test_remote_with_checksum_jobs(dvc): + set_config_opts( + dvc, + [ + ('remote "with_checksum_jobs"', "url", "s3://bucket/name"), + ('remote "with_checksum_jobs"', "checksum_jobs", 100), + ("core", "checksum_jobs", 200), + ], + ) + + remote = Remote(dvc, name="with_checksum_jobs") + assert remote.checksum_jobs == 100 + + +def test_remote_without_checksum_jobs(dvc): + set_config_opts( + dvc, + [ + ('remote "without_checksum_jobs"', "url", "s3://bucket/name"), + ("core", "checksum_jobs", "200"), + ], + ) + + remote = Remote(dvc, name="without_checksum_jobs") + assert remote.checksum_jobs == 200 + + +def test_remote_without_checksum_jobs_default(dvc): + set_config_opts( + dvc, [('remote "without_checksum_jobs"', "url", "s3://bucket/name")] + ) + + remote = Remote(dvc, name="without_checksum_jobs") + assert remote.checksum_jobs == remote.CHECKSUM_JOBS From 5bddfa7a9239587cb5fbf93fcd7052d53481bbdc Mon Sep 17 00:00:00 2001 From: Casper da Costa-Luis Date: Wed, 15 Jan 2020 15:10:56 +0000 Subject: [PATCH 46/78] major deploy tidy - fixes #3152 --- .travis.yml | 81 +++++++++++++++++------------------------------------ 1 file changed, 26 insertions(+), 55 deletions(-) diff --git a/.travis.yml b/.travis.yml index 4bddf32e6..23b895392 100644 --- a/.travis.yml +++ b/.travis.yml @@ -13,8 +13,6 @@ stages: - check - test - build - - name: deploy - if: (branch = master OR tag is present) AND repo = iterative/dvc # Travis doesn't support testing python on Mac yet, so we need # to workaround it by installing it directly with brew. # @@ -75,7 +73,6 @@ jobs: language: python python: 3.7 script: ./scripts/build_package.sh - # build snap in parallel with other pkgs - name: Snapcraft snap addons: snaps: @@ -95,57 +92,6 @@ jobs: - sudo snapcraft --use-lxd after_failure: - sudo journalctl -u snapd - # deploy jobs - - name: "Pypi pkgs" - if: tag is present - stage: deploy - language: python - python: 3.7 - script: ./scripts/build_package.sh - deploy: - - provider: pypi - user: - secure: "IOzNfgPoG3ICg3+s4AuZCCnawgd/KqMzeiAB9R0B9Xb6ProPQJp9O3GBZrtg5OJMHdWQQtwULeOh/LWk4sFdXMcX1iNEgm5Tb7DwO7TDtltW14jUgN9AqfY2twj4UIsW/ON8w0P8v36ZoQq1kO6Pfgjijdm3w+9h6rpps8rJ6jXYpVZVO+SSzzijWddanMkEz4GgctZ2r7U5MWlmm1UogF60kEZIZCQykz0yJnhTpP9nMFDKw6Maki/mcfVwTQ/v3RqjzttHei/aaEMGQXYr3TOX8NPYJXerBpjShDZqcgcCfqPzhi7uCwVYaPvRsMMYH5jgtMz/SO3J6ToeGKT0f1F1ihRyCdusg325TMK7Vv8yHJzL4ngeUbb/oqxWtxhLY7f77Lkx6hBwe6ROpTqiQHIfZad5qyIHihKcDcIHo4FwLHVzxR4y+Ds2js5dXwBkUBaEAx0Dmc6ZdgXqxd815GDwv+VVpzXJhtO6qXmKE36F/Oe6eCNG3dk5MdOK4C2DrM9M8MXllqeQtX6JrM2stWlBalAc9Jn4b0xxAoghX3x3G3hNsLc9Uist1VinY2+IcPozusqj66c+Vbi5YngWSbICOjlIcq2GaOlBi3xioGts396442ztf1eLjMVuvykCI3b4Yz/vJYZOZjUvG/6nLfaFQTJLSXkTvI4do3foeY4=" - password: - secure: "ts/3FSPL+w4euXryip9wCJWUqUwuJ2CqHi0nEpYVM10KL0cp9aMkQXN/7C8ih+j/JZ25kaO+tis1nx0JERVnKrg/1F/fJhxHTpakCP4OSIlhKRXDt7yUUHcKKt5QdM2XCKhApptP6RTpWL1vlc36CMrhdvTcMYp7hlDOiWjdhhim24FGQzabL42Ko59ttyyCROqgcnjHqj+WlhJl7iYO+lv2PLOtslpkIYY5EMjgBLuGyOvxTqZsAXtworsASdB2CrsTE6iMRcrM1afgkyBlUqDezoiv9TwfZrZ85oRoAxPB7su4QWK7lEP0wGQV/SDz/RL7c+cNfRaauxUKVcC8I2LfaEpudv8vi205+v/08wzPbS4xSOnf89irUJnyGxUy+bYLoXPPVPMyzcQRAUnrQl+ENCmN62HU+/2pkIyiUJvt0wYYP0vp+3pIbXbYQGmAIMq7nZJd7K3rikfnQIGKI03xZsFe3PEdjzhDI9Ej/tRUbJx4pgg084QuYYca6MSYn/zJ5pqIG4A7ngCSxoxoWZT0duo8PrUWQeT17L73Q86HeEv3OzZhWfEF2fNDN3Qbsu4CllSZnbE7DXgAuysTwTYHS/na6lp7wejOqdRUXrHbidVla9Mwxyc9JdWNWzUDxALvNxAdNbV6l85mO8izy8wjQF6M1O7idqTfjQeR4pc=" - skip_existing: true - file_glob: true - file: - - dist/dvc*.whl - - dist/dvc*.tar.gz - skip_cleanup: true - on: - tags: true - repo: iterative/dvc - - name: Snapcraft snap - if: type != pull_request - stage: deploy - addons: - snaps: - - name: snapcraft - channel: stable - confinement: classic - - name: lxd - channel: stable - env: - - SNAPCRAFT_IMAGE_INFO: '{"build_url": "$TRAVIS_BUILD_URL"}' - before_install: - - sudo /snap/bin/lxd.migrate -yes - - sudo /snap/bin/lxd waitready - - sudo /snap/bin/lxd init --auto - install: - script: - - sudo snapcraft --use-lxd - after_failure: - - sudo journalctl -u snapd - deploy: - - provider: snap - snap: dvc_*.snap - channel: ${SOURCE_TAG:+stable}${SOURCE_TAG:-edge} - skip_cleanup: true - on: - branch: master - repo: iterative/dvc before_install: - bash ./scripts/ci/before_install.sh @@ -157,7 +103,6 @@ script: - bash ./scripts/ci/script.sh deploy: - provider: releases - condition: "[[ $(./scripts/ci/deploy_condition.sh) == 'true' ]]" api_key: secure: nyo17gF4Xl6DFL/KVkMewC805Cv+eK99L7SiYCU+WUAYDIzzFL3YJm7fm8bznPeor7DmpO7uSLSveMnJETBtdurW6JWhCTUss4yD66yFzpk6Z5cvHf/1UA5pdMHkkMM54fRaTRt6L2Yal7RPwGEQTBECLXW1Xmf78Y6+0l8lTvXQ9jXOQSRO/63VDKI5Zww2+XytVicUFlr0hLMGKUuaCjjXcQlAPzBgQz3/5Iz6EtzeTf2o+gTPjocbRbSFPikfiBXUn8XlJ8jBj7i114XJRJffw59KxYGDzxp+enfLBULGNoW0Np2hZqkKkPUIdB08C8XVbRXlFYX8cEYBczjqhv8A05p5F5CnltnqQ0KazcoUP2awPVqn68dWNRF+kqrnMBus8euYKIZ4dy/6bVEKJRa84ESJXZVDcwXTAXRqyovSxOlXqg9ZgAImMKBfKn/fpDX2xIvQPaXcnJxKbmcePfdbC1d0HYJ5jkbNaxKE/T15VvhzIho7mJ2WAv1r8QouQUuzqtpzIGbRn/KXHRUzCtyP0k4+eMKMLFOPzg848WsDarP3v8VYiSy4nFY7ZP7DRd3KttvW5bgVbSeO4jPmTgS6TnnpRwual9BJXSG7bjbfpIMii3bNZ7jWL4zpKT9Jfm8w9RGKLcX9HqkZ3+y60+LtYdhEfBvqsIlWQoIJBXg= file_glob: true @@ -168,6 +113,32 @@ deploy: - dvc*.exe skip_cleanup: true on: + condition: "$(./scripts/ci/deploy_condition.sh)" tags: true repo: iterative/dvc stage: build + - provider: pypi + user: + secure: "IOzNfgPoG3ICg3+s4AuZCCnawgd/KqMzeiAB9R0B9Xb6ProPQJp9O3GBZrtg5OJMHdWQQtwULeOh/LWk4sFdXMcX1iNEgm5Tb7DwO7TDtltW14jUgN9AqfY2twj4UIsW/ON8w0P8v36ZoQq1kO6Pfgjijdm3w+9h6rpps8rJ6jXYpVZVO+SSzzijWddanMkEz4GgctZ2r7U5MWlmm1UogF60kEZIZCQykz0yJnhTpP9nMFDKw6Maki/mcfVwTQ/v3RqjzttHei/aaEMGQXYr3TOX8NPYJXerBpjShDZqcgcCfqPzhi7uCwVYaPvRsMMYH5jgtMz/SO3J6ToeGKT0f1F1ihRyCdusg325TMK7Vv8yHJzL4ngeUbb/oqxWtxhLY7f77Lkx6hBwe6ROpTqiQHIfZad5qyIHihKcDcIHo4FwLHVzxR4y+Ds2js5dXwBkUBaEAx0Dmc6ZdgXqxd815GDwv+VVpzXJhtO6qXmKE36F/Oe6eCNG3dk5MdOK4C2DrM9M8MXllqeQtX6JrM2stWlBalAc9Jn4b0xxAoghX3x3G3hNsLc9Uist1VinY2+IcPozusqj66c+Vbi5YngWSbICOjlIcq2GaOlBi3xioGts396442ztf1eLjMVuvykCI3b4Yz/vJYZOZjUvG/6nLfaFQTJLSXkTvI4do3foeY4=" + password: + secure: "ts/3FSPL+w4euXryip9wCJWUqUwuJ2CqHi0nEpYVM10KL0cp9aMkQXN/7C8ih+j/JZ25kaO+tis1nx0JERVnKrg/1F/fJhxHTpakCP4OSIlhKRXDt7yUUHcKKt5QdM2XCKhApptP6RTpWL1vlc36CMrhdvTcMYp7hlDOiWjdhhim24FGQzabL42Ko59ttyyCROqgcnjHqj+WlhJl7iYO+lv2PLOtslpkIYY5EMjgBLuGyOvxTqZsAXtworsASdB2CrsTE6iMRcrM1afgkyBlUqDezoiv9TwfZrZ85oRoAxPB7su4QWK7lEP0wGQV/SDz/RL7c+cNfRaauxUKVcC8I2LfaEpudv8vi205+v/08wzPbS4xSOnf89irUJnyGxUy+bYLoXPPVPMyzcQRAUnrQl+ENCmN62HU+/2pkIyiUJvt0wYYP0vp+3pIbXbYQGmAIMq7nZJd7K3rikfnQIGKI03xZsFe3PEdjzhDI9Ej/tRUbJx4pgg084QuYYca6MSYn/zJ5pqIG4A7ngCSxoxoWZT0duo8PrUWQeT17L73Q86HeEv3OzZhWfEF2fNDN3Qbsu4CllSZnbE7DXgAuysTwTYHS/na6lp7wejOqdRUXrHbidVla9Mwxyc9JdWNWzUDxALvNxAdNbV6l85mO8izy8wjQF6M1O7idqTfjQeR4pc=" + skip_existing: true + file_glob: true + file: + - dist/dvc*.whl + - dist/dvc*.tar.gz + skip_cleanup: true + on: + condition: "$(ls dist/dvc*.whl dist/dvc*.tar.gz 2>&1)" + tags: true + repo: iterative/dvc + stage: build + - provider: snap + snap: dvc_*.snap + channel: ${SOURCE_TAG:+stable}${SOURCE_TAG:-edge} + skip_cleanup: true + on: + all_branches: true + condition: "$(ls dvc_*.snap 2>&1) && (($TRAVIS_BRANCH = master) || (-n $TRAVIS_TAG))" + repo: iterative/dvc + stage: build From 930b6cf45e033bbaae9c3b7d4c6383e2f11571ea Mon Sep 17 00:00:00 2001 From: Casper da Costa-Luis Date: Wed, 15 Jan 2020 15:16:29 +0000 Subject: [PATCH 47/78] fix pkg deploy condition --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 23b895392..3f556f963 100644 --- a/.travis.yml +++ b/.travis.yml @@ -113,7 +113,7 @@ deploy: - dvc*.exe skip_cleanup: true on: - condition: "$(./scripts/ci/deploy_condition.sh)" + condition: "$(./scripts/ci/deploy_condition.sh) && $(ls dvc*.rpm dvc*.deb dvc*.pkg dvc*.exe 2>&1)" tags: true repo: iterative/dvc stage: build From d6030486985060e3bdcc568abbf03d258914b78c Mon Sep 17 00:00:00 2001 From: Casper da Costa-Luis Date: Wed, 15 Jan 2020 15:20:09 +0000 Subject: [PATCH 48/78] capitalisation --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 3f556f963..48f608a69 100644 --- a/.travis.yml +++ b/.travis.yml @@ -69,7 +69,7 @@ jobs: before_install: install: script: ./scripts/build_posix.sh - - name: "Pypi pkgs" + - name: "PyPI pkgs" language: python python: 3.7 script: ./scripts/build_package.sh From 7b94181a0d0578692178f9f4e4d7503423f81c32 Mon Sep 17 00:00:00 2001 From: Casper da Costa-Luis Date: Wed, 15 Jan 2020 15:26:29 +0000 Subject: [PATCH 49/78] deploy only on push --- .travis.yml | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/.travis.yml b/.travis.yml index 48f608a69..e736d322a 100644 --- a/.travis.yml +++ b/.travis.yml @@ -113,7 +113,7 @@ deploy: - dvc*.exe skip_cleanup: true on: - condition: "$(./scripts/ci/deploy_condition.sh) && $(ls dvc*.rpm dvc*.deb dvc*.pkg dvc*.exe 2>&1)" + condition: "$(./scripts/ci/deploy_condition.sh) && $TRAVIS_EVENT_TYPE = push && $(ls dvc*.rpm dvc*.deb dvc*.pkg dvc*.exe 2>&1)" tags: true repo: iterative/dvc stage: build @@ -129,7 +129,7 @@ deploy: - dist/dvc*.tar.gz skip_cleanup: true on: - condition: "$(ls dist/dvc*.whl dist/dvc*.tar.gz 2>&1)" + condition: "$(ls dist/dvc*.whl dist/dvc*.tar.gz 2>&1) && $TRAVIS_EVENT_TYPE = push" tags: true repo: iterative/dvc stage: build @@ -139,6 +139,6 @@ deploy: skip_cleanup: true on: all_branches: true - condition: "$(ls dvc_*.snap 2>&1) && (($TRAVIS_BRANCH = master) || (-n $TRAVIS_TAG))" + condition: "$(ls dvc_*.snap 2>&1) && ($TRAVIS_BRANCH = master || -n $TRAVIS_TAG) && $TRAVIS_EVENT_TYPE = push" repo: iterative/dvc stage: build From 4b16252c48a9a5fee8f811df98b7f5a553dd9a90 Mon Sep 17 00:00:00 2001 From: Casper da Costa-Luis Date: Wed, 15 Jan 2020 15:38:16 +0000 Subject: [PATCH 50/78] tidy deploy_condition --- .travis.yml | 6 +++--- scripts/ci/deploy_condition.sh | 12 ++++++++++-- 2 files changed, 13 insertions(+), 5 deletions(-) diff --git a/.travis.yml b/.travis.yml index e736d322a..690f28d8d 100644 --- a/.travis.yml +++ b/.travis.yml @@ -113,7 +113,7 @@ deploy: - dvc*.exe skip_cleanup: true on: - condition: "$(./scripts/ci/deploy_condition.sh) && $TRAVIS_EVENT_TYPE = push && $(ls dvc*.rpm dvc*.deb dvc*.pkg dvc*.exe 2>&1)" + condition: "$(./scripts/ci/deploy_condition.sh dvc*.rpm dvc*.deb dvc*.pkg dvc*.exe)" tags: true repo: iterative/dvc stage: build @@ -129,7 +129,7 @@ deploy: - dist/dvc*.tar.gz skip_cleanup: true on: - condition: "$(ls dist/dvc*.whl dist/dvc*.tar.gz 2>&1) && $TRAVIS_EVENT_TYPE = push" + condition: "$(./scripts/ci/deploy_condition.sh dist/dvc*.whl dist/dvc*.tar.gz)" tags: true repo: iterative/dvc stage: build @@ -139,6 +139,6 @@ deploy: skip_cleanup: true on: all_branches: true - condition: "$(ls dvc_*.snap 2>&1) && ($TRAVIS_BRANCH = master || -n $TRAVIS_TAG) && $TRAVIS_EVENT_TYPE = push" + condition: "$(./scripts/ci/deploy_condition.sh dvc_*.snap) && ($TRAVIS_BRANCH = master || -n $TRAVIS_TAG)" repo: iterative/dvc stage: build diff --git a/scripts/ci/deploy_condition.sh b/scripts/ci/deploy_condition.sh index 1446aec2e..d4763d915 100755 --- a/scripts/ci/deploy_condition.sh +++ b/scripts/ci/deploy_condition.sh @@ -3,8 +3,16 @@ set -e if [[ $TRAVIS_OS_NAME == "osx" && $TRAVIS_OSX_IMAGE != "xcode8.3" ]]; then - echo false - exit 0 + exit 1 +fi + +if [[ $TRAVIS_EVENT_TYPE = pull_request || $TRAVIS_EVENT_TPYE = cron ]]; then + exit 2 +fi + +# ensure at least one positional arg exists +if [[ ${#} -ge 1 ]]; then + [[ -n "$(ls $@ 2>/dev/null)" ]] || exit 3 fi echo true From d8e6e34c341728b64d26e5beae7077dd9ad789de Mon Sep 17 00:00:00 2001 From: Casper da Costa-Luis Date: Wed, 15 Jan 2020 16:23:59 +0000 Subject: [PATCH 51/78] minor use of new public API env var --- .travis.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.travis.yml b/.travis.yml index 690f28d8d..9aa2cca1f 100644 --- a/.travis.yml +++ b/.travis.yml @@ -135,7 +135,7 @@ deploy: stage: build - provider: snap snap: dvc_*.snap - channel: ${SOURCE_TAG:+stable}${SOURCE_TAG:-edge} + channel: ${TRAVIS_TAG:+stable}${TRAVIS_TAG:-edge} skip_cleanup: true on: all_branches: true From 97a29a1f7eb82bd50bb83dba978c401ae35c4e2a Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Wed, 15 Jan 2020 19:13:29 +0200 Subject: [PATCH 52/78] dvc: use pydrive2 instead of pydrive PyDrive2 is our maintained fork. --- dvc/remote/gdrive.py | 12 ++++++------ setup.py | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/dvc/remote/gdrive.py b/dvc/remote/gdrive.py index 06e4f0814..8b788c684 100644 --- a/dvc/remote/gdrive.py +++ b/dvc/remote/gdrive.py @@ -34,7 +34,7 @@ class GDriveMissedCredentialKeyError(DvcException): @decorator def _wrap_pydrive_retriable(call): from apiclient import errors - from pydrive.files import ApiRequestError + from pydrive2.files import ApiRequestError try: result = call() @@ -61,7 +61,7 @@ gdrive_retry = compose( class RemoteGDrive(RemoteBASE): scheme = Schemes.GDRIVE path_cls = CloudURLInfo - REQUIRES = {"pydrive": "pydrive"} + REQUIRES = {"pydrive2": "pydrive2"} GDRIVE_USER_CREDENTIALS_DATA = "GDRIVE_USER_CREDENTIALS_DATA" DEFAULT_USER_CREDENTIALS_FILE = "gdrive-user-credentials.json" @@ -177,11 +177,11 @@ class RemoteGDrive(RemoteBASE): @property @wrap_with(threading.RLock()) def drive(self): - from pydrive.auth import RefreshError + from pydrive2.auth import RefreshError if not hasattr(self, "_gdrive"): - from pydrive.auth import GoogleAuth - from pydrive.drive import GoogleDrive + from pydrive2.auth import GoogleAuth + from pydrive2.drive import GoogleDrive if os.getenv(RemoteGDrive.GDRIVE_USER_CREDENTIALS_DATA): with open( @@ -227,7 +227,7 @@ class RemoteGDrive(RemoteBASE): self.gdrive_user_credentials_path, str(exc) ) ) - # Handle pydrive.auth.AuthenticationError and others auth failures + # Handle pydrive2.auth.AuthenticationError and others auth failures except Exception as exc: raise DvcException( "Google Drive authentication failed" diff --git a/setup.py b/setup.py index 38d1574d1..d9d13a4bd 100644 --- a/setup.py +++ b/setup.py @@ -86,7 +86,7 @@ gs = ["google-cloud-storage==1.19.0"] # google-api-python-client is internal dependency of pydrive. After merge of # https://github.com/gsuitedevs/PyDrive/pull/180 into pydrive's master, # usage of google-api-python-client can be removed from DVC. -gdrive = ["pydrive>=1.3.1", "google-api-python-client>=1.2"] +gdrive = ["pydrive2>=1.4.0", "google-api-python-client>=1.2"] s3 = ["boto3>=1.9.201"] azure = ["azure-storage-blob==2.1.0"] oss = ["oss2==2.6.1"] From 2d2ba0addf18e11c9cfd2805cc8b36c78b3da071 Mon Sep 17 00:00:00 2001 From: Max Risuhin Date: Wed, 15 Jan 2020 13:10:55 -0800 Subject: [PATCH 53/78] Fix gdrive multiple parents file list query --- dvc/remote/gdrive.py | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/dvc/remote/gdrive.py b/dvc/remote/gdrive.py index 8b788c684..aa87f1bbd 100644 --- a/dvc/remote/gdrive.py +++ b/dvc/remote/gdrive.py @@ -261,8 +261,11 @@ class RemoteGDrive(RemoteBASE): def get_remote_item(self, name, parents_ids): if not parents_ids: return None - query = " or ".join( - "'{}' in parents".format(parent_id) for parent_id in parents_ids + query = "({})".format( + " or ".join( + "'{}' in parents".format(parent_id) + for parent_id in parents_ids + ) ) query += " and trashed=false and title='{}'".format(name) @@ -331,8 +334,10 @@ class RemoteGDrive(RemoteBASE): if not self.cached_ids: return - query = " or ".join( - "'{}' in parents".format(dir_id) for dir_id in self.cached_ids + query = "({})".format( + " or ".join( + "'{}' in parents".format(dir_id) for dir_id in self.cached_ids + ) ) query += " and trashed=false" From 48326620ec88019642bd3e2ecdf2c8fbf9b93f80 Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Wed, 15 Jan 2020 17:25:35 -0600 Subject: [PATCH 54/78] setup: remove py2 backports --- setup.py | 1 - 1 file changed, 1 deletion(-) diff --git a/setup.py b/setup.py index 38d1574d1..a0edb216e 100644 --- a/setup.py +++ b/setup.py @@ -46,7 +46,6 @@ class build_py(_build_py): install_requires = [ "python-dateutil<2.8.1,>=2.1", # Consolidates azure-blob-storage and boto3 "ply>=3.9", # See https://github.com/pyinstaller/pyinstaller/issues/1945 - "configparser>=3.5.0", "colorama>=0.3.9", "configobj>=5.0.6", "gitpython>=2.1.8", From 388fcd6d325c99a57ca9c30dbc30fecd9844f3ff Mon Sep 17 00:00:00 2001 From: "Mr. Outis" Date: Wed, 15 Jan 2020 21:31:09 -0600 Subject: [PATCH 55/78] api: identify local repos with os.path.exists Close #3111 --- dvc/api.py | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/dvc/api.py b/dvc/api.py index 655594bf1..9d6a321d9 100644 --- a/dvc/api.py +++ b/dvc/api.py @@ -2,7 +2,6 @@ from builtins import open as builtin_open import importlib import os import sys -from urllib.parse import urlparse from contextlib import contextmanager, _GeneratorContextManager as GCM import threading @@ -96,7 +95,7 @@ def read(path, repo=None, rev=None, remote=None, mode="r", encoding=None): @contextmanager def _make_repo(repo_url, rev=None): - if not repo_url or urlparse(repo_url).scheme == "": + if not repo_url or os.path.exists(repo_url): assert rev is None, "Custom revision is not supported for local repo" yield Repo(repo_url) else: From a4b55a00ecc9d9b75b120f86b5eca71b79bba020 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 14:45:26 +0100 Subject: [PATCH 56/78] conftest: remove dvc_repo, erepo, git, git_erepo --- tests/conftest.py | 79 +---------------------------------------------- 1 file changed, 1 insertion(+), 78 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index 7ae4ed571..e147ca7eb 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2,12 +2,9 @@ import os import mockssh import pytest -from git import Repo -from git.exc import GitCommandNotFound from dvc.remote.ssh.connection import SSHConnection -from dvc.repo import Repo as DvcRepo -from .basic_env import TestDirFixture, TestDvcGitFixture, TestGitFixture +from .basic_env import TestDirFixture from .dir_helpers import * # noqa @@ -42,38 +39,6 @@ def repo_dir(): old_fixture.tearDown() -# NOTE: this duplicates code from GitFixture, -# would fix itself once class-based fixtures are removed -@pytest.fixture -def git(repo_dir): - # NOTE: handles EAGAIN error on BSD systems (osx in our case). - # Otherwise when running tests you might get this exception: - # - # GitCommandNotFound: Cmd('git') not found due to: - # OSError('[Errno 35] Resource temporarily unavailable') - retries = 5 - while True: - try: - git = Repo.init() - break - except GitCommandNotFound: - retries -= 1 - if not retries: - raise - - try: - git.index.add([repo_dir.CODE]) - git.index.commit("add code") - yield git - finally: - git.close() - - -@pytest.fixture -def dvc_repo(repo_dir): - yield DvcRepo.init(repo_dir._root_dir, no_scm=True) - - here = os.path.abspath(os.path.dirname(__file__)) user = "user" @@ -98,51 +63,9 @@ def ssh(ssh_server): yield SSHConnection(**ssh_server.test_creds) -@pytest.fixture -def erepo(repo_dir): - repo = TestDvcGitFixture() - repo.setUp() - try: - stage_foo = repo.dvc.add(repo.FOO)[0] - stage_bar = repo.dvc.add(repo.BAR)[0] - stage_data_dir = repo.dvc.add(repo.DATA_DIR)[0] - repo.dvc.scm.add([stage_foo.path, stage_bar.path, stage_data_dir.path]) - repo.dvc.scm.commit("init repo") - - repo.create("version", "master") - repo.dvc.add("version") - repo.dvc.scm.add([".gitignore", "version.dvc"]) - repo.dvc.scm.commit("master") - - repo.dvc.scm.checkout("branch", create_new=True) - os.unlink(os.path.join(repo.root_dir, "version")) - repo.create("version", "branch") - repo.dvc.add("version") - repo.dvc.scm.add([".gitignore", "version.dvc"]) - repo.dvc.scm.commit("branch") - - repo.dvc.scm.checkout("master") - - repo.dvc.scm.close() - repo.git.close() - - os.chdir(repo._saved_dir) - yield repo - finally: - repo.tearDown() - - @pytest.fixture(scope="session", autouse=True) def _close_pools(): from dvc.remote.pool import close_pools yield close_pools() - - -@pytest.fixture -def git_erepo(): - repo = TestGitFixture() - repo.setUp() - yield repo - repo.tearDown() From c2f5e35f1d42461dd0e6682598007e17435de4a7 Mon Sep 17 00:00:00 2001 From: pawel Date: Tue, 14 Jan 2020 15:52:33 +0100 Subject: [PATCH 57/78] conftest: remove repo_dir --- tests/conftest.py | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/tests/conftest.py b/tests/conftest.py index e147ca7eb..6e01739aa 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -4,7 +4,6 @@ import mockssh import pytest from dvc.remote.ssh.connection import SSHConnection -from .basic_env import TestDirFixture from .dir_helpers import * # noqa @@ -28,17 +27,6 @@ def reset_loglevel(request, caplog): yield -# Wrap class like fixture as pytest-like one to avoid code duplication -@pytest.fixture -def repo_dir(): - old_fixture = TestDirFixture() - old_fixture.setUp() - try: - yield old_fixture - finally: - old_fixture.tearDown() - - here = os.path.abspath(os.path.dirname(__file__)) user = "user" From 18446996ab9380657e64fa81fe9bdceb324b05a9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Redzy=C5=84ski?= Date: Thu, 16 Jan 2020 10:43:13 +0100 Subject: [PATCH 58/78] get: tes_get_from_non_dvc_master: capture git logs at INFO --- tests/func/test_get.py | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/tests/func/test_get.py b/tests/func/test_get.py index cdbbe4bbd..d7b272e26 100644 --- a/tests/func/test_get.py +++ b/tests/func/test_get.py @@ -174,7 +174,9 @@ def test_get_from_non_dvc_master(tmp_dir, erepo_dir, caplog): caplog.clear() dst = "file_imported" - with caplog.at_level(logging.INFO, logger="dvc"): + with caplog.at_level(logging.INFO, logger="dvc"), caplog.at_level( + logging.INFO, logger="git" + ): Repo.get(fspath(erepo_dir), "some_file", out=dst, rev="branch") assert caplog.text == "" From 36c64c66db258690265a9c0c7a2a4cb9f63fabd4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Redzy=C5=84ski?= Date: Thu, 16 Jan 2020 11:09:35 +0100 Subject: [PATCH 59/78] test_get_from_non_dvc_master: add comment about git logs --- tests/func/test_get.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/func/test_get.py b/tests/func/test_get.py index d7b272e26..c6bd1adaa 100644 --- a/tests/func/test_get.py +++ b/tests/func/test_get.py @@ -174,8 +174,11 @@ def test_get_from_non_dvc_master(tmp_dir, erepo_dir, caplog): caplog.clear() dst = "file_imported" - with caplog.at_level(logging.INFO, logger="dvc"), caplog.at_level( - logging.INFO, logger="git" + + # removing `git` import in conftest resulted in unexpected logs from + # that package, see https://github.com/iterative/dvc/issues/3167 + with caplog.at_level(logging.INFO, logger="git"), caplog.at_level( + logging.INFO, logger="dvc" ): Repo.get(fspath(erepo_dir), "some_file", out=dst, rev="branch") From a0bdaa4c9e8101666dd6066b01eb4285708bd6e1 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Thu, 16 Jan 2020 12:51:20 +0000 Subject: [PATCH 60/78] use checksum facilities --- dvc/dependency/repo.py | 72 +++++++----------------------------------- dvc/repo/status.py | 2 +- dvc/stage.py | 4 +-- 3 files changed, 14 insertions(+), 64 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index f8ffeb9f6..e137c41a8 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -1,7 +1,6 @@ import copy import os from contextlib import contextmanager -import filecmp from funcy import merge @@ -48,13 +47,8 @@ class DependencyREPO(DependencyLOCAL): with external_repo(**merge(self.def_repo, overrides)) as repo: yield repo - def _get_checksum_in_repo(self, repo): - return repo.cache.local.get_checksum( - PathInfo(os.path.join(repo.root_dir, self.def_path)) - ) - # fileinfo is a dictionary containing "checksum" and "path" - def _get_fileinfo(self, updated=False): + def _get_checksum(self, updated=False): rev_lock = None if not updated: rev_lock = self.def_repo.get(self.PARAM_REV_LOCK) @@ -62,17 +56,14 @@ class DependencyREPO(DependencyLOCAL): try: with self._make_repo(rev_lock=rev_lock) as repo: path = os.path.join(repo.root_dir, self.def_path) + try: - checksum = repo.find_out_by_relpath(self.def_path).info[ - "md5" - ] + return repo.find_out_by_relpath(self.def_path).info["md5"] except OutputNotFoundError: - repo.state.load() - checksum = repo.cache.local.get_checksum(PathInfo(path)) - - assert checksum - - return {"checksum": checksum, "path": path, "repo": repo} + with repo.state: + return self.repo.cache.local.get_checksum( + PathInfo(path) + ) except NotDvcRepoError: repo_path = cached_clone( @@ -80,54 +71,13 @@ class DependencyREPO(DependencyLOCAL): rev=rev_lock or self.def_repo.get(self.PARAM_REV), ) path = os.path.join(repo_path, self.def_path) - return {"checksum": None, "path": path, "repo": None} - - @staticmethod - def _paths_changed(current_path, updated_path): - if not os.path.exists(current_path): - raise FileNotFoundError(current_path) - - if not os.path.exists(updated_path): - raise FileNotFoundError(updated_path) - - is_dir = os.path.isdir(current_path) - - assert is_dir == os.path.isdir(updated_path) - - if is_dir: - comparison = filecmp.dircmp(current_path, updated_path) - return not ( - comparison.left_only - or comparison.right_only - or comparison.diff_files - ) - - return not filecmp.cmp(current_path, updated_path, shallow=False) - - def _checkout_if_needed(self, fileinfo): - if os.path.exists(fileinfo["path"]): - return - if "repo" not in fileinfo: - return - - fileinfo["repo"].checkout([fileinfo["path"] + ".dvc"], recursive=True) + return self.repo.cache.local.get_checksum(PathInfo(path)) def status(self): - current_fileinfo = self._get_fileinfo(updated=False) - updated_fileinfo = self._get_fileinfo(updated=True) + current_checksum = self._get_checksum(updated=False) + updated_checksum = self._get_checksum(updated=True) - if current_fileinfo["checksum"] and updated_fileinfo["checksum"]: - has_changed = ( - current_fileinfo["checksum"] != updated_fileinfo["checksum"] - ) - else: - self._checkout_if_needed(current_fileinfo) - self._checkout_if_needed(updated_fileinfo) - has_changed = DependencyREPO._paths_changed( - current_fileinfo["path"], updated_fileinfo["path"] - ) - - if has_changed: + if current_checksum != updated_checksum: return {str(self): "update available"} return {} diff --git a/dvc/repo/status.py b/dvc/repo/status.py index 85299d8ff..3528e6243 100644 --- a/dvc/repo/status.py +++ b/dvc/repo/status.py @@ -22,7 +22,7 @@ def _joint_status(stages): ) ) - status.update(stage.status(for_status_command=True)) + status.update(stage.status(check_updates=True)) return status diff --git a/dvc/stage.py b/dvc/stage.py index 85832720b..28076bad6 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -1006,10 +1006,10 @@ class Stage(object): return ret @rwlocked(read=["deps", "outs"]) - def status(self, for_status_command=False): + def status(self, check_updates=False): ret = [] - show_import = self.is_repo_import and for_status_command + show_import = self.is_repo_import and check_updates if not self.locked or show_import: deps_status = self._status(self.deps) From 5883d3dabfdaadf6d732b9ed03587e4b8802043e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Thu, 16 Jan 2020 12:54:05 +0000 Subject: [PATCH 61/78] reorder imports --- tests/func/test_status.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/func/test_status.py b/tests/func/test_status.py index d8a65b9c7..8b8d3d2ba 100644 --- a/tests/func/test_status.py +++ b/tests/func/test_status.py @@ -6,8 +6,8 @@ from mock import patch from dvc.repo import Repo from dvc.main import main from dvc.compat import fspath -from tests.basic_env import TestDvc from dvc.external_repo import clean_repos +from tests.basic_env import TestDvc class TestStatus(TestDvc): From a9bdaec63c9c1408ee3873f946fa70949d51e923 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Thu, 16 Jan 2020 13:37:13 +0000 Subject: [PATCH 62/78] rework code path; remove extraneous context generator --- dvc/dependency/repo.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index e137c41a8..be57e0aba 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -47,7 +47,6 @@ class DependencyREPO(DependencyLOCAL): with external_repo(**merge(self.def_repo, overrides)) as repo: yield repo - # fileinfo is a dictionary containing "checksum" and "path" def _get_checksum(self, updated=False): rev_lock = None if not updated: @@ -55,15 +54,11 @@ class DependencyREPO(DependencyLOCAL): try: with self._make_repo(rev_lock=rev_lock) as repo: - path = os.path.join(repo.root_dir, self.def_path) - try: return repo.find_out_by_relpath(self.def_path).info["md5"] except OutputNotFoundError: - with repo.state: - return self.repo.cache.local.get_checksum( - PathInfo(path) - ) + # Fall through to after exception handler + path = os.path.join(repo.root_dir, self.def_path) except NotDvcRepoError: repo_path = cached_clone( @@ -71,7 +66,8 @@ class DependencyREPO(DependencyLOCAL): rev=rev_lock or self.def_repo.get(self.PARAM_REV), ) path = os.path.join(repo_path, self.def_path) - return self.repo.cache.local.get_checksum(PathInfo(path)) + + return self.repo.cache.local.get_checksum(PathInfo(path)) def status(self): current_checksum = self._get_checksum(updated=False) From 4509f82bc32d837fbad89ecf402a07b56e215bdf Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Wed, 15 Jan 2020 20:58:39 +0545 Subject: [PATCH 63/78] get: implement --show-url to display only url/path to remote --- dvc/command/get.py | 26 ++++++++++++++++++++++++++ tests/func/test_get.py | 13 +++++++++++++ tests/unit/command/test_get.py | 14 ++++++++++++++ 3 files changed, 53 insertions(+) diff --git a/dvc/command/get.py b/dvc/command/get.py index 22856f1c0..e01ce4180 100644 --- a/dvc/command/get.py +++ b/dvc/command/get.py @@ -10,7 +10,28 @@ logger = logging.getLogger(__name__) class CmdGet(CmdBaseNoRepo): + def _run_get_url(self): + from dvc.api import get_url + + try: + url = get_url( + self.args.path, repo=self.args.url, rev=self.args.rev + ) + logger.info("{} {}".format(self.args.path, url)) + except DvcException: + logger.exception( + "failed to show url for '{}' from '{}'".format( + self.args.path, self.args.url + ) + ) + return 1 + return 0 + def run(self): + + if self.args.show_url: + return self._run_get_url() + from dvc.repo import Repo try: @@ -55,4 +76,9 @@ def add_parser(subparsers, parent_parser): get_parser.add_argument( "--rev", nargs="?", help="Git revision (e.g. branch, tag, SHA)" ) + get_parser.add_argument( + "--show-url", + action="store_true", + help="Returns path/url to the location in remote for specified path", + ) get_parser.set_defaults(func=CmdGet) diff --git a/tests/func/test_get.py b/tests/func/test_get.py index c6bd1adaa..7ee1ba40f 100644 --- a/tests/func/test_get.py +++ b/tests/func/test_get.py @@ -5,6 +5,7 @@ import pytest from dvc.cache import Cache from dvc.config import Config +from dvc.main import main from dvc.repo.get import GetDVCFileError, PathMissingError from dvc.repo import Repo from dvc.system import System @@ -184,3 +185,15 @@ def test_get_from_non_dvc_master(tmp_dir, erepo_dir, caplog): assert caplog.text == "" assert (tmp_dir / dst).read_text() == "some_contents" + + +def test_get_url(tmp_dir, erepo_dir): + with erepo_dir.chdir(): + erepo_dir.dvc_gen("foo", "foo") + assert main(["get", ".", "foo", "--show-url"]) == 0 + + +def test_get_url_not_existing(tmp_dir, erepo_dir): + with erepo_dir.chdir(): + erepo_dir.dvc_gen("foo", "foo") + assert main(["get", ".", "food", "--show-url"]) == 1 diff --git a/tests/unit/command/test_get.py b/tests/unit/command/test_get.py index 2c464ecc9..6327b4293 100644 --- a/tests/unit/command/test_get.py +++ b/tests/unit/command/test_get.py @@ -14,3 +14,17 @@ def test_get(mocker): assert cmd.run() == 0 m.assert_called_once_with("repo_url", path="src", out="out", rev="version") + + +def test_get_url(mocker): + cli_args = parse_args( + ["get", "repo_url", "src", "--rev", "version", "--show-url"] + ) + assert cli_args.func == CmdGet + + cmd = cli_args.func(cli_args) + m = mocker.patch("dvc.api.get_url") + + assert cmd.run() == 0 + + m.assert_called_once_with("src", repo="repo_url", rev="version") From eb252e055bb646d4292e0d222dcc6fd0612f0b45 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Thu, 16 Jan 2020 11:46:40 +0545 Subject: [PATCH 64/78] get: split command for getting url and getting files from url functions --- dvc/command/get.py | 24 +++++++++++++++++++----- tests/func/test_get.py | 27 +++++++++++++++++++++------ tests/unit/command/test_get.py | 1 + 3 files changed, 41 insertions(+), 11 deletions(-) diff --git a/dvc/command/get.py b/dvc/command/get.py index e01ce4180..eb0decf39 100644 --- a/dvc/command/get.py +++ b/dvc/command/get.py @@ -3,21 +3,32 @@ import logging from .base import append_doc_link from .base import CmdBaseNoRepo -from dvc.exceptions import DvcException +from dvc.exceptions import DvcException, NotDvcRepoError logger = logging.getLogger(__name__) class CmdGet(CmdBaseNoRepo): - def _run_get_url(self): + def _show_url(self): from dvc.api import get_url try: url = get_url( self.args.path, repo=self.args.url, rev=self.args.rev ) - logger.info("{} {}".format(self.args.path, url)) + logger.info(url) + except NotDvcRepoError: + # NOTE: there's no way to get direct links to files inside + # git repository. + logger.exception( + "Could not show url for '{}' from '{}'. ".format( + self.args.path, self.args.url + ) + + "Only DVC repository is supported currently.", + exc_info=None, + ) + return 1 except DvcException: logger.exception( "failed to show url for '{}' from '{}'".format( @@ -25,13 +36,16 @@ class CmdGet(CmdBaseNoRepo): ) ) return 1 + return 0 def run(self): - if self.args.show_url: - return self._run_get_url() + return self._show_url() + return self._get_file_from_repo() + + def _get_file_from_repo(self): from dvc.repo import Repo try: diff --git a/tests/func/test_get.py b/tests/func/test_get.py index 7ee1ba40f..a208a99d2 100644 --- a/tests/func/test_get.py +++ b/tests/func/test_get.py @@ -187,13 +187,28 @@ def test_get_from_non_dvc_master(tmp_dir, erepo_dir, caplog): assert (tmp_dir / dst).read_text() == "some_contents" -def test_get_url(tmp_dir, erepo_dir): +def test_get_url_positive(tmp_dir, erepo_dir, caplog): with erepo_dir.chdir(): erepo_dir.dvc_gen("foo", "foo") - assert main(["get", ".", "foo", "--show-url"]) == 0 + + caplog.clear() + with caplog.at_level(logging.ERROR, logger="dvc"): + assert main(["get", fspath(erepo_dir), "foo", "--show-url"]) == 0 + assert caplog.text == "" -def test_get_url_not_existing(tmp_dir, erepo_dir): - with erepo_dir.chdir(): - erepo_dir.dvc_gen("foo", "foo") - assert main(["get", ".", "food", "--show-url"]) == 1 +def test_get_url_not_existing(tmp_dir, erepo_dir, caplog): + with caplog.at_level(logging.ERROR, logger="dvc"): + assert ( + main(["get", fspath(erepo_dir), "not-existing-file", "--show-url"]) + == 1 + ) + assert "failed to show url for 'not-existing-file'" in caplog.text + + +def test_get_url_git_only_repo(tmp_dir, scm, caplog): + tmp_dir.scm_gen({"foo": "foo"}, commit="initial") + + with caplog.at_level(logging.ERROR): + assert main(["get", fspath(tmp_dir), "foo", "--show-url"]) == 1 + assert "Only DVC repository is supported currently" in caplog.text diff --git a/tests/unit/command/test_get.py b/tests/unit/command/test_get.py index 6327b4293..f62659e04 100644 --- a/tests/unit/command/test_get.py +++ b/tests/unit/command/test_get.py @@ -24,6 +24,7 @@ def test_get_url(mocker): cmd = cli_args.func(cli_args) m = mocker.patch("dvc.api.get_url") + m.return_value = "url" assert cmd.run() == 0 From fadf60eea6262e8a3b828385d5ebb325f026fc28 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Thu, 16 Jan 2020 19:11:42 +0545 Subject: [PATCH 65/78] api: throw exception by get_url if not a dvc repo --- dvc/api.py | 26 ++++++++++++++++++++------ 1 file changed, 20 insertions(+), 6 deletions(-) diff --git a/dvc/api.py b/dvc/api.py index 9d6a321d9..85c5c0777 100644 --- a/dvc/api.py +++ b/dvc/api.py @@ -10,7 +10,7 @@ import ruamel.yaml from voluptuous import Schema, Required, Invalid from dvc.repo import Repo -from dvc.exceptions import DvcException +from dvc.exceptions import DvcException, NotDvcRepoError from dvc.external_repo import external_repo @@ -43,13 +43,27 @@ class SummonError(DvcException): pass +class UrlNotDvcRepoError(DvcException): + """Thrown if given url is not a DVC repository. + + Args: + url (str): url to the repository. + """ + + def __init__(self, url): + super().__init__("URL '{}' is not a dvc repository.".format(url)) + + def get_url(path, repo=None, rev=None, remote=None): """Returns an url of a resource specified by path in repo""" - with _make_repo(repo, rev=rev) as _repo: - abspath = os.path.join(_repo.root_dir, path) - out, = _repo.find_outs_by_path(abspath) - remote_obj = _repo.cloud.get_remote(remote) - return str(remote_obj.checksum_to_path_info(out.checksum)) + try: + with _make_repo(repo, rev=rev) as _repo: + abspath = os.path.join(_repo.root_dir, path) + out, = _repo.find_outs_by_path(abspath) + remote_obj = _repo.cloud.get_remote(remote) + return str(remote_obj.checksum_to_path_info(out.checksum)) + except NotDvcRepoError: + raise UrlNotDvcRepoError(repo) def open(path, repo=None, rev=None, remote=None, mode="r", encoding=None): From f130e18e2c0263d69ac5201b55f0d1335ff5ee2d Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Thu, 16 Jan 2020 19:13:24 +0545 Subject: [PATCH 66/78] get: use new exception from public api --- dvc/command/get.py | 19 ++----------------- tests/func/test_get.py | 4 ++-- 2 files changed, 4 insertions(+), 19 deletions(-) diff --git a/dvc/command/get.py b/dvc/command/get.py index eb0decf39..1386d8ca4 100644 --- a/dvc/command/get.py +++ b/dvc/command/get.py @@ -3,7 +3,7 @@ import logging from .base import append_doc_link from .base import CmdBaseNoRepo -from dvc.exceptions import DvcException, NotDvcRepoError +from dvc.exceptions import DvcException logger = logging.getLogger(__name__) @@ -18,23 +18,8 @@ class CmdGet(CmdBaseNoRepo): self.args.path, repo=self.args.url, rev=self.args.rev ) logger.info(url) - except NotDvcRepoError: - # NOTE: there's no way to get direct links to files inside - # git repository. - logger.exception( - "Could not show url for '{}' from '{}'. ".format( - self.args.path, self.args.url - ) - + "Only DVC repository is supported currently.", - exc_info=None, - ) - return 1 except DvcException: - logger.exception( - "failed to show url for '{}' from '{}'".format( - self.args.path, self.args.url - ) - ) + logger.exception("failed to show url") return 1 return 0 diff --git a/tests/func/test_get.py b/tests/func/test_get.py index a208a99d2..87d8ceea3 100644 --- a/tests/func/test_get.py +++ b/tests/func/test_get.py @@ -203,7 +203,7 @@ def test_get_url_not_existing(tmp_dir, erepo_dir, caplog): main(["get", fspath(erepo_dir), "not-existing-file", "--show-url"]) == 1 ) - assert "failed to show url for 'not-existing-file'" in caplog.text + assert "failed to show url" in caplog.text def test_get_url_git_only_repo(tmp_dir, scm, caplog): @@ -211,4 +211,4 @@ def test_get_url_git_only_repo(tmp_dir, scm, caplog): with caplog.at_level(logging.ERROR): assert main(["get", fspath(tmp_dir), "foo", "--show-url"]) == 1 - assert "Only DVC repository is supported currently" in caplog.text + assert "failed to show url" in caplog.text From aa7119a74110208beb57fa4093f95123e1e1186d Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Thu, 16 Jan 2020 19:22:22 +0545 Subject: [PATCH 67/78] get: test get_url raises exception if not a dvc repo --- tests/func/test_api.py | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/tests/func/test_api.py b/tests/func/test_api.py index 6ba3aef47..41cd0f743 100644 --- a/tests/func/test_api.py +++ b/tests/func/test_api.py @@ -6,7 +6,7 @@ import ruamel.yaml import pytest from dvc import api -from dvc.api import SummonError +from dvc.api import SummonError, UrlNotDvcRepoError from dvc.compat import fspath from dvc.exceptions import FileMissingError from dvc.main import main @@ -51,6 +51,15 @@ def test_get_url_external(erepo_dir, remote_url): assert api.get_url("foo", repo=repo_url) == expected_url +def test_get_url_git_only_repo_throws_exception(tmp_dir, scm): + tmp_dir.scm_gen({"foo": "foo"}, commit="initial") + + with pytest.raises(UrlNotDvcRepoError) as exc_info: + api.get_url("foo", fspath(tmp_dir)) + + assert fspath(tmp_dir) in str(exc_info) + + @pytest.mark.parametrize("remote_url", all_remote_params, indirect=True) def test_open(remote_url, tmp_dir, dvc): run_dvc("remote", "add", "-d", "upstream", remote_url) From cd3ef1850de1e8759a0eaff3c5181602c86206d2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Thu, 16 Jan 2020 14:44:09 +0000 Subject: [PATCH 68/78] simplify control flow --- dvc/dependency/repo.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index be57e0aba..aed3369a7 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -54,20 +54,19 @@ class DependencyREPO(DependencyLOCAL): try: with self._make_repo(rev_lock=rev_lock) as repo: - try: - return repo.find_out_by_relpath(self.def_path).info["md5"] - except OutputNotFoundError: - # Fall through to after exception handler - path = os.path.join(repo.root_dir, self.def_path) + return repo.find_out_by_relpath(self.def_path).info["md5"] - except NotDvcRepoError: - repo_path = cached_clone( - self.def_repo[self.PARAM_URL], - rev=rev_lock or self.def_repo.get(self.PARAM_REV), - ) - path = os.path.join(repo_path, self.def_path) + except (NotDvcRepoError, OutputNotFoundError): + # Fall through and clone + pass - return self.repo.cache.local.get_checksum(PathInfo(path)) + repo_path = cached_clone( + self.def_repo[self.PARAM_URL], + rev=rev_lock or self.def_repo.get(self.PARAM_REV), + ) + path = PathInfo(os.path.join(repo_path, self.def_path)) + + return self.repo.cache.local.get_checksum(path) def status(self): current_checksum = self._get_checksum(updated=False) From fcb7681683d625aaf9edb62e306482e2f1d654d8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?F=C3=A1bio=20Santos?= Date: Thu, 16 Jan 2020 14:49:44 +0000 Subject: [PATCH 69/78] take exception transformation into account --- dvc/dependency/repo.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index aed3369a7..3bcfd3e65 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -9,6 +9,7 @@ from dvc.external_repo import cached_clone from dvc.external_repo import external_repo from dvc.exceptions import NotDvcRepoError from dvc.exceptions import OutputNotFoundError +from dvc.exceptions import NoOutputInExternalRepoError from dvc.exceptions import PathMissingError from dvc.utils.fs import fs_copy from dvc.path_info import PathInfo @@ -55,8 +56,7 @@ class DependencyREPO(DependencyLOCAL): try: with self._make_repo(rev_lock=rev_lock) as repo: return repo.find_out_by_relpath(self.def_path).info["md5"] - - except (NotDvcRepoError, OutputNotFoundError): + except (NotDvcRepoError, NoOutputInExternalRepoError): # Fall through and clone pass From 50206d44dc1b0dccd595e873136ea8779ffa8f91 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Thu, 16 Jan 2020 21:06:52 +0545 Subject: [PATCH 70/78] test: fix test related to path on windows On windows, `str(exc_info)` is returning escaped path, eg: '' This commit just replaces the double slashes with single one. Bit of a hack, I know. --- tests/func/test_api.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/func/test_api.py b/tests/func/test_api.py index 41cd0f743..b3f0c0aba 100644 --- a/tests/func/test_api.py +++ b/tests/func/test_api.py @@ -57,7 +57,8 @@ def test_get_url_git_only_repo_throws_exception(tmp_dir, scm): with pytest.raises(UrlNotDvcRepoError) as exc_info: api.get_url("foo", fspath(tmp_dir)) - assert fspath(tmp_dir) in str(exc_info) + # On windows, `exc_info` has path escaped, eg: `C:\\\\Users\\\\travis` + assert fspath(tmp_dir) in str(exc_info).replace("\\\\", "\\") @pytest.mark.parametrize("remote_url", all_remote_params, indirect=True) From 083a630b44287acc323602d7060ea676db086dad Mon Sep 17 00:00:00 2001 From: rxxg Date: Thu, 16 Jan 2020 17:08:07 +0100 Subject: [PATCH 71/78] Fallback to standard Python code in case of missing packages --- dvc/system.py | 34 +++++++++++++++++++--------------- 1 file changed, 19 insertions(+), 15 deletions(-) diff --git a/dvc/system.py b/dvc/system.py index 7a6a70250..c300daac5 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -2,13 +2,30 @@ import errno import logging import os import platform +import shutil from dvc.compat import fspath from dvc.exceptions import DvcException - logger = logging.getLogger(__name__) +if platform.system() == "Windows": + try: + import speedcopy + + speedcopy.patch_copyfile() + except ImportError: + pass +else: + import sys + + if sys.version_info < (3, 8): + try: + # Importing the module monkey-patches shutil.copyfile + import pyfastcopy # noqa: F401 + except ImportError: + pass + class System(object): @staticmethod @@ -18,20 +35,7 @@ class System(object): @staticmethod def copy(src, dest): src, dest = fspath(src), fspath(dest) - system = platform.system() - if system == "Windows": - import speedcopy - - return speedcopy.copyfile(src, dest) - else: - import shutil - import sys - - if sys.version_info < (3, 8): - # Importing the module monkey-patches shutil.copyfile - import pyfastcopy # noqa: F401 - - return shutil.copyfile(src, dest) + return shutil.copyfile(src, dest) @staticmethod def hardlink(source, link_name): From 02a73a0d9b0d40112b8111dd462f9fc76cd14316 Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Thu, 16 Jan 2020 22:52:35 +0545 Subject: [PATCH 72/78] unit: test: check output of get --show-url command --- tests/unit/command/test_get.py | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/tests/unit/command/test_get.py b/tests/unit/command/test_get.py index f62659e04..391332490 100644 --- a/tests/unit/command/test_get.py +++ b/tests/unit/command/test_get.py @@ -16,7 +16,7 @@ def test_get(mocker): m.assert_called_once_with("repo_url", path="src", out="out", rev="version") -def test_get_url(mocker): +def test_get_url(mocker, caplog): cli_args = parse_args( ["get", "repo_url", "src", "--rev", "version", "--show-url"] ) @@ -27,5 +27,6 @@ def test_get_url(mocker): m.return_value = "url" assert cmd.run() == 0 + assert "url" in caplog.text m.assert_called_once_with("src", repo="repo_url", rev="version") From 98f6d56e8c82cefa112ec6942c2844565e669c8e Mon Sep 17 00:00:00 2001 From: Saugat Pachhai Date: Thu, 16 Jan 2020 22:59:58 +0545 Subject: [PATCH 73/78] test: refactor, apply suggestion from @pared MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Paweł Redzyński --- tests/unit/command/test_get.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/tests/unit/command/test_get.py b/tests/unit/command/test_get.py index 391332490..6ac5e4bb5 100644 --- a/tests/unit/command/test_get.py +++ b/tests/unit/command/test_get.py @@ -23,10 +23,9 @@ def test_get_url(mocker, caplog): assert cli_args.func == CmdGet cmd = cli_args.func(cli_args) - m = mocker.patch("dvc.api.get_url") - m.return_value = "url" + m = mocker.patch("dvc.api.get_url", return_value="resource_url") assert cmd.run() == 0 - assert "url" in caplog.text + assert "resource_url" in caplog.text m.assert_called_once_with("src", repo="repo_url", rev="version") From d34aa4471b2e681ef36a64e48eaef743e128d400 Mon Sep 17 00:00:00 2001 From: Casper da Costa-Luis Date: Thu, 16 Jan 2020 20:36:42 +0000 Subject: [PATCH 74/78] minor comments - explains review concerns in #3158 --- scripts/ci/deploy_condition.sh | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/scripts/ci/deploy_condition.sh b/scripts/ci/deploy_condition.sh index d4763d915..9c2a600b5 100755 --- a/scripts/ci/deploy_condition.sh +++ b/scripts/ci/deploy_condition.sh @@ -2,16 +2,13 @@ set -e -if [[ $TRAVIS_OS_NAME == "osx" && $TRAVIS_OSX_IMAGE != "xcode8.3" ]]; then - exit 1 -fi - if [[ $TRAVIS_EVENT_TYPE = pull_request || $TRAVIS_EVENT_TPYE = cron ]]; then exit 2 fi -# ensure at least one positional arg exists +# positional args are assumed to be file glob patterns to deploy if [[ ${#} -ge 1 ]]; then + # ensure at least one file exists [[ -n "$(ls $@ 2>/dev/null)" ]] || exit 3 fi From 34d3261e58c5508276b26c984dab57f0f6be0b56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Redzy=C5=84ski?= Date: Thu, 16 Jan 2020 15:56:23 +0100 Subject: [PATCH 75/78] utils: resolve_output: normpath before extracting basename --- dvc/utils/__init__.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index edb77e93b..bf02ecdb2 100644 --- a/dvc/utils/__init__.py +++ b/dvc/utils/__init__.py @@ -336,7 +336,7 @@ def env2bool(var, undefined=False): def resolve_output(inp, out): from urllib.parse import urlparse - name = os.path.basename(urlparse(inp).path) + name = os.path.basename(os.path.normpath(urlparse(inp).path)) if not out: return name if os.path.isdir(out): From cf08529362b917535dceb18cf384aaf343534a7a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Pawe=C5=82=20Redzy=C5=84ski?= Date: Thu, 16 Jan 2020 16:14:04 +0100 Subject: [PATCH 76/78] utils: add tests for resolve out --- tests/unit/utils/test_utils.py | 27 ++++++++++++++++++++++++++- 1 file changed, 26 insertions(+), 1 deletion(-) diff --git a/tests/unit/utils/test_utils.py b/tests/unit/utils/test_utils.py index e53c8df21..62f31446b 100644 --- a/tests/unit/utils/test_utils.py +++ b/tests/unit/utils/test_utils.py @@ -4,7 +4,7 @@ import os import pytest from dvc.path_info import PathInfo -from dvc.utils import file_md5 +from dvc.utils import file_md5, resolve_output from dvc.utils import fix_env from dvc.utils import relpath from dvc.utils import to_chunks @@ -104,3 +104,28 @@ def test_relpath(): path_info = PathInfo(path) assert relpath(path) == relpath(path_info) + + +@pytest.mark.parametrize( + "inp,out,is_dir,expected", + [ + ["target", None, False, "target"], + ["target", "dir", True, os.path.join("dir", "target")], + ["target", "file_target", False, "file_target"], + [ + "target", + os.path.join("dir", "subdir"), + True, + os.path.join("dir", "subdir", "target"), + ], + ["dir/", None, False, "dir"], + ["dir", None, False, "dir"], + ["dir", "other_dir", False, "other_dir"], + ["dir", "other_dir", True, os.path.join("other_dir", "dir")], + ], +) +def test_resolve_output(inp, out, is_dir, expected, mocker): + with mocker.patch("os.path.isdir", return_value=is_dir): + result = resolve_output(inp, out) + + assert result == expected From 0bd433d1539a70c83690ab8602ab3a38cd243d1f Mon Sep 17 00:00:00 2001 From: Andy Sarroff Date: Fri, 17 Jan 2020 09:39:10 -0500 Subject: [PATCH 77/78] s3: copy large objects without chunking When executing `boto3.s3.Client.Copy`, large files are chunked into multiple parts. The default chunk threshold and size is 8MB. This caused an issue when caching an object larger than 8M, where the original object had not been uploaded in multiple parts. The original object and the cached object would have different ETags, causing an exception. This fixes the issue by dynamically setting the chunk threshold to be one byte larger than the size of the original object therefore avoiding multipart copies. Fixes #3174 --- dvc/remote/s3.py | 9 ++++++++- 1 file changed, 8 insertions(+), 1 deletion(-) diff --git a/dvc/remote/s3.py b/dvc/remote/s3.py index ba043afcd..b6a24c6bb 100644 --- a/dvc/remote/s3.py +++ b/dvc/remote/s3.py @@ -3,6 +3,7 @@ import logging import os import threading +from boto3.s3.transfer import TransferConfig from funcy import cached_property, wrap_prop @@ -169,7 +170,13 @@ class RemoteS3(RemoteBASE): ) else: source = {"Bucket": from_info.bucket, "Key": from_info.path} - s3.copy(source, to_info.bucket, to_info.path, ExtraArgs=extra_args) + s3.copy( + source, + to_info.bucket, + to_info.path, + ExtraArgs=extra_args, + Config=TransferConfig(multipart_threshold=size + 1) + ) cached_etag = cls.get_etag(s3, to_info.bucket, to_info.path) if etag != cached_etag: From c3628c87d5d4fb1ca0995e8e293623a9cbfbc0d0 Mon Sep 17 00:00:00 2001 From: "Restyled.io" Date: Fri, 17 Jan 2020 18:00:19 +0000 Subject: [PATCH 78/78] Restyled by black --- dvc/remote/s3.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/remote/s3.py b/dvc/remote/s3.py index b6a24c6bb..bcc1cffb8 100644 --- a/dvc/remote/s3.py +++ b/dvc/remote/s3.py @@ -175,7 +175,7 @@ class RemoteS3(RemoteBASE): to_info.bucket, to_info.path, ExtraArgs=extra_args, - Config=TransferConfig(multipart_threshold=size + 1) + Config=TransferConfig(multipart_threshold=size + 1), ) cached_etag = cls.get_etag(s3, to_info.bucket, to_info.path)