support subrepos for get/list/api/import#4465
Conversation
pared
left a comment
pared
left a comment
There was a problem hiding this comment.
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.
skshetry
force-pushed
the
external_repo
branch
from
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 |
skshetry
changed the title
This comment has been minimized.
This comment has been minimized.
This adds support for get/list/api/import/ls on a subrepo. Also fixes treeverse#3180, by adding a granular url support for `dvc.api.get_url` And, of course it fixes treeverse#3369.
skshetry
force-pushed
the
external_repo
branch
from
be62966 to
e906bfa
Compare
skshetry
changed the title
| metadata = _repo.repo_tree.metadata(path_info) | ||
|
|
||
| if not metadata.is_dvc: | ||
| raise OutputNotFoundError(path, repo) |
There was a problem hiding this comment.
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.
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class IsADVCRepoError(DvcException): |
There was a problem hiding this comment.
Thrown when the path is a root of the dvc repo during get/import.
| def test_run_without_cmd(tmp_dir, dvc, kwargs): | ||
| with pytest.raises(InvalidArgumentError) as exc: | ||
| Repo().run(**kwargs) | ||
| dvc.run(**kwargs) |
There was a problem hiding this comment.
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. |
| 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.
Hmm, maybe api.open should use utf-8 by default?
There was a problem hiding this comment.
@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.
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
skshetry
changed the title
4 participants
❗ 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_urlAnd, of course it fixes #3369.