From f37ab9431b963a065751c8d4e0f487e3b3a6cae4 Mon Sep 17 00:00:00 2001 From: Alexander Schepanovski Date: Tue, 23 Jul 2019 15:29:49 +0700 Subject: [PATCH 1/3] perf: stop on first changed thing in Stage.changed() --- dvc/stage.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dvc/stage.py b/dvc/stage.py index 509d49a62..8ac073c8c 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -277,8 +277,9 @@ class Stage(object): return False def changed(self): - ret = any( - [self._changed_deps(), self._changed_outs(), self._changed_md5()] + # Short-circuit order: stage md5 is fast, deps are expected to change + ret = ( + self._changed_md5() or self._changed_deps() or self._changed_outs() ) if ret: From f00a32511407085c6d6787548256f5260c2e5862 Mon Sep 17 00:00:00 2001 From: Alexander Schepanovski Date: Tue, 23 Jul 2019 15:46:24 +0700 Subject: [PATCH 2/3] dvc: refactor dict_md5() - stop using mutable default value - use list and dict comprehensions to speed it up and make nicer - employ exhaustive if..elif..else --- dvc/utils/__init__.py | 30 +++++++++++++----------------- 1 file changed, 13 insertions(+), 17 deletions(-) diff --git a/dvc/utils/__init__.py b/dvc/utils/__init__.py index ed04b8ace..b7c98211f 100644 --- a/dvc/utils/__init__.py +++ b/dvc/utils/__init__.py @@ -88,32 +88,28 @@ def bytes_md5(byts): return hasher.hexdigest() -def dict_filter(d, exclude=[]): +def dict_filter(d, exclude=()): """ Exclude specified keys from a nested dict """ + def fix_key(k): + return str(k) if isinstance(k, builtin_str) else k + if isinstance(d, list): - ret = [] - for e in d: - ret.append(dict_filter(e, exclude)) - return ret + return [dict_filter(e, exclude) for e in d] + elif isinstance(d, dict): - ret = {} - for k, v in d.items(): - if isinstance(k, builtin_str): - k = str(k) + items = ((fix_key(k), v) for k, v in d.items()) + return { + k: dict_filter(v, exclude) for k, v in items if k not in exclude + } - assert isinstance(k, str) - if k in exclude: - continue - ret[k] = dict_filter(v, exclude) - return ret - - return d + else: + return d -def dict_md5(d, exclude=[]): +def dict_md5(d, exclude=()): filtered = dict_filter(d, exclude) byts = json.dumps(filtered, sort_keys=True).encode("utf-8") return bytes_md5(byts) From 35a8d69c4f46c4c8d92f115963ee3b27b147c657 Mon Sep 17 00:00:00 2001 From: Alexander Schepanovski Date: Tue, 23 Jul 2019 12:16:07 +0300 Subject: [PATCH 3/3] dvc: ignore Stage.meta in md5 calculations Meta change surely should not trigger a repro run. Closes #2209. --- dvc/stage.py | 6 ++-- tests/unit/test_stage.py | 72 ++++++++++++++++++---------------------- 2 files changed, 35 insertions(+), 43 deletions(-) diff --git a/dvc/stage.py b/dvc/stage.py index 8ac073c8c..e93d5ab92 100644 --- a/dvc/stage.py +++ b/dvc/stage.py @@ -657,9 +657,9 @@ class Stage(object): d = self.dumpd() - # NOTE: removing md5 manually in order to not affect md5s in deps/outs - if self.PARAM_MD5 in d.keys(): - del d[self.PARAM_MD5] + # Remove md5 and meta, these should not affect stage md5 + d.pop(self.PARAM_MD5, None) + d.pop(self.PARAM_META, None) # Ignore the wdir default value. In this case DVC-file w/o # wdir has the same md5 as a file with the default value specified. diff --git a/tests/unit/test_stage.py b/tests/unit/test_stage.py index c39a7f567..a477059a8 100644 --- a/tests/unit/test_stage.py +++ b/tests/unit/test_stage.py @@ -7,51 +7,43 @@ import pytest from unittest import TestCase -class TestStageChecksum(TestCase): - def test(self): - stage = Stage(None, "path") - outs = [{"path": "a", "md5": "123456789"}] - deps = [{"path": "b", "md5": "987654321"}] - d = {"md5": "123456", "cmd": "mycmd", "outs": outs, "deps": deps} +TEST_STAGE_DICT = { + "md5": "123456", + "cmd": "mycmd", + "outs": [{"path": "a", "md5": "123456789"}], + "deps": [{"path": "b", "md5": "987654321"}], +} - with mock.patch.object(stage, "dumpd", return_value=d): - self.assertEqual( - stage._compute_md5(), "e9521a22111493406ea64a88cda63e0b" - ) - def test_wdir_default_ignored(self): - stage = Stage(None, "path") - outs = [{"path": "a", "md5": "123456789"}] - deps = [{"path": "b", "md5": "987654321"}] - d = { - "md5": "123456", - "cmd": "mycmd", - "outs": outs, - "deps": deps, - "wdir": ".", - } +def test_stage_checksum(): + stage = Stage(None, "path") - with mock.patch.object(stage, "dumpd", return_value=d): - self.assertEqual( - stage._compute_md5(), "e9521a22111493406ea64a88cda63e0b" - ) + with mock.patch.object(stage, "dumpd", return_value=TEST_STAGE_DICT): + assert stage._compute_md5() == "e9521a22111493406ea64a88cda63e0b" - def test_wdir_non_default_is_not_ignored(self): - stage = Stage(None, "path") - outs = [{"path": "a", "md5": "123456789"}] - deps = [{"path": "b", "md5": "987654321"}] - d = { - "md5": "123456", - "cmd": "mycmd", - "outs": outs, - "deps": deps, - "wdir": "..", - } - with mock.patch.object(stage, "dumpd", return_value=d): - self.assertEqual( - stage._compute_md5(), "2ceba15e87f6848aa756502c1e6d24e9" - ) +def test_wdir_default_ignored(): + stage = Stage(None, "path") + d = dict(TEST_STAGE_DICT, wdir=".") + + with mock.patch.object(stage, "dumpd", return_value=d): + assert stage._compute_md5() == "e9521a22111493406ea64a88cda63e0b" + + +def test_wdir_non_default_is_not_ignored(): + stage = Stage(None, "path") + d = dict(TEST_STAGE_DICT, wdir="..") + + with mock.patch.object(stage, "dumpd", return_value=d): + assert stage._compute_md5() == "2ceba15e87f6848aa756502c1e6d24e9" + + +def test_meta_ignored(): + stage = Stage(None, "path") + d = dict(TEST_STAGE_DICT, meta={"author": "Suor"}) + + with mock.patch.object(stage, "dumpd", return_value=d): + assert stage._compute_md5() == "e9521a22111493406ea64a88cda63e0b" class TestPathConversion(TestCase):