dvc: replace pkg concept with external repo #2160

efiop · 2019-06-19T23:55:39Z

dvc import is now renamed to dvc import-url. dvc pkg import renamed into dvc import and dvc pkg get into dvc get. Also, for symmetry, introduced dvc get-url. Along with some other minor modifications. The workflow now looks like:

# downloads and creates dvc file
dvc import https://github.com/iterative/dvc path/data --rev 0.41.3

# only downloads data
dvc get https://github.com/iterative/dvc path/data --rev 0.41.3

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

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.

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

shcheklein · 2019-06-21T08:05:22Z

dvc/external_repo.py

+            return
+
+        try:
+            os.makedirs(self.repos_dir)


why not compat makedirs?

shcheklein · 2019-06-21T08:06:14Z

dvc/external_repo.py

+        #
+        # Note that tempfile.TemporaryDirectory is using symlinks to tmpfs, so
+        # we won't be able to use move properly.
+        tmp_dir = os.path.join(self.repos_dir, "." + str(uuid.uuid4()))


is it possible to use pathlib?

Not really, our Repo and other stuff are not meant to be used with it.

dvc/external_repo.py

shcheklein · 2019-06-21T08:45:09Z

dvc/external_repo.py

+            cache_config.set_dir(cache_dir, level=Config.LEVEL_LOCAL)
+            repo.scm.git.close()
+
+        if self.installed:


should be probably part of the def install

shcheklein · 2019-06-21T08:47:15Z

dvc/command/get_url.py

