Repository: iterative/dvc

Issue:
summon api: caching during interactive sessions
```
$ dvc -V
0.80.0+7f92cc
```

In Python session:
```python
>>> import dvc.api
>>> m = dvc.api.summon('resnext101', 'https://github.com/dmpetrov/dvc-pretrained-resnet', 'test_summon', 'resnext101_32x8d.yml')
...
dvc.api.SummonError: extra keys not allowed @ data['objects'][0]['summon']['file'] at 'resnext101_32x8d.yml' in 'https://github.com/dmpetrov/dvc-pretrained-resnet'
```

I fix the error in the repo without leaving the Python session and summon the object again:

```python
>>> import dvc.api
>>> m = dvc.api.summon('resnext101', 'https://github.com/dmpetrov/dvc-pretrained-resnet', 'test_summon', 'resnext101_32x8d.yml')
...
dvc.api.SummonError: extra keys not allowed @ data['objects'][0]['summon']['file'] at 'resnext101_32x8d.yml' in 'https://github.com/dmpetrov/dvc-pretrained-resnet'
```

The same error while the issue is fixed.

This issue affects one of the summon scenarios - using data objects from interactive IDEs such as Jupyther Notebooks and RStudio.

Discussion:

Patch:
diff --git a/dvc/api.py b/dvc/api.py
index 7efaf524a..2ae39bae7 100644
--- a/dvc/api.py
+++ b/dvc/api.py
@@ -10,7 +10,7 @@ import ruamel.yaml
 from voluptuous import Schema, Required, Invalid
 
 from dvc.repo import Repo
-from dvc.exceptions import DvcException, NotDvcRepoError
+from dvc.exceptions import DvcException
 from dvc.external_repo import external_repo
 
 
