external repo: checkout revision before initializing dvc repo #2852

pared · 2019-11-26T11:54:30Z

❗ 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. 🙏

pared · 2019-11-26T11:59:54Z

If this change gets accepted we should consider replacing repo_dir with empty_dir in some tests. It seems to me that repo_dir has been used as empty dir initializer at least in some cases. Using empty_dir will make potential test debuggin easier.

pared · 2019-11-26T12:02:18Z

tests/func/test_get.py

+    rconfig = RemoteConfig(dvc_repo.config)
+    rconfig.add("upstream", dvc_repo.cache.local.cache_dir, default=True)
+    dvc_repo.scm.add([dvc_repo.config.config_file])


Can be removed after #2780

We only use it once. Why not just inline it into the test?

Suor · 2019-11-26T13:07:34Z

tests/func/test_get.py

+def test_get_from_non_dvc_master(empty_dir, erepo_no_dvc_master):
+    Repo.get(
+        erepo_no_dvc_master._root_dir,
+        "foo",
+        out="foo",
+        rev=erepo_no_dvc_master.dvc_branch,
+    )


We don't check that it works.

Suor · 2019-11-26T13:20:56Z

tests/func/test_get.py

+    rconfig = RemoteConfig(dvc_repo.config)
+    rconfig.add("upstream", dvc_repo.cache.local.cache_dir, default=True)
+    dvc_repo.scm.add([dvc_repo.config.config_file])


We only use it once. Why not just inline it into the test?

Suor · 2019-11-28T21:04:13Z

dvc/external_repo.py

+    # Checkout first in case of non dvc master
+    checkout_revision(new_path, rev)
+
    repo = Repo(new_path)


Please add a comment why we are doing it.

@Suor I guess "# Adjust new clone/copy to fit rev and cache_dir" is for this, though it looks weird detached like that now. 🙂

@pared i suppose you didn't push yet?

Logically it should be:

if rev is not None: _git_checkout(new_path, rev)

To stay in line with the rest )

Suor · 2019-11-28T21:05:52Z

dvc/external_repo.py

        if cache_dir is not None:
            cache_config = CacheConfig(repo.config)
            cache_config.set_dir(cache_dir, level=Config.LEVEL_LOCAL)


Maybe we can skip creating Repo and use Config directly here?

I agree that it can be done with just config, though it would require a change in test_get_from_non_dvc_repo. I'll create an issue for that.

It should not require changing any tests. This should be a self-contained change.

ghost · 2019-11-29T14:54:50Z

tests/basic_env.py

@@ -21,7 +21,55 @@
 logger = logging.getLogger("dvc")


-class TestDirFixture(object):
+class EmptyDirFixture(object):


@pared, not sure if it is the same, but what about?

@pytest.fixture def chdir_tmp(tmp_path): old_directory = os.cwd() new_directory = fspath_py35(tmp_path) os.chdir(new_directory) yield os.chdir(old_directory)

Also, why refactoring something that we will deprecate eventually) ?

ghost · 2019-11-29T14:56:43Z

tests/conftest.py

+
+
+@pytest.fixture
+def empty_dir():


see this comment: https://github.com/iterative/dvc/pull/2852/files#r352164905

This won't be needed when new dir_helper fixtures land. So maybe save effort on this refactoring. If you simply need an empty dir you may use tmp_path pytest standard fixture and monkeypatch to chdir to it:

@pytest.fixture def empty_dir(tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) return tmp_path

Suor · 2019-11-30T11:42:50Z

tests/conftest.py

+
+
+@pytest.fixture
+def empty_dir():


This won't be needed when new dir_helper fixtures land. So maybe save effort on this refactoring. If you simply need an empty dir you may use tmp_path pytest standard fixture and monkeypatch to chdir to it:

@pytest.fixture def empty_dir(tmp_path, monkeypatch): monkeypatch.chdir(tmp_path) return tmp_path

Suor · 2019-11-30T11:49:32Z

tests/func/test_get.py

@@ -87,3 +89,38 @@ def test_get_to_dir(dname, erepo):

    assert os.path.isdir(dname)
    assert filecmp.cmp(erepo.FOO, dst, shallow=False)
+
+
+def test_get_from_non_dvc_master(empty_dir, git_erepo, caplog):


How about using erepo just removing .dvc dir at the start of the test?

~~I can do that, but test won't be much more readable due to fact that the default remote set inside erepo creation is its own cache and I need to provide a new one during this particular test.~~
EDIT: look at next comment

After giving it some thought, that is not possible. Such a case would defeat the test purpose. External repo is cloning the target, so it doesn't matter that I remove .dvc from erepo, it will still be dvc repo after clone.

If you remove .dvc and commit that then that won't be a dvc repo :) Now you make a lot of manipulations.

dvc/external_repo.py

efiop · 2019-12-02T17:01:01Z

@pared check the tests, lots of them are failing.

Suor · 2019-12-02T20:24:31Z

dvc/external_repo.py

+    git = Git(repo_path)
+    git.checkout(revision)


We should probably close is explicitly or we might fail removing it later on Windows.

