dvc: drop cached external outputs

Fixes iterative#9531

Docs iterative/dvc.org#4574 Kudos @dberenbaum

Author: efiop

dvc/cachemgr.py

@@ -32,13 +32,6 @@ def _get_odb(
 
 class CacheManager:
     CACHE_DIR = "cache"
-    CLOUD_SCHEMES = [
-        Schemes.S3,
-        Schemes.GS,
-        Schemes.SSH,
-        Schemes.HDFS,
-        Schemes.WEBHDFS,
-    ]
     FILES_DIR = "files"
 
     def __init__(self, repo):
@@ -91,16 +84,12 @@ def _init_odb(self, schemes):
             )
 
     def __getattr__(self, name):
-        if name not in self._odb and name in self.CLOUD_SCHEMES:
-            self._init_odb([name])
-
         try:
             return self._odb[name]
         except KeyError as exc:
             raise AttributeError from exc
 
     def by_scheme(self):
-        self._init_odb(self.CLOUD_SCHEMES)
         yield from self._odb.items()
 
     @property

dvc/commands/add.py

@@ -24,8 +24,6 @@ def validate_args(self) -> None:
                 invalid_opt = "--glob option"
             elif args.no_commit:
                 invalid_opt = "--no-commit option"
-            elif args.external:
-                invalid_opt = "--external option"
         else:
             message = "{option} can't be used without --to-remote"
             if args.remote:
@@ -49,7 +47,6 @@ def run(self):
             self.repo.add(
                 self.args.targets,
                 no_commit=self.args.no_commit,
-                external=self.args.external,
                 glob=self.args.glob,
                 out=self.args.out,
                 remote=self.args.remote,
@@ -82,12 +79,6 @@ def add_parser(subparsers, parent_parser):
         default=False,
         help="Don't put files/directories into cache.",
     )
-    parser.add_argument(
-        "--external",
-        action="store_true",
-        default=False,
-        help="Allow targets that are outside of the DVC repository.",
-    )
     parser.add_argument(
         "--glob",
         action="store_true",

dvc/config_schema.py

@@ -154,13 +154,13 @@ def __call__(self, data):
         "machine": Lower,
     },
     "cache": {
-        "local": str,
-        "s3": str,
-        "gs": str,
-        "hdfs": str,
-        "webhdfs": str,
-        "ssh": str,
-        "azure": str,
+        "local": str,  # obsoleted
+        "s3": str,  # obsoleted
+        "gs": str,  # obsoleted
+        "hdfs": str,  # obsoleted
+        "webhdfs": str,  # obsoleted
+        "ssh": str,  # obsoleted
+        "azure": str,  # obsoleted
         # This is for default local cache
         "dir": str,
         **LOCAL_COMMON,

dvc/exceptions.py

@@ -299,21 +299,6 @@ def __init__(self, url):
         super().__init__(f"The path '{url}' does not exist")
 
 
-class RemoteCacheRequiredError(DvcException):
-    def __init__(self, scheme, fs_path):
-        super().__init__(
-            (
-                "Current operation was unsuccessful because '{}' requires "
-                "existing cache on '{}' remote. See {} for information on how "
-                "to set up remote cache."
-            ).format(
-                fs_path,
-                scheme,
-                format_link("https://man.dvc.org/config#cache"),
-            )
-        )
-
-
 class IsADirectoryError(DvcException):  # noqa,pylint:disable=redefined-builtin
     """Raised when a file operation is requested on a directory."""
 

dvc/output.py

@@ -19,7 +19,6 @@
     ConfirmRemoveError,
     DvcException,
     MergeError,
-    RemoteCacheRequiredError,
 )
 from dvc.utils.objects import cached_property
 from dvc_data.hashfile import check as ocheck
@@ -526,14 +525,9 @@ def use_scm_ignore(self):
     def cache(self):
         from dvc.cachemgr import LEGACY_HASH_NAMES
 
-        if self.is_in_repo:
-            odb_name = "legacy" if self.hash_name in LEGACY_HASH_NAMES else "repo"
-        else:
-            odb_name = self.protocol
-        odb = getattr(self.repo.cache, odb_name)
-        if self.use_cache and odb is None:
-            raise RemoteCacheRequiredError(self.fs.protocol, self.fs_path)
-        return odb
+        assert self.is_in_repo
+        odb_name = "legacy" if self.hash_name in LEGACY_HASH_NAMES else "repo"
+        return getattr(self.repo.cache, odb_name)
 
     @property
     def local_cache(self):

dvc/repo/add.py

