[WIP] Support for alternative hashes (#1676) #2022

shcheklein · 2019-05-19T23:53:48Z

[x ] Have you followed the guidelines in our
Contributing document?
Does your PR affect documented changes or does it add new functionality
that should be documented? If yes, have you created a PR for
dvc.org documenting it or at
least opened an issue for it? If so, please add a link to it.

Based on a fork by @vyloy .

shcheklein

#1920 - address comments from the previous PR

Suor · 2019-05-21T07:25:28Z

dvc/config.py

+        checksum_types, modchecksum.LOCAL_SUPPORTED_CHECKSUM_TYPES
+    ):
+        return True
+    return False


Why not simply:

return bool(modchecksum.checksum_types_from_str(...))

?

Suor · 2019-05-21T07:31:26Z

dvc/output/base.py

+            info = self.info.get(t)
+            if info:
+                return info
+        return None


Could be:

return next((self.info[t] for t in self.remote.checksum_types() if t in self.info), None) # or return next(filter(bool, map(self.info.get, self.remote.checksum_types())), None)

:)

Or

return self.info[self.checksum]

Suor · 2019-05-21T07:36:26Z

dvc/remote/base.py

+
+    def checksum_type(self):
+        return self._checksum_types[0]
+


Make this properties as above?

Also is that an error if we have more than one here? If yes than I suggest adding an assert.

Suor · 2019-05-21T07:38:10Z

dvc/remote/base.py

@@ -275,13 +299,55 @@ def get_checksum(self, path_info):
            checksum = self.get_file_checksum(path_info)

        if checksum:
-            self.state.save(path_info, checksum)
+            self.state.save(path_info, checksum, checksum_type)


Use a decorator to abstract away caching?

Suor · 2019-05-21T07:43:03Z

dvc/remote/base.py

+        types = set(checksum_info.keys())
+        supported_types = set(self.checksum_types())
+        intersection = supported_types.intersection(types)
+        if not supported_types.intersection(types):


No need to call .keys() just types = set(checksum_info). Actually Python has optimized code path to make sets from dicts. intersection is computed twice.

Suor · 2019-05-21T07:47:58Z

dvc/remote/base.py

+                            addition_check, path_info, checksum_info
+                        )
+                    )
+                    return True


Is this addition_check only purpose to pass there self.changed_cache? If so won't it be more clear to simply use bool parameter?

Suor · 2019-05-21T07:53:29Z

dvc/utils/checksum.py

+AZURE_SUPPORTED_CHECKSUM_TYPES = [CHECKSUM_ETAG]
+GS_SUPPORTED_CHECKSUM_TYPES = [CHECKSUM_MD5]
+HDFS_SUPPORTED_CHECKSUM_TYPES = [CHECKSUM_CHECKSUM]
+S3_SUPPORTED_CHECKSUM_TYPES = [CHECKSUM_ETAG]


Shouldn't this go to appropriate modules and just stay contained there? This will eliminate one level of indirection - these constants.

They will go to the appropriate modules for sure 👍

Suor · 2019-05-21T07:57:49Z

dvc/remote/local/__init__.py

@@ -207,7 +203,7 @@ def walk(self, path_info):
        return os.walk(path_info.path)

    def get_file_checksum(self, path_info):
-        return file_md5(path_info.path)[0]
+        return file_checksum(path_info.path, self._checksum_types[0])[0]


Looks like file_md5/file_checksum second result is never used. Maybe now is a good time to drop it and return only hex.

Suor · 2019-05-21T08:02:51Z

dvc/stage.py

        Optional(PARAM_CMD): Or(str, None),
        Optional(PARAM_WDIR): Or(str, None),
        Optional(PARAM_DEPS): Or(And(list, Schema([dependency.SCHEMA])), None),
        Optional(PARAM_OUTS): Or(And(list, Schema([output.SCHEMA])), None),
        Optional(PARAM_LOCKED): bool,
        Optional(PARAM_META): object,
-    }
+    }.items():
+        SCHEMA[k] = v


This is just:

SCHEMA.update({ Optional(PARAM_CMD): Or(str, None), ... })

Suor · 2019-05-21T08:07:27Z

dvc/stage.py

+                self._changed_deps(),
+                self._changed_outs(),
+                self._changed_checksum(),
+            ]
        )


Using ors instead of any() here will simplify code and provide short-circuit behavior.

Suor · 2019-05-21T08:11:03Z

dvc/stage.py

+            v = d.get(k)
+            if v:
+                checksum[k] = v
+


checksum = {k: d[k] for k in modchecksum.CHECKSUM_MAP if d.get(k)} # Or if you don't expect falsy values in `d`: checksum = {k: d[k] for k in modchecksum.CHECKSUM_MAP if k in d}

