Repository: iterative/dvc

Issue:
DVC does fails when the md5 hash contains only numbers
The description is in the title. When an md5 hash randomly contains only numbers DVC fails,  reporting

    ERROR: failed to update 'step_X.dvc'. - DVC-file 'step_X.dvc' format error: expected str for dictionary value @ data['md5']

This should happen on average once for every 3 million hashes. For a big project with a lot of files that get changed, that's not so implausible. 

Discussion:

Patch:
diff --git a/dvc/output/__init__.py b/dvc/output/__init__.py
index abcf68191..e86deeba5 100644
--- a/dvc/output/__init__.py
+++ b/dvc/output/__init__.py
@@ -1,5 +1,5 @@
 from urllib.parse import urlparse
-from voluptuous import Any, Required
+from voluptuous import Any, Required, Lower, Length, Coerce, And, SetTo
 
 from dvc.output.base import OutputBase
 from dvc.output.gs import OutputGS
@@ -29,6 +29,12 @@ OUTS_MAP = {
     Schemes.LOCAL: OutputLOCAL,
 }
 
+CHECKSUM_SCHEMA = Any(
+    None,
+    And(str, Length(max=0), SetTo(None)),
+    And(Any(str, And(int, Coerce(str))), Length(min=3), Lower),
+)
+
 # NOTE: currently there are only 3 possible checksum names:
 #
 #    1) md5 (LOCAL, SSH, GS);
