import: allow downloading regular files/dirs tracked by git #2889

Baranowski · 2019-12-04T15:03:27Z

❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Baranowski · 2019-12-04T15:09:17Z

This is rebased on top of #2837 by danihodovic (for some reason I couldn't find his repo on the list of available forks when submitting the PR).

Baranowski · 2019-12-04T15:10:23Z

dvc/dependency/repo.py

+            to.checkout()
+        except NoOutputInExternalRepoError as e:
+            try:
+                with self._make_repo(


Not sure what's an idiomatic way to avoid this duplication from fetch(), so I left it like this and I'm open to suggestions.

efiop

Hi @Baranowski ! Thanks for the PR 🙏 We need to also test this case #2837 (comment) . Here is an example:

#!/bin/bash

set -x
set -e

rm -rf mytest
mkdir mytest
cd mytest

mkdir erepo
pushd erepo
git init
dvc init
dvc run -O foo 'echo foo > foo'  # note that foo is not cached by dvc, hence the "-O"
git add .
git commit -m "init"
popd

mkdir repo
pushd repo
git init
dvc init
dvc import ../erepo foo
popd

Baranowski · 2019-12-04T18:45:23Z

Related dvc.org issue: iterative/dvc.org#835

Baranowski · 2019-12-04T18:47:17Z

@efiop, sure I will see if danihodovic writes the test soonish so that I can copy it for import. If not, I will write it myself.

efiop · 2019-12-04T20:56:37Z

@Baranowski The tests would be different, so you could work async, I guess :) Looks like the only piece that you are reusing here is _copy_git_file, which is not that crucial.

tests/func/test_import.py

Baranowski · 2019-12-05T13:10:43Z

@efiop, added. The test passed without code modifications, somewhat to my surprise.

efiop · 2019-12-05T14:21:52Z

@Baranowski Btw, please note that get PR was revisited so you might want to adjust your PR accordingly. Sorry for this clash of PRs 🙁

Baranowski · 2019-12-05T19:28:28Z

Thanks for pointing that out, @efiop. Rebased and updated.

efiop · 2019-12-10T17:22:26Z

@Baranowski Fixed get implementation, please rebase. Sorry for the delay 🙁

Baranowski · 2019-12-11T12:26:38Z

@efiop, this is rebased and ready for a review

efiop · 2019-12-11T17:30:55Z

@Baranowski Please check the tests, they are failing on py2.

dvc/dependency/repo.py

efiop · 2019-12-12T04:09:28Z

dvc/dependency/repo.py

-        to.checkout()
+        try:
+            if self._copy_if_git_file(to.fspath):
+                return


Just a note: Not setting to.info as we do down below is fine, as git files are tiny and the hash will be computed later in the output itself.

tests/func/test_import.py

Fixes iterative#2862

This reverts commit 4d59bcd.

efiop · 2019-12-14T18:38:43Z

dvc/dependency/repo.py

