Fix import issue without remote config in the target #3120

sharidas · 2020-01-12T17:03:58Z

Fix the import issue when target does not have
remote config.

Fixes get/import: don't require default remote for get/import in certain cases #2780

Signed-off-by: Sujith H sujith.h@gmail.com

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

sharidas · 2020-01-12T17:08:22Z

This PR is still a work in progress. In this PR I have tried to capture the NoRemoteError execption and then look for the file/dir inside the dvc repo where we are importing. If the file/dir is found then no further action required as its available in the cache.

I have not added condition to check: what if the cache is not available in the reciever of the import. I need to check the same.

Any feedback regarding the approach is appreciated.

dvc/dependency/repo.py

sharidas · 2020-01-15T16:59:30Z

Would appreciate review for this PR.

shcheklein · 2020-01-15T19:30:45Z

@sharidas should we add a test for this?

ghost · 2020-01-15T20:14:02Z

dvc/dependency/repo.py

+                try:
+                    repo.cloud.pull(out.get_used_cache())
+                except NoRemoteError:
+                    # Check if the file is available in the cache


The comment is describing what the code is doing, but I would appreciate more a comment about why it is doing such thing.

I have updated the comment. Let me know if this looks ok?

sharidas · 2020-01-16T07:08:07Z

@sharidas should we add a test for this?

Updated the PR by adding test.

efiop · 2020-01-16T11:06:50Z

tests/func/test_import.py

+    erepo_dir.scm_gen({src: "hello"}, commit="add a regular file")
+    os.remove(erepo_dir.dvc.config.config_file)
+
+    with tmp_dir.chdir():


You are already inside of a tmp_dir, no need to chdir. 🙂

efiop · 2020-01-16T11:08:32Z

tests/func/test_import.py

+        f = open(dst, "w")
+        f.write("hello")
+        f.close()
+        tmp_dir.dvc.add([fspath(tmp_dir / dst)])


You are adding by full path, which will make dvc treat it as an external output. What you want is:

tmp_dir.dvc_gen({dst: "hello"})

looks like.

Side note: using open + write + close is a bad style even without pathlib, where you should use with open() instead.

efiop · 2020-01-16T11:09:01Z

tests/func/test_import.py

+        f.write("hello")
+        f.close()
+        tmp_dir.dvc.add([fspath(tmp_dir / dst)])
+        os.remove(tmp_dir / dst)


Suggested change

os.remove(tmp_dir / dst)

(tmp_dir / dst).unlink()

efiop · 2020-01-16T11:12:04Z

dvc/dependency/repo.py

+                except NoRemoteError:
+                    # It would not be good idea to raise exception if the
+                    # file is already present in the cache
+                    if os.path.exists(self.repo.cache.local.get(out.checksum)):


Let's use

Suggested change

if os.path.exists(self.repo.cache.local.get(out.checksum)):

if not self.repo.cache.local.changed_cache(out.checksum):

to make it fully work with directories and be more robust in general. There is nothing wrong with your implementation, just an obscure internal stuff that is more appropriate here.

efiop · 2020-01-16T11:14:03Z

tests/func/test_import.py

+    dst = "some_file_imported"
+
+    erepo_dir.scm_gen({src: "hello"}, commit="add a regular file")
+    os.remove(erepo_dir.dvc.config.config_file)


Hm, I don't think this affects the test at all, because dvc import clones erepo and non-commited changes won't be cloned anyways.

So what happens in this test currently is that dvc automatically sets default remote to erepo_dir's .dvc/cache and pulls from there. https://github.com/iterative/dvc/blob/master/dvc/external_repo.py#L91 Need to somehow disable it for this test or workaround it. 🤔

@efiop If my understanding is correct https://github.com/iterative/dvc/blob/master/tests/dir_helpers.py#L282-L283 is the code which adds remote configuration. Or am I missing something. I didn't quite get the point here. I was thinking: if removal of config from the .dvc/config from the erepo would help to replicate the scenario, while writing the test.

