dvcfs: use paths relative to the repo root

Currently we use both absolute and relative (depend on cwd) paths, which is
terrible for an api, as their interpretation depends on external factors.
This PR makes dvcfs accept only paths relative to the repo root for regular
outputs and absolute paths for external outputs.

Pre-requisite for making repofs use same relative paths.

Author: efiop

dvc/api.py

@@ -27,7 +27,8 @@ def get_url(path, repo=None, rev=None, remote=None):
             raise OutputNotFoundError(path, repo)
 
         cloud = info["repo"].cloud
-        md5 = info["repo"].dvcfs.info(fs_path)["md5"]
+        dvc_path = _repo.fs.path.relpath(fs_path, info["repo"].root_dir)
+        md5 = info["repo"].dvcfs.info(dvc_path)["md5"]
         return cloud.get_url_for(remote, checksum=md5)
 
 

dvc/fs/dvc.py

@@ -32,16 +32,18 @@ def config(self):
         raise NotImplementedError
 
     def _get_key(self, path):
+        from dvc.fs.local import LocalFileSystem
+
         from . import get_cloud_fs
 
         cls, kwargs, fs_path = get_cloud_fs(None, url=path)
 
-        if not path.startswith(self.repo.root_dir):
+        if cls != LocalFileSystem or os.path.isabs(path):
             fs = cls(**kwargs)
             return (cls.scheme, *fs.path.parts(fs_path))
 
         fs_key = "repo"
-        key = self.path.relparts(path, self.repo.root_dir)
+        key = self.path.parts(path)
         if key == (".",):
             key = ()
 
@@ -132,9 +134,8 @@ def _walk(self, root, topdown=True, **kwargs):
 
     def walk(self, top, topdown=True, onerror=None, **kwargs):
         assert topdown
-        root = os.path.abspath(top)
         try:
-            info = self.info(root)
+            info = self.info(top)
         except FileNotFoundError:
             if onerror is not None:
                 onerror(FileNotFoundError(top))
@@ -145,7 +146,7 @@ def walk(self, top, topdown=True, onerror=None, **kwargs):
                 onerror(NotADirectoryError(top))
             return
 
-        yield from self._walk(root, topdown=topdown, **kwargs)
+        yield from self._walk(top, topdown=topdown, **kwargs)
 
     def find(self, path, prefix=None):
         for root, _, files in self.walk(path):
@@ -165,9 +166,7 @@ def isdvc(self, path, recursive=False, strict=True):
     def info(self, path):
         from dvc.data.meta import Meta
 
-        abspath = os.path.abspath(path)
-
-        key = self._get_key(abspath)
+        key = self._get_key(path)
 
         try:
             outs = list(self.repo.index.tree.iteritems(key))

dvc/fs/repo.py

@@ -18,6 +18,11 @@
 RepoFactory = Union[Callable[[str], "Repo"], Type["Repo"]]
 
 