+    def _git_status(self):
+        cache_dir = self.repo.cache.local.cache_dir
+        with self._make_repo(
+            cache_dir=os.path.join(cache_dir, "old")


No need to pass cache dir, as you are not pulling anything anyway

Suggested change

cache_dir=os.path.join(cache_dir, "old")

efiop · 2019-12-14T18:38:58Z

dvc/dependency/repo.py

+            cache_dir=os.path.join(cache_dir, "old")
+        ) as old_repo:
+            with self._make_repo(
+                cache_dir=os.path.join(cache_dir, "new"), rev_lock=None


Same here

Suggested change

cache_dir=os.path.join(cache_dir, "new"), rev_lock=None

rev_lock=None

Without cache_dir specified explicitly, both repos seem to be pulled into the same directory, and so the update to the imported file is never detected.

@Baranowski But cache_dir has nothing to do with the update, right? So I suppose the reason might be a bug in the caching system in dvc/external_repo.py?

You are right, @efiop, I think I can see the bug in external_repo.py. Thanks!

@Baranowski please see #2889 (comment) . Let's create a separate ticket.

Suor · 2019-12-15T03:29:22Z

tests/func/test_import.py

+    assert os.path.isfile(str(tmp_dir / dst))
+    assert filecmp.cmp(str(erepo_dir / src), str(tmp_dir / dst), shallow=False)
+    assert tmp_dir.scm.repo.git.check_ignore(str(tmp_dir / dst))


Should only use str() on Path-like objects for presentation purposes, to get str file path one should use fspath(). Or fspath_py35() when passing to an util working with Path-likes in Python 3.6+.

Also, Path has .is_file() and .is_dir(). May also skip .exists() since it is covered with is_file() anyway.

This reverts commit 8985759.

This reverts commit 48bf449.

efiop · 2019-12-17T12:10:25Z

Thanks @Baranowski ! 🙏

This update reflects iterative/dvc#2889

Baranowski · 2019-12-17T18:23:28Z

Other than the dvc status issue, there are also two things worth pointing out:

dvc update for Git-tracked import-ed files fails with the same error msg as dvc status,
You cannot dvc import a file from a Git repo that's not a DVC repo. There is still must be a DVC repo in the remote Git repo. It's sufficient if the DVC repo is empty.

Apologies for raising this only after merging the PR.

shcheklein · 2019-12-17T23:26:35Z

@Baranowski @efiop create issue just to keep track of those?

Baranowski · 2019-12-18T15:50:39Z

Created #2976 and #2977

This update reflects iterative/dvc#2889

Baranowski commented Dec 4, 2019

View reviewed changes

Baranowski mentioned this pull request Dec 4, 2019

import: allow downloading regular files/dirs tracked by Git? #2862

Closed

efiop suggested changes Dec 4, 2019

View reviewed changes

Baranowski mentioned this pull request Dec 4, 2019

import: allows to download/copy also files that are not output iterative/dvc.org#835

Closed

Baranowski commented Dec 5, 2019

View reviewed changes

tests/func/test_import.py Outdated Show resolved Hide resolved

Baranowski force-pushed the import_regulars.squashed branch from 49707d4 to 0738afd Compare December 5, 2019 18:26

jorgeorpinel changed the title ~~WiP import: allow downloading regular files/dirs tracked by git~~ [WIP] import: allow downloading regular files/dirs tracked by git Dec 5, 2019

Baranowski force-pushed the import_regulars.squashed branch from 0738afd to 7fb0a68 Compare December 5, 2019 19:26

weekly-digest bot mentioned this pull request Dec 8, 2019

Weekly Digest (1 December, 2019 - 8 December, 2019) #2919

Closed

Baranowski force-pushed the import_regulars.squashed branch from 7fb0a68 to e137887 Compare December 11, 2019 12:22

Baranowski changed the title ~~[WIP] import: allow downloading regular files/dirs tracked by git~~ import: allow downloading regular files/dirs tracked by git Dec 11, 2019

Baranowski marked this pull request as ready for review December 11, 2019 12:25

efiop reviewed Dec 12, 2019

View reviewed changes

dvc/dependency/repo.py Show resolved Hide resolved

efiop reviewed Dec 12, 2019

View reviewed changes

Suor suggested changes Dec 12, 2019

View reviewed changes

tests/func/test_import.py Outdated Show resolved Hide resolved

Baranowski added 5 commits December 14, 2019 14:08

import: allow downloading regular files/dirs tracked by git

c04f5a0

Fixes iterative#2862

New tests use tmp_dir

0ee6998

dependency/repo: fix py2 support

a36ac0c

os.path.isfile(str(...))

5ceaf5a

os.path.isdir(str(...))

c8c2855

Baranowski added 3 commits December 14, 2019 14:08

Wrap tmp_dir in str()

11d890a

DependencyREPO.status: handle git-tracked files

48bf449

_git_status(): use different cache dirs

8985759

Baranowski force-pushed the import_regulars.squashed branch from 9aa4c06 to 8985759 Compare December 14, 2019 12:43

Baranowski added 2 commits December 14, 2019 19:38

test_status_imported_git_file

4d59bcd

Revert "test_status_imported_git_file"

5915602

This reverts commit 4d59bcd.

efiop reviewed Dec 14, 2019

View reviewed changes

Suor suggested changes Dec 15, 2019

View reviewed changes

Baranowski mentioned this pull request Dec 15, 2019

status: implement support for import-ed files #2959

Closed

Baranowski added 3 commits December 15, 2019 18:14

Revert "_git_status(): use different cache dirs"

7461f14

This reverts commit 8985759.

Revert "DependencyREPO.status: handle git-tracked files"

c02c016

This reverts commit 48bf449.

Use fspath() in tests

ad98f0f

efiop approved these changes Dec 17, 2019

View reviewed changes

efiop requested review from pared and a user December 17, 2019 01:48

Suor approved these changes Dec 17, 2019

View reviewed changes

efiop merged commit 032d094 into iterative:master Dec 17, 2019

Baranowski added a commit to Baranowski/dvc.org that referenced this pull request Dec 17, 2019

import: supports also non-output files/dirs

4bff952

This update reflects iterative/dvc#2889

Baranowski mentioned this pull request Dec 17, 2019

import: supports also non-output files/dirs iterative/dvc.org#873

Merged

This was referenced Dec 18, 2019

update: Handle Git-tracked import-ed files #2976

Closed

import: Handle non-DVC Git repositories #2977

Closed

Baranowski added a commit to Baranowski/dvc.org that referenced this pull request Dec 18, 2019

import: supports also non-output files/dirs

41cb722

This update reflects iterative/dvc#2889

weekly-digest bot mentioned this pull request Dec 22, 2019

Weekly Digest (15 December, 2019 - 22 December, 2019) #2996

Closed

jorgeorpinel mentioned this pull request Jan 6, 2020

get/import: help output should reflect non-DVC Git repo support #3077

Closed

	cache_dir=os.path.join(cache_dir, "new"), rev_lock=None
	rev_lock=None

import: allow downloading regular files/dirs tracked by git #2889

import: allow downloading regular files/dirs tracked by git #2889

Uh oh!

Conversation

Baranowski commented Dec 4, 2019 • edited by efiop Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Baranowski commented Dec 4, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop left a comment

Choose a reason for hiding this comment

Uh oh!

Baranowski commented Dec 4, 2019

Uh oh!

Baranowski commented Dec 4, 2019

Uh oh!

efiop commented Dec 4, 2019

Uh oh!

Uh oh!

Baranowski commented Dec 5, 2019

Uh oh!

efiop commented Dec 5, 2019

Uh oh!

Baranowski commented Dec 5, 2019

Uh oh!

efiop commented Dec 10, 2019

Uh oh!

Baranowski commented Dec 11, 2019

Uh oh!

efiop commented Dec 11, 2019

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Baranowski Dec 14, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop commented Dec 17, 2019

Uh oh!

Baranowski commented Dec 17, 2019

Uh oh!

shcheklein commented Dec 17, 2019

Uh oh!

Baranowski commented Dec 18, 2019

Uh oh!

Uh oh!

Baranowski commented Dec 4, 2019 •

edited by efiop

Loading

efiop Dec 14, 2019 •

edited

Loading

Baranowski Dec 14, 2019 •

edited

Loading