@sharidasan Oh, good point! I've totally forgotten about that one. We used to use that before https://github.com/iterative/dvc/blob/master/dvc/external_repo.py#L91 logic. But still, as you can see that file is commited and here you are only removing it from the workspace, not committing it, so clone of this erepo would still have that config intact.

sharidas · 2020-01-16T15:47:58Z

@efiop I have updated the change again. In the latest change set I have introduced a new fixture ( similar to erepo_dir ) which differs from erepo_dir where no config data is present. So when the erepo is cloned an empty config file is created. Let me know if this looks kind of ok ...

efiop · 2020-01-16T15:57:55Z

tests/dir_helpers.py

+@pytest.fixture
+def erepo_dir_no_remote_config(tmp_path_factory, monkeypatch):
+    from dvc.repo import Repo
+
+    path = TmpDir(fspath_py35(tmp_path_factory.mktemp("erepo")))
+
+    # Chdir for git and dvc to work locally
+    with path.chdir():
+        _git_init()
+        path.dvc = Repo.init()
+        path.scm = path.dvc.scm
+        path.scm.commit("init dvc")
+
+        path.dvc.close()
+
+    return path


Unfortunately removing that config part still won't help, as https://github.com/iterative/dvc/blob/master/dvc/external_repo.py#L91 is still in place, which automatically sets it back when cloning 🙁

How about we simply mock cloud.pull to raise NoRemoteError in our test?

I am bit confused here. When I execute:

python -m tests tests/func/test_import.py::test_import_cached_file

I do see a config file under location /tmp/pytest-of-sujith/pytest-0/popen-gw0/erepo0/.dvc/config in my machine. The file is actually empty. So while cloning it sets back but the content is empty.. Kindly correct me if my obeservation is wrong.

@sharidasan It creates .dvc/config.local though, not .dvc/config.

efiop · 2020-01-16T15:59:58Z

dvc/dependency/repo.py

+                try:
+                    repo.cloud.pull(out.get_used_cache())
+                except NoRemoteError:
+                    # It would not be good idea to raise exception if the
+                    # file is already present in the cache
+                    if not self.repo.cache.local.changed_cache(out.checksum):
+                        return out
+                    raise


Just a side note: we might later consider making pull in general not require remote if everything is already locally available. This is out of focus for this PR, current implementation works great for what we need it for 👍 So no actions required here.

Suor · 2020-01-16T16:12:53Z

dvc/dependency/repo.py

+                except NoRemoteError:
+                    # It would not be good idea to raise exception if the
+                    # file is already present in the cache
+                    if not self.repo.cache.local.changed_cache(out.checksum):


I don't think this is a correct logic for dir outs, it should be just out.changed_cache().

However, I don't understand the overall approach, we just created that repo it has empty cache. What's the point?

Oh, I see, this is for local import/shared cache case. I would say we should change .pull() implementation to check local cache first and only go for upstream when something is missing in local cache.

@Suor please see #3120 (comment) . out.change_cache() would be better, true.

Suor · 2020-01-16T16:15:52Z

tests/dir_helpers.py

+
+
+@pytest.fixture
+def erepo_dir_no_remote_config(tmp_path_factory, monkeypatch):


It's an overkill creating a fixture for single test. Any of the following would be better:

use existing erepo_dir, drop upstream in a test

change erepo_dir to not have upstream, fix any issues it causes (if that is not too hard)

@Suor Def don't need that new fixture, but change erepo_dir to not have upstream won't help, see #3120 (comment)

Suor · 2020-01-16T16:30:29Z

dvc/dependency/repo.py

+                except NoRemoteError:
+                    # It would not be good idea to raise exception if the
+                    # file is already present in the cache
+                    if not self.repo.cache.local.changed_cache(out.checksum):


Oh, I see, this is for local import/shared cache case. I would say we should change .pull() implementation to check local cache first and only go for upstream when something is missing in local cache.

sharidas · 2020-01-16T16:37:20Z

I would say we should change .pull() implementation to check local cache first and only go for upstream when something is missing in local cache.

This case is related get/import failure when remote config is missing. Meaning when we get/import the file/dir from the remote url. I am sharing my understanding with respect to the ticket here :)