+def _wrap_walk(dvc_fs, *args, **kwargs):
+    for root, dnames, fnames in dvc_fs.walk(*args, **kwargs):
+        yield dvc_fs.path.join(dvc_fs.repo.root_dir, root), dnames, fnames
+
+
 class RepoFileSystem(FileSystem):  # pylint:disable=abstract-method
     """DVC + git-tracked files fs.
 
@@ -191,7 +196,9 @@ def _is_dvc_repo(self, dir_path):
         repo_path = os.path.join(dir_path, Repo.DVC_DIR)
         return self._main_repo.fs.isdir(repo_path)
 
-    def _get_fs_pair(self, path) -> Tuple[FileSystem, Optional[DvcFileSystem]]:
+    def _get_fs_pair(
+        self, path
+    ) -> Tuple[FileSystem, Optional[DvcFileSystem], str]:
         """
         Returns a pair of fss based on repo the path falls in, using prefix.
         """
@@ -202,27 +209,33 @@ def _get_fs_pair(self, path) -> Tuple[FileSystem, Optional[DvcFileSystem]]:
         repo = self._get_repo(path) or self._main_repo
 
         dvc_fs = self._dvcfss.get(repo.root_dir)
-        return repo.fs, dvc_fs
+
+        if path.startswith(repo.root_dir):
+            dvc_path = path[len(repo.root_dir) + 1 :]
+        else:
+            dvc_path = path
+
+        return repo.fs, dvc_fs, dvc_path
 
     def open(
         self, path, mode="r", encoding="utf-8", **kwargs
     ):  # pylint: disable=arguments-renamed
         if "b" in mode:
             encoding = None
 
-        fs, dvc_fs = self._get_fs_pair(path)
+        fs, dvc_fs, dvc_path = self._get_fs_pair(path)
         try:
             return fs.open(path, mode=mode, encoding=encoding)
         except FileNotFoundError:
             if not dvc_fs:
                 raise
 
-        return dvc_fs.open(path, mode=mode, encoding=encoding, **kwargs)
+        return dvc_fs.open(dvc_path, mode=mode, encoding=encoding, **kwargs)
 
     def exists(self, path) -> bool:
         path = os.path.abspath(path)
 
-        fs, dvc_fs = self._get_fs_pair(path)
+        fs, dvc_fs, dvc_path = self._get_fs_pair(path)
 
         if not dvc_fs:
             return fs.exists(path)
@@ -233,7 +246,7 @@ def exists(self, path) -> bool:
         if fs.exists(path):
             return True
 
-        if not dvc_fs.exists(path):
+        if not dvc_fs.exists(dvc_path):
             return False
 
         for p in self.path.parents(path):
@@ -248,7 +261,7 @@ def exists(self, path) -> bool:
     def isdir(self, path):  # pylint: disable=arguments-renamed
         path = os.path.abspath(path)
 
-        fs, dvc_fs = self._get_fs_pair(path)
+        fs, dvc_fs, dvc_path = self._get_fs_pair(path)
 
         if dvc_fs and dvc_fs.repo.dvcignore.is_ignored_dir(path):
             return False
@@ -264,7 +277,7 @@ def isdir(self, path):  # pylint: disable=arguments-renamed
             return False
 
         try:
-            info = dvc_fs.info(path)
+            info = dvc_fs.info(dvc_path)
         except FileNotFoundError:
             return False
 
@@ -278,13 +291,13 @@ def isdir(self, path):  # pylint: disable=arguments-renamed
         return info["type"] == "directory"
 
     def isdvc(self, path, **kwargs):
-        _, dvc_fs = self._get_fs_pair(path)
-        return dvc_fs is not None and dvc_fs.isdvc(path, **kwargs)
+        _, dvc_fs, dvc_path = self._get_fs_pair(path)
+        return dvc_fs is not None and dvc_fs.isdvc(dvc_path, **kwargs)
 
     def isfile(self, path):  # pylint: disable=arguments-renamed
         path = os.path.abspath(path)
 
-        fs, dvc_fs = self._get_fs_pair(path)
+        fs, dvc_fs, dvc_path = self._get_fs_pair(path)
 
         if dvc_fs and dvc_fs.repo.dvcignore.is_ignored_file(path):
             return False
@@ -300,7 +313,7 @@ def isfile(self, path):  # pylint: disable=arguments-renamed
             return False
 
         try:
-            info = dvc_fs.info(path)
+            info = dvc_fs.info(dvc_path)
         except FileNotFoundError:
             return False
 
@@ -328,10 +341,10 @@ def _subrepo_walk(self, dir_path, **kwargs):
         NOTE: subrepo will only be discovered when walking if
         ignore_subrepos is set to False.
         """
-        fs, dvc_fs = self._get_fs_pair(dir_path)
+        fs, dvc_fs, dvc_path = self._get_fs_pair(dir_path)
         fs_walk = fs.walk(dir_path, topdown=True)
         if dvc_fs:
-            dvc_walk = dvc_fs.walk(dir_path, topdown=True, **kwargs)
+            dvc_walk = _wrap_walk(dvc_fs, dvc_path, topdown=True, **kwargs)
         else:
             dvc_walk = None
         yield from self._walk(fs_walk, dvc_walk, **kwargs)
@@ -420,26 +433,26 @@ def walk(self, top, topdown=True, onerror=None, **kwargs):
         repo = self._get_repo(os.path.abspath(top))
         dvcfiles = kwargs.pop("dvcfiles", False)
 
-        fs, dvc_fs = self._get_fs_pair(top)
+        fs, dvc_fs, dvc_path = self._get_fs_pair(top)
         repo_exists = fs.exists(top)
 
         repo_walk = repo.dvcignore.walk(
             fs, top, topdown=topdown, onerror=onerror, **kwargs
         )
 
-        if not dvc_fs or (repo_exists and dvc_fs.isdvc(top)):
+        if not dvc_fs or (repo_exists and dvc_fs.isdvc(dvc_path)):
             yield from self._walk(repo_walk, None, dvcfiles=dvcfiles)
             return
 
         if not repo_exists:
-            yield from dvc_fs.walk(
-                top, topdown=topdown, onerror=onerror, **kwargs
+            yield from _wrap_walk(
+                dvc_fs, dvc_path, topdown=topdown, onerror=onerror, **kwargs
             )
 
         dvc_walk = None
-        if dvc_fs.exists(top):
-            dvc_walk = dvc_fs.walk(
-                top, topdown=topdown, onerror=onerror, **kwargs
+        if dvc_fs.exists(dvc_path):
+            dvc_walk = _wrap_walk(
+                dvc_fs, dvc_path, topdown=topdown, onerror=onerror, **kwargs
             )
 
         yield from self._walk(repo_walk, dvc_walk, dvcfiles=dvcfiles)
@@ -452,7 +465,7 @@ def find(self, path, prefix=None):
     def get_file(
         self, from_info, to_file, callback=DEFAULT_CALLBACK, **kwargs
     ):
-        fs, dvc_fs = self._get_fs_pair(from_info)
+        fs, dvc_fs, dvc_path = self._get_fs_pair(from_info)
         try:
             fs.get_file(  # pylint: disable=protected-access
                 from_info, to_file, callback=callback, **kwargs
@@ -463,14 +476,14 @@ def get_file(
                 raise
 
         dvc_fs.get_file(  # pylint: disable=protected-access
-            from_info, to_file, callback=callback, **kwargs
+            dvc_path, to_file, callback=callback, **kwargs
         )
 
     def info(self, path):
-        fs, dvc_fs = self._get_fs_pair(path)
+        fs, dvc_fs, dvc_path = self._get_fs_pair(path)
 
         try:
-            dvc_info = dvc_fs.info(path)
+            dvc_info = dvc_fs.info(dvc_path)
         except FileNotFoundError:
             dvc_info = None
 
@@ -503,9 +516,9 @@ def info(self, path):
             return dvc_info
 
     def checksum(self, path):
-        fs, dvc_fs = self._get_fs_pair(path)
+        fs, dvc_fs, dvc_path = self._get_fs_pair(path)
 
         try:
             return fs.checksum(path)
         except FileNotFoundError:
-            return dvc_fs.checksum(path)
+            return dvc_fs.checksum(dvc_path)

tests/func/test_fs.py

@@ -305,7 +305,7 @@ def test_download_callbacks_on_dvc_git_fs(tmp_dir, dvc, scm, fs_type):
 
     callback = fsspec.Callback()
     fs.download_file(
-        (tmp_dir / "file").fs_path,
+        "file",
         (tmp_dir / "file2").fs_path,
         callback=callback,
     )
@@ -317,7 +317,7 @@ def test_download_callbacks_on_dvc_git_fs(tmp_dir, dvc, scm, fs_type):
 
     callback = fsspec.Callback()
     fs.download(
-        (tmp_dir / "dir").fs_path,
+        "dir",
         (tmp_dir / "dir2").fs_path,
         callback=callback,
     )