-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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.
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
support subrepos for get/list/api/import #4465
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this is a good change, "configuring" external repo from the start feels much better than using nested with
statements which change existing instance.
0cb2c1e
to
8cce4b0
Compare
Looks good! But I see that a lot of the changes are not actually about subrepos and could be introduced without mentioning subrepos at all (e.g. that |
@efiop, subrepo support is already there. This PR is more about enabling it, which is not possible without changing APIs of External(Git)Repo, which in turn affects |
This comment has been minimized.
This comment has been minimized.
This adds support for get/list/api/import/ls on a subrepo. Also fixes iterative#3180, by adding a granular url support for `dvc.api.get_url` And, of course it fixes iterative#3369.
be62966
to
e906bfa
Compare
metadata = _repo.repo_tree.metadata(path_info) | ||
|
||
if not metadata.is_dvc: | ||
raise OutputNotFoundError(path, repo) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looked into this, we used to throw OutputNotFoundError
before as well, so this does not change anything.
Regarding UrlNotDvcRepoError
, it's not precise to throw now as a git-repo can have subrepo inside of it.
@@ -28,30 +31,44 @@ | |||
logger = logging.getLogger(__name__) | |||
|
|||
|
|||
class IsADVCRepoError(DvcException): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thrown when the path is a root of the dvc repo during get/import
.
with pytest.raises(InvalidArgumentError) as exc: | ||
Repo().run(**kwargs) | ||
dvc.run(**kwargs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, we used to wait for 5s for lock timeout, so this used to work. But, since we reduced the timeout, this has become flakey.
@efiop @pared @pmrowla, this is ready for a review. The only issue right now is with Also, I have separated tests and implementation commits to make it easier for review. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, some minor things.
def open( | ||
self, path, mode="r", encoding="utf-8" | ||
self, path, mode="r", encoding=None | ||
): # pylint: disable=arguments-differ | ||
assert mode in {"r", "rb"} | ||
encoding = encoding or "utf-8" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, maybe api.open
should use utf-8
by default?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@pared Ideally - no, it should use your default system encoding. This is how things like pathlib's Path do it. I believe, Saugat just changed this for a better practice for optional args and to match BaseTree.open (there are still some discrepancies in tree methods, that I left untouched when adding these trees).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it should use default system encoding, I just tried to make api.open()
work without changing any behavior here.
Something to fix it later.
* use `spy` instead of `wraps` * remove dvc fixture in a test * set `cache_types` on Git repo
β I have followed the Contributing to DVC checklist.
π If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.
Thank you for the contribution - we'll try to review it as soon as possible. π
This adds support for get/list/api/import/ls on a subrepo.
Also fixes #3180, by adding a granular url support for
dvc.api.get_url
And, of course it fixes #3369.