@@ -54,7 +54,6 @@ def find_targets(
 def get_or_create_stage(
     repo: "Repo",
     target: str,
-    external: bool = False,
     out: Optional[str] = None,
     to_remote: bool = False,
     force: bool = False,
@@ -79,7 +78,6 @@ def get_or_create_stage(
             fname=path,
             wdir=wdir,
             outs=[out],
-            external=external,
             force=force,
         )
         return StageInfo(stage, output_exists=False)
@@ -199,7 +197,6 @@ def add(
     repo: "Repo",
     targets: Union["StrOrBytesPath", Iterator["StrOrBytesPath"]],
     no_commit: bool = False,
-    external: bool = False,
     glob: bool = False,
     out: Optional[str] = None,
     remote: Optional[str] = None,
@@ -215,7 +212,6 @@ def add(
         target: get_or_create_stage(
             repo,
             target,
-            external=external,
             out=out,
             to_remote=to_remote,
             force=force,

dvc/stage/__init__.py

@@ -89,7 +89,7 @@ class RawData:
     generated_from: Optional[str] = None
 
 
-def create_stage(cls: Type[_T], repo, path, external=False, **kwargs) -> _T:
+def create_stage(cls: Type[_T], repo, path, **kwargs) -> _T:
     from dvc.dvcfile import check_dvcfile_path
 
     wdir = os.path.abspath(kwargs.get("wdir", None) or os.curdir)
@@ -101,8 +101,7 @@ def create_stage(cls: Type[_T], repo, path, external=False, **kwargs) -> _T:
 
     stage = loads_from(cls, repo, path, wdir, kwargs)
     fill_stage_outputs(stage, **kwargs)
-    if not external:
-        check_no_externals(stage)
+    check_no_externals(stage)
     fill_stage_dependencies(
         stage, **project(kwargs, ["deps", "erepo", "params", "fs_config"])
     )

dvc/stage/cache.py

@@ -44,7 +44,12 @@ def _can_hash(stage):
             return False
 
     for out in stage.outs:
-        if out.protocol != "local" or not out.def_path or out.persist:
+        if (
+            out.protocol != "local"
+            or not out.def_path
+            or out.persist
+            or not out.is_in_repo
+        ):
             return False
 
     return True
@@ -112,7 +117,6 @@ def _create_stage(self, cache, wdir=None):
             cmd=cache["cmd"],
             wdir=wdir,
             outs=[out["path"] for out in cache["outs"]],
-            external=True,
         )
         StageLoader.fill_from_lock(stage, cache)
         return stage
@@ -137,7 +141,7 @@ def _uncached_outs(self, stage, cache):
         # NOTE: using copy link to make it look like a git-tracked file
         with self._cache_type_copy():
             for out in cached_stage.outs:
-                if out.def_path in outs_no_cache:
+                if out.def_path in outs_no_cache and out.is_in_repo:
                     yield out
 
     def save(self, stage):

dvc/stage/utils.py

@@ -87,17 +87,13 @@ def fill_stage_dependencies(stage, deps=None, erepo=None, params=None, fs_config
 
 
 def check_no_externals(stage):
-    from urllib.parse import urlparse
-
     from dvc.utils import format_link
 
     # NOTE: preventing users from accidentally using external outputs. See
     # https://github.com/iterative/dvc/issues/1545 for more details.
 
     def _is_external(out):
-        # NOTE: in case of `remote://` notation, the user clearly knows that
-        # this is an advanced feature and so we shouldn't error-out.
-        if out.is_in_repo or urlparse(out.def_path).scheme == "remote":
+        if out.is_in_repo or not out.use_cache:
             return False
         return True
 

dvc/testing/workspace_tests.py

@@ -153,57 +153,6 @@ def test_import_no_download(self, tmp_dir, dvc, remote_version_aware):
         assert dvc.status() == {}
 
 
-class TestAdd:
-    @pytest.fixture
-    def hash_name(self):
-        pytest.skip()
-
-    @pytest.fixture
-    def hash_value(self):
-        pytest.skip()
-
-    @pytest.fixture
-    def dir_hash_value(self):
-        pytest.skip()
-
-    def test_add(self, tmp_dir, dvc, workspace, hash_name, hash_value):
-        from dvc.stage.exceptions import StageExternalOutputsError
-
-        workspace.gen("file", "file")
-
-        with pytest.raises(StageExternalOutputsError):
-            dvc.add(workspace.url)
-
-        dvc.add("remote://workspace/file")
-        assert (tmp_dir / "file.dvc").read_text() == (
-            "outs:\n"
-            f"- {hash_name}: {hash_value}\n"
-            "  size: 4\n"
-            "  hash: md5\n"
-            "  path: remote://workspace/file\n"
-        )
-        assert (workspace / "file").read_text() == "file"
-        assert (
-            workspace / "cache" / "files" / "md5" / hash_value[:2] / hash_value[2:]
-        ).read_text() == "file"
-
-        assert dvc.status() == {}
-
-    # pylint: disable-next=unused-argument
-    def test_add_dir(self, tmp_dir, dvc, workspace, hash_name, dir_hash_value):
-        workspace.gen({"dir": {"file": "file", "subdir": {"subfile": "subfile"}}})
-
-        dvc.add("remote://workspace/dir")
-        assert (
-            workspace
-            / "cache"
-            / "files"
-            / "md5"
-            / dir_hash_value[:2]
-            / dir_hash_value[2:]
-        ).is_file()
-
-
 def match_files(fs, entries, expected):
     entries_content = {(fs.path.normpath(d["path"]), d["isdir"]) for d in entries}
     expected_content = {(fs.path.normpath(d["path"]), d["isdir"]) for d in expected}

tests/func/test_add.py

@@ -22,7 +22,6 @@
 )
 from dvc.stage import Stage
 from dvc.stage.exceptions import StageExternalOutputsError, StagePathNotFoundError
-from dvc.testing.workspace_tests import TestAdd
 from dvc.utils.fs import path_isin
 from dvc.utils.serialize import YAMLFileCorruptedError
 from dvc_data.hashfile.hash import file_md5
@@ -234,55 +233,6 @@ def test_add_filtered_files_in_dir(
         assert stage.outs[0].def_path in expected_def_paths
 
 
-class TestAddExternal(TestAdd):
-    @pytest.fixture
-    def hash_name(self):
-        return "md5"
-
-    @pytest.fixture
-    def hash_value(self):
-        return "8c7dd922ad47494fc02c388e12c00eac"
-
-    @pytest.fixture
-    def dir_hash_value(self):
-        return "b6dcab6ccd17ca0a8bf4a215a37d14cc.dir"
-
-
-def test_add_external_relpath(tmp_dir, dvc, local_cloud):
-    (fpath,) = local_cloud.gen("file", "file")
-    rel = os.path.relpath(fpath)
-
-    with pytest.raises(StageExternalOutputsError):
-        dvc.add(rel)
-
-    dvc.add(rel, external=True)
-    assert (tmp_dir / "file.dvc").read_text() == (
-        "outs:\n"
-        "- md5: 8c7dd922ad47494fc02c388e12c00eac\n"
-        "  size: 4\n"
-        "  hash: md5\n"
-        f"  path: {rel}\n"
-    )
-    assert fpath.read_text() == "file"
-    assert dvc.status() == {}
-
-
-def test_add_local_remote_file(tmp_dir, dvc):
-    """
-    Making sure that 'remote' syntax is handled properly for local outs.
-    """
-    tmp_dir.gen({"foo": "foo", "bar": "bar"})
-    tmp_dir.add_remote(url=tmp_dir.fs_path, name="myremote")
-
-    assert main(["add", "remote://myremote/foo"]) == 0
-    d = (tmp_dir / "foo.dvc").load_yaml()
-    assert d["outs"][0]["path"] == "remote://myremote/foo"
-
-    assert main(["add", (tmp_dir / "bar").fs_path]) == 0
-    d = (tmp_dir / "bar.dvc").load_yaml()
-    assert d["outs"][0]["path"] == "bar"
-
-
 def test_cmd_add(tmp_dir, dvc):
     tmp_dir.gen("foo", "foo")
     ret = main(["add", "foo"])
@@ -755,15 +705,9 @@ def test_add_symlink_file(tmp_dir, dvc):
     dvc.add(os.path.join("dir", "foo"))
 
 
-@pytest.mark.parametrize("external", [True, False])
-def test_add_symlink_dir(make_tmp_dir, tmp_dir, dvc, external):
-    if external:
-        data_dir = make_tmp_dir("data")
-        data_dir.gen({"foo": "foo"})
-        target = os.fspath(data_dir)
-    else:
-        tmp_dir.gen({"data": {"foo": "foo"}})
-        target = os.path.join(".", "data")
+def test_add_symlink_dir(make_tmp_dir, tmp_dir, dvc):
+    tmp_dir.gen({"data": {"foo": "foo"}})
+    target = os.path.join(".", "data")
 
     tmp_dir.gen({"data": {"foo": "foo"}})
 
@@ -774,15 +718,9 @@ def test_add_symlink_dir(make_tmp_dir, tmp_dir, dvc, external):
         dvc.add("dir")
 
 
-@pytest.mark.parametrize("external", [True, False])
-def test_add_file_in_symlink_dir(make_tmp_dir, tmp_dir, dvc, external):
-    if external:
-        data_dir = make_tmp_dir("data")
-        data_dir.gen({"dir": {"foo": "foo"}})
-        target = os.fspath(data_dir / "dir")
-    else:
-        tmp_dir.gen({"data": {"foo": "foo"}})
-        target = os.path.join(".", "data")
+def test_add_file_in_symlink_dir(make_tmp_dir, tmp_dir, dvc):
+    tmp_dir.gen({"data": {"foo": "foo"}})
+    target = os.path.join(".", "data")
 
     (tmp_dir / "dir").symlink_to(target)