get: handle non-DVC repositories #3097

fabiosantoscode · 2020-01-09T22:12:52Z

Allows us to dvc get from non-DVC source repositories.

Will send PR for doc site tomorrow. Just need to change some parts which imply that the remote repository needs to be DVC-enabled.

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

docs: iterative/dvc.org#911

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

fabiosantoscode · 2020-01-09T22:43:57Z

DeepSource is complaining about some unused arguments in tests. I suppose I shouldn't clean them up at this point.

efiop

Thanks for the PR! Please see some comments below.

dvc/repo/get.py

efiop · 2020-01-09T23:25:15Z

dvc/repo/get.py

+            if not output.use_cache:
+                # Catch this below and go for a plain old fs_copy
+                raise _DoPlainCopy


This is a very strange flow control, I have never ever seen one like that, but maybe I haven't seen enough python projects, so please correct me if I'm wrong. Regardless, I'm pretty sure you could simply do something like:

try: repo = Repo(tmp_dir) output = repo.find_out_by_relpath(path) if output.use_cache: ... _get_cached(repo, output, out) return except (NotDvcRepoError, OutputNotFoundError): pass if os.path.isabs(path): raise FileNotFoundError fs_copy(os.path.join(tmp_dir, path), out)

and avoid that.

Yes, that is way nicer!

efiop · 2020-01-09T23:26:23Z

dvc/repo/get.py

            #
            # Also, we can't use theoretical "move" link type here, because
            # the same cache file might be used a few times in a directory.


This comment should stay over repo.cache.local.cache_types = ["reflink", "hardlink", "copy"] line, otherwise it loses some of its meaning.

efiop · 2020-01-09T23:28:55Z

DeepSource is complaining about some unused arguments in tests. I suppose I shouldn't clean them up at this point.

@fabiosantoscode Yep, take it as a heads-up from DS. In this case there are no actions you should take. 🙂

fabiosantoscode · 2020-01-10T16:48:21Z

@efiop I believe I've addressed the comments.

WRT the clone_dir argument and the lost optimization, I removed the argument, and instead of changing the clone target, I changed the Repo's cache to tmp_dir which is next to the place the get will drop the file at.

dvc/repo/get.py

fabiosantoscode · 2020-01-10T17:01:44Z

Cool @efiop, I've reverted the code to just clone into tmp_dir and addressed the remaining comments 👍

efiop · 2020-01-10T17:18:49Z

WRT the clone_dir argument and the lost optimization, I removed the argument, and instead of changing the clone target, I changed the Repo's cache to tmp_dir which is next to the place the get will drop the file at.

Sorry, not sure I follow. I see that you still pass tmp_path to cached_clone , instead of keeping it as it was before and only changing the cache dir. Is it meant that way?

fabiosantoscode · 2020-01-10T17:26:56Z

@efiop I wrote that before you pointed out that it was enough to just clone the repo to tmp_dir, do disregard it.

jorgeorpinel

Some language suggestions.

BTW I think this addresses half of #3077! Maybe apply also to dvc import in this same PR and mark it as closing that issue as well? 🗿 -> 🐦 🐦

dvc/command/get.py

fabiosantoscode · 2020-01-10T18:11:09Z

@jorgeorpinel thanks for your suggestions, it reads a lot clearer now.

As per closing #3077, sure, let's do it :)

fabiosantoscode · 2020-01-10T18:38:23Z

@jorgeorpinel the docs for import are now updated. Check them out and see if they make sense to you.

@efiop your comments have all been addressed, I believe.

And @restyled-io has closed its PR, since the code style is on par. I ran black dvc locally just to confirm.

fabiosantoscode · 2020-01-10T18:50:03Z

I'll update the docs when this is confirmed to be the final help text.

efiop · 2020-01-10T19:00:10Z

dvc/repo/get.py

@@ -43,7 +44,10 @@ def get(url, path, out=None, rev=None):
    dpath = os.path.dirname(os.path.abspath(out))
    tmp_dir = os.path.join(dpath, "." + str(shortuuid.uuid()))
    try:
