Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

api: fallback to external_repo if not a DVC repository #3222

Merged
merged 2 commits into from
Jan 23, 2020
Merged

api: fallback to external_repo if not a DVC repository #3222

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
003b5c3
api: fallback to external_repo on error
skshetry Jan 23, 2020
2e0b143
api: use current directory for repo if not specified
skshetry Jan 23, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 8 additions & 5 deletions dvc/api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from voluptuous import Schema, Required, Invalid

from dvc.repo import Repo
from dvc.exceptions import DvcException
from dvc.exceptions import DvcException, NotDvcRepoError
from dvc.external_repo import external_repo


Expand Down Expand Up @@ -110,10 +110,13 @@ def read(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
@contextmanager
def _make_repo(repo_url, rev=None):
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:
yield repo
try:
yield Repo(repo_url)
return
except NotDvcRepoError:
pass # fallthrough to external_repo
with external_repo(url=repo_url, rev=rev) as repo:
skshetry marked this conversation as resolved.
Show resolved Hide resolved
yield repo


def summon(name, repo=None, rev=None, summon_file="dvcsummon.yaml", args=None):
Expand Down
4 changes: 2 additions & 2 deletions dvc/external_repo.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,10 +128,10 @@ def pull_to(self, path, to_info):
raise PathMissingError(path, self.url)

@contextmanager
def open_by_relpath(self, path, mode="r", encoding=None):
def open_by_relpath(self, path, mode="r", encoding=None, **kwargs):
Copy link
Member Author

@skshetry skshetry Jan 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

open_by_relpath on the Repo also takes more arguments such as remote, etc which does not make sense here.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it works it should stay as is. Let's not add ignored arguments, this will lead to silent errors.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It does not as it also gets unexpected arguments from Repo.open_by_relpath:

dvc/dvc/repo/__init__.py

Line 430 in 4903996

def open_by_relpath(self, path, remote=None, mode="r", encoding=None):

Copy link
Member Author

@skshetry skshetry Jan 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could though add remote, or not even send one, but, I prefer this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand. This line is never called from Repo.open_by_relpath().

Copy link
Member Author

@skshetry skshetry Jan 23, 2020 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's called by dvc.api.open() here with following args:

dvc/dvc/api.py

Lines 70 to 80 in 4903996

def open(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
"""Context manager to open a file artifact as a file object."""
args = (path,)
kwargs = {
"repo": repo,
"remote": remote,
"rev": rev,
"mode": mode,
"encoding": encoding,
}
return _OpenContextManager(_open, args, kwargs)

And, successively, _open calls this line:

dvc/dvc/api.py

Lines 94 to 100 in 4903996

def _open(path, repo=None, rev=None, remote=None, mode="r", encoding=None):
with _make_repo(repo, rev=rev) as _repo:
with _repo.open_by_relpath(
path, remote=remote, mode=mode, encoding=encoding
) as fd:
yield fd

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ok, then we should add only remote param not **kwargs and assert remote is None. This way things won't be silently ignored.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@Suor assert is not needed, as the point of that method is to provide the same interface for git and dvc repos, so it should be silently ignored there. Or am I missing something?

try:
abs_path = os.path.join(self.root_dir, path)
with open(abs_path, mode, encoding) as fd:
with open(abs_path, mode, encoding=encoding) as fd:
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Third argument is buffering.

yield fd
except FileNotFoundError:
raise PathMissingError(path, self.url)
Expand Down
4 changes: 2 additions & 2 deletions tests/func/test_api.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@
from dvc import api
from dvc.api import SummonError, UrlNotDvcRepoError
from dvc.compat import fspath
from dvc.exceptions import FileMissingError, NotDvcRepoError
from dvc.exceptions import FileMissingError
from dvc.main import main
from dvc.path_info import URLInfo
from dvc.remote.config import RemoteConfig
Expand Down Expand Up @@ -54,7 +54,7 @@ def test_get_url_external(erepo_dir, remote_url):
def test_get_url_requires_dvc(tmp_dir, scm):
tmp_dir.scm_gen({"foo": "foo"}, commit="initial")

with pytest.raises(NotDvcRepoError, match="not inside of a DVC repo"):
with pytest.raises(UrlNotDvcRepoError, match="not a DVC repository"):
api.get_url("foo", repo=fspath(tmp_dir))

with pytest.raises(UrlNotDvcRepoError):
Expand Down