@@ -108,16 +108,12 @@ def read(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
 
 
 @contextmanager
-def _make_repo(repo_url=None, rev=None):
-    repo_url = repo_url or os.getcwd()
-    if rev is None and os.path.exists(repo_url):
-        try:
-            yield Repo(repo_url)
-            return
-        except NotDvcRepoError:
-            pass  # fallthrough to external_repo
-    with external_repo(url=repo_url, rev=rev) as repo:
-        yield repo
+def _make_repo(repo_url, rev=None):
+    if rev is None and (not repo_url or os.path.exists(repo_url)):
+        yield Repo(repo_url)
+    else:
+        with external_repo(url=repo_url, rev=rev) as repo:
+            yield repo
 
 
 def summon(name, repo=None, rev=None, summon_file="dvcsummon.yaml", args=None):
diff --git a/dvc/external_repo.py b/dvc/external_repo.py
index 306e7d7c6..3c35e205e 100644
--- a/dvc/external_repo.py
+++ b/dvc/external_repo.py
@@ -1,9 +1,11 @@
+import logging
 import os
 import tempfile
 from contextlib import contextmanager
 from distutils.dir_util import copy_tree
+import threading
 
-from funcy import retry, suppress, memoize, cached_property
+from funcy import retry, suppress, wrap_with, cached_property
 
 from dvc.compat import fspath
 from dvc.repo import Repo
@@ -16,6 +18,9 @@ from dvc.utils.fs import remove, fs_copy
 from dvc.scm import SCM
 
 
+logger = logging.getLogger(__name__)
+
+
 @contextmanager
 def external_repo(url, rev=None):
     path = _cached_clone(url, rev)
@@ -40,8 +45,8 @@ def external_repo(url, rev=None):
 
 def clean_repos():
     # Outside code should not see cache while we are removing
-    repo_paths = list(_cached_clone.memory.values())
-    _cached_clone.memory.clear()
+    repo_paths = list(REPOS_CACHE.values())
+    REPOS_CACHE.clear()
 
     for path in repo_paths:
         _remove(path)
@@ -128,28 +133,37 @@ class ExternalGitRepo:
             raise PathMissingError(path, self.url)
 
     @contextmanager
-    def open_by_relpath(self, path, mode="r", encoding=None, **kwargs):
+    def open_by_relpath(self, path, mode="r", encoding=None):
         try:
             abs_path = os.path.join(self.root_dir, path)
-            with open(abs_path, mode, encoding=encoding) as fd:
+            with open(abs_path, mode, encoding) as fd:
                 yield fd
         except FileNotFoundError:
             raise PathMissingError(path, self.url)
 
 
-@memoize
+REPOS_CACHE = {}
+
+
+@wrap_with(threading.Lock())
 def _cached_clone(url, rev):
     """Clone an external git repo to a temporary directory.
 
     Returns the path to a local temporary directory with the specified
     revision checked out.
     """
+    if (url, rev) in REPOS_CACHE:
+        path = REPOS_CACHE[url, rev]
+        _git_pull(path, rev)
+        return path
+
     new_path = tempfile.mkdtemp("dvc-erepo")
 
-    if url in _cached_clone.memory:
+    if url in REPOS_CACHE:
         # Copy and an existing clean clone
         # This one unlike shutil.copytree() works with an existing dir
-        copy_tree(_cached_clone.memory[url], new_path)
+        copy_tree(REPOS_CACHE[url], new_path)
+        _git_pull(new_path, None)
     else:
         # Create a new clone
         _clone_repo(url, new_path)
@@ -157,21 +171,37 @@ def _cached_clone(url, rev):
         # Save clean clone dir so that we will have access to a default branch
         clean_clone_path = tempfile.mkdtemp("dvc-erepo")
         copy_tree(new_path, clean_clone_path)
-        _cached_clone.memory[url] = clean_clone_path
+        REPOS_CACHE[url] = clean_clone_path
 
     # Check out the specified revision
     if rev is not None:
         _git_checkout(new_path, rev)
 
+    REPOS_CACHE[url, rev] = new_path
     return new_path
 
 
-def _git_checkout(repo_path, revision):
+def _git_checkout(repo_path, rev):
     from dvc.scm import Git
 
     git = Git(repo_path)
     try:
-        git.checkout(revision)
+        git.checkout(rev)
+    finally:
+        git.close()
+
+
+def _git_pull(repo_path, rev):
+    import git
+
+    # Do not try to pull in a detached mode
+    if rev and git.Repo.re_hexsha_only.search(rev):
+        return
+
+    git = git.Repo(repo_path)
+    try:
+        msg = git.git.pull()
+        logger.debug("external repo: git pull: {}", msg)
     finally:
         git.close()
 
diff --git a/dvc/version.py b/dvc/version.py
index 80fb60b54..8d27b8027 100644
--- a/dvc/version.py
+++ b/dvc/version.py
@@ -7,7 +7,7 @@ import os
 import subprocess
 
 
-_BASE_VERSION = "0.82.2"
+_BASE_VERSION = "0.82.1"
 
 
 def _generate_version(base_version):


Test patch:
diff --git a/tests/func/test_api.py b/tests/func/test_api.py
index 04f707826..687476d61 100644
--- a/tests/func/test_api.py
+++ b/tests/func/test_api.py
@@ -8,7 +8,7 @@ import pytest
 from dvc import api
 from dvc.api import SummonError, UrlNotDvcRepoError
 from dvc.compat import fspath
-from dvc.exceptions import FileMissingError
+from dvc.exceptions import FileMissingError, NotDvcRepoError
 from dvc.main import main
 from dvc.path_info import URLInfo
 from dvc.remote.config import RemoteConfig
@@ -54,7 +54,7 @@ def test_get_url_external(erepo_dir, remote_url):
 def test_get_url_requires_dvc(tmp_dir, scm):
     tmp_dir.scm_gen({"foo": "foo"}, commit="initial")
 
-    with pytest.raises(UrlNotDvcRepoError, match="not a DVC repository"):
+    with pytest.raises(NotDvcRepoError, match="not inside of a DVC repo"):
         api.get_url("foo", repo=fspath(tmp_dir))
 
     with pytest.raises(UrlNotDvcRepoError):
diff --git a/tests/func/test_external_repo.py b/tests/func/test_external_repo.py
index 4426ef1e1..8aa6837ca 100644
--- a/tests/func/test_external_repo.py
+++ b/tests/func/test_external_repo.py
@@ -23,3 +23,16 @@ def test_external_repo(erepo_dir):
                 assert fd.read() == "branch"
 
         assert mock.call_count == 1
+
+
+def test_source_change(erepo_dir):
+    url = fspath(erepo_dir)
+    with external_repo(url) as repo:
+        old_rev = repo.scm.get_rev()
+
+    erepo_dir.scm_gen("file", "text", commit="a change")
+
+    with external_repo(url) as repo:
+        new_rev = repo.scm.get_rev()
+
+    assert old_rev != new_rev
diff --git a/tests/func/test_import_url.py b/tests/func/test_import_url.py
index 4eea90317..bf042e32b 100644
--- a/tests/func/test_import_url.py
+++ b/tests/func/test_import_url.py
@@ -40,7 +40,7 @@ class TestDefaultOutput(TestDvc):
             self.assertEqual(fd.read(), "content")
 
 
-def test_should_remove_outs_before_import(tmp_dir, dvc, mocker, erepo_dir):
+def test_should_remove_outs_before_import(mocker, erepo_dir):
     erepo_dir.gen({"foo": "foo"})
 
     remove_outs_call_counter = mocker.spy(Stage, "remove_outs")
diff --git a/tests/func/test_status.py b/tests/func/test_status.py
index f8e5964bc..35b66aa51 100644
--- a/tests/func/test_status.py
+++ b/tests/func/test_status.py
@@ -4,7 +4,6 @@ from mock import patch
 
 from dvc.main import main
 from dvc.compat import fspath
-from dvc.external_repo import clean_repos
 from tests.basic_env import TestDvc
 
 
@@ -33,19 +32,12 @@ def test_status_non_dvc_repo_import(tmp_dir, dvc, git_dir):
 
     dvc.imp(fspath(git_dir), "file", "file", rev="branch")
 
-    status = dvc.status(["file.dvc"])
-
-    assert status == {}
-
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
+    assert dvc.status(["file.dvc"]) == {}
 
     with git_dir.branch("branch", new=False):
         git_dir.scm_gen("file", "second version", commit="update file")
 
     status, = dvc.status(["file.dvc"])["file.dvc"]
-
     assert status == {
         "changed deps": {"file ({})".format(git_dir): "update available"}
     }
@@ -68,12 +60,7 @@ def test_status_before_and_after_dvc_init(tmp_dir, dvc, git_dir):
 
     assert old_rev != new_rev
 
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
-
     status, = dvc.status(["file.dvc"])["file.dvc"]
-
     assert status == {
         "changed deps": {
             "file ({})".format(fspath(git_dir)): "update available"
diff --git a/tests/func/test_update.py b/tests/func/test_update.py
index 37639e9aa..e2c68de40 100644
--- a/tests/func/test_update.py
+++ b/tests/func/test_update.py
@@ -3,7 +3,6 @@ import os
 
 from dvc.stage import Stage
 from dvc.compat import fspath
-from dvc.external_repo import clean_repos
 
 
 @pytest.mark.parametrize("cached", [True, False])
@@ -16,43 +15,21 @@ def test_update_import(tmp_dir, dvc, erepo_dir, cached):
 
     stage = dvc.imp(fspath(erepo_dir), "version", "version", rev="branch")
 
-    imported = tmp_dir / "version"
-    assert imported.is_file()
-    assert imported.read_text() == "branch"
-    assert stage.deps[0].def_repo == {
-        "url": fspath(erepo_dir),
-        "rev": "branch",
-        "rev_lock": old_rev,
-    }
+    assert (tmp_dir / "version").read_text() == "branch"
+    assert stage.deps[0].def_repo["rev_lock"] == old_rev
 
+    # Update version file
     with erepo_dir.branch("branch", new=False), erepo_dir.chdir():
         gen("version", "updated", "update version content")
         new_rev = erepo_dir.scm.get_rev()
 
     assert old_rev != new_rev
 
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
-
-    status, = dvc.status([stage.path])["version.dvc"]
-    changed_dep, = list(status["changed deps"].items())
-    assert changed_dep[0].startswith("version ")
-    assert changed_dep[1] == "update available"
-
     dvc.update(stage.path)
-
-    assert dvc.status([stage.path]) == {}
-
-    assert imported.is_file()
-    assert imported.read_text() == "updated"
+    assert (tmp_dir / "version").read_text() == "updated"
 
     stage = Stage.load(dvc, stage.path)
-    assert stage.deps[0].def_repo == {
-        "url": fspath(erepo_dir),
-        "rev": "branch",
-        "rev_lock": new_rev,
-    }
+    assert stage.deps[0].def_repo["rev_lock"] == new_rev
 
 
 def test_update_import_after_remote_updates_to_dvc(tmp_dir, dvc, erepo_dir):
@@ -82,10 +59,6 @@ def test_update_import_after_remote_updates_to_dvc(tmp_dir, dvc, erepo_dir):
 
     assert old_rev != new_rev
 
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
-
     status, = dvc.status([stage.path])["version.dvc"]
     changed_dep, = list(status["changed deps"].items())
     assert changed_dep[0].startswith("version ")
@@ -122,10 +95,6 @@ def test_update_before_and_after_dvc_init(tmp_dir, dvc, git_dir):
 
     assert old_rev != new_rev
 
-    # Caching in external repos doesn't see upstream updates within single
-    # cli call, so we need to clean the caches to see the changes.
-    clean_repos()
-
     assert dvc.status([stage.path]) == {
         "file.dvc": [
             {
diff --git a/tests/remotes.py b/tests/remotes.py
index 35876b676..2cf1c19b1 100644
--- a/tests/remotes.py
+++ b/tests/remotes.py
@@ -38,9 +38,9 @@ TEST_GCP_CREDS_FILE = os.path.abspath(
 os.environ["GOOGLE_APPLICATION_CREDENTIALS"] = TEST_GCP_CREDS_FILE
 
 TEST_GDRIVE_CLIENT_ID = (
-    "217948389181-rs7it4a635b3qrf8dnmklmoj2kimun9n.apps.googleusercontent.com"
+    "719861249063-v4an78j9grdtuuuqg3lnm0sugna6v3lh.apps.googleusercontent.com"
 )
-TEST_GDRIVE_CLIENT_SECRET = "LNg9n_cK7bohI8gEHn4bUeMX"
+TEST_GDRIVE_CLIENT_SECRET = "2fy_HyzSwkxkGzEken7hThXb"
 
 always_test = staticmethod(lambda: True)