-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
cli: add: add an --output argument #5284
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,12 +1,14 @@ | ||
import logging | ||
import os | ||
import shutil | ||
from typing import TYPE_CHECKING | ||
|
||
import colorama | ||
|
||
from ..exceptions import ( | ||
CacheLinkError, | ||
OutputDuplicationError, | ||
OutputNotCreatedError, | ||
OverlappingOutputPathsError, | ||
RecursiveAddingWhileUsingFilename, | ||
) | ||
|
@@ -30,6 +32,7 @@ def add( | |
recursive=False, | ||
no_commit=False, | ||
fname=None, | ||
output=None, | ||
**kwargs, | ||
): | ||
from dvc.utils.collections import ensure_list | ||
|
@@ -38,6 +41,20 @@ def add( | |
raise RecursiveAddingWhileUsingFilename() | ||
|
||
targets = ensure_list(targets) | ||
|
||
if output: | ||
try: | ||
dname = os.path.dirname(output) | ||
|
||
if dname and not os.path.isdir(dname): | ||
os.makedirs(dname) | ||
|
||
shutil.copy2(targets[0], output) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This will be really slow with big files/dirs. We could be able to add it without this uneeded copy. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also, you are checking that there is only one target in CLI, but not in API (they could be used separately), so |
||
|
||
targets = [output] | ||
except OSError: | ||
raise OutputNotCreatedError(output) | ||
|
||
link_failures = [] | ||
stages_list = [] | ||
num_targets = len(targets) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,6 +4,7 @@ | |
import os | ||
import shutil | ||
import stat | ||
import tempfile | ||
import textwrap | ||
import time | ||
|
||
|
@@ -59,6 +60,40 @@ def test_add(tmp_dir, dvc): | |
} | ||
|
||
|
||
def test_add_output(tmp_dir, dvc): | ||
(fd, fname) = tempfile.mkstemp() | ||
os.write(fd, b"foo") | ||
os.close(fd) | ||
|
||
ret = main(["add", "--output", "foo", "bar", "baz"]) | ||
assert ret == 1 | ||
|
||
md5, _ = file_md5(fname) | ||
stage = dvc.add(fname, output="foo")[0] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You are actually implementing straight-to-cache functionality #4520 for local files here π We have a WIP for a general straight-to-remote, where we also use |
||
|
||
assert stage is not None | ||
|
||
assert isinstance(stage, Stage) | ||
assert os.path.isfile(stage.path) | ||
assert len(stage.outs) == 1 | ||
assert len(stage.deps) == 0 | ||
assert stage.cmd is None | ||
assert stage.outs[0].hash_info == HashInfo("md5", md5) | ||
assert stage.md5 is None | ||
|
||
assert load_yaml("foo.dvc") == { | ||
"outs": [ | ||
{ | ||
"md5": "acbd18db4cc2f85cedef654fccc4a4d8", | ||
"path": "foo", | ||
"size": 3, | ||
} | ||
], | ||
} | ||
|
||
os.unlink(fname) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. maybe let's just use tmp_path fixture or smth like that to avoid doing this manually and using file descriptors? |
||
|
||
|
||
@pytest.mark.skipif(os.name == "nt", reason="can't set exec bit on Windows") | ||
def test_add_executable(tmp_dir, dvc): | ||
tmp_dir.gen("foo", "foo") | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This will clash with
--out
being introduced in #5198There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It should be
--out
, but it won't clash otherwise. They are actually the same thing :)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can someone explain what this does with an example, perhaps?
It's also not obvious to me how this is a step towards #4657 which doesn't seem to involve adding external data (but this does).
Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I just noticed your answer @efiop but my questions remain because
--out
(from #5198) is not stand-alone, it can only be used along with--to-remote
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BTW should this maybe be a stand-alone, general option that
add
can always use (whether the target is external or not)? As disucssed in #5198 (review) π