~~What do you mean by closing it, @Suor ?~~

Never mind 😅

https://github.com/gitpython-developers/GitPython/blob/85cf7e89682d061ea86514c112dfb684af664d45/git/repo/base.py#L226-L239

@pared You still need to close it git.checkout fails. Add try&finally

Suor · 2019-12-02T20:27:21Z

tests/func/test_get.py

@@ -87,3 +89,38 @@ def test_get_to_dir(dname, erepo):

    assert os.path.isdir(dname)
    assert filecmp.cmp(erepo.FOO, dst, shallow=False)
+
+
+def test_get_from_non_dvc_master(empty_dir, git_erepo, caplog):


If you remove .dvc and commit that then that won't be a dvc repo :) Now you make a lot of manipulations.

Suor · 2019-12-02T20:29:16Z

tests/func/test_get.py

+    dvc_branch = "dvc_test"
+    git_erepo.git.git.checkout("master", b=dvc_branch)
+
+    dvc_repo = Repo.init(git_erepo._root_dir)


So you make a dvc repo in master branch, looks like this breaks the purpose of the test.

AFAIK line 98 is equivalent to:
git checkout -b dvc_test
so we are actually on "dvc_test" :)

efiop · 2019-12-03T13:22:16Z

@pared Tests are failing, please take a look 🙂

efiop · 2019-12-03T22:40:02Z

tests/conftest.py

        repo.dvc.scm.add([repo.dvc.config.config_file])
        repo.dvc.scm.commit("add remote")

        repo.create("version", "master")
        repo.dvc.add("version")
        repo.dvc.scm.add([".gitignore", "version.dvc"])
        repo.dvc.scm.commit("master")
+        repo.dvc.push()


Why is this needed? And all upstream_path stuff in general? Sorry, might've missed some discussion somewhere.