-        with external_repo(cache_dir=tmp_dir, url=url, rev=rev) as repo:
+        cached_clone(url, rev=rev, clone_path=tmp_dir)


Sorry, I might've not worded it clearly enough, but I meant that we should keep it as it was before this change, meaning that we shouldn't pass tmp_dir to cached_clone and let it clone wherever it needs to. And we need to setup external cache dir pointing to tmp_dir, the same way we do that in external_repo. Also, take a look at external_repo, it is doing things like setting a default remote, which you've lost here.

You might be able to flip the logic over and do something like:

try: with external_repo(): ... if out.use_cache: _get_cached() return # fallthrough except NotDvcRepoError: pass path = cached_clone(...) # copy from git

Done! I didn't fold it like that because I would have to do the cached_clone conditionally, and instead duplicated the file copy call.

jorgeorpinel

Thanks for addressing #3077 along the way @fabiosantoscode! Here are more small language suggestions, some of them my bad from previous review, and some of them may require reformatting code (new lines) but black on the pre-commit hook or Restyled can do that for you.

dvc/command/imp.py

dvc/command/get.py

dvc/command/imp.py

dvc/command/get.py

dvc/command/imp.py

Allows us to `dvc get` from non-DVC source repositories. Fixes iterative#3089

…t be DVC enabled

Co-Authored-By: Ruslan Kuprieiev <[email protected]>

This reverts commit 8e49228.

Co-Authored-By: Jorge Orpinel <[email protected]>

Fixes iterative#3077

fabiosantoscode · 2020-01-10T22:37:10Z

My bad @jorgeorpinel I should've made the help texts for get and import uniform myself. Thanks for doing that for me in the end.

@efiop now I'm using external_repo like before, to leverage its DVC-specific logic, and a simple cached_clone followed by a file copy in case it's not a DVC repo.

I'm wondering if a simple file move would be the best for the simple git repository case. After all, the repository is destroyed just after and moves within a single filesystem are very fast.

shcheklein · 2020-01-10T22:51:06Z

dvc/repo/get.py

                # Either an uncached out with absolute path or a user error
+


minor: remove this extra line?

efiop · 2020-01-10T23:05:25Z

dvc/repo/get.py

+                if output and output.use_cache:
+                    _get_cached(repo, output, out)
+                    return
+
                # Either an uncached out with absolute path or a user error
                if os.path.isabs(path):


Absolute paths are not allowed even for pure-git repos, hence why I've suggested that example that has the same code handling both non-cached and git files. No need to worry much about second cached_clone being redundand, as we cache those anyways, so the overhead is not big.

Got it. I tried to fold it like you demonstrated before, but it looks like the files are missing if I try to do the copy outside of the context manager.

So I extracted the part that checks if the path is absolute into a function and called it twice.

efiop · 2020-01-10T23:11:09Z

dvc/repo/get.py

+        if raw_git_dir:
+            remove(raw_git_dir)


I don't think there is any need in removing it, it is a cached copy after all 🙂 And if you check external_repo, it doesn't delete those either. Or am I missing something?

That's good news. I figured there was some automatic cleanup happening but haven't seen how it works yet.

efiop · 2020-01-10T23:19:37Z

dvc/repo/get.py

                # Either an uncached out with absolute path or a user error
-                if os.path.isabs(path):
-                    raise FileNotFoundError
+                _forbid_absolute_path(path)

                fs_copy(os.path.join(repo.root_dir, path), out)
+                return
+


Why not just pass it through? You are doing the same thing down below anyway.

First time I tried it I got file not found errors and figured the context manager was removing the repository. I was wrong. I got it working now.

jorgeorpinel

All good with command output now. Let's just reflect it on iterative/dvc.org/pull/912 please (including in the cmd ref Synopsis sections). Thanks.

efiop

Thanks!

dvc/command/get.py

dvc/repo/get.py

fabiosantoscode · 2020-01-18T22:42:42Z

