Skip to content

Commit

Permalink
run: use -f/--force to overwrite, fix msg
Browse files Browse the repository at this point in the history
Also, makes fixes for `--overwrite` rename to `--force`
and also remove `remove_with_prompt` which was only being
used with `force\=True` anyway.

Fix #4003
  • Loading branch information
skshetry committed Jun 10, 2020
1 parent 91a7194 commit 21e65dd
Show file tree
Hide file tree
Showing 9 changed files with 48 additions and 59 deletions.
5 changes: 3 additions & 2 deletions dvc/command/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,7 @@ def run(self):
fname=self.args.file,
wdir=self.args.wdir,
no_exec=self.args.no_exec,
overwrite=self.args.overwrite,
force=self.args.force,
run_cache=not self.args.no_run_cache,
no_commit=self.args.no_commit,
outs_persist=self.args.outs_persist,
Expand Down Expand Up @@ -176,7 +176,8 @@ def add_parser(subparsers, parent_parser):
help="Only create stage file without actually running it.",
)
run_parser.add_argument(
"--overwrite",
"-f",
"--force",
action="store_true",
default=False,
help="Overwrite existing stage",
Expand Down
18 changes: 0 additions & 18 deletions dvc/dvcfile.py
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,9 @@

from voluptuous import MultipleInvalid

import dvc.prompt as prompt
from dvc.exceptions import DvcException
from dvc.stage import serialize
from dvc.stage.exceptions import (
StageFileAlreadyExistsError,
StageFileBadNameError,
StageFileDoesNotExistError,
StageFileFormatError,
Expand Down Expand Up @@ -109,9 +107,6 @@ def validate(cls, d, fname=None):
except MultipleInvalid as exc:
raise StageFileFormatError(f"'{fname}' format error: {exc}")

def remove_with_prompt(self, force=False):
raise NotImplementedError

def remove(self, force=False):
with contextlib.suppress(FileNotFoundError):
os.unlink(self.path)
Expand Down Expand Up @@ -148,19 +143,6 @@ def dump(self, stage, **kwargs):
dump_yaml(self.path, serialize.to_single_stage_file(stage))
self.repo.scm.track_file(self.relpath)

def remove_with_prompt(self, force=False):
if not self.exists():
return

msg = (
"'{}' already exists. Do you wish to run the command and "
"overwrite it?".format(relpath(self.path))
)
if not (force or prompt.confirm(msg)):
raise StageFileAlreadyExistsError(self.path)

self.remove()

def remove_stage(self, stage):
self.remove()

Expand Down
2 changes: 1 addition & 1 deletion dvc/repo/add.py
Original file line number Diff line number Diff line change
Expand Up @@ -145,7 +145,7 @@ def _create_stages(repo, targets, fname, pbar=None, external=False):
external=external,
)
if stage:
Dvcfile(repo, stage.path).remove_with_prompt(force=True)
Dvcfile(repo, stage.path).remove()

repo._reset()

Expand Down
2 changes: 1 addition & 1 deletion dvc/repo/imp_url.py
Original file line number Diff line number Diff line change
Expand Up @@ -39,7 +39,7 @@ def imp_url(self, url, out=None, fname=None, erepo=None, frozen=True):
return None

dvcfile = Dvcfile(self, stage.path)
dvcfile.remove_with_prompt(force=True)
dvcfile.remove()

try:
self.check_modified_graph([stage])
Expand Down
29 changes: 21 additions & 8 deletions dvc/repo/run.py
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
from funcy import concat, first, lfilter

from dvc.exceptions import InvalidArgumentError
from dvc.stage import PipelineStage
from dvc.stage.exceptions import (
DuplicateStageName,
InvalidStageName,
Expand Down Expand Up @@ -56,10 +57,25 @@ def _get_file_path(kwargs):
)


def _check_stage_exists(dvcfile, stage):
if not dvcfile.exists():
return

hint = "Use '--force' to overwrite."
if stage.__class__ != PipelineStage:
raise StageFileAlreadyExistsError(
f"'{stage.relpath}' already exists. {hint}"
)
elif stage.name and stage.name in dvcfile.stages:
raise DuplicateStageName(
f"Stage '{stage.name}' already exists in '{stage.relpath}'. {hint}"
)


@locked
@scm_context
def run(self, fname=None, no_exec=False, single_stage=False, **kwargs):
from dvc.stage import PipelineStage, Stage, create_stage
from dvc.stage import Stage, create_stage
from dvc.dvcfile import Dvcfile, PIPELINE_FILE

if not kwargs.get("cmd"):
Expand Down Expand Up @@ -93,13 +109,10 @@ def run(self, fname=None, no_exec=False, single_stage=False, **kwargs):
return None

dvcfile = Dvcfile(self, stage.path)
if dvcfile.exists():
if kwargs.get("overwrite", True):
dvcfile.remove_stage(stage)
elif stage_cls != PipelineStage:
raise StageFileAlreadyExistsError(dvcfile.relpath)
elif stage_name and stage_name in dvcfile.stages:
raise DuplicateStageName(stage_name, dvcfile)
if kwargs.get("force", True):
dvcfile.remove_stage(stage)
else:
_check_stage_exists(dvcfile, stage)

try:
self.check_modified_graph([stage])
Expand Down
11 changes: 2 additions & 9 deletions dvc/stage/exceptions.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,7 @@ def __init__(self, fname):


class StageFileAlreadyExistsError(DvcException):
def __init__(self, relpath):
msg = f"not overwriting '{relpath}'"
super().__init__(msg)
pass


class StageFileIsNotDvcFileError(DvcException):
Expand Down Expand Up @@ -100,12 +98,7 @@ def __init__(self, file):


class DuplicateStageName(DvcException):
def __init__(self, name, file):
super().__init__(
"Stage '{name}' already exists in '{relpath}'.".format(
name=name, relpath=file.relpath
)
)
pass


class InvalidStageName(DvcException):
Expand Down
4 changes: 2 additions & 2 deletions tests/func/test_run_multistage.py
Original file line number Diff line number Diff line change
Expand Up @@ -218,8 +218,8 @@ def test_run_already_exists(tmp_dir, dvc, run_copy):
tmp_dir.dvc_gen("foo", "foo")
run_copy("foo", "bar", name="copy")
with pytest.raises(DuplicateStageName):
run_copy("bar", "foobar", name="copy", overwrite=False)
run_copy("bar", "foobar", name="copy", overwrite=True)
run_copy("bar", "foobar", name="copy", force=False)
run_copy("bar", "foobar", name="copy", force=True)


supported_params = {
Expand Down
28 changes: 14 additions & 14 deletions tests/func/test_run_single_stage.py
Original file line number Diff line number Diff line change
Expand Up @@ -350,7 +350,7 @@ def test(self):
ret = main(
[
"run",
"--overwrite",
"--force",
"--no-run-cache",
"--single-stage",
"-d",
Expand Down Expand Up @@ -408,7 +408,7 @@ def test(self):
ret = main(
[
"run",
"--overwrite",
"--force",
"--no-run-cache",
"--single-stage",
"-d",
Expand Down Expand Up @@ -467,7 +467,7 @@ def test(self):
ret = main(
[
"run",
"--overwrite",
"--force",
"--no-run-cache",
"--single-stage",
"-d",
Expand Down Expand Up @@ -552,7 +552,7 @@ def test(self):
self.FOO,
"-d",
self.CODE,
"--overwrite",
"--force",
"--no-run-cache",
"--single-stage",
"-o",
Expand All @@ -576,7 +576,7 @@ def test(self):
ret = main(
[
"run",
"--overwrite",
"--force",
"--single-stage",
"--file",
"out.dvc",
Expand Down Expand Up @@ -676,19 +676,19 @@ def test_rerun_deterministic_ignore_cache(tmp_dir, run_copy):


def test_rerun_callback(dvc):
def run_callback(overwrite=False):
def run_callback(force=False):
return dvc.run(
cmd="echo content > out",
outs=["out"],
deps=[],
overwrite=overwrite,
force=force,
single_stage=True,
)

assert run_callback() is not None
with pytest.raises(StageFileAlreadyExistsError):
assert run_callback() is not None
assert run_callback(overwrite=True) is not None
assert run_callback(force=True) is not None


def test_rerun_changed_dep(tmp_dir, run_copy):
Expand All @@ -697,8 +697,8 @@ def test_rerun_changed_dep(tmp_dir, run_copy):

tmp_dir.gen("foo", "changed content")
with pytest.raises(StageFileAlreadyExistsError):
run_copy("foo", "out", overwrite=False, single_stage=True)
assert run_copy("foo", "out", overwrite=True, single_stage=True)
run_copy("foo", "out", force=False, single_stage=True)
assert run_copy("foo", "out", force=True, single_stage=True)


def test_rerun_changed_stage(tmp_dir, run_copy):
Expand All @@ -707,7 +707,7 @@ def test_rerun_changed_stage(tmp_dir, run_copy):

tmp_dir.gen("bar", "bar content")
with pytest.raises(StageFileAlreadyExistsError):
run_copy("bar", "out", overwrite=False, single_stage=True)
run_copy("bar", "out", force=False, single_stage=True)


def test_rerun_changed_out(tmp_dir, run_copy):
Expand All @@ -716,7 +716,7 @@ def test_rerun_changed_out(tmp_dir, run_copy):

Path("out").write_text("modification")
with pytest.raises(StageFileAlreadyExistsError):
run_copy("foo", "out", overwrite=False, single_stage=True)
run_copy("foo", "out", force=False, single_stage=True)


class TestRunCommit(TestDvc):
Expand Down Expand Up @@ -870,7 +870,7 @@ def _run_twice_with_same_outputs(self):
"run",
self._outs_command,
self.FOO,
"--overwrite",
"--force",
"--single-stage",
f"echo {self.BAR_CONTENTS} >> {self.FOO}",
]
Expand Down Expand Up @@ -946,7 +946,7 @@ def test_ignore_run_cache(self):

cmd = [
"run",
"--overwrite",
"--force",
"--single-stage",
"--deps",
"immutable",
Expand Down
8 changes: 4 additions & 4 deletions tests/unit/command/test_run.py
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ def test_run(mocker, dvc):
"--wdir",
"wdir",
"--no-exec",
"--overwrite",
"--force",
"--no-run-cache",
"--no-commit",
"--outs-persist",
Expand Down Expand Up @@ -64,7 +64,7 @@ def test_run(mocker, dvc):
fname="file",
wdir="wdir",
no_exec=True,
overwrite=True,
force=True,
run_cache=False,
no_commit=True,
always_changed=True,
Expand Down Expand Up @@ -94,7 +94,7 @@ def test_run_args_from_cli(mocker, dvc):
fname=None,
wdir=None,
no_exec=False,
overwrite=False,
force=False,
run_cache=True,
no_commit=False,
always_changed=False,
Expand Down Expand Up @@ -124,7 +124,7 @@ def test_run_args_with_spaces(mocker, dvc):
fname=None,
wdir=None,
no_exec=False,
overwrite=False,
force=False,
run_cache=True,
no_commit=False,
always_changed=False,
Expand Down

0 comments on commit 21e65dd

Please sign in to comment.