external: refactor external_repo() and its users #3190
Conversation
|
@Suor Please rebase. |
|
@efiop rebased. Moved no remote/cache present fix to new code, left a TODO there because issues tend to get lost. I will fix it in a subsequent PR, doesn't look small enough to fit here. |
|
@Suor TODOs get lost more, please remove it and create an issue. Add it to the next sprint if you want to handle it later and not get lost. |
|
@Suor there is some problem with an author of your commits, what is going on? |
|
We have lots of dead issues and no lost TODOs ) |
Some things are fixed along the way: - no cache shared between incompatible funcs - some incosistent exceptions are now consistent - import updates for git files now works both for dvc and git repos - "auto-generated-upstream already exists" fixed - dvc.api.open()/read() works with git repos now Fixes iterative#3124, iterative#2976. Closes iterative#3172.
|
rebased |
| if rev is None and (not repo_url or os.path.exists(repo_url)): | ||
| yield Repo(repo_url) | ||
| else: | ||
| with external_repo(url=repo_url, rev=rev) as repo: |
There was a problem hiding this comment.
Why is external_repo() used if there's a rev? What if I want to access a file in the current DVC repo but from a previous commit? I'm trying to test this but I'm getting AttributeError: module 'dvc' has no attribute 'api'
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
There was a problem hiding this comment.
@jorgeorpinel, it should just clone the current repo and checkout to given rev and access that file.
There was a problem hiding this comment.
Testing this, I see it works anyway. But is it using external_repo() to clone the current repo in a temporary folder and then use that instead of just the current Git repo's tree?
There was a problem hiding this comment.
p.s. my test script:
import dvc.api
print(dvc.api.read('data', rev='HEAD^'))
datashould be a DVC-tracked file with 2 different consecutive versions (inHEAD^and inHEAD).
There was a problem hiding this comment.
My point is I think Repo() also accepts rev, just wondering why it's not passed, and whether that would be a better implementation.
There was a problem hiding this comment.
Repo does not accept rev. Repo is all about current workspace, so, you cannot just checkout the user's workspace and modify/read. So, even though it's still is a Repo underneath (if it's a DVC repo), it's cloned, checked out to a rev, put into a tmp cache directory and operated inside of it.
There was a problem hiding this comment.
Got it. I was just confused by the name external_repo then. I guess that's the only way to get a previous rev from the current repo unfortunately.
Some things are fixed along the way:
revfor local repos nowFixes #3124, fixes #2976, fixes #3179, and fixes #2824. Closes #3172.