move: support moving output from import/import-url stages (#10889)

Author: skshetry

dvc/output.py

@@ -997,8 +997,9 @@ def move(self, out):
         self.save()
         self.commit()
 
-        if self.protocol == "local" and self.use_scm_ignore:
-            self.repo.scm_context.ignore(self.fspath)
+        # should already be ignored in self.save()
+        # if self.protocol == "local" and self.use_scm_ignore:
+        #     self.repo.scm_context.ignore(self.fspath)
 
     def transfer(
         self, source, odb=None, jobs=None, update=False, no_progress_bar=False

dvc/repo/move.py

@@ -32,9 +32,10 @@ def move(self, from_path, to_path):
     It only works with outputs generated by `add` or `import`,
     also known as data sources.
     """
-    from dvc import output
+    from dvc import dependency, output
     from dvc.dvcfile import DVC_FILE_SUFFIX
     from dvc.stage import Stage
+    from dvc_objects.fs.local import LocalFileSystem
 
     from_out = output.loads_from(Stage(self), [from_path])[0]
     assert from_out.protocol == "local"
@@ -45,6 +46,7 @@ def move(self, from_path, to_path):
     assert len(outs) == 1
     out = outs[0]
     stage = out.stage
+    deps = stage.deps
 
     if not stage.is_data_source:
         raise MoveNotDataSourceError(stage.addressing)
@@ -64,15 +66,28 @@ def move(self, from_path, to_path):
             wdir=new_wdir,
             outs=[to_path],
             meta=stage.meta,
+            frozen=stage.frozen,
+            always_changed=stage.always_changed,
+            desc=stage.desc,
         )
 
         os.unlink(stage.path)
         stage = new_stage
     else:
         to_path = os.path.relpath(to_path, stage.wdir)
 
-    to_out = output.loads_from(stage, [to_path], out.use_cache, out.metric)[0]
+    def with_dep_path_adjusted(dep: dependency.Dependency):
+        d = dep.dumpd()
+        if isinstance(dep.fs, LocalFileSystem) and not os.path.isabs(dep.def_path):
+            path = os.path.relpath(os.path.abspath(dep.def_path), stage.wdir)
+            return d | {"path": path}
+        return d
 
-    out.move(to_out)
-    stage.save()
+    stage.outs = output.loadd_from(stage, [out.dumpd() | {"path": to_path}])
+    stage.deps = dependency.loadd_from(
+        stage, [with_dep_path_adjusted(dep) for dep in deps]
+    )
+
+    out.move(stage.outs[0])
+    stage.md5 = stage.compute_md5()
     stage.dump()

tests/func/test_move.py

@@ -8,12 +8,14 @@
 from dvc.exceptions import MoveNotDataSourceError, OutputNotFoundError
 
 
-def test_move(tmp_dir, dvc):
-    tmp_dir.dvc_gen("foo", "foo")
-    dvc.move("foo", "foo1")
+def test_move(tmp_dir, dvc, scm):
+    tmp_dir.dvc_gen("foo", "bar")
+    dvc.move("foo", "bar")
 
     assert not (tmp_dir / "foo").is_file()
-    assert (tmp_dir / "foo1").is_file()
+    assert (tmp_dir / "bar").is_file()
+    # should only have the new path in the .gitignore, and only once
+    assert (tmp_dir / ".gitignore").read_text().splitlines() == ["/bar"]
 
 
 def test_move_non_existent_file(dvc):
@@ -195,3 +197,167 @@ def test_move_meta(tmp_dir, dvc):
           custom_key: 42
     """
     )
+
+
+def test_import(tmp_dir, dvc, scm):
+    tmp_dir.dvc_gen("foo", "foo", commit="add foo")
+    imp_stage = dvc.imp(os.curdir, "foo", "foo_imported")
+
+    dvc.move("foo_imported", "foo_moved")
+
+    (stage,) = dvc.stage.collect("foo_moved.dvc")
+    assert imp_stage.md5 != stage.md5
+    res = (tmp_dir / "foo_moved.dvc").read_text()
+    assert res == textwrap.dedent(
+        f"""\
+        md5: {stage.md5}
+        frozen: true
+        deps:
+        - path: foo
+          repo:
+            url: {os.curdir}
+            rev_lock: {scm.get_rev()}
+        outs:
+        - md5: acbd18db4cc2f85cedef654fccc4a4d8
+          size: 3
+          hash: md5
+          path: foo_moved
+    """
+    )
+
+
+@pytest.mark.parametrize(
+    "path_func",
+    [pytest.param(os.path.abspath, id="abs"), pytest.param(os.path.relpath, id="rel")],
+)
+def test_import_url_in_repo(tmp_dir, dvc, path_func):
+    tmp_dir.gen("foo", "foo")
+    imp_stage = dvc.imp_url(path_func(tmp_dir / "foo"), "foo_imported")
+    (tmp_dir / "data").mkdir()
+
+    dvc.move("foo_imported", os.path.join("data", "foo_moved"))
+
+    (stage,) = dvc.stage.collect(os.path.join("data", "foo_moved.dvc"))
+    assert imp_stage.md5 != stage.md5
+    res = (tmp_dir / "data" / "foo_moved.dvc").read_text()
+    assert res == textwrap.dedent(
+        f"""\
+        md5: {stage.md5}
+        frozen: true
+        deps:
+        - md5: acbd18db4cc2f85cedef654fccc4a4d8
+          size: 3
+          hash: md5
+          path: ../foo
+        outs:
+        - md5: acbd18db4cc2f85cedef654fccc4a4d8
+          size: 3
+          hash: md5
+          path: foo_moved
+    """
+    )
+
+
+@pytest.mark.parametrize(
+    "path_func",
+    [pytest.param(os.path.abspath, id="abs"), pytest.param(os.path.relpath, id="rel")],
+)
+def test_import_url_out_of_repo(tmp_dir, dvc, scm, path_func, make_tmp_dir):
+    external = make_tmp_dir("external")
+    external.gen("foo", "foo")
+
+    imp_stage = dvc.imp_url(path_func(external / "foo"), "foo_imported")
+
+    data_dir = tmp_dir / "data"
+    data_dir.mkdir()
+
+    new_path = data_dir / "foo_moved"
+    new_dvcfile = new_path.with_suffix(".dvc")
+    dvc.move("foo_imported", os.fspath(new_path))
+
+    (stage,) = dvc.stage.collect(os.fspath(new_dvcfile))
+    assert imp_stage.md5 != stage.md5
+
+    with data_dir.chdir():
+        expected_path = path_func(external / "foo")
+
+    assert new_dvcfile.parse() == {
+        "md5": stage.md5,
+        "frozen": True,
+        "deps": [
+            {
+                "md5": "acbd18db4cc2f85cedef654fccc4a4d8",
+                "size": 3,
+                "hash": "md5",
+                "path": expected_path,
+            }
+        ],
+        "outs": [
+            {
+                "md5": "acbd18db4cc2f85cedef654fccc4a4d8",
+                "size": 3,
+                "hash": "md5",
+                "path": "foo_moved",
+            }
+        ],
+    }
+
+
+@pytest.mark.parametrize(
+    "path_func",
+    [pytest.param(os.path.abspath, id="abs"), pytest.param(os.path.relpath, id="rel")],
+)
+def test_all_metadata_are_preserved(tmp_dir, dvc, make_tmp_dir, path_func):
+    external = make_tmp_dir("external")
+    external.gen("foo", "foo")
+
+    contents = {
+        "md5": "bad",  # placeholder, does not matter for the test
+        "frozen": True,
+        "desc": "this is a stage description",
+        "always_changed": True,
+        "meta": {"custom_key": 42},
+        "deps": [
+            {
+                "md5": "acbd18db4cc2f85cedef654fccc4a4d8",
+                "size": 3,
+                "hash": "md5",
+                "path": path_func(external / "foo"),
+            }
+        ],
+        "outs": [
+            {
+                "md5": "acbd18db4cc2f85cedef654fccc4a4d8",
+                "path": "foo_imported",
+                "persist": True,
+                "hash": "md5",
+                "size": 3,
+                "desc": "this is a description",
+                "type": "model",
+                "labels": ["label1", "label2"],
+                "meta": {"custom_key": 42},
+                "cache": False,
+                "remote": "myremote",
+                "push": False,
+            }
+        ],
+    }
+    (tmp_dir / "foo_imported.dvc").dump(contents)
+    (tmp_dir / "foo_imported").write_text("foo")
+
+    data_dir = tmp_dir / "data"
+    data_dir.mkdir()
+
+    new_path = data_dir / "foo_moved"
+    new_dvcfile = new_path.with_suffix(".dvc")
+    dvc.move("foo_imported", os.fspath(new_path))
+
+    (stage,) = dvc.stage.collect(os.fspath(new_dvcfile))
+
+    with data_dir.chdir():
+        expected_path = path_func(external / "foo")
+
+    contents["outs"][0] |= {"path": "foo_moved"}
+    contents["deps"][0] |= {"path": expected_path}
+    contents |= {"md5": stage.md5}
+    assert new_dvcfile.parse() == contents