From d7ae801ef9faea2f034987c57f09a83bcd5f0c29 Mon Sep 17 00:00:00 2001
From: Ruslan Kuprieiev <ruslan@iterative.ai>
Date: Wed, 25 Mar 2020 04:05:48 +0200
Subject: [PATCH] metrics: diff: handle deleted/new metric

Partial fix for https://github.com/iterative/dvc/issues/3469
---
 dvc/repo/metrics/diff.py           | 34 ++++++++++++++----------------
 tests/func/test_metrics.py         | 32 ++++++++++++++++++++++++++++
 tests/unit/command/test_metrics.py | 18 ++++++++++++++++
 3 files changed, 66 insertions(+), 18 deletions(-)

diff --git a/dvc/repo/metrics/diff.py b/dvc/repo/metrics/diff.py
index e3ad902d60..1ed20e3876 100644
--- a/dvc/repo/metrics/diff.py
+++ b/dvc/repo/metrics/diff.py
@@ -7,7 +7,7 @@
 
 
 def _parse(raw):
-    if isinstance(raw, (dict, list, int, float)):
+    if raw is None or isinstance(raw, (dict, list, int, float)):
         return raw
 
     assert isinstance(raw, str)
@@ -34,29 +34,27 @@ def _diff_vals(old, new):
     return res
 
 
-def _diff_dicts(old_dict, new_dict):
-    old_default = None
-    new_default = None
+def _flatten(d):
+    if not d:
+        return defaultdict(lambda: None)
+
+    if isinstance(d, dict):
+        return defaultdict(lambda: None, flatten(d, "."))
 
-    if isinstance(new_dict, dict):
-        new = flatten(new_dict, ".")
-    else:
-        new = defaultdict(lambda: "not a dict")
-        new_default = "unable to parse"
+    return defaultdict(lambda: "unable to parse")
 
-    if isinstance(old_dict, dict):
-        old = flatten(old_dict, ".")
-    else:
-        old = defaultdict(lambda: "not a dict")
-        old_default = "unable to parse"
+
+def _diff_dicts(old_dict, new_dict):
+    new = _flatten(new_dict)
+    old = _flatten(old_dict)
 
     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)
+        old_val = old[xpath]
+        new_val = new[xpath]
         val_diff = _diff_vals(old_val, new_val)
         if val_diff:
             res[xpath] = val_diff
@@ -78,7 +76,7 @@ def _get_metrics(repo, *args, rev=None, **kwargs):
         metrics = repo.metrics.show(
             *args, **kwargs, revs=[rev] if rev else None
         )
-        return metrics[rev or ""]
+        return metrics.get(rev or "", {})
     except NoMetricsError:
         return {}
 
@@ -92,7 +90,7 @@ def diff(repo, *args, a_rev=None, b_rev=None, **kwargs):
 
     res = defaultdict(dict)
     for path in paths:
-        path_diff = _diff(old[path], new[path])
+        path_diff = _diff(old.get(path), new.get(path))
         if path_diff:
             res[path] = path_diff
     return dict(res)
diff --git a/tests/func/test_metrics.py b/tests/func/test_metrics.py
index 09f9e229a9..8339b45ba3 100644
--- a/tests/func/test_metrics.py
+++ b/tests/func/test_metrics.py
@@ -940,3 +940,35 @@ def test_metrics_diff_broken_json(tmp_dir, scm, dvc):
 def test_metrics_diff_no_metrics(tmp_dir, scm, dvc):
     tmp_dir.scm_gen({"foo": "foo"}, commit="add foo")
     assert dvc.metrics.diff(a_rev="HEAD~1") == {}
+
+
+def test_metrics_diff_new_metric(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"])
+
+    assert dvc.metrics.diff() == {
+        "m.json": {
+            "a.b.c": {"old": None, "new": 1},
+            "a.b.d": {"old": None, "new": 1},
+            "a.b.e": {"old": None, "new": "3"},
+        }
+    }
+
+
+def test_metrics_diff_deleted_metric(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").unlink()
+
+    assert dvc.metrics.diff() == {
+        "m.json": {
+            "a.b.c": {"old": 1, "new": None},
+            "a.b.d": {"old": 1, "new": None},
+            "a.b.e": {"old": "3", "new": None},
+        }
+    }
diff --git a/tests/unit/command/test_metrics.py b/tests/unit/command/test_metrics.py
index 856d24b483..2127c5c787 100644
--- a/tests/unit/command/test_metrics.py
+++ b/tests/unit/command/test_metrics.py
@@ -65,3 +65,21 @@ def test_metrics_diff_no_diff():
 
 def test_metrics_diff_no_changes():
     assert _show_diff({}) == "No changes."
+
+
+def test_metrics_diff_new_metric():
+    assert _show_diff(
+        {"other.json": {"a.b.d": {"old": None, "new": "new"}}}
+    ) == (
+        "   Path      Metric   Value         Change      \n"
+        "other.json   a.b.d    new     diff not supported"
+    )
+
+
+def test_metrics_diff_deleted_metric():
+    assert _show_diff(
+        {"other.json": {"a.b.d": {"old": "old", "new": None}}}
+    ) == (
+        "   Path      Metric   Value         Change      \n"
+        "other.json   a.b.d    None    diff not supported"
+    )