Fix the import issue when target does not have remote config. Signed-off-by: Sujith H <sujith.h@gmail.com>

sharidas · 2020-01-19T06:52:11Z

I have updated the test. Also verified the expected code flow to trigger the excecption for NoRemoteError. Let me know if this change set looks ok? @efiop @shcheklein

efiop

Thanks @sharidas ! 🙏

sharidas changed the title ~~Fix import issue without remote config in the target~~ [WIP] Fix import issue without remote config in the target Jan 12, 2020

sharidas force-pushed the no_remote_issue branch from e3e9520 to db76bc5 Compare January 12, 2020 17:10

efiop reviewed Jan 12, 2020

View reviewed changes

dvc/dependency/repo.py Outdated Show resolved Hide resolved

sharidas force-pushed the no_remote_issue branch from db76bc5 to bf3b069 Compare January 13, 2020 06:18

sharidas changed the title ~~[WIP] Fix import issue without remote config in the target~~ Fix import issue without remote config in the target Jan 15, 2020

ghost reviewed Jan 15, 2020

View reviewed changes

sharidas force-pushed the no_remote_issue branch from bf3b069 to 777a493 Compare January 16, 2020 07:07

efiop reviewed Jan 16, 2020

View reviewed changes

sharidas force-pushed the no_remote_issue branch from 777a493 to 25a797a Compare January 16, 2020 15:45

efiop reviewed Jan 16, 2020

View reviewed changes

Suor suggested changes Jan 16, 2020

View reviewed changes

Fix import issue without remote config in the target

1b7eda2

Fix the import issue when target does not have remote config. Signed-off-by: Sujith H <sujith.h@gmail.com>

sharidas force-pushed the no_remote_issue branch from 25a797a to 1b7eda2 Compare January 19, 2020 06:12

efiop approved these changes Jan 19, 2020

View reviewed changes

efiop merged commit cf01357 into iterative:master Jan 19, 2020

weekly-digest bot mentioned this pull request Jan 26, 2020

Weekly Digest (19 January, 2020 - 26 January, 2020) #3237

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix import issue without remote config in the target #3120

Fix import issue without remote config in the target #3120

sharidas commented Jan 12, 2020 •

edited by efiop

Loading

sharidas commented Jan 12, 2020

sharidas commented Jan 15, 2020

shcheklein commented Jan 15, 2020

ghost Jan 15, 2020

sharidas Jan 16, 2020

sharidas commented Jan 16, 2020

efiop Jan 16, 2020

efiop Jan 16, 2020

efiop Jan 16, 2020

efiop Jan 16, 2020

efiop Jan 16, 2020

efiop Jan 16, 2020

efiop Jan 16, 2020

sharidas Jan 16, 2020

efiop Jan 16, 2020

sharidas commented Jan 16, 2020

efiop Jan 16, 2020

sharidas Jan 16, 2020

efiop Jan 16, 2020

efiop Jan 16, 2020

Suor Jan 16, 2020

Suor Jan 16, 2020

efiop Jan 16, 2020

Suor Jan 16, 2020

efiop Jan 16, 2020

Suor Jan 16, 2020

sharidas commented Jan 16, 2020

sharidas commented Jan 19, 2020

efiop left a comment

	if os.path.exists(self.repo.cache.local.get(out.checksum)):
	if not self.repo.cache.local.changed_cache(out.checksum):



		@pytest.fixture
		def erepo_dir_no_remote_config(tmp_path_factory, monkeypatch):

Fix import issue without remote config in the target #3120

Fix import issue without remote config in the target #3120

Conversation

sharidas commented Jan 12, 2020 • edited by efiop Loading

sharidas commented Jan 12, 2020

sharidas commented Jan 15, 2020

shcheklein commented Jan 15, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas commented Jan 16, 2020

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

sharidas commented Jan 16, 2020

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

sharidas commented Jan 16, 2020

sharidas commented Jan 19, 2020

efiop left a comment

Choose a reason for hiding this comment

sharidas commented Jan 12, 2020 •

edited by efiop

Loading