Initial pkg implementation by efiop · Pull Request #2012 · treeverse/dvc

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)

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

?

efiop · 2019-06-06T11:52:36Z

@Suor I'm not sure how you want to handle them as the same class. They are different instances, with different fields, not sure how you want to get rid of both in favor of one. The current patch at least uses factories instead of junky def Output/Dependency functions and generalizes a lot of stuff. I'll move that logic from this PR into a separate PR, so we could discuss that there.

efiop · 2019-06-06T13:11:38Z

Moved config-related stuff to #2098 . The factory refactorings will come separately.

Suor · 2019-06-06T14:48:12Z

@efiop I just want hordes of code to be moved to mirror files only for really good reason.

efiop · 2019-06-06T15:09:47Z

@Suor Unless something went wrong there, moving shouldn't have deleted the history. Like git mv doesn't, even if you introduce changes on top. Inheritance logic from this PR from dependency to output makes more sense than the one that we currently have in master. Maybe you have some specific way to reorganize those on your mind? Because it seems like I don't quite follow.

Suor · 2019-06-07T02:40:19Z

@efiop why output subslassing dependencies is better? It's the same but the other way around. So just moving code for no reason. Even if that has some sense, which I don't get, it is still not worth the hassle as code itself is not improving, i.e. won't be easier to support in the future, and making it abstractly correct in whatever sense is low value.

efiop · 2019-06-11T13:58:50Z

@Suor Sorry, forgot to reply. The refactoring does make the code better, since it removes weird if self.is_dependency calls from outputs. Let's discuss it in more details in the refactoring PRs.

In the meantime, I've pushed MVP that is detached from all the previous refactorings (which are going to be submitted separately). Todo:

don't push pkg imported data to remote
add .dvc/pkg-list.yml similar to https://github.com/sindresorhus/package-json
add .dvc/pkg-lock.yml similar to https://flaviocopes.com/package-lock-json/ (install should add an entry to it)
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.
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)

So currently there are three commands: dvc pkg insta/uninstall/import.

pared

Shouldn't dvc/repo/_ignore() flist include self.pkg.pkg_dir ?

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

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

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

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

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.

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

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

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 <ruslan@iterative.ai>

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

Comment thread dvc/repo/pkg/__init__.py Outdated

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) treeverse/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 2 times, most recently from 4473fee to 910f9fd Compare June 11, 2019 13:45

shcheklein reviewed Jun 13, 2019

View reviewed changes

Comment thread dvc/command/pkg.py Outdated

shcheklein reviewed Jun 13, 2019

View reviewed changes

Comment thread dvc/stage.py Outdated

shcheklein reviewed Jun 13, 2019

View reviewed changes

Comment thread dvc/pkg.py Outdated

shcheklein reviewed Jun 13, 2019

View reviewed changes

Comment thread dvc/pkg.py Outdated

pared suggested changes Jun 13, 2019

View reviewed changes

Comment thread tests/func/test_pkg.py Outdated

Comment thread dvc/pkg.py Outdated

Comment thread tests/func/test_pkg.py Outdated

efiop mentioned this pull request Jun 13, 2019

dvc: move Updater out of Repo() #2130

Merged

2 tasks

shcheklein reviewed Jun 15, 2019

View reviewed changes

Comment thread dvc/pkg.py Outdated

shcheklein reviewed Jun 15, 2019

View reviewed changes

shcheklein reviewed Jun 16, 2019

View reviewed changes

shcheklein suggested changes Jun 16, 2019

View reviewed changes

Ruslan Kuprieiev added 2 commits June 17, 2019 18:12

dvc: initial 'dvc pkg' implementation

5941ace

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>

pkg: implement dvc pkg get command

914a587

Signed-off-by: Ruslan Kuprieiev <ruslan@iterative.ai>

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

shcheklein approved these changes Jun 17, 2019

View reviewed changes

This was referenced Jun 17, 2019

pkg: deploy model or dataset #1768

Closed

test: pkg: add func test #1772

Closed

Conversation

efiop commented May 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Suor commented May 27, 2019

Uh oh!

efiop commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efiop commented Jun 6, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

efiop commented Jun 6, 2019

Uh oh!

Suor commented Jun 6, 2019

Uh oh!

efiop commented Jun 6, 2019

Uh oh!

Suor commented Jun 7, 2019

Uh oh!

efiop commented Jun 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

pared left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

efiop Jun 16, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

shcheklein left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

efiop commented May 16, 2019 •

edited

Loading

efiop commented May 28, 2019 •

edited

Loading

efiop commented Jun 6, 2019 •

edited

Loading

efiop commented Jun 11, 2019 •

edited

Loading

efiop Jun 16, 2019 •

edited

Loading