From b3a6d70d9186faf451789409b04b16e71c0c7b44 Mon Sep 17 00:00:00 2001 From: username <45409043+vernt@users.noreply.github.com> Date: Sun, 2 Dec 2018 00:35:20 -0800 Subject: [PATCH 01/21] [WIP] Add dvc run --deterministic option. Tests currently failing. --- dvc/cli.py | 6 ++++++ dvc/command/run.py | 3 ++- dvc/project.py | 27 +++++++++++++++---------- dvc/stage.py | 50 +++++++++++++++++++++++++++++++++++++++++++++- tests/test_run.py | 37 ++++++++++++++++++++++++++++++++++ 5 files changed, 110 insertions(+), 13 deletions(-) diff --git a/dvc/cli.py b/dvc/cli.py index 4f3ceb90b..833fddb3d 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -271,6 +271,12 @@ def parse_args(argv=None): help="Automatic 'yes' answer to all prompts. E.g. " "when '.dvc' file exists and dvc asks if you " "want to overwrite it.") + run_parser.add_argument( + '--deterministic', + action='store_true', + default=False, + help=("Use cached result if previously run with " + "current dependencies and command.")) run_parser.add_argument( 'command', nargs=argparse.REMAINDER, diff --git a/dvc/command/run.py b/dvc/command/run.py index 0fa2c2036..2a052c4ab 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -31,7 +31,8 @@ class CmdRun(CmdBase): deps=self.args.deps, fname=self.args.file, cwd=self.args.cwd, - no_exec=self.args.no_exec) + no_exec=self.args.no_exec, + deterministic=self.args.deterministic) except DvcException as ex: self.project.logger.error('Failed to run command', ex) return 1 diff --git a/dvc/project.py b/dvc/project.py index 7703dea5c..4e10598df 100644 --- a/dvc/project.py +++ b/dvc/project.py @@ -363,18 +363,23 @@ class Project(object): fname=None, cwd=os.curdir, no_exec=False, - overwrite=False): - from dvc.stage import Stage + overwrite=False, + deterministic=False): - stage = Stage.loads(project=self, - fname=fname, - cmd=cmd, - cwd=cwd, - outs=outs, - outs_no_cache=outs_no_cache, - metrics_no_cache=metrics_no_cache, - deps=deps, - overwrite=overwrite) + with self.state: + stage = Stage.loads(project=self, + fname=fname, + cmd=cmd, + cwd=cwd, + outs=outs, + outs_no_cache=outs_no_cache, + metrics_no_cache=metrics_no_cache, + deps=deps, + overwrite=overwrite, + deterministic=deterministic) + + if stage is None: + return None all_stages = self.stages() diff --git a/dvc/stage.py b/dvc/stage.py index 30cd5b085..f9aa418cc 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -1,3 +1,4 @@ +import collections import os import yaml import posixpath @@ -70,6 +71,42 @@ class MissingDataSource(DvcException): msg = u'missing data {}: {}'.format(source, ', '.join(missing_files)) super(MissingDataSource, self).__init__(msg) +def _identify_dicts(dicts): + return collections.Counter(frozenset(d.items()) for d in dicts) + + +def _deps_equal(old, new): + old_identities = _identify_dicts(dep.dumpd() for dep in old.deps) + new_identities = _identify_dicts(dep.dumpd() for dep in new.deps) + return old_identities == new_identities + + +def _outs_equal(old, new): + old_dicts = [out.dumpd() for out in old.outs] + new_dicts = [out.dumpd() for out in new.outs] + for d in old_dicts: + del d['md5'] + old_identities = _identify_dicts(old_dicts) + new_identities = _identify_dicts(new_dicts) + return old_identities == new_identities + + +def _stage_equal(old, new): + deps_unchanged = all(dep.changed() is False for dep in old.deps) + if not deps_unchanged: + Logger.debug('Deps changed') + return False + if not _deps_equal(old, new): + Logger.debug('Deps unequal') + return False + + if not _outs_equal(old, new): + Logger.debug('Outs unequal') + return False + + Logger.debug('stage equal') + return True + class Stage(object): STAGE_FILE = 'Dvcfile' @@ -301,7 +338,9 @@ class Stage(object): cwd=os.curdir, locked=False, add=False, - overwrite=True): + overwrite=True, + deterministic=False): + stage = Stage(project=project, cwd=cwd, cmd=cmd, @@ -326,6 +365,14 @@ class Stage(object): path = os.path.join(cwd, fname) if os.path.exists(path): + + old = Stage.load(project=project, fname=fname) + for dep in stage.deps: + dep.save() + if deterministic and _stage_equal(old, stage): + Logger.info('Stage is cached, skipping.') + return None + relpath = os.path.relpath(path) msg = "'{}' already exists. " \ "Do you wish to run the command and overwrite it?" @@ -333,6 +380,7 @@ class Stage(object): and not project.prompt.prompt(msg.format(relpath), False): raise DvcException("'{}' already exists".format(relpath)) + stage.cwd = cwd stage.path = path diff --git a/tests/test_run.py b/tests/test_run.py index 2e5f0a932..ac575f26b 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -180,3 +180,40 @@ class TestCmdRun(TestDvc): def test_keyboard_interrupt(self, mock_popen): ret = main(['run', 'mycmd']) self.assertEqual(ret, 252) + + +class TestRunDeterministic(TestDvc): + + def test(self): + + def _run(target, deterministic=False): + + command = [ + 'dvc', 'run', '--verbose', '-f', '{}.dvc'.format(target), '-d', 'file1', '-o', target, + 'python', '-c', shlex.quote("open('{}', 'w').write('hi')").format(target), + ] + if deterministic: + command.insert(2, '--deterministic') + + return command + + + + + subprocess.check_call([ + 'dvc', 'run', '--verbose', '-f', 'f1.dvc', '-o', 'file1', + 'bash', '-c', '"echo hi > file1"', + ]) + + subprocess.check_call(_run('file2', deterministic=True)) + + # Does not run because its inputs already exist. + subprocess.check_call(_run('file3', deterministic=True)) + + subprocess.check_call(_run('file4')) + + self.assertTrue(os.path.exists('file1')) + self.assertTrue(os.path.exists('file2')) + self.assertTrue(os.path.exists('file4')) + + self.assertFalse(os.path.exists('file3')) From 8dbb674c3c2feabcb76ae71243272c8388525d2e Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 2 Dec 2018 18:08:52 +0200 Subject: [PATCH 02/21] project: run: don't forget to import Stage --- dvc/project.py | 1 + 1 file changed, 1 insertion(+) diff --git a/dvc/project.py b/dvc/project.py index 4e10598df..01891ec3e 100644 --- a/dvc/project.py +++ b/dvc/project.py @@ -365,6 +365,7 @@ class Project(object): no_exec=False, overwrite=False, deterministic=False): + from dvc.stage import Stage with self.state: stage = Stage.loads(project=self, From a2b29582378b13f23f0d490ea9192c74c58820b1 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 2 Dec 2018 18:09:34 +0200 Subject: [PATCH 03/21] stage: fix spacing --- dvc/stage.py | 1 + 1 file changed, 1 insertion(+) diff --git a/dvc/stage.py b/dvc/stage.py index f9aa418cc..abb99d45e 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -71,6 +71,7 @@ class MissingDataSource(DvcException): msg = u'missing data {}: {}'.format(source, ', '.join(missing_files)) super(MissingDataSource, self).__init__(msg) + def _identify_dicts(dicts): return collections.Counter(frozenset(d.items()) for d in dicts) From 40724bffd6a37a34ca71cde593a6a0009058a5a5 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 2 Dec 2018 18:10:03 +0200 Subject: [PATCH 04/21] stage: fix spacing --- dvc/stage.py | 1 - 1 file changed, 1 deletion(-) diff --git a/dvc/stage.py b/dvc/stage.py index abb99d45e..61f4e07c9 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -381,7 +381,6 @@ class Stage(object): and not project.prompt.prompt(msg.format(relpath), False): raise DvcException("'{}' already exists".format(relpath)) - stage.cwd = cwd stage.path = path From e75a2f7ddec226f034942843955b7f74e235c176 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 2 Dec 2018 18:44:01 +0200 Subject: [PATCH 05/21] test: don't forget to import shlex --- tests/test_run.py | 1 + 1 file changed, 1 insertion(+) diff --git a/tests/test_run.py b/tests/test_run.py index ac575f26b..0f391fa00 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -187,6 +187,7 @@ class TestRunDeterministic(TestDvc): def test(self): def _run(target, deterministic=False): + import shlex command = [ 'dvc', 'run', '--verbose', '-f', '{}.dvc'.format(target), '-d', 'file1', '-o', target, From 6d4240dd8a9463751808758e43f68c840d277a8c Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 2 Dec 2018 18:58:32 +0200 Subject: [PATCH 06/21] test: fix shlex import --- tests/test_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_run.py b/tests/test_run.py index 0f391fa00..282455116 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -187,7 +187,7 @@ class TestRunDeterministic(TestDvc): def test(self): def _run(target, deterministic=False): - import shlex + from shlex import shlex command = [ 'dvc', 'run', '--verbose', '-f', '{}.dvc'.format(target), '-d', 'file1', '-o', target, From e1f44158c88bfaba7156d26f2ebe1e5dd46d1197 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 2 Dec 2018 19:45:43 +0200 Subject: [PATCH 07/21] test: use pipes.quote on py2 --- tests/test_run.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/test_run.py b/tests/test_run.py index 282455116..b0e26a92a 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -187,7 +187,10 @@ class TestRunDeterministic(TestDvc): def test(self): def _run(target, deterministic=False): - from shlex import shlex + try: # py3 + from shlex import quote + except ImportError: # py2 + from pipes import quote command = [ 'dvc', 'run', '--verbose', '-f', '{}.dvc'.format(target), '-d', 'file1', '-o', target, From 1878c41c8006907f853b66b45d094f845be46f50 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 2 Dec 2018 19:55:22 +0200 Subject: [PATCH 08/21] test: use quote instead of shlex.quote --- tests/test_run.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/test_run.py b/tests/test_run.py index b0e26a92a..357f326bb 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -194,7 +194,7 @@ class TestRunDeterministic(TestDvc): command = [ 'dvc', 'run', '--verbose', '-f', '{}.dvc'.format(target), '-d', 'file1', '-o', target, - 'python', '-c', shlex.quote("open('{}', 'w').write('hi')").format(target), + 'python', '-c', quote("open('{}', 'w').write('hi')").format(target), ] if deterministic: command.insert(2, '--deterministic') From 1211abf35c9dd98ae2990bcb8b06a53303c37de4 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Sun, 2 Dec 2018 22:55:33 +0200 Subject: [PATCH 09/21] test: add a proper test for `dvc run --deterministic` Signed-off-by: Ruslan Kuprieiev --- tests/test_run.py | 89 +++++++++++++++++++++++++++++++++-------------- 1 file changed, 62 insertions(+), 27 deletions(-) diff --git a/tests/test_run.py b/tests/test_run.py index 357f326bb..d6dac32df 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -1,8 +1,11 @@ import os import mock +import time +import shutil import filecmp import subprocess +from dvc.project import Project from dvc.main import main from dvc.utils import file_md5 from dvc.stage import Stage, StageFileBadNameError, MissingDep @@ -181,43 +184,75 @@ class TestCmdRun(TestDvc): ret = main(['run', 'mycmd']) self.assertEqual(ret, 252) + @mock.patch.object(Project, 'run') + def test_deterministic(self, mock_run): + ret = main(['run', + '-d', self.FOO, + '-d', self.CODE, + '--deterministic', + '-o', 'out', + '-f', 'out.dvc', + 'python', self.CODE, self.FOO, 'out']) + self.assertEqual(ret, 0) + mock_run.assert_called_once() + args, kwargs = mock_run.call_args + self.assertIn(('deterministic', True), kwargs.items()) + class TestRunDeterministic(TestDvc): - def test(self): + out_file = 'out' + stage_file = out_file + '.dvc' + cmd = 'python {} {} {}'.format(self.CODE, self.FOO, out_file) + self.dvc.add(self.FOO) + stage = self.dvc.run(cmd=cmd, + fname=stage_file, + deterministic=False, + overwrite=True, + deps=[self.FOO, self.CODE], + outs=[out_file]) + self.assertTrue(stage is not None) - def _run(target, deterministic=False): - try: # py3 - from shlex import quote - except ImportError: # py2 - from pipes import quote + # save timestamps to make sure that files didn't change. + stage_file_mtime_1 = os.path.getmtime(stage_file) + out_file_mtime_1 = os.path.getmtime(out_file) - command = [ - 'dvc', 'run', '--verbose', '-f', '{}.dvc'.format(target), '-d', 'file1', '-o', target, - 'python', '-c', quote("open('{}', 'w').write('hi')").format(target), - ] - if deterministic: - command.insert(2, '--deterministic') + # sleep to make sure that mtime changes even on filesystems + # with very low timestamp resolution(e.g. 1 sec on APFS). + time.sleep(2) - return command + # dependencies didn't change, so deterministic stage shouldn't + # be ran again. + stage = self.dvc.run(cmd=cmd, + fname=stage_file, + deterministic=True, + overwrite=True, + deps=[self.FOO, self.CODE], + outs=[out_file]) + self.assertTrue(stage is None) + stage_file_mtime_2 = os.path.getmtime(stage_file) + out_file_mtime_2 = os.path.getmtime(out_file) + self.assertEqual(stage_file_mtime_1, stage_file_mtime_2) + self.assertEqual(out_file_mtime_1, out_file_mtime_2) + time.sleep(2) - subprocess.check_call([ - 'dvc', 'run', '--verbose', '-f', 'f1.dvc', '-o', 'file1', - 'bash', '-c', '"echo hi > file1"', - ]) + # now change one of the deps and make sure stage did run + os.unlink(self.FOO) + shutil.copy(self.BAR, self.FOO) - subprocess.check_call(_run('file2', deterministic=True)) + stage = self.dvc.run(cmd=cmd, + fname=stage_file, + deterministic=True, + overwrite=True, + deps=[self.FOO, self.CODE], + outs=[out_file]) + self.assertTrue(stage is not None) - # Does not run because its inputs already exist. - subprocess.check_call(_run('file3', deterministic=True)) + stage_file_mtime_3 = os.path.getmtime(stage_file) + out_file_mtime_3 = os.path.getmtime(out_file) - subprocess.check_call(_run('file4')) - - self.assertTrue(os.path.exists('file1')) - self.assertTrue(os.path.exists('file2')) - self.assertTrue(os.path.exists('file4')) - - self.assertFalse(os.path.exists('file3')) + self.assertNotEqual(stage_file_mtime_2, stage_file_mtime_3) + self.assertNotEqual(out_file_mtime_2, out_file_mtime_3) From ae5ac21bf8af7da62f36f0a7654775894936ba8b Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 3 Dec 2018 01:28:16 +0200 Subject: [PATCH 10/21] run: deterministic: copare the whole dicts We should check command, as well as any other fields. E.g. `locked`. Signed-off-by: Ruslan Kuprieiev --- dvc/stage.py | 81 ++++++++++++++----------------- tests/test_run.py | 121 +++++++++++++++++++++++++++------------------- 2 files changed, 107 insertions(+), 95 deletions(-) diff --git a/dvc/stage.py b/dvc/stage.py index 61f4e07c9..b58d1b636 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -1,4 +1,3 @@ -import collections import os import yaml import posixpath @@ -72,43 +71,6 @@ class MissingDataSource(DvcException): super(MissingDataSource, self).__init__(msg) -def _identify_dicts(dicts): - return collections.Counter(frozenset(d.items()) for d in dicts) - - -def _deps_equal(old, new): - old_identities = _identify_dicts(dep.dumpd() for dep in old.deps) - new_identities = _identify_dicts(dep.dumpd() for dep in new.deps) - return old_identities == new_identities - - -def _outs_equal(old, new): - old_dicts = [out.dumpd() for out in old.outs] - new_dicts = [out.dumpd() for out in new.outs] - for d in old_dicts: - del d['md5'] - old_identities = _identify_dicts(old_dicts) - new_identities = _identify_dicts(new_dicts) - return old_identities == new_identities - - -def _stage_equal(old, new): - deps_unchanged = all(dep.changed() is False for dep in old.deps) - if not deps_unchanged: - Logger.debug('Deps changed') - return False - if not _deps_equal(old, new): - Logger.debug('Deps unequal') - return False - - if not _outs_equal(old, new): - Logger.debug('Outs unequal') - return False - - Logger.debug('stage equal') - return True - - class Stage(object): STAGE_FILE = 'Dvcfile' STAGE_FILE_SUFFIX = '.dvc' @@ -328,6 +290,37 @@ class Stage(object): if not os.path.realpath(cwd).startswith(proj_dir): raise StageBadCwdError(cwd) + def is_cached(self): + """ + Checks if this stage has been already ran and saved to the same + dvc file. + """ + from dvc.remote.local import RemoteLOCAL + from dvc.remote.s3 import RemoteS3 + + old = Stage.load(self.project, self.path) + if old.changed(): + return False + + # NOTE: need to save checksums for deps in order to compare them + # with what is written in the old stage. + for dep in self.deps: + dep.save() + + old_d = old.dumpd() + new_d = self.dumpd() + + # NOTE: need to remove checksums from old dict in order to compare + # it to the new one, since the new one doesn't have checksums yet. + old_d.pop(self.PARAM_MD5, None) + new_d.pop(self.PARAM_MD5, None) + outs = old_d.get(self.PARAM_OUTS, []) + for out in outs: + out.pop(RemoteLOCAL.PARAM_MD5, None) + out.pop(RemoteS3.PARAM_ETAG, None) + + return old_d == new_d + @staticmethod def loads(project=None, cmd=None, @@ -365,12 +358,11 @@ class Stage(object): cwd = os.path.abspath(cwd) path = os.path.join(cwd, fname) - if os.path.exists(path): + stage.cwd = cwd + stage.path = path - old = Stage.load(project=project, fname=fname) - for dep in stage.deps: - dep.save() - if deterministic and _stage_equal(old, stage): + if os.path.exists(path): + if deterministic and stage.is_cached(): Logger.info('Stage is cached, skipping.') return None @@ -381,9 +373,6 @@ class Stage(object): and not project.prompt.prompt(msg.format(relpath), False): raise DvcException("'{}' already exists".format(relpath)) - stage.cwd = cwd - stage.path = path - return stage @staticmethod diff --git a/tests/test_run.py b/tests/test_run.py index d6dac32df..b47f76703 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -199,60 +199,83 @@ class TestCmdRun(TestDvc): self.assertIn(('deterministic', True), kwargs.items()) -class TestRunDeterministic(TestDvc): +class TestRunDeterministicBase(TestDvc): + def setUp(self): + super(TestRunDeterministicBase, self).setUp() + self.out_file = 'out' + self.stage_file = self.out_file + '.dvc' + self.cmd = 'python {} {} {}'.format(self.CODE, self.FOO, self.out_file) + self.deterministic = True + self.deps = [self.FOO, self.CODE] + self.outs = [self.out_file] + + self._run(check=False) + self.assertTrue(self.stage is not None) + self.stage_file_mtime = os.path.getmtime(self.stage_file) + self.out_file_mtime = os.path.getmtime(self.out_file) + + def _run(self, should_run=True, check=True): + self.stage = self.dvc.run(cmd=self.cmd, + fname=self.stage_file, + deterministic=self.deterministic, + overwrite=True, + deps=self.deps, + outs=self.outs) + + if not check: + return + + stage_file_mtime = os.path.getmtime(self.stage_file) + out_file_mtime = os.path.getmtime(self.out_file) + + if should_run: + self.assertTrue(self.stage is not None) + self.assertNotEqual(self.stage_file_mtime, stage_file_mtime) + self.assertNotEqual(self.out_file_mtime, out_file_mtime) + else: + self.assertTrue(self.stage is None) + self.assertEqual(self.stage_file_mtime, stage_file_mtime) + self.assertEqual(self.out_file_mtime, out_file_mtime) + + +class TestRunDeterministic(TestRunDeterministicBase): def test(self): - out_file = 'out' - stage_file = out_file + '.dvc' - cmd = 'python {} {} {}'.format(self.CODE, self.FOO, out_file) - self.dvc.add(self.FOO) - stage = self.dvc.run(cmd=cmd, - fname=stage_file, - deterministic=False, - overwrite=True, - deps=[self.FOO, self.CODE], - outs=[out_file]) - self.assertTrue(stage is not None) + self._run(False) - # save timestamps to make sure that files didn't change. - stage_file_mtime_1 = os.path.getmtime(stage_file) - out_file_mtime_1 = os.path.getmtime(out_file) - # sleep to make sure that mtime changes even on filesystems - # with very low timestamp resolution(e.g. 1 sec on APFS). - time.sleep(2) - - # dependencies didn't change, so deterministic stage shouldn't - # be ran again. - stage = self.dvc.run(cmd=cmd, - fname=stage_file, - deterministic=True, - overwrite=True, - deps=[self.FOO, self.CODE], - outs=[out_file]) - self.assertTrue(stage is None) - - stage_file_mtime_2 = os.path.getmtime(stage_file) - out_file_mtime_2 = os.path.getmtime(out_file) - - self.assertEqual(stage_file_mtime_1, stage_file_mtime_2) - self.assertEqual(out_file_mtime_1, out_file_mtime_2) - - time.sleep(2) - - # now change one of the deps and make sure stage did run +class TestRunDeterministicChangedDep(TestRunDeterministicBase): + def test(self): os.unlink(self.FOO) shutil.copy(self.BAR, self.FOO) + self._run() - stage = self.dvc.run(cmd=cmd, - fname=stage_file, - deterministic=True, - overwrite=True, - deps=[self.FOO, self.CODE], - outs=[out_file]) - self.assertTrue(stage is not None) - stage_file_mtime_3 = os.path.getmtime(stage_file) - out_file_mtime_3 = os.path.getmtime(out_file) +class TestRunDeterministicChangedDepsList(TestRunDeterministicBase): + def test(self): + self.deps = [self.BAR, self.CODE] + self._run() - self.assertNotEqual(stage_file_mtime_2, stage_file_mtime_3) - self.assertNotEqual(out_file_mtime_2, out_file_mtime_3) + +class TestRunDeterministicNewDep(TestRunDeterministicBase): + def test(self): + self.deps = [self.FOO, self.BAR, self.CODE] + self._run() + + +class TestRunDeterministicRemoveDep(TestRunDeterministicBase): + def test(self): + self.deps = [self.CODE] + self._run() + + +class TestRunDeterministicChangedOut(TestRunDeterministicBase): + def test(self): + os.unlink(self.out_file) + self.out_file_mtime = None + self._run() + + +class TestRunDeterministicChangedCmd(TestRunDeterministicBase): + def test(self): + self.cmd += ' arg' + self._run() From 7434a5fde9bad96b5b6b61c5389be06a84306a2b Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 3 Dec 2018 01:38:54 +0200 Subject: [PATCH 11/21] cli: remove unneeded parentheses Signed-off-by: Ruslan Kuprieiev --- dvc/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dvc/cli.py b/dvc/cli.py index 833fddb3d..214dc04ca 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -275,8 +275,8 @@ def parse_args(argv=None): '--deterministic', action='store_true', default=False, - help=("Use cached result if previously run with " - "current dependencies and command.")) + help="Use cached result if previously run with " + "current dependencies and command.") run_parser.add_argument( 'command', nargs=argparse.REMAINDER, From 298ae356cd17f57e20d0f52140210b73bd8d3c3e Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 3 Dec 2018 04:50:48 +0200 Subject: [PATCH 12/21] run: make --deterministic default Signed-off-by: Ruslan Kuprieiev --- dvc/cli.py | 6 ---- dvc/command/run.py | 3 +- dvc/project.py | 6 ++-- dvc/stage.py | 66 ++++++++++++++++++++------------------- tests/test_run.py | 77 +++++++++++++++++++--------------------------- 5 files changed, 70 insertions(+), 88 deletions(-) diff --git a/dvc/cli.py b/dvc/cli.py index 214dc04ca..4f3ceb90b 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -271,12 +271,6 @@ def parse_args(argv=None): help="Automatic 'yes' answer to all prompts. E.g. " "when '.dvc' file exists and dvc asks if you " "want to overwrite it.") - run_parser.add_argument( - '--deterministic', - action='store_true', - default=False, - help="Use cached result if previously run with " - "current dependencies and command.") run_parser.add_argument( 'command', nargs=argparse.REMAINDER, diff --git a/dvc/command/run.py b/dvc/command/run.py index 2a052c4ab..0fa2c2036 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -31,8 +31,7 @@ class CmdRun(CmdBase): deps=self.args.deps, fname=self.args.file, cwd=self.args.cwd, - no_exec=self.args.no_exec, - deterministic=self.args.deterministic) + no_exec=self.args.no_exec) except DvcException as ex: self.project.logger.error('Failed to run command', ex) return 1 diff --git a/dvc/project.py b/dvc/project.py index 01891ec3e..8dbc8d429 100644 --- a/dvc/project.py +++ b/dvc/project.py @@ -363,8 +363,7 @@ class Project(object): fname=None, cwd=os.curdir, no_exec=False, - overwrite=False, - deterministic=False): + overwrite=False): from dvc.stage import Stage with self.state: @@ -376,8 +375,7 @@ class Project(object): outs_no_cache=outs_no_cache, metrics_no_cache=metrics_no_cache, deps=deps, - overwrite=overwrite, - deterministic=deterministic) + overwrite=overwrite) if stage is None: return None diff --git a/dvc/stage.py b/dvc/stage.py index b58d1b636..09d3b846c 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -29,6 +29,12 @@ class StageFileDoesNotExistError(DvcException): super(StageFileDoesNotExistError, self).__init__(msg) +class StageFileAlreadyExistsError(DvcException): + def __init__(self, relpath): + msg = "Stage '{}' already exists".format(relpath) + super(StageFileAlreadyExistsError, self).__init__(msg) + + class StageFileIsNotDvcFileError(DvcException): def __init__(self, fname): msg = "'{}' is not a dvc file".format(fname) @@ -149,7 +155,10 @@ class Stage(object): len(self.deps) == 1 and \ len(self.outs) == 1 - def _changed_deps(self, print_info, log): + def _changed_deps(self, log): + if self.locked: + return False + if self.is_callback: msg = "Dvc file '{}' is a 'callback' stage (has a command and " \ "no dependencies) and thus always considered as changed." @@ -159,37 +168,35 @@ class Stage(object): for dep in self.deps: if not dep.changed(): continue - if print_info: - msg = "Dependency '{}' of '{}' changed." - log(msg.format(dep, self.relpath)) + log("Dependency '{}' of '{}' changed.".format(dep, self.relpath)) return True return False - def changed(self, print_info=False): - ret = False + def _changed_outs(self, log): + for out in self.outs: + if not out.changed(): + continue + log("Output '{}' of '{}' changed.".format(out, self.relpath)) + return True + return False + + def _changed_md5(self, log): + if self.changed_md5(): + log("Dvc file '{}' changed.".format(self.relpath)) + return True + return False + + def changed(self, print_info=False): if print_info: log = self.project.logger.info else: log = self.project.logger.debug - if not self.locked: - ret = self._changed_deps(print_info, log) - - for out in self.outs: - if not out.changed(): - continue - if print_info: - msg = "Output '{}' of '{}' changed." - log(msg.format(out, self.relpath)) - ret = True - - if self.changed_md5(): - if print_info: - msg = "Dvc file '{}' changed." - log(msg.format(self.relpath)) - ret = True + ret = any([self._changed_deps(log), + self._changed_outs(log), + self._changed_md5(log)]) if ret: msg = "Stage '{}' changed.".format(self.relpath) @@ -299,7 +306,7 @@ class Stage(object): from dvc.remote.s3 import RemoteS3 old = Stage.load(self.project, self.path) - if old.changed(): + if old._changed_outs(log=self.project.logger.debug): return False # NOTE: need to save checksums for deps in order to compare them @@ -332,8 +339,7 @@ class Stage(object): cwd=os.curdir, locked=False, add=False, - overwrite=True, - deterministic=False): + overwrite=True): stage = Stage(project=project, cwd=cwd, @@ -361,17 +367,15 @@ class Stage(object): stage.cwd = cwd stage.path = path - if os.path.exists(path): - if deterministic and stage.is_cached(): + if os.path.exists(path) and not overwrite: + if stage.is_cached(): Logger.info('Stage is cached, skipping.') return None - relpath = os.path.relpath(path) msg = "'{}' already exists. " \ "Do you wish to run the command and overwrite it?" - if not overwrite \ - and not project.prompt.prompt(msg.format(relpath), False): - raise DvcException("'{}' already exists".format(relpath)) + if not project.prompt.prompt(msg.format(stage.relpath), False): + raise StageFileAlreadyExistsError(stage.relpath) return stage diff --git a/tests/test_run.py b/tests/test_run.py index b47f76703..5f7ee848f 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -9,7 +9,7 @@ from dvc.project import Project from dvc.main import main from dvc.utils import file_md5 from dvc.stage import Stage, StageFileBadNameError, MissingDep -from dvc.stage import StageBadCwdError +from dvc.stage import StageBadCwdError, StageFileAlreadyExistsError from dvc.command.run import CmdRun from dvc.exceptions import (OutputDuplicationError, CircularDependencyError, @@ -184,20 +184,6 @@ class TestCmdRun(TestDvc): ret = main(['run', 'mycmd']) self.assertEqual(ret, 252) - @mock.patch.object(Project, 'run') - def test_deterministic(self, mock_run): - ret = main(['run', - '-d', self.FOO, - '-d', self.CODE, - '--deterministic', - '-o', 'out', - '-f', 'out.dvc', - 'python', self.CODE, self.FOO, 'out']) - self.assertEqual(ret, 0) - mock_run.assert_called_once() - args, kwargs = mock_run.call_args - self.assertIn(('deterministic', True), kwargs.items()) - class TestRunDeterministicBase(TestDvc): def setUp(self): @@ -205,77 +191,78 @@ class TestRunDeterministicBase(TestDvc): self.out_file = 'out' self.stage_file = self.out_file + '.dvc' self.cmd = 'python {} {} {}'.format(self.CODE, self.FOO, self.out_file) - self.deterministic = True self.deps = [self.FOO, self.CODE] self.outs = [self.out_file] + self.overwrite = False - self._run(check=False) - self.assertTrue(self.stage is not None) - self.stage_file_mtime = os.path.getmtime(self.stage_file) - self.out_file_mtime = os.path.getmtime(self.out_file) + self._run() - def _run(self, should_run=True, check=True): + def _run(self): self.stage = self.dvc.run(cmd=self.cmd, fname=self.stage_file, - deterministic=self.deterministic, - overwrite=True, + overwrite=self.overwrite, deps=self.deps, outs=self.outs) - if not check: - return - - stage_file_mtime = os.path.getmtime(self.stage_file) - out_file_mtime = os.path.getmtime(self.out_file) - - if should_run: - self.assertTrue(self.stage is not None) - self.assertNotEqual(self.stage_file_mtime, stage_file_mtime) - self.assertNotEqual(self.out_file_mtime, out_file_mtime) - else: - self.assertTrue(self.stage is None) - self.assertEqual(self.stage_file_mtime, stage_file_mtime) - self.assertEqual(self.out_file_mtime, out_file_mtime) - class TestRunDeterministic(TestRunDeterministicBase): def test(self): - self._run(False) + self._run() + + +class TestRunDeterministicOverwrite(TestRunDeterministicBase): + def test(self): + self.overwrite = True + self._run() + + +class TestRunDeterministicCallback(TestRunDeterministicBase): + def test(self): + self.stage.remove() + self.deps = [] + self._run() + self._run() class TestRunDeterministicChangedDep(TestRunDeterministicBase): def test(self): os.unlink(self.FOO) shutil.copy(self.BAR, self.FOO) - self._run() + with self.assertRaises(StageFileAlreadyExistsError): + self._run() class TestRunDeterministicChangedDepsList(TestRunDeterministicBase): def test(self): self.deps = [self.BAR, self.CODE] - self._run() + with self.assertRaises(StageFileAlreadyExistsError): + self._run() class TestRunDeterministicNewDep(TestRunDeterministicBase): def test(self): self.deps = [self.FOO, self.BAR, self.CODE] - self._run() + with self.assertRaises(StageFileAlreadyExistsError): + self._run() class TestRunDeterministicRemoveDep(TestRunDeterministicBase): def test(self): self.deps = [self.CODE] - self._run() + with self.assertRaises(StageFileAlreadyExistsError): + self._run() class TestRunDeterministicChangedOut(TestRunDeterministicBase): def test(self): os.unlink(self.out_file) self.out_file_mtime = None - self._run() + with self.assertRaises(StageFileAlreadyExistsError): + self._run() class TestRunDeterministicChangedCmd(TestRunDeterministicBase): def test(self): self.cmd += ' arg' - self._run() + with self.assertRaises(StageFileAlreadyExistsError): + self._run() From e81c6f2b34a0b74dee2d58fa8ba9c37240446cc0 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Mon, 3 Dec 2018 05:13:54 +0200 Subject: [PATCH 13/21] run: introduce --force option Signed-off-by: Ruslan Kuprieiev --- dvc/cli.py | 9 ++++++++- dvc/command/run.py | 6 ++---- tests/test_run.py | 17 +++++++++++++++++ 3 files changed, 27 insertions(+), 5 deletions(-) diff --git a/dvc/cli.py b/dvc/cli.py index 4f3ceb90b..4d218b2a8 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -268,9 +268,16 @@ def parse_args(argv=None): '--yes', action='store_true', default=False, - help="Automatic 'yes' answer to all prompts. E.g. " + help="(OBSOLETED, use --force instead) Automatic " + "'yes' answer to all prompts. E.g. " "when '.dvc' file exists and dvc asks if you " "want to overwrite it.") + run_parser.add_argument( + '--force', + action='store_true', + default=False, + help="Overwrite existing dvc file without asking " + "for confirmation.") run_parser.add_argument( 'command', nargs=argparse.REMAINDER, diff --git a/dvc/command/run.py b/dvc/command/run.py index 0fa2c2036..b6559e8f3 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -21,9 +21,6 @@ class CmdRun(CmdBase): def run(self): try: - if self.args.yes: - self.project.prompt.default = True - self.project.run(cmd=self._joined_cmd(), outs=self.args.outs, outs_no_cache=self.args.outs_no_cache, @@ -31,7 +28,8 @@ class CmdRun(CmdBase): deps=self.args.deps, fname=self.args.file, cwd=self.args.cwd, - no_exec=self.args.no_exec) + no_exec=self.args.no_exec, + overwrite=(self.args.yes or self.args.force)) except DvcException as ex: self.project.logger.error('Failed to run command', ex) return 1 diff --git a/tests/test_run.py b/tests/test_run.py index 5f7ee848f..bf5a3e229 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -185,6 +185,23 @@ class TestCmdRun(TestDvc): self.assertEqual(ret, 252) +class TestCmdRunOverwrite(TestDvc): + def test(self): + ret = main(['run', + '-d', self.FOO, + '-d', self.CODE, + '-o', 'out', + '-f', 'out.dvc', + 'python', self.CODE, self.FOO, 'out']) + self.assertEqual(ret, 0) + + ret = main(['run', + '--force', + '-f', 'out.dvc', + '-d', self.BAR]) + self.assertEqual(ret, 0) + + class TestRunDeterministicBase(TestDvc): def setUp(self): super(TestRunDeterministicBase, self).setUp() From ac824267883380ac5995ec14816f808d9a579e92 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 4 Dec 2018 07:17:16 +0200 Subject: [PATCH 14/21] run: introduce --overwrite-dvcfile and --ignore-build-cache Signed-off-by: Ruslan Kuprieiev --- dvc/cli.py | 9 ++++++++- dvc/command/run.py | 4 +++- dvc/project.py | 6 ++++-- dvc/stage.py | 13 +++++++------ tests/test_run.py | 44 +++++++++++++++++++++++++++++++++++++++++++- 5 files changed, 65 insertions(+), 11 deletions(-) diff --git a/dvc/cli.py b/dvc/cli.py index 4d218b2a8..6ff2a81d8 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -273,11 +273,18 @@ def parse_args(argv=None): "when '.dvc' file exists and dvc asks if you " "want to overwrite it.") run_parser.add_argument( - '--force', + '--overwrite-dvcfile', action='store_true', default=False, help="Overwrite existing dvc file without asking " "for confirmation.") + run_parser.add_argument( + '--ignore-build-cache', + action='store_true', + default=False, + help="Run this stage even if it has been already " + "ran with the same command/dependencies/outputs/etc " + "before.") run_parser.add_argument( 'command', nargs=argparse.REMAINDER, diff --git a/dvc/command/run.py b/dvc/command/run.py index b6559e8f3..cba595a8e 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -20,6 +20,7 @@ class CmdRun(CmdBase): return cmd def run(self): + overwrite = (self.args.yes or self.args.overwrite_dvcfile) try: self.project.run(cmd=self._joined_cmd(), outs=self.args.outs, @@ -29,7 +30,8 @@ class CmdRun(CmdBase): fname=self.args.file, cwd=self.args.cwd, no_exec=self.args.no_exec, - overwrite=(self.args.yes or self.args.force)) + overwrite=overwrite, + ignore_build_cache=self.args.ignore_build_cache) except DvcException as ex: self.project.logger.error('Failed to run command', ex) return 1 diff --git a/dvc/project.py b/dvc/project.py index 8dbc8d429..a56c50cfa 100644 --- a/dvc/project.py +++ b/dvc/project.py @@ -363,7 +363,8 @@ class Project(object): fname=None, cwd=os.curdir, no_exec=False, - overwrite=False): + overwrite=False, + ignore_build_cache=False): from dvc.stage import Stage with self.state: @@ -375,7 +376,8 @@ class Project(object): outs_no_cache=outs_no_cache, metrics_no_cache=metrics_no_cache, deps=deps, - overwrite=overwrite) + overwrite=overwrite, + ignore_build_cache=ignore_build_cache) if stage is None: return None diff --git a/dvc/stage.py b/dvc/stage.py index 09d3b846c..58b36c348 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -339,7 +339,8 @@ class Stage(object): cwd=os.curdir, locked=False, add=False, - overwrite=True): + overwrite=True, + ignore_build_cache=False): stage = Stage(project=project, cwd=cwd, @@ -367,14 +368,14 @@ class Stage(object): stage.cwd = cwd stage.path = path - if os.path.exists(path) and not overwrite: - if stage.is_cached(): + if os.path.exists(path): + if not ignore_build_cache and stage.is_cached(): Logger.info('Stage is cached, skipping.') return None - msg = "'{}' already exists. " \ - "Do you wish to run the command and overwrite it?" - if not project.prompt.prompt(msg.format(stage.relpath), False): + msg = "'{}' already exists. Do you wish to run the command and " \ + "overwrite it?".format(stage.relpath) + if not overwrite and not project.prompt.prompt(msg, False): raise StageFileAlreadyExistsError(stage.relpath) return stage diff --git a/tests/test_run.py b/tests/test_run.py index bf5a3e229..8770620bd 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -195,12 +195,51 @@ class TestCmdRunOverwrite(TestDvc): 'python', self.CODE, self.FOO, 'out']) self.assertEqual(ret, 0) + stage_mtime = os.path.getmtime('out.dvc') + out_mtime = os.path.getmtime('out') + ret = main(['run', - '--force', + '-d', self.FOO, + '-d', self.CODE, + '-o', 'out', + '-f', 'out.dvc', + 'python', self.CODE, self.FOO, 'out']) + self.assertEqual(ret, 0) + + # NOTE: check output and dvcfile were NOT overwritten + self.assertEqual(stage_mtime, os.path.getmtime('out.dvc')) + self.assertEqual(out_mtime, os.path.getmtime('out')) + + stage_mtime = os.path.getmtime('out.dvc') + out_mtime = os.path.getmtime('out') + + ret = main(['run', + '-d', self.FOO, + '-d', self.CODE, + '--overwrite-dvcfile', + '--ignore-build-cache', + '-o', 'out', + '-f', 'out.dvc', + 'python', self.CODE, self.FOO, 'out']) + self.assertEqual(ret, 0) + + # NOTE: check output and dvcfile were overwritten + self.assertNotEqual(stage_mtime, os.path.getmtime('out.dvc')) + self.assertNotEqual(out_mtime, os.path.getmtime('out')) + + stage_mtime = os.path.getmtime('out.dvc') + out_mtime = os.path.getmtime('out') + + ret = main(['run', + '--overwrite-dvcfile', '-f', 'out.dvc', '-d', self.BAR]) self.assertEqual(ret, 0) + # NOTE: check output was NOT overwritten, but dvcfile was + self.assertEqual(out_mtime, os.path.getmtime('out')) + self.assertNotEqual(stage_mtime, os.path.getmtime('out.dvc')) + class TestRunDeterministicBase(TestDvc): def setUp(self): @@ -211,6 +250,7 @@ class TestRunDeterministicBase(TestDvc): self.deps = [self.FOO, self.CODE] self.outs = [self.out_file] self.overwrite = False + self.ignore_build_cache = False self._run() @@ -218,6 +258,7 @@ class TestRunDeterministicBase(TestDvc): self.stage = self.dvc.run(cmd=self.cmd, fname=self.stage_file, overwrite=self.overwrite, + ignore_build_cache=self.ignore_build_cache, deps=self.deps, outs=self.outs) @@ -230,6 +271,7 @@ class TestRunDeterministic(TestRunDeterministicBase): class TestRunDeterministicOverwrite(TestRunDeterministicBase): def test(self): self.overwrite = True + self.ignore_build_cache = True self._run() From f35cf3cf9957d41bacbede91071a93cd9a64a03b Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 4 Dec 2018 07:20:17 +0200 Subject: [PATCH 15/21] cli: fix typo in `dvc run` description Signed-off-by: Ruslan Kuprieiev --- dvc/cli.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/dvc/cli.py b/dvc/cli.py index 6ff2a81d8..15a92bc55 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -268,8 +268,8 @@ def parse_args(argv=None): '--yes', action='store_true', default=False, - help="(OBSOLETED, use --force instead) Automatic " - "'yes' answer to all prompts. E.g. " + help="(OBSOLETED, use --overwrite-dvcfile instead) " + "Automatic 'yes' answer to all prompts. E.g. " "when '.dvc' file exists and dvc asks if you " "want to overwrite it.") run_parser.add_argument( From 83ee6e46e037d7c471c8da70d3af689203e2a266 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 4 Dec 2018 07:49:21 +0200 Subject: [PATCH 16/21] run: introduce --remove-outs option Signed-off-by: Ruslan Kuprieiev --- dvc/cli.py | 5 +++++ dvc/command/run.py | 3 ++- dvc/project.py | 6 ++++-- dvc/stage.py | 7 ++++++- tests/test_run.py | 17 +++++++++++++++++ 5 files changed, 34 insertions(+), 4 deletions(-) diff --git a/dvc/cli.py b/dvc/cli.py index 15a92bc55..7522557f0 100644 --- a/dvc/cli.py +++ b/dvc/cli.py @@ -285,6 +285,11 @@ def parse_args(argv=None): help="Run this stage even if it has been already " "ran with the same command/dependencies/outputs/etc " "before.") + run_parser.add_argument( + '--remove-outs', + action='store_true', + default=False, + help="Remove outputs before running the command.") run_parser.add_argument( 'command', nargs=argparse.REMAINDER, diff --git a/dvc/command/run.py b/dvc/command/run.py index cba595a8e..79857919d 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -31,7 +31,8 @@ class CmdRun(CmdBase): cwd=self.args.cwd, no_exec=self.args.no_exec, overwrite=overwrite, - ignore_build_cache=self.args.ignore_build_cache) + ignore_build_cache=self.args.ignore_build_cache, + remove_outs=self.args.remove_outs) except DvcException as ex: self.project.logger.error('Failed to run command', ex) return 1 diff --git a/dvc/project.py b/dvc/project.py index a56c50cfa..84d7ddf58 100644 --- a/dvc/project.py +++ b/dvc/project.py @@ -364,7 +364,8 @@ class Project(object): cwd=os.curdir, no_exec=False, overwrite=False, - ignore_build_cache=False): + ignore_build_cache=False, + remove_outs=False): from dvc.stage import Stage with self.state: @@ -377,7 +378,8 @@ class Project(object): metrics_no_cache=metrics_no_cache, deps=deps, overwrite=overwrite, - ignore_build_cache=ignore_build_cache) + ignore_build_cache=ignore_build_cache, + remove_outs=remove_outs) if stage is None: return None diff --git a/dvc/stage.py b/dvc/stage.py index 58b36c348..9c1b754e0 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -340,7 +340,8 @@ class Stage(object): locked=False, add=False, overwrite=True, - ignore_build_cache=False): + ignore_build_cache=False, + remove_outs=False): stage = Stage(project=project, cwd=cwd, @@ -368,6 +369,10 @@ class Stage(object): stage.cwd = cwd stage.path = path + # NOTE: remove outs before we check build cache + if remove_outs: + stage.remove_outs(ignore_remove=False) + if os.path.exists(path): if not ignore_build_cache and stage.is_cached(): Logger.info('Stage is cached, skipping.') diff --git a/tests/test_run.py b/tests/test_run.py index 8770620bd..158929583 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -185,6 +185,23 @@ class TestCmdRun(TestDvc): self.assertEqual(ret, 252) +class TestRunRemoveOuts(TestDvc): + def test(self): + with open(self.CODE, 'w+') as fobj: + fobj.write("import sys\n") + fobj.write("import os\n") + fobj.write("if os.path.exists(sys.argv[1]):\n") + fobj.write(" sys.exit(1)\n") + fobj.write("open(sys.argv[1], 'w+').close()\n") + + ret = main(['run', + '--remove-outs', + '-d', self.CODE, + '-o', self.FOO, + 'python', self.CODE, self.FOO]) + self.assertEqual(ret, 0) + + class TestCmdRunOverwrite(TestDvc): def test(self): ret = main(['run', From a7c177d757c0efce353a29255305f9f9b42cc289 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 4 Dec 2018 09:41:18 +0200 Subject: [PATCH 17/21] run: unprotect outs by default Signed-off-by: Ruslan Kuprieiev --- dvc/stage.py | 11 +++++ tests/test_run.py | 120 +++++++++++++++++++++++++++++++++++++++++----- 2 files changed, 120 insertions(+), 11 deletions(-) diff --git a/dvc/stage.py b/dvc/stage.py index 9c1b754e0..952197872 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -213,6 +213,12 @@ class Stage(object): for out in self.outs: out.remove(ignore_remove=ignore_remove) + def unprotect_outs(self): + for out in self.outs: + if out.path_info['scheme'] != 'local' or not out.exists: + continue + self.project.unprotect(out.path) + def remove(self): self.remove_outs(ignore_remove=True) os.unlink(self.path) @@ -372,6 +378,11 @@ class Stage(object): # NOTE: remove outs before we check build cache if remove_outs: stage.remove_outs(ignore_remove=False) + project.logger.warn("Build cache is ignored when using " + "--remove-outs.") + ignore_build_cache = True + else: + stage.unprotect_outs() if os.path.exists(path): if not ignore_build_cache and stage.is_cached(): diff --git a/tests/test_run.py b/tests/test_run.py index 158929583..2a979a4b7 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -202,6 +202,112 @@ class TestRunRemoveOuts(TestDvc): self.assertEqual(ret, 0) +class TestRunUnprotectOutsCopy(TestDvc): + def test(self): + with open(self.CODE, 'w+') as fobj: + fobj.write("import sys\n") + fobj.write("with open(sys.argv[1], 'a+') as fobj:\n") + fobj.write(" fobj.write('foo')\n") + + ret = main(['config', 'cache.protected', 'true']) + self.assertEqual(ret, 0) + + ret = main(['config', 'cache.type', 'copy']) + self.assertEqual(ret, 0) + + ret = main(['run', + '-d', self.CODE, + '-o', self.FOO, + 'python', self.CODE, self.FOO]) + self.assertEqual(ret, 0) + self.assertFalse(os.access(self.FOO, os.W_OK)) + self.assertEqual(open(self.FOO, 'r').read(), 'foofoo') + + ret = main(['run', + '--overwrite-dvcfile', + '--ignore-build-cache', + '-d', self.CODE, + '-o', self.FOO, + 'python', self.CODE, self.FOO]) + self.assertEqual(ret, 0) + self.assertFalse(os.access(self.FOO, os.W_OK)) + self.assertEqual(open(self.FOO, 'r').read(), 'foofoofoo') + + +class TestRunUnprotectOutsSymlink(TestDvc): + def test(self): + with open(self.CODE, 'w+') as fobj: + fobj.write("import sys\n") + fobj.write("import os\n") + fobj.write("assert os.path.exists(sys.argv[1])\n") + fobj.write("with open(sys.argv[1], 'a+') as fobj:\n") + fobj.write(" fobj.write('foo')\n") + + ret = main(['config', 'cache.protected', 'true']) + self.assertEqual(ret, 0) + + ret = main(['config', 'cache.type', 'symlink']) + self.assertEqual(ret, 0) + + self.assertEqual(ret, 0) + ret = main(['run', + '-d', self.CODE, + '-o', self.FOO, + 'python', self.CODE, self.FOO]) + self.assertEqual(ret, 0) + self.assertFalse(os.access(self.FOO, os.W_OK)) + self.assertTrue(os.path.islink(self.FOO)) + self.assertEqual(open(self.FOO, 'r').read(), 'foofoo') + + ret = main(['run', + '--overwrite-dvcfile', + '--ignore-build-cache', + '-d', self.CODE, + '-o', self.FOO, + 'python', self.CODE, self.FOO]) + self.assertEqual(ret, 0) + self.assertFalse(os.access(self.FOO, os.W_OK)) + self.assertTrue(os.path.islink(self.FOO)) + self.assertEqual(open(self.FOO, 'r').read(), 'foofoofoo') + + +class TestRunUnprotectOutsHardlink(TestDvc): + def test(self): + with open(self.CODE, 'w+') as fobj: + fobj.write("import sys\n") + fobj.write("import os\n") + fobj.write("assert os.path.exists(sys.argv[1])\n") + fobj.write("with open(sys.argv[1], 'a+') as fobj:\n") + fobj.write(" fobj.write('foo')\n") + + ret = main(['config', 'cache.protected', 'true']) + self.assertEqual(ret, 0) + + ret = main(['config', 'cache.type', 'hardlink']) + self.assertEqual(ret, 0) + + self.assertEqual(ret, 0) + ret = main(['run', + '-d', self.CODE, + '-o', self.FOO, + 'python', self.CODE, self.FOO]) + self.assertEqual(ret, 0) + self.assertFalse(os.access(self.FOO, os.W_OK)) + self.assertEqual(os.stat(self.FOO).st_nlink, 2) + self.assertEqual(open(self.FOO, 'r').read(), 'foofoo') + + ret = main(['run', + '--overwrite-dvcfile', + '--ignore-build-cache', + '-d', self.CODE, + '-o', self.FOO, + 'python', self.CODE, self.FOO]) + self.assertEqual(ret, 0) + self.assertFalse(os.access(self.FOO, os.W_OK)) + self.assertEqual(os.stat(self.FOO).st_nlink, 2) + self.assertEqual(open(self.FOO, 'r').read(), 'foofoofoo') + + class TestCmdRunOverwrite(TestDvc): def test(self): ret = main(['run', @@ -213,7 +319,6 @@ class TestCmdRunOverwrite(TestDvc): self.assertEqual(ret, 0) stage_mtime = os.path.getmtime('out.dvc') - out_mtime = os.path.getmtime('out') ret = main(['run', '-d', self.FOO, @@ -223,12 +328,9 @@ class TestCmdRunOverwrite(TestDvc): 'python', self.CODE, self.FOO, 'out']) self.assertEqual(ret, 0) - # NOTE: check output and dvcfile were NOT overwritten + # NOTE: check that dvcfile was NOT overwritten self.assertEqual(stage_mtime, os.path.getmtime('out.dvc')) - self.assertEqual(out_mtime, os.path.getmtime('out')) - stage_mtime = os.path.getmtime('out.dvc') - out_mtime = os.path.getmtime('out') ret = main(['run', '-d', self.FOO, @@ -240,12 +342,9 @@ class TestCmdRunOverwrite(TestDvc): 'python', self.CODE, self.FOO, 'out']) self.assertEqual(ret, 0) - # NOTE: check output and dvcfile were overwritten + # NOTE: check that dvcfile was overwritten self.assertNotEqual(stage_mtime, os.path.getmtime('out.dvc')) - self.assertNotEqual(out_mtime, os.path.getmtime('out')) - stage_mtime = os.path.getmtime('out.dvc') - out_mtime = os.path.getmtime('out') ret = main(['run', '--overwrite-dvcfile', @@ -253,8 +352,7 @@ class TestCmdRunOverwrite(TestDvc): '-d', self.BAR]) self.assertEqual(ret, 0) - # NOTE: check output was NOT overwritten, but dvcfile was - self.assertEqual(out_mtime, os.path.getmtime('out')) + # NOTE: check that dvcfile was overwritten self.assertNotEqual(stage_mtime, os.path.getmtime('out.dvc')) From 2d9ec1005b1fb5dbaabfb1779ad18d4ba05adcb8 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 4 Dec 2018 10:46:10 +0200 Subject: [PATCH 18/21] unprotect: only process symlinks and hardlinks Fixes #1373 Signed-off-by: Ruslan Kuprieiev --- dvc/project.py | 15 ++++++++++----- dvc/system.py | 19 +++++++++++++++++++ 2 files changed, 29 insertions(+), 5 deletions(-) diff --git a/dvc/project.py b/dvc/project.py index 84d7ddf58..0ce183565 100644 --- a/dvc/project.py +++ b/dvc/project.py @@ -325,16 +325,21 @@ class Project(object): def _unprotect_file(self, path): import stat import uuid + from dvc.system import System from dvc.utils import copyfile, move, remove - self.logger.debug("Unprotecting '{}'".format(path)) + if System.is_symlink(path) or System.is_hardlink(path): + self.logger.debug("Unprotecting '{}'".format(path)) - tmp = os.path.join(os.path.dirname(path), '.' + str(uuid.uuid4())) - move(path, tmp) + tmp = os.path.join(os.path.dirname(path), '.' + str(uuid.uuid4())) + move(path, tmp) - copyfile(tmp, path) + copyfile(tmp, path) - remove(tmp) + remove(tmp) + else: + self.logger.debug("Skipping copying for '{}', since it is not " + "a symlink or a hardlink.".format(path)) os.chmod(path, os.stat(path).st_mode | stat.S_IWRITE) diff --git a/dvc/system.py b/dvc/system.py index bba11fa1e..6c3c3fca8 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -240,3 +240,22 @@ class System(object): return System._wait_for_input_posix(timeout) else: return System._wait_for_input_windows(timeout) + + @staticmethod + def is_symlink(path): + if System.is_unix(): + return os.path.islink(path) + + # https://docs.microsoft.com/en-us/windows/desktop/fileio/ + # file-attribute-constants + FILE_ATTRIBUTE_REPARSE_POINT = 0x400 + info = System.getdirinfo(path) + return info.dwFileAttributes & FILE_ATTRIBUTE_REPARSE_POINT + + @staticmethod + def is_hardlink(path): + if System.is_unix(): + return os.stat(path).st_nlink > 1 + + info = System.getdirinfo(path) + return info.nNumberOfLinks > 1 From 4230b8180bc53ac691642991f8faf2f2df411f70 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 4 Dec 2018 11:12:55 +0200 Subject: [PATCH 19/21] test: use System.is_{sym,hard}link Signed-off-by: Ruslan Kuprieiev --- tests/test_run.py | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) diff --git a/tests/test_run.py b/tests/test_run.py index 2a979a4b7..445264920 100644 --- a/tests/test_run.py +++ b/tests/test_run.py @@ -8,6 +8,7 @@ import subprocess from dvc.project import Project from dvc.main import main from dvc.utils import file_md5 +from dvc.system import System from dvc.stage import Stage, StageFileBadNameError, MissingDep from dvc.stage import StageBadCwdError, StageFileAlreadyExistsError from dvc.command.run import CmdRun @@ -256,7 +257,7 @@ class TestRunUnprotectOutsSymlink(TestDvc): 'python', self.CODE, self.FOO]) self.assertEqual(ret, 0) self.assertFalse(os.access(self.FOO, os.W_OK)) - self.assertTrue(os.path.islink(self.FOO)) + self.assertTrue(System.is_symlink(self.FOO)) self.assertEqual(open(self.FOO, 'r').read(), 'foofoo') ret = main(['run', @@ -267,7 +268,7 @@ class TestRunUnprotectOutsSymlink(TestDvc): 'python', self.CODE, self.FOO]) self.assertEqual(ret, 0) self.assertFalse(os.access(self.FOO, os.W_OK)) - self.assertTrue(os.path.islink(self.FOO)) + self.assertTrue(System.is_symlink(self.FOO)) self.assertEqual(open(self.FOO, 'r').read(), 'foofoofoo') @@ -293,7 +294,7 @@ class TestRunUnprotectOutsHardlink(TestDvc): 'python', self.CODE, self.FOO]) self.assertEqual(ret, 0) self.assertFalse(os.access(self.FOO, os.W_OK)) - self.assertEqual(os.stat(self.FOO).st_nlink, 2) + self.assertTrue(System.is_hardlink(self.FOO)) self.assertEqual(open(self.FOO, 'r').read(), 'foofoo') ret = main(['run', @@ -304,7 +305,7 @@ class TestRunUnprotectOutsHardlink(TestDvc): 'python', self.CODE, self.FOO]) self.assertEqual(ret, 0) self.assertFalse(os.access(self.FOO, os.W_OK)) - self.assertEqual(os.stat(self.FOO).st_nlink, 2) + self.assertTrue(System.is_hardlink(self.FOO)) self.assertEqual(open(self.FOO, 'r').read(), 'foofoofoo') From 2da24b38a71f36b8f38e7738431d2291efa77d6c Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 4 Dec 2018 11:49:58 +0200 Subject: [PATCH 20/21] windows: don't follow symlink Signed-off-by: Ruslan Kuprieiev --- dvc/system.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/dvc/system.py b/dvc/system.py index 6c3c3fca8..40591c7af 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -127,6 +127,11 @@ class System(object): from ctypes import c_void_p, c_wchar_p, Structure, WinError, POINTER from ctypes.wintypes import DWORD, HANDLE, BOOL + # NOTE: use this flag to open symlink itself and not the target + # See https://docs.microsoft.com/en-us/windows/desktop/api/ + # fileapi/nf-fileapi-createfilew#symbolic-link-behavior + FILE_FLAG_OPEN_REPARSE_POINT = 0x00200000 + FILE_FLAG_BACKUP_SEMANTICS = 0x02000000 FILE_SHARE_READ = 0x00000001 OPEN_EXISTING = 3 @@ -147,7 +152,7 @@ class System(object): ("nFileIndexHigh", DWORD), ("nFileIndexLow", DWORD)] - flags = FILE_FLAG_BACKUP_SEMANTICS + flags = FILE_FLAG_BACKUP_SEMANTICS & FILE_FLAG_OPEN_REPARSE_POINT func = ctypes.windll.kernel32.CreateFileW func.argtypes = [c_wchar_p, From ee16d2f1d761a69c6529f8e13b37a75a0c9d2980 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Tue, 4 Dec 2018 12:00:01 +0200 Subject: [PATCH 21/21] system: fix typo Signed-off-by: Ruslan Kuprieiev --- dvc/system.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/dvc/system.py b/dvc/system.py index 40591c7af..767b12607 100644 --- a/dvc/system.py +++ b/dvc/system.py @@ -152,7 +152,7 @@ class System(object): ("nFileIndexHigh", DWORD), ("nFileIndexLow", DWORD)] - flags = FILE_FLAG_BACKUP_SEMANTICS & FILE_FLAG_OPEN_REPARSE_POINT + flags = FILE_FLAG_BACKUP_SEMANTICS | FILE_FLAG_OPEN_REPARSE_POINT func = ctypes.windll.kernel32.CreateFileW func.argtypes = [c_wchar_p,