+        formatter_class=argparse.RawDescriptionHelpFormatter,
+    )
+    get_parser.add_argument(
+        "url", help="See `dvc import-url -h` for full list of supported URLs."


import-url -> get-url

I see, it's intentional, right?

dvc/command/get_url.py

shcheklein · 2019-06-21T08:55:11Z

dvc/repo/get_url.py

+        src = os.path.abspath(url)
+        out = os.path.abspath(out)
+
+    dep, = dependency.loads_from(None, [src])


will it support remote:// protocol? it needs to read a global config for that, right?

It won't, at least for now. get-url doesn't care about Repo's at all. I would keep it this way untill someone asks for this feature.

shcheklein · 2019-06-21T09:01:14Z

dvc/repo/get_url.py

+    out = out or os.path.basename(urlparse(url).path)
+
+    if os.path.exists(url):
+        src = os.path.abspath(url)


src is undefined if not os.path.exists(url) and is referenced below

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

dvc: address review from #2160

ghost · 2019-06-25T20:42:05Z

what about backwards compatibility, @efiop ? dvc import -> dvc import-url, if this is already released, should we let know the users somehow?

efiop · 2019-06-25T21:26:49Z

@MrOutis We do let them know with a warning in dvc/command/imp.py.

ghost · 2019-06-25T23:01:55Z

True, @efiop 👍

guysmoilov · 2019-06-26T17:03:13Z

Hey guys, I just tripped all over this when running our tutorial at https://dagshub.com/docs/pipeline/
in preparation for the workshop on DVC we're planning to do on Sunday.
I guess I'm out of date here, but why rename import to import-url instead of giving the new command the new name?
Also, the docs in the main site are out of date: https://dvc.org/doc/commands-reference/import

guysmoilov · 2019-06-26T17:04:46Z

I would also consider people who may have written shell scripts with dvc import, a warning message would not really be relaxing for them...

efiop · 2019-06-26T17:20:39Z

@guysmoilov Thanks for the feedback! We've decided to rename it so that a more powerful repo-import feature would have a better name(we've also introduced new dvc get command for downloading from repos, and it felt weird having it in pair with repo-import or import-repo, so we've decided to sacrifice import-url). Sorry for the inconvenience. We've added a warning and bumped the version to something significant(not a major version though 🙁) to help somewhat mitigate that, but it was clearly not enough.

As to docs, we are working on it right now. We've decided to do a release anyway, because there were a lot of important features/bugfixes stacked up already. Also we felt like we were ready to collect some feedback about new features.

ghost · 2019-06-26T17:22:23Z

Agree, @guysmoilov ! @efiop , maybe writing something in the #announcements channel on Discord?

ghost · 2019-06-26T17:23:53Z

@shcheklein just did it 😅

shcheklein · 2019-06-26T17:28:14Z

@MrOutis @efiop I have posted an announcement. @guysmoilov sorry for the confusion and inconvenience :( It was not an easy decision.I really hope that you will try the new set of commands that are in a preview mode - import and get. I think they alone can create lot of value to users and enable really great use cases around them.

As for the documentation update - it's in progress. Import will be renamed in a few hours.

jorgeorpinel · 2019-06-29T01:00:06Z

Probably too late but I agree with Guy. Reusing a previous command name for a similar feature as before seems really confusing and unnecessary. There's an endless number of other words that could've been used. Anyway, probably not the end of the world and I'm sure there was a deep discussion about it and good reasons to do it so, moving on... Working on the full doc updates in PR iterative/dvc.org#464

efiop force-pushed the repo branch 3 times, most recently from 1951506 to 278b9f7 Compare June 20, 2019 16:58

efiop changed the title ~~[WIP] dvc: replace pkg concept with external repo~~ dvc: replace pkg concept with external repo Jun 20, 2019

efiop force-pushed the repo branch 5 times, most recently from 628b3e4 to f675e72 Compare June 20, 2019 22:07

dvc: replace pkg concept with external repo

1ba8e01

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

efiop force-pushed the repo branch from f675e72 to 1ba8e01 Compare June 20, 2019 22:16

shcheklein requested a review from Suor June 21, 2019 02:20

efiop merged commit 95096e5 into iterative:master Jun 21, 2019

efiop deleted the repo branch June 21, 2019 07:36

shcheklein reviewed Jun 21, 2019

View reviewed changes

dvc/external_repo.py

return

try:

os.makedirs(self.repos_dir)

Copy link

Member

shcheklein Jun 21, 2019

Choose a reason for hiding this comment

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

why not compat makedirs?

efiop reacted with thumbs up emoji

shcheklein reviewed Jun 21, 2019

View reviewed changes

dvc/external_repo.py Show resolved Hide resolved

shcheklein reviewed Jun 21, 2019

View reviewed changes

dvc/command/get_url.py Show resolved Hide resolved

shcheklein reviewed Jun 21, 2019

View reviewed changes

efiop added a commit to efiop/dvc that referenced this pull request Jun 21, 2019

dvc: address review from iterative#2160

d795cdf

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

efiop added a commit that referenced this pull request Jun 21, 2019

Merge pull request #2163 from efiop/repo

074ea61

dvc: address review from #2160

efiop mentioned this pull request Jun 21, 2019

WIP: dvc pkg cmd ref (and other misc updates) iterative/dvc.org#417

Closed

efiop added the enhancement Enhances DVC label Jun 21, 2019

shcheklein mentioned this pull request Jun 21, 2019

import: add a mechanism to lock external dependencies #2139

Closed

efiop mentioned this pull request Jun 27, 2019

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

Closed

jorgeorpinel mentioned this pull request Jul 1, 2019

Write import, get, and get-url refs; std. existing refs; other doc format/lang. updates iterative/dvc.org#464

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dvc: replace pkg concept with external repo #2160

dvc: replace pkg concept with external repo #2160

efiop commented Jun 19, 2019 •

edited

Loading

shcheklein Jun 21, 2019

shcheklein Jun 21, 2019

efiop Jun 21, 2019

shcheklein Jun 21, 2019

shcheklein Jun 21, 2019

shcheklein Jun 21, 2019

efiop Jun 21, 2019

shcheklein Jun 21, 2019

efiop Jun 21, 2019

shcheklein Jun 21, 2019

ghost commented Jun 25, 2019

efiop commented Jun 25, 2019

ghost commented Jun 25, 2019

guysmoilov commented Jun 26, 2019

guysmoilov commented Jun 26, 2019

efiop commented Jun 26, 2019 •

edited

Loading

ghost commented Jun 26, 2019

ghost commented Jun 26, 2019

shcheklein commented Jun 26, 2019

jorgeorpinel commented Jun 29, 2019 •

edited

Loading

dvc: replace pkg concept with external repo #2160

dvc: replace pkg concept with external repo #2160

Conversation

efiop commented Jun 19, 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

ghost commented Jun 25, 2019

efiop commented Jun 25, 2019

ghost commented Jun 25, 2019

guysmoilov commented Jun 26, 2019

guysmoilov commented Jun 26, 2019

efiop commented Jun 26, 2019 • edited Loading

ghost commented Jun 26, 2019

ghost commented Jun 26, 2019

shcheklein commented Jun 26, 2019

jorgeorpinel commented Jun 29, 2019 • edited Loading

efiop commented Jun 19, 2019 •

edited

Loading

efiop commented Jun 26, 2019 •

edited

Loading

jorgeorpinel commented Jun 29, 2019 •

edited

Loading