From e04f1499a3ae8bd1b5a5a0e414fc4d5dfea1582e Mon Sep 17 00:00:00 2001 From: skshetry <18718008+skshetry@users.noreply.github.com> Date: Thu, 5 Jun 2025 08:30:22 +0545 Subject: [PATCH] fix(dvcfs): rename argument `url` to `repo` (#10754) See #10700 for the motivation. --- dvc/dependency/repo.py | 2 +- dvc/fs/dvc.py | 23 ++++++++++++++++------- dvc/testing/api_tests.py | 13 ++++++++----- tests/unit/fs/test_dvc.py | 17 +++++++++++++++++ tests/unit/fs/test_dvcfs.py | 24 +++++++++++++++++++++++- 5 files changed, 65 insertions(+), 14 deletions(-) diff --git a/dvc/dependency/repo.py b/dvc/dependency/repo.py index c1eb39c7d..b6fceb2d7 100644 --- a/dvc/dependency/repo.py +++ b/dvc/dependency/repo.py @@ -154,7 +154,7 @@ class RepoDependency(Dependency): config["cache"]["dir"] = self.repo.cache.local_cache_dir return DVCFileSystem( - url=self.def_repo[self.PARAM_URL], + repo=self.def_repo[self.PARAM_URL], rev=rev or self._get_rev(locked=locked), subrepos=True, config=config, diff --git a/dvc/fs/dvc.py b/dvc/fs/dvc.py index f1370de95..2a94a6c86 100644 --- a/dvc/fs/dvc.py +++ b/dvc/fs/dvc.py @@ -83,11 +83,10 @@ class _DVCFileSystem(AbstractFileSystem): cachable = False root_marker = "/" - def __init__( # noqa: PLR0913 + def __init__( self, - url: Optional[str] = None, + repo: Union["Repo", os.PathLike[str], str, None] = None, rev: Optional[str] = None, - repo: Optional["Repo"] = None, subrepos: bool = False, repo_factory: Optional[RepoFactory] = None, fo: Optional[str] = None, @@ -101,7 +100,8 @@ class _DVCFileSystem(AbstractFileSystem): """DVC + git-tracked files fs. Args: - path (str, optional): URL or path to a DVC/Git repository. + repo (str | os.PathLike[str] | Repo, optional): A url or a path to a DVC/Git + repository, or a `Repo` instance. Defaults to a DVC repository in the current working directory. Both HTTP and SSH protocols are supported for remote Git repos (e.g. [user@]server:project.git). @@ -111,7 +111,6 @@ class _DVCFileSystem(AbstractFileSystem): In case of a local repository, if rev is unspecified, it will default to the working directory. If the repo is not a Git repo, this option is ignored. - repo (:obj:`Repo`, optional): `Repo` instance. subrepos (bool): traverse to subrepos. By default, it ignores subrepos. repo_factory (callable): A function to initialize subrepo with. @@ -136,13 +135,23 @@ class _DVCFileSystem(AbstractFileSystem): ... rev="main", ... ) """ + from dvc.repo import Repo + + # kwargs.get("url") is for maintaining backward compatibility + repo = repo or fo or kwargs.get("url") + if isinstance(repo, Repo): + self._repo: Optional[Repo] = repo + url = None + else: + self._repo = None + url = os.fspath(repo) if repo else None + super().__init__() - self._repo = repo self._repo_factory = repo_factory self._traverse_subrepos = subrepos self._repo_stack = ExitStack() self._repo_kwargs = { - "url": url if url is not None else fo, + "url": url, "rev": rev, "subrepos": subrepos, "config": config, diff --git a/dvc/testing/api_tests.py b/dvc/testing/api_tests.py index e78147004..b05de30db 100644 --- a/dvc/testing/api_tests.py +++ b/dvc/testing/api_tests.py @@ -32,12 +32,13 @@ class TestAPI: "fs_kwargs", [ {}, - {"url": "{path}"}, - {"url": "{path}", "rev": "{default_branch}"}, - {"url": "file://{posixpath}"}, - {"url": "file://{posixpath}", "rev": "{default_branch}"}, + {"repo": "{path}"}, + {"repo": "{path}", "rev": "{default_branch}"}, + {"repo": "file://{posixpath}"}, + {"repo": "file://{posixpath}", "rev": "{default_branch}"}, + {"url": "{path}"}, # test for backward compatibility ], - ids=["current", "local", "local_rev", "git", "git_rev"], + ids=["current", "local", "local_rev", "git", "git_rev", "local_url"], ) def test_filesystem( self, @@ -59,6 +60,8 @@ class TestAPI: if clear_cache: remove(dvc.cache.repo.path) + if repo := fs_kwargs.get("repo"): + fs_kwargs["repo"] = repo.format(path=tmp_dir, posixpath=tmp_dir.as_posix()) if url := fs_kwargs.get("url"): fs_kwargs["url"] = url.format(path=tmp_dir, posixpath=tmp_dir.as_posix()) if rev := fs_kwargs.get("rev"): diff --git a/tests/unit/fs/test_dvc.py b/tests/unit/fs/test_dvc.py index 728f045e9..f2659a829 100644 --- a/tests/unit/fs/test_dvc.py +++ b/tests/unit/fs/test_dvc.py @@ -696,3 +696,20 @@ def test_fsid_url(erepo_dir): fs = DVCFileSystem(repo=dvc) assert fs.fsid != old_fsid assert fs.fsid == "dvcfs_" + tokenize(url, erepo_dir.scm.get_rev()) + + +@pytest.mark.parametrize( + "fs_kwargs", + [ + lambda tmp_dir, dvc: {}, # noqa: ARG005 + lambda tmp_dir, dvc: {"repo": tmp_dir}, # noqa: ARG005 + lambda tmp_dir, dvc: {"repo": os.fspath(tmp_dir)}, # noqa: ARG005 + lambda tmp_dir, dvc: {"url": tmp_dir}, # noqa: ARG005 + lambda tmp_dir, dvc: {"url": os.fspath(tmp_dir)}, # noqa: ARG005 + lambda tmp_dir, dvc: {"repo": dvc}, # noqa: ARG005 + ], +) +def test_init_arg(tmp_dir, dvc, fs_kwargs): + fs = DVCFileSystem(**fs_kwargs(tmp_dir, dvc)) + + assert fs.repo.root_dir == dvc.root_dir diff --git a/tests/unit/fs/test_dvcfs.py b/tests/unit/fs/test_dvcfs.py index 3dbc2ea7c..c3ad6b598 100644 --- a/tests/unit/fs/test_dvcfs.py +++ b/tests/unit/fs/test_dvcfs.py @@ -779,7 +779,7 @@ def test_maxdepth(tmp_dir, dvc, scm): commit="add dir", ) - fs = DVCFileSystem(url=tmp_dir) + fs = DVCFileSystem(tmp_dir) fs.get("dir", "dir1", recursive=True, maxdepth=1) assert (tmp_dir / "dir1").read_text() == {"file1": "file1"} @@ -803,3 +803,25 @@ def test_maxdepth(tmp_dir, dvc, scm): "subdir2": {"file3": "file3", "subdir3": {"file4": "file4"}}, }, } + + +@pytest.mark.parametrize( + "fs_args", + [ + lambda tmp_dir, dvc: ((), {}), # noqa: ARG005 + lambda tmp_dir, dvc: ((dvc,), {}), # noqa: ARG005 + lambda tmp_dir, dvc: ((tmp_dir,), {}), # noqa: ARG005 + lambda tmp_dir, dvc: ((str(tmp_dir),), {}), # noqa: ARG005 + lambda tmp_dir, dvc: ((), {"repo": tmp_dir}), # noqa: ARG005 + lambda tmp_dir, dvc: ((), {"repo": os.fspath(tmp_dir)}), # noqa: ARG005 + # url= is deprecated, but is still supported for backward compatibility + lambda tmp_dir, dvc: ((), {"url": tmp_dir}), # noqa: ARG005 + lambda tmp_dir, dvc: ((), {"url": os.fspath(tmp_dir)}), # noqa: ARG005 + lambda tmp_dir, dvc: ((), {"repo": dvc}), # noqa: ARG005 + ], +) +def test_init_arg(tmp_dir, dvc, fs_args): + args, kwargs = fs_args(tmp_dir, dvc) + fs = DVCFileSystem(*args, **kwargs) + + assert fs.repo.root_dir == dvc.root_dir