Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add, import-url: always resolve the output in the local path for --to-remote #5473

Merged
merged 3 commits into from
Feb 19, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
14 changes: 12 additions & 2 deletions dvc/repo/add.py
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ def add( # noqa: C901
invalid_opt = "--no-commit option"
elif recursive:
invalid_opt = "--recursive option"
elif kwargs.get("external"):
invalid_opt = "--external option"
else:
message = "{option} can't be used without --to-remote"
if kwargs.get("remote"):
Expand Down Expand Up @@ -88,7 +90,12 @@ def add( # noqa: C901
)

stages = _create_stages(
repo, sub_targets, fname, pbar=pbar, **kwargs,
repo,
sub_targets,
fname,
pbar=pbar,
transfer=to_remote or to_cache,
**kwargs,
)

try:
Expand Down Expand Up @@ -230,6 +237,7 @@ def _create_stages(
external=False,
glob=False,
desc=None,
transfer=False,
**kwargs,
):
from dvc.dvcfile import Dvcfile
Expand All @@ -246,7 +254,9 @@ def _create_stages(
):
if kwargs.get("out"):
out = resolve_output(out, kwargs["out"])
path, wdir, out = resolve_paths(repo, out)
path, wdir, out = resolve_paths(
repo, out, always_local=transfer and not kwargs.get("out")
)
stage = create_stage(
Stage,
repo,
Expand Down
4 changes: 3 additions & 1 deletion dvc/repo/imp_url.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ def imp_url(
from dvc.stage import Stage, create_stage, restore_meta

out = resolve_output(url, out)
path, wdir, out = resolve_paths(self, out)
path, wdir, out = resolve_paths(
self, out, always_local=to_remote and not out
)

if to_remote and no_exec:
raise InvalidArgumentError(
Expand Down
5 changes: 4 additions & 1 deletion dvc/utils/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -357,7 +357,7 @@ def resolve_output(inp, out):
return ret


def resolve_paths(repo, out):
def resolve_paths(repo, out, always_local=False):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IIUC, always_local is a high-level concept. Here, it does not convey what it does properly: using the base path as an output name. But, I don't have a good name to suggest either (that is succinct), so I'm fine with it as well. 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I also couldn't think of a better one 🤔

from urllib.parse import urlparse

from ..dvcfile import DVC_FILE_SUFFIX
Expand Down Expand Up @@ -393,6 +393,9 @@ def resolve_paths(repo, out):
wdir = dirname
out = base

if always_local:
out = base

path = os.path.join(wdir, base + DVC_FILE_SUFFIX)

return (path, wdir, out)
Expand Down
29 changes: 26 additions & 3 deletions tests/func/test_add.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,10 @@
from dvc.output.base import OutputAlreadyTrackedError, OutputIsStageFileError
from dvc.repo import Repo as DvcRepo
from dvc.stage import Stage
from dvc.stage.exceptions import StagePathNotFoundError
from dvc.stage.exceptions import (
StageExternalOutputsError,
StagePathNotFoundError,
)
from dvc.system import System
from dvc.utils import LARGE_DIR_SIZE, file_md5, relpath
from dvc.utils.fs import path_isin
Expand Down Expand Up @@ -324,8 +327,6 @@ def test_add_filtered_files_in_dir(
indirect=["workspace"],
)
def test_add_external_file(tmp_dir, dvc, workspace, hash_name, hash_value):
from dvc.stage.exceptions import StageExternalOutputsError

workspace.gen("file", "file")

with pytest.raises(StageExternalOutputsError):
Expand Down Expand Up @@ -1013,12 +1014,34 @@ def test_add_to_remote(tmp_dir, dvc, local_cloud, local_remote):
assert local_remote.hash_to_path_info(hash_info.value).read_text() == "foo"


def test_add_to_remote_absolute(tmp_dir, make_tmp_dir, dvc, local_remote):
tmp_abs_dir = make_tmp_dir("abs")
tmp_foo = tmp_abs_dir / "foo"
tmp_foo.write_text("foo")

dvc.add(str(tmp_foo), to_remote=True)
tmp_foo.unlink()

foo = tmp_dir / "foo"
assert foo.with_suffix(".dvc").exists()
assert not os.path.exists(tmp_foo)

dvc.pull("foo")
assert not os.path.exists(tmp_foo)
assert foo.read_text() == "foo"

with pytest.raises(StageExternalOutputsError):
tmp_bar = tmp_abs_dir / "bar"
dvc.add(str(tmp_foo), out=str(tmp_bar), to_remote=True)


@pytest.mark.parametrize(
"invalid_opt, kwargs",
[
("multiple targets", {"targets": ["foo", "bar", "baz"]}),
("--no-commit", {"targets": ["foo"], "no_commit": True}),
("--recursive", {"targets": ["foo"], "recursive": True},),
("--external", {"targets": ["foo"], "external": True}),
],
)
def test_add_to_remote_invalid_combinations(dvc, invalid_opt, kwargs):
Expand Down
15 changes: 15 additions & 0 deletions tests/func/test_import_url.py
Original file line number Diff line number Diff line change
Expand Up @@ -340,6 +340,21 @@ def test_import_url_to_remote_directory(tmp_dir, dvc, workspace, local_remote):
)


def test_import_url_to_remote_absolute(
tmp_dir, make_tmp_dir, dvc, local_remote
):
tmp_abs_dir = make_tmp_dir("abs")
tmp_foo = tmp_abs_dir / "foo"
tmp_foo.write_text("foo")

stage = dvc.imp_url(str(tmp_foo), to_remote=True)

foo = tmp_dir / "foo"
assert stage.deps[0].fspath == str(tmp_foo)
assert stage.outs[0].fspath == os.fspath(foo)
assert foo.with_suffix(".dvc").exists()


def test_import_url_to_remote_invalid_combinations(dvc):
with pytest.raises(InvalidArgumentError, match="--no-exec"):
dvc.imp_url("s3://bucket/foo", no_exec=True, to_remote=True)