From 21e65ddf8a22ca881d989397f03e921ae50baf5e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Saugat=20Pachhai=20=28=E0=A4=B8=E0=A5=8C=E0=A4=97=E0=A4=BE?= =?UTF-8?q?=E0=A4=A4=29?= Date: Wed, 10 Jun 2020 19:22:24 +0545 Subject: [PATCH] run: use -f/--force to overwrite, fix msg 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 --- dvc/command/run.py | 5 +++-- dvc/dvcfile.py | 18 ------------------ dvc/repo/add.py | 2 +- dvc/repo/imp_url.py | 2 +- dvc/repo/run.py | 29 +++++++++++++++++++++-------- dvc/stage/exceptions.py | 11 ++--------- tests/func/test_run_multistage.py | 4 ++-- tests/func/test_run_single_stage.py | 28 ++++++++++++++-------------- tests/unit/command/test_run.py | 8 ++++---- 9 files changed, 48 insertions(+), 59 deletions(-) diff --git a/dvc/command/run.py b/dvc/command/run.py index 6c2be979ec..1a3ac6d6d0 100644 --- a/dvc/command/run.py +++ b/dvc/command/run.py @@ -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, @@ -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", diff --git a/dvc/dvcfile.py b/dvc/dvcfile.py index df8a61b7c9..eb06817dc7 100644 --- a/dvc/dvcfile.py +++ b/dvc/dvcfile.py @@ -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, @@ -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) @@ -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() diff --git a/dvc/repo/add.py b/dvc/repo/add.py index 26922aa961..c1fee55d22 100644 --- a/dvc/repo/add.py +++ b/dvc/repo/add.py @@ -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() diff --git a/dvc/repo/imp_url.py b/dvc/repo/imp_url.py index 7d511fa1ad..b6aabfefcd 100644 --- a/dvc/repo/imp_url.py +++ b/dvc/repo/imp_url.py @@ -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]) diff --git a/dvc/repo/run.py b/dvc/repo/run.py index 36c52726ca..a5bb81f466 100644 --- a/dvc/repo/run.py +++ b/dvc/repo/run.py @@ -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, @@ -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"): @@ -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]) diff --git a/dvc/stage/exceptions.py b/dvc/stage/exceptions.py index eda6db716f..84d5a86564 100644 --- a/dvc/stage/exceptions.py +++ b/dvc/stage/exceptions.py @@ -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): @@ -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): diff --git a/tests/func/test_run_multistage.py b/tests/func/test_run_multistage.py index 50377423d0..cafb65f861 100644 --- a/tests/func/test_run_multistage.py +++ b/tests/func/test_run_multistage.py @@ -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 = { diff --git a/tests/func/test_run_single_stage.py b/tests/func/test_run_single_stage.py index 04c30bf6e1..48df721477 100644 --- a/tests/func/test_run_single_stage.py +++ b/tests/func/test_run_single_stage.py @@ -350,7 +350,7 @@ def test(self): ret = main( [ "run", - "--overwrite", + "--force", "--no-run-cache", "--single-stage", "-d", @@ -408,7 +408,7 @@ def test(self): ret = main( [ "run", - "--overwrite", + "--force", "--no-run-cache", "--single-stage", "-d", @@ -467,7 +467,7 @@ def test(self): ret = main( [ "run", - "--overwrite", + "--force", "--no-run-cache", "--single-stage", "-d", @@ -552,7 +552,7 @@ def test(self): self.FOO, "-d", self.CODE, - "--overwrite", + "--force", "--no-run-cache", "--single-stage", "-o", @@ -576,7 +576,7 @@ def test(self): ret = main( [ "run", - "--overwrite", + "--force", "--single-stage", "--file", "out.dvc", @@ -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): @@ -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): @@ -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): @@ -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): @@ -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}", ] @@ -946,7 +946,7 @@ def test_ignore_run_cache(self): cmd = [ "run", - "--overwrite", + "--force", "--single-stage", "--deps", "immutable", diff --git a/tests/unit/command/test_run.py b/tests/unit/command/test_run.py index 26510cdd0f..ba718e17b6 100644 --- a/tests/unit/command/test_run.py +++ b/tests/unit/command/test_run.py @@ -27,7 +27,7 @@ def test_run(mocker, dvc): "--wdir", "wdir", "--no-exec", - "--overwrite", + "--force", "--no-run-cache", "--no-commit", "--outs-persist", @@ -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, @@ -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, @@ -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,