From f47d21b0c29f5603bff09cc51c4a14a1bdf962c6 Mon Sep 17 00:00:00 2001 From: Ruslan Kuprieiev Date: Fri, 1 Sep 2023 02:17:17 +0300 Subject: [PATCH] [WIP] ls-url/get-url/import-url: introduce --fs-config (#9888) ls-url/get-url/import-url: introduce --fs-config Fixes #9886 --- dvc/commands/get_url.py | 10 +++++++++- dvc/commands/imp_url.py | 10 +++++++++- dvc/commands/ls_url.py | 15 +++++++++++++-- dvc/dependency/__init__.py | 6 +++++- dvc/output.py | 8 ++++++++ dvc/repo/get_url.py | 4 ++-- dvc/repo/ls_url.py | 4 ++-- dvc/testing/workspace_tests.py | 16 ++++++++-------- tests/func/test_import_url.py | 10 ++++++++++ tests/unit/command/test_get_url.py | 2 +- tests/unit/command/test_imp_url.py | 3 +++ tests/unit/command/test_ls_url.py | 4 ++-- 12 files changed, 72 insertions(+), 20 deletions(-) diff --git a/dvc/commands/get_url.py b/dvc/commands/get_url.py index e96deb50e..9270f57a3 100644 --- a/dvc/commands/get_url.py +++ b/dvc/commands/get_url.py @@ -3,7 +3,7 @@ import logging from dvc.cli import completion from dvc.cli.command import CmdBaseNoRepo -from dvc.cli.utils import append_doc_link +from dvc.cli.utils import DictAction, append_doc_link from dvc.exceptions import DvcException logger = logging.getLogger(__name__) @@ -19,6 +19,7 @@ class CmdGetUrl(CmdBaseNoRepo): out=self.args.out, jobs=self.args.jobs, force=self.args.force, + fs_config=self.args.fs_config, ) return 0 except DvcException: @@ -58,4 +59,11 @@ def add_parser(subparsers, parent_parser): default=False, help="Override local file or folder if exists.", ) + get_parser.add_argument( + "--fs-config", + type=str, + nargs="*", + action=DictAction, + help="Config options for the target url.", + ) get_parser.set_defaults(func=CmdGetUrl) diff --git a/dvc/commands/imp_url.py b/dvc/commands/imp_url.py index 21d826c57..039c34545 100644 --- a/dvc/commands/imp_url.py +++ b/dvc/commands/imp_url.py @@ -3,7 +3,7 @@ import logging from dvc.cli import completion from dvc.cli.command import CmdBase -from dvc.cli.utils import append_doc_link +from dvc.cli.utils import DictAction, append_doc_link from dvc.exceptions import DvcException logger = logging.getLogger(__name__) @@ -22,6 +22,7 @@ class CmdImportUrl(CmdBase): jobs=self.args.jobs, force=self.args.force, version_aware=self.args.version_aware, + fs_config=self.args.fs_config, ) except DvcException: logger.exception( @@ -114,4 +115,11 @@ def add_parser(subparsers, parent_parser): default=False, help="Import using cloud versioning. Implied if the URL contains a version ID.", ) + import_parser.add_argument( + "--fs-config", + type=str, + nargs="*", + action=DictAction, + help="Config options for the target url.", + ) import_parser.set_defaults(func=CmdImportUrl) diff --git a/dvc/commands/ls_url.py b/dvc/commands/ls_url.py index 80034f549..fbbdd86d0 100644 --- a/dvc/commands/ls_url.py +++ b/dvc/commands/ls_url.py @@ -2,7 +2,7 @@ import argparse import logging from dvc.cli.command import CmdBaseNoRepo -from dvc.cli.utils import append_doc_link +from dvc.cli.utils import DictAction, append_doc_link from .ls import show_entries @@ -13,7 +13,11 @@ class CmdListUrl(CmdBaseNoRepo): def run(self): from dvc.repo import Repo - entries = Repo.ls_url(self.args.url, recursive=self.args.recursive) + entries = Repo.ls_url( + self.args.url, + recursive=self.args.recursive, + fs_config=self.args.fs_config, + ) if entries: show_entries(entries, with_color=True, with_size=self.args.size) return 0 @@ -43,4 +47,11 @@ def add_parser(subparsers, parent_parser): action="store_true", help="Show sizes.", ) + lsurl_parser.add_argument( + "--fs-config", + type=str, + nargs="*", + action=DictAction, + help="Config options for the target url.", + ) lsurl_parser.set_defaults(func=CmdListUrl) diff --git a/dvc/dependency/__init__.py b/dvc/dependency/__init__.py index bf2c64204..2904da8a8 100644 --- a/dvc/dependency/__init__.py +++ b/dvc/dependency/__init__.py @@ -15,6 +15,7 @@ SCHEMA: Mapping[str, Any] = { **ARTIFACT_SCHEMA, **RepoDependency.REPO_SCHEMA, Output.PARAM_FILES: [DIR_FILES_SCHEMA], + Output.PARAM_FS_CONFIG: dict, } @@ -36,7 +37,10 @@ def loadd_from(stage, d_list): p = d.pop(Output.PARAM_PATH, None) files = d.pop(Output.PARAM_FILES, None) hash_name = d.pop(Output.PARAM_HASH, None) - ret.append(_get(stage, p, d, files=files, hash_name=hash_name)) + fs_config = d.pop(Output.PARAM_FS_CONFIG, None) + ret.append( + _get(stage, p, d, files=files, hash_name=hash_name, fs_config=fs_config) + ) return ret diff --git a/dvc/output.py b/dvc/output.py index ae8b303fb..5cf213300 100644 --- a/dvc/output.py +++ b/dvc/output.py @@ -97,6 +97,7 @@ def loadd_from(stage, d_list): files = d.pop(Output.PARAM_FILES, None) push = d.pop(Output.PARAM_PUSH, True) hash_name = d.pop(Output.PARAM_HASH, None) + fs_config = d.pop(Output.PARAM_FS_CONFIG, None) ret.append( _get( stage, @@ -111,6 +112,7 @@ def loadd_from(stage, d_list): files=files, push=push, hash_name=hash_name, + fs_config=fs_config, ) ) return ret @@ -313,6 +315,7 @@ class Output: PARAM_PUSH = "push" PARAM_CLOUD = "cloud" PARAM_HASH = "hash" + PARAM_FS_CONFIG = "fs_config" DoesNotExistError: Type[DvcException] = OutputDoesNotExistError IsNotFileOrDirError: Type[DvcException] = OutputIsNotFileOrDirError @@ -351,6 +354,7 @@ class Output: if meta.version_id or files: fs_kwargs["version_aware"] = True + self.def_fs_config = fs_config if fs_config is not None: fs_kwargs.update(**fs_config) @@ -872,6 +876,9 @@ class Output: ret[self.PARAM_PATH] = path + if self.def_fs_config: + ret[self.PARAM_FS_CONFIG] = self.def_fs_config + if not self.IS_DEPENDENCY: ret.update(self.annot.to_dict()) if not self.use_cache: @@ -1537,4 +1544,5 @@ SCHEMA = { Output.PARAM_REMOTE: str, Output.PARAM_PUSH: bool, Output.PARAM_FILES: [DIR_FILES_SCHEMA], + Output.PARAM_FS_CONFIG: dict, } diff --git a/dvc/repo/get_url.py b/dvc/repo/get_url.py index 7b6ab4f8f..ff0e57cf4 100644 --- a/dvc/repo/get_url.py +++ b/dvc/repo/get_url.py @@ -6,12 +6,12 @@ from dvc.fs import download, parse_external_url from dvc.utils import resolve_output -def get_url(url, out=None, *, config=None, jobs=None, force=False): +def get_url(url, out=None, *, fs_config=None, jobs=None, force=False): out = resolve_output(url, out, force=force) out = os.path.abspath(out) (out,) = output.loads_from(None, [out], use_cache=False) - src_fs, src_path = parse_external_url(url, config) + src_fs, src_path = parse_external_url(url, fs_config) if not src_fs.exists(src_path): raise URLMissingError(url) download(src_fs, src_path, out.fs_path, jobs=jobs) diff --git a/dvc/repo/ls_url.py b/dvc/repo/ls_url.py index 496daa128..bae2e9ff3 100644 --- a/dvc/repo/ls_url.py +++ b/dvc/repo/ls_url.py @@ -2,8 +2,8 @@ from dvc.exceptions import URLMissingError from dvc.fs import parse_external_url -def ls_url(url, *, config=None, recursive=False): - fs, fs_path = parse_external_url(url, config=config) +def ls_url(url, *, fs_config=None, recursive=False): + fs, fs_path = parse_external_url(url, config=fs_config) try: info = fs.info(fs_path) except FileNotFoundError as exc: diff --git a/dvc/testing/workspace_tests.py b/dvc/testing/workspace_tests.py index 216c83ff1..e99860114 100644 --- a/dvc/testing/workspace_tests.py +++ b/dvc/testing/workspace_tests.py @@ -173,7 +173,7 @@ class TestLsUrl: def test_file(self, cloud, fname): cloud.gen({fname: "foo contents"}) fs, fs_path = parse_external_url(cloud.url, cloud.config) - result = ls_url(str(cloud / fname), config=cloud.config) + result = ls_url(str(cloud / fname), fs_config=cloud.config) match_files( fs, result, @@ -185,7 +185,7 @@ class TestLsUrl: if not (cloud / "dir").is_dir(): pytest.skip("Cannot create directories on this cloud") fs, _ = parse_external_url(cloud.url, cloud.config) - result = ls_url(str(cloud / "dir"), config=cloud.config) + result = ls_url(str(cloud / "dir"), fs_config=cloud.config) match_files( fs, result, @@ -200,7 +200,7 @@ class TestLsUrl: if not (cloud / "dir").is_dir(): pytest.skip("Cannot create directories on this cloud") fs, _ = parse_external_url(cloud.url, cloud.config) - result = ls_url(str(cloud / "dir"), config=cloud.config, recursive=True) + result = ls_url(str(cloud / "dir"), fs_config=cloud.config, recursive=True) match_files( fs, result, @@ -212,14 +212,14 @@ class TestLsUrl: def test_nonexistent(self, cloud): with pytest.raises(URLMissingError): - ls_url(str(cloud / "dir"), config=cloud.config) + ls_url(str(cloud / "dir"), fs_config=cloud.config) class TestGetUrl: def test_get_file(self, cloud, tmp_dir): cloud.gen({"foo": "foo contents"}) - Repo.get_url(str(cloud / "foo"), "foo_imported", config=cloud.config) + Repo.get_url(str(cloud / "foo"), "foo_imported", fs_config=cloud.config) assert (tmp_dir / "foo_imported").is_file() assert (tmp_dir / "foo_imported").read_text() == "foo contents" @@ -229,7 +229,7 @@ class TestGetUrl: if not (cloud / "foo").is_dir(): pytest.skip("Cannot create directories on this cloud") - Repo.get_url(str(cloud / "foo"), "foo_imported", config=cloud.config) + Repo.get_url(str(cloud / "foo"), "foo_imported", fs_config=cloud.config) assert (tmp_dir / "foo_imported").is_dir() assert (tmp_dir / "foo_imported" / "foo").is_file() @@ -242,14 +242,14 @@ class TestGetUrl: pytest.skip("Cannot create directories on this cloud") tmp_dir.gen({"dir": {"subdir": {}}}) - Repo.get_url(str(cloud / "src" / "foo"), dname, config=cloud.config) + Repo.get_url(str(cloud / "src" / "foo"), dname, fs_config=cloud.config) assert (tmp_dir / dname).is_dir() assert (tmp_dir / dname / "foo").read_text() == "foo contents" def test_get_url_nonexistent(self, cloud): with pytest.raises(URLMissingError): - Repo.get_url(str(cloud / "nonexistent"), config=cloud.config) + Repo.get_url(str(cloud / "nonexistent"), fs_config=cloud.config) class TestToRemote: diff --git a/tests/func/test_import_url.py b/tests/func/test_import_url.py index 02eecbbfd..e8ae45d72 100644 --- a/tests/func/test_import_url.py +++ b/tests/func/test_import_url.py @@ -249,9 +249,19 @@ def test_import_url_fs_config(tmp_dir, dvc, workspace, mocker): dep_init = mocker.spy(Dependency, "__init__") dvc.imp_url(url, fs_config={"jobs": 42}) + stage = load_file(dvc, "foo.dvc").stage + assert stage.deps[0].def_fs_config == {"jobs": 42} + dep_init_kwargs = dep_init.call_args[1] assert dep_init_kwargs.get("fs_config") == {"jobs": 42} assert get_fs_config.call_args_list[0][1] == {"url": "foo"} assert get_fs_config.call_args_list[1][1] == {"url": url, "jobs": 42} assert get_fs_config.call_args_list[2][1] == {"name": "workspace"} + + dep_init.reset_mock() + + dvc.pull("foo.dvc") + + dep_init_kwargs = dep_init.call_args[1] + assert dep_init_kwargs.get("fs_config") == {"jobs": 42} diff --git a/tests/unit/command/test_get_url.py b/tests/unit/command/test_get_url.py index a2b27495b..617940f15 100644 --- a/tests/unit/command/test_get_url.py +++ b/tests/unit/command/test_get_url.py @@ -11,4 +11,4 @@ def test_get_url(mocker): assert cmd.run() == 0 - m.assert_called_once_with("src", out="out", jobs=5, force=False) + m.assert_called_once_with("src", out="out", jobs=5, force=False, fs_config=None) diff --git a/tests/unit/command/test_imp_url.py b/tests/unit/command/test_imp_url.py index 8944cd4c2..3804f988e 100644 --- a/tests/unit/command/test_imp_url.py +++ b/tests/unit/command/test_imp_url.py @@ -34,6 +34,7 @@ def test_import_url(mocker, dvc): jobs=4, force=False, version_aware=False, + fs_config=None, ) @@ -83,6 +84,7 @@ def test_import_url_no_exec_download_flags(mocker, flag, expected, dvc): jobs=None, force=False, version_aware=False, + fs_config=None, **expected, ) @@ -115,6 +117,7 @@ def test_import_url_to_remote(mocker, dvc): jobs=None, force=False, version_aware=False, + fs_config=None, ) diff --git a/tests/unit/command/test_ls_url.py b/tests/unit/command/test_ls_url.py index 84f341856..20e2676bf 100644 --- a/tests/unit/command/test_ls_url.py +++ b/tests/unit/command/test_ls_url.py @@ -10,7 +10,7 @@ def test_ls_url(mocker): assert cmd.run() == 0 - m.assert_called_once_with("src", recursive=False) + m.assert_called_once_with("src", recursive=False, fs_config=None) def test_recursive(mocker): @@ -21,4 +21,4 @@ def test_recursive(mocker): assert cmd.run() == 0 - m.assert_called_once_with("src", recursive=True) + m.assert_called_once_with("src", recursive=True, fs_config=None)