It seems to me that erepo had default remote configured pointing to its own cache, in order to not trigger infamous NoRemoteError on any test related to get and import. In new test test_from_non_dvc_master, where I am removing .dvc I had to either manually specify new storage for erepo, or adjust erepo fixture. I choose the latter because I believe that setting cache as default remote is not realistic use case, which also omits some important details (eg you don't need to push to "add" something to remote cache, because its local cache and adding is done automatically) so we were testing some very specific use cases everywhere where we used erepo.

@pared But you are not even using "upstream_path" in your tests. And you have to modify a bunch of unrelated code for no good reason. In your test you simply need .dvc to not be tracked on master, which you could do by simply adding .dvc to gitignore on master and commiting (and doing that in your test without affecting other tests).

The point about it being not realistic is a good one, but we will soon do that automatically by-default #2599 anyway, and, well, it is not nice to break other tests because of 1 test for specific use case.

Ok, sorry I didn't noticed that I am modifying repo, its leftover from some previous changes.
Still, I believe this change is desirable. I can achieve the desired master state with properly editing .gitignore but the actual problem is that erepo is not an actual use case. For example, test_get_repo_file should fail in original erepo setup. We were using a trick to make it pass, but without the push that we are commenting on, it should not pass.

#2599 should fix our discussion, but since it's not in the current sprint, I wouldn't just leave it be, especially that this change is not that big.

@pared Sorry, I don't get the point about test_get_repo_file, don't see any tricks in it.

def test_get_repo_file(erepo): src = erepo.FOO dst = erepo.FOO + "_imported" Repo.get(erepo.root_dir, src, dst) assert os.path.exists(dst) assert os.path.isfile(dst) assert filecmp.cmp(erepo.FOO, dst, shallow=False)

My point is that you don't need to modify erepo and stuff around for your 1 specific test, you can do it with 2 lines that will add .dvc dir to gitignore and commit it on master.

There is no trick in the test, the trick is inside erepo:

rconfig = RemoteConfig(repo.dvc.config) rconfig.add("upstream", repo.dvc.cache.local.cache_dir, default=True)

It's not about modifying whole erepo for one test. I believe erepo is not being set up properly for tests.

@pared Ah, got it. But that trick is something that we will implement in #2599 by default, and it doesn't seem to break any other tests, it is just an internal implementation detail for erepo. But I also understand your point about it being hacky. Maybe let's create an issue for it and for now stick with gitignore, so that we don't have to touch all other tests? It doesn't seem like it is in the scope of this PR.

efiop

#2852 (comment)

efiop · 2019-12-04T11:48:31Z

tests/func/test_get.py

+    with caplog.at_level(logging.INFO, logger="dvc"):
+        Repo.get(erepo._root_dir, erepo.FOO, out=imported_file, rev="branch")
+
+    assert caplog.text == ""


Hm, why do we test that there is no info output to logger?

#2858
I think that we should use caplog more often in order to prevent spawning of UI issues that are quite common recently. Do you think I should also check capsys?

You'll probably get progress bars in capsys, not easy to test, especially automatically, but your call, I don't have a strong opinion here 🙂

pbars go to stderr by default, so shouldn't be an issue.

I wonder why Repo.get() has not output though.

tests/func/test_get.py

efiop

Thanks! 🙏

pared commented Nov 26, 2019

View reviewed changes

pared changed the title ~~external repo: checkout revision before initializing dvc repo~~ [WIP] external repo: checkout revision before initializing dvc repo Nov 26, 2019

pared force-pushed the 2848_get_no_dvc_master branch 2 times, most recently from 9d2d745 to 8d313c6 Compare November 26, 2019 12:56

pared changed the title ~~[WIP] external repo: checkout revision before initializing dvc repo~~ external repo: checkout revision before initializing dvc repo Nov 26, 2019

pared requested review from Suor, a user and efiop November 26, 2019 13:00

Suor reviewed Nov 26, 2019

View reviewed changes

pared requested a review from Suor November 27, 2019 09:02

pared force-pushed the 2848_get_no_dvc_master branch 2 times, most recently from eaf5b1b to ec8d46f Compare November 29, 2019 14:19

Suor suggested changes Nov 29, 2019

View reviewed changes

ghost reviewed Nov 29, 2019

View reviewed changes

Suor suggested changes Nov 30, 2019

View reviewed changes

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

Weekly Digest (24 November, 2019 - 1 December, 2019) #2876

Closed

pared force-pushed the 2848_get_no_dvc_master branch from aec6eea to 42a9f34 Compare December 2, 2019 10:40

pared requested review from a user and Suor December 2, 2019 10:45

efiop reviewed Dec 2, 2019

View reviewed changes

dvc/external_repo.py Outdated Show resolved Hide resolved

pared force-pushed the 2848_get_no_dvc_master branch from 42a9f34 to 65bbe3c Compare December 2, 2019 16:26

pared force-pushed the 2848_get_no_dvc_master branch from 65bbe3c to 673ae54 Compare December 2, 2019 17:31

Suor suggested changes Dec 2, 2019

View reviewed changes

pared force-pushed the 2848_get_no_dvc_master branch from fc5d389 to 88351ab Compare December 3, 2019 10:05

pared requested a review from Suor December 3, 2019 10:18

pared force-pushed the 2848_get_no_dvc_master branch from 4445a59 to f0069b6 Compare December 3, 2019 18:56

efiop reviewed Dec 3, 2019

View reviewed changes

efiop suggested changes Dec 3, 2019

View reviewed changes

pared force-pushed the 2848_get_no_dvc_master branch 2 times, most recently from 1756ee0 to cca3f10 Compare December 4, 2019 11:29

efiop reviewed Dec 4, 2019

View reviewed changes

pared added 7 commits December 4, 2019 13:10

test env: extract empty dir fixture to class

06d6c2c

conftest: create empty dir fixture

b788a99

external repo: checkout rev before initializing repo

b9d5933

rename TestDirFixtureBase

5c0c423

test: get: inline erepo with non-dvc master

5e5c826

external repo: move revision check out of checkout function

97e4b02

conftest: empty dir is tmp_path now

f1d59dd

pared force-pushed the 2848_get_no_dvc_master branch 2 times, most recently from 8180515 to a78e836 Compare December 4, 2019 12:45

pared requested a review from efiop December 4, 2019 12:58

pared mentioned this pull request Dec 4, 2019

tests: erepo upstream should not be its cache #2888

Closed

efiop reviewed Dec 4, 2019

View reviewed changes

tests/func/test_get.py Show resolved Hide resolved

efiop reviewed Dec 4, 2019

View reviewed changes

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

pared force-pushed the 2848_get_no_dvc_master branch from a78e836 to d9ca23f Compare December 4, 2019 13:33

get: test: from non-dvc master: refactor

ca85217

pared force-pushed the 2848_get_no_dvc_master branch from d9ca23f to ca85217 Compare December 4, 2019 13:34

pared requested a review from efiop December 4, 2019 13:41

efiop approved these changes Dec 4, 2019

View reviewed changes

efiop merged commit 516e3b1 into iterative:master Dec 4, 2019

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

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

Closed

pared deleted the 2848_get_no_dvc_master branch December 17, 2019 13:12

external repo: checkout revision before initializing dvc repo #2852

external repo: checkout revision before initializing dvc repo #2852

Conversation

pared commented Nov 26, 2019 • edited Loading

pared commented Nov 26, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop Nov 29, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Suor Nov 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pared Dec 2, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop commented Dec 2, 2019

Choose a reason for hiding this comment

ghost Dec 3, 2019 • edited by ghost Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pared Dec 3, 2019 • edited Loading

Choose a reason for hiding this comment

efiop commented Dec 3, 2019

Choose a reason for hiding this comment

pared Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

efiop Dec 4, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop left a comment

Choose a reason for hiding this comment

pared commented Nov 26, 2019 •

edited

Loading

pared commented Nov 26, 2019 •

edited

Loading

efiop Nov 29, 2019 •

edited

Loading

Suor Nov 30, 2019 •

edited

Loading

pared Dec 2, 2019 •

edited

Loading

ghost Dec 3, 2019 •

edited by ghost

Loading

pared Dec 3, 2019 •

edited

Loading

pared Dec 4, 2019 •

edited

Loading

efiop Dec 4, 2019 •

edited

Loading