@@ -37,15 +43,15 @@ OUTS_MAP = {
 #
 # so when a few types of outputs share the same name, we only need
 # specify it once.
-CHECKSUM_SCHEMA = {
-    RemoteLOCAL.PARAM_CHECKSUM: Any(str, None),
-    RemoteS3.PARAM_CHECKSUM: Any(str, None),
-    RemoteHDFS.PARAM_CHECKSUM: Any(str, None),
+CHECKSUMS_SCHEMA = {
+    RemoteLOCAL.PARAM_CHECKSUM: CHECKSUM_SCHEMA,
+    RemoteS3.PARAM_CHECKSUM: CHECKSUM_SCHEMA,
+    RemoteHDFS.PARAM_CHECKSUM: CHECKSUM_SCHEMA,
 }
 
-TAGS_SCHEMA = {str: CHECKSUM_SCHEMA}
+TAGS_SCHEMA = {str: CHECKSUMS_SCHEMA}
 
-SCHEMA = CHECKSUM_SCHEMA.copy()
+SCHEMA = CHECKSUMS_SCHEMA.copy()
 SCHEMA[Required(OutputBase.PARAM_PATH)] = str
 SCHEMA[OutputBase.PARAM_CACHE] = bool
 SCHEMA[OutputBase.PARAM_METRIC] = OutputBase.METRIC_SCHEMA
diff --git a/dvc/progress.py b/dvc/progress.py
index d03b037c0..a8ab61951 100644
--- a/dvc/progress.py
+++ b/dvc/progress.py
@@ -55,7 +55,8 @@ class Tqdm(tqdm):
         desc  : persists after `close()`
         level  : effective logging level for determining `disable`;
             used only if `disable` is unspecified
-        disable  : If (default: None), will be determined by logging level.
+        disable  : If (default: None) or False,
+            will be determined by logging level.
             May be overridden to `True` due to non-TTY status.
             Skip override by specifying env var `DVC_IGNORE_ISATTY`.
         kwargs  : anything accepted by `tqdm.tqdm()`
@@ -69,7 +70,7 @@ class Tqdm(tqdm):
             file = sys.stderr
         self.desc_persist = desc
         # auto-disable based on `logger.level`
-        if disable is None:
+        if not disable:
             disable = logger.getEffectiveLevel() > level
         # auto-disable based on TTY
         if (
diff --git a/dvc/repo/add.py b/dvc/repo/add.py
index fbbf7ed91..f216d6dde 100644
--- a/dvc/repo/add.py
+++ b/dvc/repo/add.py
@@ -73,7 +73,7 @@ def add(repo, targets, recursive=False, no_commit=False, fname=None):
                 total=len(stages),
                 desc="Processing",
                 unit="file",
-                disable=True if len(stages) == 1 else None,
+                disable=len(stages) == 1,
             ) as pbar_stages:
                 for stage in stages:
                     try:
@@ -120,7 +120,7 @@ def _create_stages(repo, targets, fname, pbar=None):
     for out in Tqdm(
         targets,
         desc="Creating DVC-files",
-        disable=True if len(targets) < LARGE_DIR_SIZE else None,
+        disable=len(targets) < LARGE_DIR_SIZE,
         unit="file",
     ):
         stage = Stage.create(
diff --git a/dvc/stage.py b/dvc/stage.py
index 954be11ba..8c9d79b8a 100644
--- a/dvc/stage.py
+++ b/dvc/stage.py
@@ -191,7 +191,7 @@ class Stage(object):
     PARAM_ALWAYS_CHANGED = "always_changed"
 
     SCHEMA = {
-        PARAM_MD5: Any(str, None),
+        PARAM_MD5: output.CHECKSUM_SCHEMA,
         PARAM_CMD: Any(str, None),
         PARAM_WDIR: Any(str, None),
         PARAM_DEPS: Any([dependency.SCHEMA], None),


Test patch:
diff --git a/tests/unit/output/test_output.py b/tests/unit/output/test_output.py
new file mode 100644
index 000000000..2cf30e968
--- /dev/null
+++ b/tests/unit/output/test_output.py
@@ -0,0 +1,42 @@
+import pytest
+
+from voluptuous import Schema, MultipleInvalid
+
+from dvc.output import CHECKSUM_SCHEMA
+
+
+@pytest.mark.parametrize(
+    "value,expected",
+    [
+        ("", None),
+        (None, None),
+        (11111, "11111"),
+        ("11111", "11111"),
+        ("aAaBa", "aaaba"),
+        (
+            "3cc286c534a71504476da009ed174423",
+            "3cc286c534a71504476da009ed174423",
+        ),  # md5
+        (
+            "d41d8cd98f00b204e9800998ecf8427e-38",
+            "d41d8cd98f00b204e9800998ecf8427e-38",
+        ),  # etag
+        (
+            "000002000000000000000000c16859d1d071c6b1ffc9c8557d4909f1",
+            "000002000000000000000000c16859d1d071c6b1ffc9c8557d4909f1",
+        ),  # hdfs checksum
+        # Not much we can do about hex and oct values without writing our own
+        # parser. So listing these test cases just to acknowledge this.
+        # See https://github.com/iterative/dvc/issues/3331.
+        (0x3451, "13393"),
+        (0o1244, "676"),
+    ],
+)
+def test_checksum_schema(value, expected):
+    assert Schema(CHECKSUM_SCHEMA)(value) == expected
+
+
+@pytest.mark.parametrize("value", ["1", "11", {}, {"a": "b"}, [], [1, 2]])
+def test_checksum_schema_fail(value):
+    with pytest.raises(MultipleInvalid):
+        Schema(CHECKSUM_SCHEMA)(value)["md5"]
diff --git a/tests/unit/test_progress.py b/tests/unit/test_progress.py
index 72005f65c..30dd7e662 100644
--- a/tests/unit/test_progress.py
+++ b/tests/unit/test_progress.py
@@ -16,6 +16,17 @@ def test_quiet_logging(caplog, capsys):
         assert out_err.err == ""
 
 
+def test_quiet_logging_disable_false(caplog, capsys):
+    with caplog.at_level(logging.CRITICAL, logger="dvc"):
+        # simulate interactive terminal
+        with mock.patch.object(sys.stderr, "isatty", return_value=True):
+            for _ in Tqdm(range(10), disable=False):
+                pass
+            out_err = capsys.readouterr()
+            assert out_err.out == ""
+            assert out_err.err == ""
+
+
 def test_quiet_notty(caplog, capsys):
     with caplog.at_level(logging.INFO, logger="dvc"):
         for _ in Tqdm(range(10)):