@Suor maybe I don't understand what you've raised here but since get now supports non DVC repositories it will never throw an exception to complain about the target repository not being DVC.

Suor · 2020-01-19T14:45:00Z

@fabiosantoscode get will never throw, but external_repo() doesn't know and should not expect all code using it to always handle certain exceptions. Anyway, this won't be an issue after #3190.

fabiosantoscode force-pushed the feature/3089-get-non-dvc-repositories branch from a13df5a to afc8886 Compare January 9, 2020 22:42

efiop suggested changes Jan 9, 2020

View reviewed changes

fabiosantoscode force-pushed the feature/3089-get-non-dvc-repositories branch from 873123a to d092854 Compare January 10, 2020 14:04

This was referenced Jan 10, 2020

get: it can download from plain (non-DVC) Git repos. iterative/dvc.org#911

Closed

DOC: reflect the fact that get allows for non-DVC repositories iterative/dvc.org#912

Merged

efiop reviewed Jan 10, 2020

View reviewed changes

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

efiop reviewed Jan 10, 2020

View reviewed changes

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

jorgeorpinel suggested changes Jan 10, 2020

View reviewed changes

dvc/command/get.py Outdated Show resolved Hide resolved

dvc/command/get.py Outdated Show resolved Hide resolved

dvc/command/get.py Outdated Show resolved Hide resolved

dvc/command/get.py Outdated Show resolved Hide resolved

restyled-io bot mentioned this pull request Jan 10, 2020

Restyle get: handle non-DVC repositories #3103

Closed

efiop reviewed Jan 10, 2020

View reviewed changes

jorgeorpinel suggested changes Jan 10, 2020

View reviewed changes

restyled-io bot mentioned this pull request Jan 10, 2020

Restyle get: handle non-DVC repositories #3106

Closed

fabiosantoscode and others added 9 commits January 10, 2020 22:28

get: handle non-DVC repositories

0dd5647

Allows us to `dvc get` from non-DVC source repositories. Fixes iterative#3089

style: improve code flow and move comments

07637a6

doc: change get documentation so that it doesn't imply the target mus…

f126f24

…t be DVC enabled

get: recoup cache optimal location

86b9a1c

Update dvc/repo/get.py

a01f81e

Co-Authored-By: Ruslan Kuprieiev <[email protected]>

Revert "get: recoup cache optimal location"

6cb7308

This reverts commit 8e49228.

remove unused exception class

7873abc

change command doc string

6a4f403

Update dvc/command/get.py

51f2fa1

Co-Authored-By: Jorge Orpinel <[email protected]>

fabiosantoscode and others added 6 commits January 10, 2020 22:28

Update dvc/command/get.py

04619d5

Co-Authored-By: Jorge Orpinel <[email protected]>

Update dvc/command/get.py

9a98622

Co-Authored-By: Jorge Orpinel <[email protected]>

Update dvc/command/get.py

5b36ffd

Co-Authored-By: Jorge Orpinel <[email protected]>

import: update documentation

c16f882

Fixes iterative#3077

get: leverage external_repo() context manager for DVC repositories

f5e40b2

run black

5fb6640

fabiosantoscode force-pushed the feature/3089-get-non-dvc-repositories branch from 802d8ab to 5fb6640 Compare January 10, 2020 22:29

shcheklein reviewed Jan 10, 2020

View reviewed changes

remove blank line

0e2ef47

efiop reviewed Jan 10, 2020

View reviewed changes

fabiosantoscode added 2 commits January 10, 2020 23:15

forbid absolute paths for plain git repositories

9f279b3

don't manually clean up git repository

1f2eed8

efiop reviewed Jan 10, 2020

View reviewed changes

fabiosantoscode added 2 commits January 10, 2020 23:26

deduplicate code

f3d234e

test removing a file outside a raw git repo

de303d4

jorgeorpinel approved these changes Jan 11, 2020

View reviewed changes

efiop approved these changes Jan 11, 2020

View reviewed changes

efiop merged commit b7f9e73 into iterative:master Jan 11, 2020

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