Or using funcy:

from funcy import project checksum = project(d, modchecksum.CHECKSUM_MAP)

Funcy is my lib :)

Suor · 2019-05-21T08:16:48Z

dvc/stage.py

+        r = {}
+        for key, value in self.checksum.items():
+            if value:
+                r[key] = value


r = {k: v for k, v in self.checksum.items() if v}

Or using funcy:

from funcy import compact r = compact(self.checksum)

Suor · 2019-05-21T08:21:45Z

dvc/stage.py

+            if value:
+                r[key] = value
+
+        return r


The whole thing is merging of two dicts with a filter:

from itertools import chain d1 = self.checksum d2 = {...} return {k: v for k, v in chain(d1.items(), d2.items()) if v} # or with funcy from funcy import merge, compact d1 = self.checksum d2 = {...} return compact(merge(d1, d2))

Suor · 2019-05-21T08:27:24Z

dvc/utils/checksum.py

+        return None
+    for h in checksum_types:
+        if h not in supported_types:
+            return None


Could be

if not set(checksum_types) <= set(supported_types): return None

Suor · 2019-05-21T08:28:33Z

dvc/utils/checksum.py

+        ret = []
+        for e in d:
+            ret.append(dict_filter(e, exclude))
+        return ret


A good place to use list comprehension.

Suor · 2019-05-21T09:18:57Z

tests/unit/remote/hash/test_hash.py

+        self.assertEqual(
+            d["sha256"],
+            "6bc1683385ab4218fed1f53195c41d95d186c085a9e929cd77f012d4f541dd34",
+        )


These look like functional tests not unit ones. Also could be easily written in new pytest style using dvc fixture:

def test_compatibility(dvc): ret = main(...) assert ret == 0 # ....

shcheklein · 2019-05-21T21:32:14Z

@Suor thanks for reviewing this. It was started by an external contributor and am trying to get it done, I don't expect it to be fast though. I'll ping you again when it's in a better shape and some fundamental issues are solved. I'll address your comments along the way, thanks!

…ent error

rename get_checksum_type_list to checksum_types

shcheklein commented May 19, 2019

View reviewed changes

shcheklein mentioned this pull request May 19, 2019

[WIP] Alterative hashings for data (#1676) #1920

Closed

Suor reviewed May 21, 2019

View reviewed changes

vyloy added 11 commits May 27, 2019 16:51

config: Alterative hashings for data (iterative#1676)

55349af

config: use hash section for hashing configuration

6453c16

utils: rename file_md5 to file_checksum, dict_md5 to dict_checksum

7ebedc4

stage: rename _compute_md5 to _compute_checksum

a367dc3

remote: fix error

e8d790c

remote: improve some details about PARAM_CHECKSUM

52f67de

config: separate hash library and improve some details

31616f3

hash: add a test about changed file

e941d7c

remote: improvements after more tests

b2ecd91

rename variable to fix python2.7 object does not support item assignm…

3f0b14b

…ent error

rename mod hash to checksum

b2a25a0

vyloy and others added 9 commits May 27, 2019 17:39

refactor the code to deal with checksum type

de52cf0

rename legacy function about md5 to checksum

90d65a2

collect schemas

f62d053

increase state version

060f632

fix error after rebase

62c6339

refactor remote.changed

44059a5

rename get_prefer_checksum_type to checksum_type

f0dbadf

rename get_checksum_type_list to checksum_types

remote: initially add checksum types config ability for other remotes

6728cea

move dict filter to utils collections

befb5ee

shcheklein force-pushed the fix-1676 branch from cf00693 to befb5ee Compare May 28, 2019 00:51

efiop mentioned this pull request May 28, 2019

overview at the end of dvc repro and the deterministic feature #2050

Closed

shcheklein closed this May 28, 2019

adrinjalali mentioned this pull request Jan 6, 2020

Support hashings other than MD5 #3069

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[WIP] Support for alternative hashes (#1676) #2022

[WIP] Support for alternative hashes (#1676) #2022

shcheklein commented May 19, 2019

shcheklein left a comment

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019 •

edited

Loading

Suor May 21, 2019

shcheklein May 21, 2019

Suor May 21, 2019 •

edited

Loading

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

Suor May 21, 2019

shcheklein commented May 21, 2019

[WIP] Support for alternative hashes (#1676) #2022

[WIP] Support for alternative hashes (#1676) #2022

Conversation

shcheklein commented May 19, 2019

shcheklein left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suor May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suor May 21, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein commented May 21, 2019

Suor May 21, 2019 •

edited

Loading

Suor May 21, 2019 •

edited

Loading