Initial pkg implementation #2012

efiop · 2019-05-16T18:06:18Z

Have you followed the guidelines in our
Contributing document?
Does your PR affect documented changes or does it add new functionality
that should be documented? If yes, have you created a PR for
dvc.org documenting it or at
least opened an issue for it? If so, please add a link to it.

Todo:

test that uninstall can remove corrupted pkg
don't push pkg imported data to remote
~~add .dvc/pkg-list.yml similar to https://github.com/sindresorhus/package-json~~ (import: add a mechanism to lock external dependencies #2139)
~~add .dvc/pkg-lock.yml similar to https://flaviocopes.com/package-lock-json/ (install should add an entry to it)~~ (import: add a mechanism to lock external dependencies #2139)
support importing the whole package with dvc import mypkg. This is going to be similar to specifying multiple datas within the package, e.g. dvc import mypkg data1 data2, but we will have to collect it ourselves for the whole package. (import: support importing the whole repo #2140)
add dvc pkg get for simply downloading data from the url
For that ^ we'll need to detach dst option into a -o flag, so that we could dvc import mypkg -o dir;
~~check that when running repro on import it will ask if it needs to remove modified outputs~~ (will be implemented by run: warn and/or prompt user when deleting an output #2027)

dvc/repo/pkg/__init__.py

Suor · 2019-05-27T07:16:34Z

I think it would be better to not add knowledge about everything to config. Config should provide .get(), .set(), .update(), etc. Load and save itself but should know nothing about its keys and structure. Otherwise we are creating a superclass.

Knowledge about config contents will be ideally contained within specific code area and not slip anywhere else. E.g. remote will be able to configure itself from config and update config as needed: add or update a remote, set default one.

efiop · 2019-05-28T20:51:40Z

@Suor

I think it would be better to not add knowledge about everything to config. Config should provide .get(), .set(), .update(), etc. Load and save itself but should know nothing about its keys and structure. Otherwise we are creating a superclass.

There is a config like that already. It is called ConfigObj 🙂 Our config is dealing with managing particular sections(e.g. remote and pkg) within it and so has the necessary notion to properly parse them(e.g. schema, path resolutions and so on).

Knowledge about config contents will be ideally contained within specific code area and not slip anywhere else. E.g. remote will be able to configure itself from config and update config as needed: add or update a remote, set default one.

Do you mean something like this

r = Remote(repo, {"url": "https://example.com"})
r.save() # saving to config
r.set_default() # setting it in config as default remote

?

Fixes iterative#2039 Required by iterative#2012 to be able to fetch data through the API without interuption from updater. Signed-off-by: Ruslan Kuprieiev <[email protected]>

shcheklein · 2019-06-15T23:36:43Z

dvc/repo/__init__.py

@@ -315,6 +321,9 @@ def used_cache(
                stages = self.stages()

            for stage in stages:
+                if stage.is_pkg_import:
+                    continue
+
                if active and not target and stage.locked:
                    logger.warning(


not related not this PR: looks like either this message should be moved down the stack or we should add continue here.

We shouldn't have continue here, because we should push/pull/etc cache for the locked stage file itself. We could move this warning to repo.graph(), but it would be too specific to use it there. Hence why we put this one here.

shcheklein · 2019-06-15T23:38:38Z

dvc/repo/__init__.py

@@ -315,6 +321,9 @@ def used_cache(
                stages = self.stages()

            for stage in stages:
+                if stage.is_pkg_import:


as far as I remember we already have a mechanism to avoid caching of an output? can we reuse it here?

@shcheklein That is an amazing idea! I didn't think about it that way. But indeed, we could treat it as --outs-no-cache. Thanks! 🙂

Turns out it is not easy to do, since things like adding to gitignore and checking out are use_cache dependant. I've added get_used_cache method to Output to handle this more gracefully, but using --outs-no-cache is not a very good option for now.

shcheklein · 2019-06-15T23:41:00Z

dvc/stage.py

@@ -213,6 +213,13 @@ def is_import(self):
        """Whether the stage file was created with `dvc import`."""
        return not self.cmd and len(self.deps) == 1 and len(self.outs) == 1

+    @property
+    def is_pkg_import(self):


if feels like an output can decide if it needs to be cached or not

Output needs to know if he is a part of import stage, so this is still used.

not necessarily, when we create an output we can pass a flag. It can be and probably should be decided on the Cmd/API level. I can imagine we will have a flag to cache output anyway.

Will keep that in mind in #2138 (added a comment about this discussion there as well).

shcheklein · 2019-06-15T23:42:55Z

dvc/scm/git/tree.py

+            self.git.git.ls_remote("origin", self.rev, exit_code=True)
+            # fetching remote tag/branch so we can reference it locally
+            self.git.git.fetch("origin", "{rev}:{rev}".format(rev=self.rev))
+        except git.exc.GitCommandError:


should we fail if does not exist?

We will fail later down the line, where we will try to use this. I'll take a second look to see if failing here would provide better UX. Thanks for the heads up!

Adjusted the error handling down the line to handle it more gracefully.

dvc/pkg.py

shcheklein · 2019-06-15T23:54:00Z

dvc/pkg.py

+            return
+
+        git.Repo.clone_from(
+            self.url, self.path, depth=1, no_single_branch=True


qq - does it avoid restoring the workspace as well?

@shcheklein Not sure what you mean. Could you elaborate?

I mean --no-checkout option of the git clone

Ah, nope. So we do checkout to have a default version installed. If any dependency is using some particular version, it will get it. But if you've installed some package of specific version by default, then all dependencies that don't have version set explicitly, will use that default installed version.

shcheklein · 2019-06-16T00:00:58Z

dvc/pkg.py

+            output, = pkg.repo.find_outs_by_path(src)
+            pkg.repo.fetch(output.stage.path)
+            output.path_info = PathInfo(os.path.abspath(out))
+            with output.repo.state:


btw, does it make sense to disable lock and sqllite for this case? just to make sure that this easy scenario works everywhere.

You mean nfs and friends, right? Good point, I'll take a look if we could disable state for that purpose. Also need to disable our own .dvc/lock as well

This is not trivial, created #2135 for it. Might be easier to just support nfs after all.

shcheklein

This is looks great! A few minor comments and questions.

@jorgeorpinel please, review the help messages and command description. Let's work together to make then user-friendly.

Signed-off-by: Ruslan Kuprieiev <[email protected]>

efiop force-pushed the pkgs branch from e3ea47d to 5be6682 Compare May 16, 2019 18:06

This was referenced May 16, 2019

import-url: allow downloading directories #1861

Closed

pkg: support "reference" #2015

Closed

efiop force-pushed the pkgs branch 2 times, most recently from 7ddf5b9 to 0acc20e Compare May 18, 2019 17:21

Suor reviewed May 21, 2019

View reviewed changes

dvc/repo/pkg/__init__.py Outdated Show resolved Hide resolved

efiop force-pushed the pkgs branch 6 times, most recently from 76098ab to 9cd10dd Compare May 25, 2019 23:24

efiop force-pushed the pkgs branch 3 times, most recently from 328a014 to 84210ed Compare May 28, 2019 20:06

efiop changed the title ~~[WIP] Initial pkg implementation~~ Initial pkg implementation May 28, 2019

efiop changed the title ~~Initial pkg implementation~~ [WIP] Initial pkg implementation May 28, 2019

efiop force-pushed the pkgs branch 5 times, most recently from 02733a9 to 15b4b97 Compare May 29, 2019 02:10

jorgeorpinel mentioned this pull request May 29, 2019

document dvc import and dvc get (previously pkg commands) iterative/dvc.org#385

Closed

efiop force-pushed the pkgs branch from 15b4b97 to 2293f5a Compare May 29, 2019 23:59

efiop changed the title ~~[WIP] Initial pkg implementation~~ Initial pkg implementation May 30, 2019

efiop changed the title ~~Initial pkg implementation~~ [WIP] Initial pkg implementation May 30, 2019

efiop force-pushed the pkgs branch 2 times, most recently from 1a8e7dc to 59e89bd Compare May 30, 2019 01:34

efiop force-pushed the pkgs branch 3 times, most recently from b7fd9cd to cf57131 Compare June 14, 2019 10:48

shcheklein reviewed Jun 15, 2019

View reviewed changes

dvc/pkg.py Show resolved Hide resolved

shcheklein reviewed Jun 15, 2019

View reviewed changes

shcheklein reviewed Jun 16, 2019

View reviewed changes

shcheklein requested changes Jun 16, 2019

View reviewed changes

efiop force-pushed the pkgs branch 2 times, most recently from 8b0e69e to 1a8594d Compare June 17, 2019 13:07

efiop added 2 commits June 17, 2019 18:12

dvc: initial 'dvc pkg' implementation

5941ace

Signed-off-by: Ruslan Kuprieiev <[email protected]>

pkg: implement dvc pkg get command

914a587

Signed-off-by: Ruslan Kuprieiev <[email protected]>

efiop force-pushed the pkgs branch from 1a8594d to 914a587 Compare June 17, 2019 15:12

efiop requested review from shcheklein and pared June 17, 2019 16:49

This was referenced Jun 17, 2019

refactor Output/Dependency/Remote to use factory-like methods #2138

Closed

import: add a mechanism to lock external dependencies #2139

Closed

import: support importing the whole repo #2140

Closed

efiop changed the title ~~[WIP] Initial pkg implementation~~ Initial pkg implementation Jun 17, 2019

shcheklein approved these changes Jun 17, 2019

View reviewed changes

efiop merged commit 573c84e into iterative:master Jun 17, 2019

efiop deleted the pkgs branch June 17, 2019 17:21

This was referenced Jun 17, 2019

pkg: deploy model or dataset #1768

Closed

test: pkg: add func test #1772

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial pkg implementation #2012

Initial pkg implementation #2012

efiop commented May 16, 2019 •

edited

Loading

Suor commented May 27, 2019

efiop commented May 28, 2019 •

edited

Loading

shcheklein Jun 15, 2019

efiop Jun 17, 2019

shcheklein Jun 15, 2019

efiop Jun 16, 2019

efiop Jun 17, 2019

shcheklein Jun 15, 2019

efiop Jun 17, 2019

shcheklein Jun 17, 2019

efiop Jun 17, 2019

shcheklein Jun 15, 2019

efiop Jun 16, 2019

efiop Jun 17, 2019

shcheklein Jun 15, 2019

efiop Jun 16, 2019

shcheklein Jun 17, 2019

efiop Jun 17, 2019

shcheklein Jun 16, 2019

efiop Jun 16, 2019 •

edited

Loading

efiop Jun 17, 2019

shcheklein left a comment

Initial pkg implementation #2012

Initial pkg implementation #2012

Conversation

efiop commented May 16, 2019 • edited Loading

Suor commented May 27, 2019

efiop commented May 28, 2019 • edited Loading

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

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop Jun 16, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein left a comment

Choose a reason for hiding this comment

efiop commented May 16, 2019 •

edited

Loading

efiop commented May 28, 2019 •

edited

Loading

efiop Jun 16, 2019 •

edited

Loading