Weekly Digest (5 January, 2020 - 12 January, 2020) #3119

Closed

fabiosantoscode deleted the feature/3089-get-non-dvc-repositories branch January 13, 2020 14:09

jorgeorpinel reviewed Jan 13, 2020

View reviewed changes

dvc/command/get.py Show resolved Hide resolved

Suor reviewed Jan 16, 2020

View reviewed changes

dvc/repo/get.py Show resolved Hide resolved

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

get: handle non-DVC repositories #3097

get: handle non-DVC repositories #3097

fabiosantoscode commented Jan 9, 2020 •

edited by jorgeorpinel

Loading

fabiosantoscode commented Jan 9, 2020 •

edited

Loading

efiop left a comment

efiop Jan 9, 2020 •

edited

Loading

fabiosantoscode Jan 10, 2020

efiop Jan 9, 2020 •

edited

Loading

fabiosantoscode Jan 10, 2020

efiop commented Jan 9, 2020

fabiosantoscode commented Jan 10, 2020

fabiosantoscode commented Jan 10, 2020

efiop commented Jan 10, 2020 •

edited

Loading

fabiosantoscode commented Jan 10, 2020

jorgeorpinel left a comment

fabiosantoscode commented Jan 10, 2020 •

edited

Loading

fabiosantoscode commented Jan 10, 2020

fabiosantoscode commented Jan 10, 2020

efiop Jan 10, 2020

efiop Jan 10, 2020 •

edited

Loading

fabiosantoscode Jan 10, 2020

jorgeorpinel left a comment •

edited

Loading

fabiosantoscode commented Jan 10, 2020

shcheklein Jan 10, 2020

fabiosantoscode Jan 10, 2020

efiop Jan 10, 2020

fabiosantoscode Jan 10, 2020

efiop Jan 10, 2020

fabiosantoscode Jan 10, 2020 •

edited

Loading

efiop Jan 10, 2020

fabiosantoscode Jan 10, 2020

jorgeorpinel left a comment •

edited

Loading

efiop left a comment

fabiosantoscode commented Jan 18, 2020

Suor commented Jan 19, 2020

get: handle non-DVC repositories #3097

get: handle non-DVC repositories #3097

Conversation

fabiosantoscode commented Jan 9, 2020 • edited by jorgeorpinel Loading

fabiosantoscode commented Jan 9, 2020 • edited Loading

efiop left a comment

Choose a reason for hiding this comment

efiop Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop Jan 9, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop commented Jan 9, 2020

fabiosantoscode commented Jan 10, 2020

fabiosantoscode commented Jan 10, 2020

efiop commented Jan 10, 2020 • edited Loading

fabiosantoscode commented Jan 10, 2020

jorgeorpinel left a comment

Choose a reason for hiding this comment

fabiosantoscode commented Jan 10, 2020 • edited Loading

fabiosantoscode commented Jan 10, 2020

fabiosantoscode commented Jan 10, 2020

Choose a reason for hiding this comment

efiop Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgeorpinel left a comment • edited Loading

Choose a reason for hiding this comment

fabiosantoscode commented Jan 10, 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

fabiosantoscode Jan 10, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jorgeorpinel left a comment • edited Loading

Choose a reason for hiding this comment

efiop left a comment

Choose a reason for hiding this comment

fabiosantoscode commented Jan 18, 2020

Suor commented Jan 19, 2020

fabiosantoscode commented Jan 9, 2020 •

edited by jorgeorpinel

Loading

fabiosantoscode commented Jan 9, 2020 •

edited

Loading

efiop Jan 9, 2020 •

edited

Loading

efiop Jan 9, 2020 •

edited

Loading

efiop commented Jan 10, 2020 •

edited

Loading

fabiosantoscode commented Jan 10, 2020 •

edited

Loading

efiop Jan 10, 2020 •

edited

Loading

jorgeorpinel left a comment •

edited

Loading

fabiosantoscode Jan 10, 2020 •

edited

Loading

jorgeorpinel left a comment •

edited

Loading