Implement straight-to-remote functionality #5198

isidentical · 2021-01-04T11:53:43Z

Part of #4520

❗ 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: Add docs regaring --to-remote option for add/import-url dvc.org#2091

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

Internal Tracker

If the provider supports it, show the total size in the progress bar (maybe with another patch, since this PR is too big already)

efiop · 2021-01-12T08:20:19Z

For the record: while we are reviewing the technical part, let's start creating docs for this, to make sure we are on the same page in terms of CLI semantics.

dvc/tree/base.py

dvc/repo/imp_url.py

dvc/command/add.py

jorgeorpinel · 2021-01-13T05:11:46Z

dvc/command/add.py

+        "--to-remote",
+        action="store_true",
+        default=False,
+        help="Download it directly to the remote",


How about something like

Suggested change

help="Download it directly to the remote",

help="Track an external target, but don't move it into the "

"workspace, nor cache it. Push it to remote storage "

"instead.",

May need formatting

jorgeorpinel · 2021-01-13T05:29:44Z

dvc/command/add.py

+    parser.add_argument(
+        "-r",
+        "--remote",
+        help="Remote storage to download to",


I think this should definitely be included in --to-remote (let it accept a remote name as argument) to simplify the UI. add --to-remote --remote looks fort of repetitive anyway.

We actually thought about it and decided for the sake of consistency with other commands who works on the remote, it would better to just split it out.

also, in most cases it'll be just --to-remote, right?

also, can an option in argparse be made to optionally accept a value - in this case we would have to make --to-remote [remote-name] where remote-name is optional (default remote if not specified)

also, in most cases it'll be just --to-remote, right?

yes

also, can an option in argparse be made to optionally accept a value - in this case we would have to make --to-remote [remote-name] where remote-name is optional (default remote if not specified)

yes, the implementation is possible.

yes, the implementation is possible.

then, I guess, I don't have a strong preference I think. --remote is good for consistency, but I would not say it looks critical to me, --to-remote already reminds it a lot.

also, in most cases it'll be just --to-remote, right?

I'm not sure that most users employ default remotes. Do we know this?

I see the consistency argument but it's not that big a deal, and forcing the exact flag name causes other issues e.g. just add --remote is meaningless (and forbidden, I think, or it it silently ignored?) + its an extra flag to doc/explain and maintain 😋

I'm not sure that most users employ default remotes. Do we know this?

that's why we have default remotes at all. if we are not sure about this we should reconsider the whole logic we work with remotes then

add --remote is meaningless

agreed, I would not do this. may be keep only --to-remote <optional-name>

dvc/repo/add.py

…om_infos

dvc/command/add.py

jorgeorpinel · 2021-01-17T22:30:35Z

Oops I meant to resolve my #5198 (review) above and accidentally I clicked on "Ready for review". Unfortunately you can't go back to draft status... Sorry!

isidentical · 2021-01-18T07:07:07Z

No worries, eventually I was going to also do that to get @efiop 's reviews!

efiop

Great stuff! 🔥

Work from this PR is needed for other PRs (e.g. overlapping with vdir functionality and even erepo refactor), so merging this as-is for now. Let's address the CLI and other leftovers on top.

efiop · 2021-01-19T10:58:26Z

dvc/cache/base.py

+        local_tree = self.repo.cache.local.tree
+        local_info = local_tree.path_info / tmp_fname()
+
+        from_tree.download(from_info, local_info)


This might not fit on the local filesystem 🙁 We could download a chunk and upload a chunk though and then assemble them. Or could just forbid transfering files as a whole for now.

jorgeorpinel · 2021-01-23T03:02:21Z

Working on iterative/dvc.org/pull/2091, I'm assuming the concerns left above will be ~~addressed~~considered in a follow-up PR. Specifically:

Review --to-remote option desc: Implement straight-to-remote functionality #5198 (review)

See also more 1.x updates... (import) dvc.org#2094 (review)
Combine --to-remote + --remote: Implement straight-to-remote functionality #5198 (comment)
Don't download (entire) files locally: Implement straight-to-remote functionality #5198 (review) just above
Other Qs: Implement straight-to-remote functionality #5198 (review)

jorgeorpinel · 2021-02-05T21:31:58Z

Another thing to consider (BTW I meant considered/discussed, not necessarily addressed as in implemented):

Similar to combining --to-remote and --remote for simplicity/usability, should --external (in add) be implied with this? (applies to --to-cache too) As of now add --to-remote /ext/path fails if I don't also add --external:

Adding...
ERROR: Output(s) outside of DVC project: /ext/path. See <https://dvc.org/doc/user-guide/managing-external-data> for more info.

Cc @dberenbaum

jorgeorpinel · 2021-02-05T21:33:26Z

p.s. on that error message (unrelated to this issue, sorry) should it give a HINT to retry with --external?

efiop · 2021-02-05T22:51:37Z

@jorgeorpinel Good catch with --external! @isidentical Could you please adjust that? 🙏

isidentical · 2021-02-08T07:11:30Z

I believe this already works with straigh-to-cache when the given output is within the repo, dvc add /some/stuff -o stuff. Though for the case of dvc add --to-remote, wouldn't it make sense to keep it with explicit --external since the actions related to that file in the future would affect paths outside of the project?

jorgeorpinel · 2021-02-09T03:59:41Z

I haven't tested --to-cache yet so not sure about the first part of your comment for now, @isidentical.

And sorry, I didn't understand the relationship between requiring explicit --external with --to-remote to the fact that the data is external (affects external paths). Conceptually though, I don't think we consider consider it "outside of the project" once it's added. In fact we call it an "extended workspace" i.e. DVC "owns" that external location (they become links to the cache and no other system should try to control the same location).

Anyway, my main rationale is that saving users from unnecessarily typing a flag that is always required is good for the UX (similar as with merging --to-remote and --remote).

isidentical · 2021-02-09T17:28:06Z

Would you create a ticket for the things in your mind (--external, --to-remote + --remote etc) @jorgeorpinel, it is hard to debate these stuff under a merged PR.

jorgeorpinel · 2021-02-10T18:33:00Z

Yeah let me do that because actually I think I'm still confused here 😅

isidentical changed the title ~~Export to remote~~ [WIP]: Implement straight-to-remote functionality Jan 4, 2021

isidentical force-pushed the export-to-remote branch 4 times, most recently from 8ab4626 to 7a89af6 Compare January 11, 2021 13:22

efiop reviewed Jan 12, 2021

View reviewed changes

dvc/tree/base.py Show resolved Hide resolved

efiop reviewed Jan 12, 2021

View reviewed changes

dvc/repo/imp_url.py Outdated Show resolved Hide resolved

isidentical mentioned this pull request Jan 12, 2021

Add docs regaring --to-remote option for add/import-url iterative/dvc.org#2091

Merged

jorgeorpinel reviewed Jan 13, 2021

View reviewed changes

dvc/command/add.py Outdated Show resolved Hide resolved

jorgeorpinel reviewed Jan 13, 2021

View reviewed changes

dvc/repo/add.py Show resolved Hide resolved

jorgeorpinel reviewed Jan 13, 2021

View reviewed changes

dvc/repo/add.py Show resolved Hide resolved

jorgeorpinel mentioned this pull request Jan 13, 2021

support adding/transfering data straight to cache/remote #4520

Closed

isidentical force-pushed the export-to-remote branch from 381c34e to 67bfc45 Compare January 14, 2021 07:14

isidentical mentioned this pull request Jan 14, 2021

get[-url]/import-url: jobs option? #5267

Closed

isidentical added 13 commits January 15, 2021 13:11

[WIP] Implement export-to-remote functionality

c57e74f

Redistrubition of functionality from tree to remote

64a3f48

Pass chunk size into the open_url

e081a45

Move functionality away from remote to cache

bd331c9

Basic progress bars for both transfer types

faf4016

Proper job calculation

9cbce0c

Redesign .open() usage in order to work with different trees

c590729

Align the signatures of other open() functions to accept **kwargs

69c5acc

Implement chunked uploading to the ssh

49c01f3

Use local cache to compute directory hash (md5)

3af38da

S3: multipart upload test

5d89eaf

Wrap local cache's hash_to_path with PathInfo

457f1e8

Use .isdir() instead of a try/except statement for calculating the fr…

0cef7a9

…om_infos

isidentical added 7 commits January 15, 2021 13:13

Inline transfer operation to .add() / .transfer()

313ef1a

Open in the 'rb' mode for the tests

41d0f1a

More tests, better invalid option handling at repo level

4fb69b3

.read() certain amount of bytes to validate the content

4f05fc0

Better invalid argument handling

9bd778c

Better progress bars by using .getsize() API

6bc2b2e

For fallback upload, use the from_info.name instead of temporary naem

d9e5385

isidentical force-pushed the export-to-remote branch from c2ee258 to d9e5385 Compare January 15, 2021 11:13

introduce --jobs for add/import-url

06e7035

jorgeorpinel reviewed Jan 15, 2021

View reviewed changes

dvc/command/add.py Show resolved Hide resolved

jorgeorpinel marked this pull request as ready for review January 17, 2021 22:18

jorgeorpinel mentioned this pull request Jan 19, 2021

cli: add: add an --output argument #5284

Closed

2 tasks

efiop changed the title ~~[WIP]: Implement straight-to-remote functionality~~ Implement straight-to-remote functionality Jan 19, 2021

efiop approved these changes Jan 19, 2021

View reviewed changes

efiop merged commit 51ada6d into iterative:master Jan 19, 2021

efiop reviewed Jan 19, 2021

View reviewed changes

isidentical mentioned this pull request Jan 28, 2021

add: implement saving straight to cache #5301

Merged

2 tasks

jorgeorpinel mentioned this pull request Feb 10, 2021

add: --to-remote needed? OR --external needed? #5445

Closed

2 tasks

This was referenced Mar 28, 2021

add/import-url: merge --to-remote and --remote #5719

Closed

ui: merge --to-remote and --remote for add and import-url #5721

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement straight-to-remote functionality #5198

Implement straight-to-remote functionality #5198

isidentical commented Jan 4, 2021 •

edited by efiop

Loading

efiop commented Jan 12, 2021

jorgeorpinel Jan 13, 2021 •

edited

Loading

This comment was marked as off-topic.

jorgeorpinel Jan 13, 2021

isidentical Jan 13, 2021 •

edited

Loading

shcheklein Jan 13, 2021

isidentical Jan 13, 2021

shcheklein Jan 13, 2021

jorgeorpinel Jan 13, 2021 •

edited

Loading

shcheklein Jan 13, 2021

jorgeorpinel commented Jan 17, 2021 •

edited

Loading

isidentical commented Jan 18, 2021 •

edited by jorgeorpinel

Loading

efiop left a comment

efiop Jan 19, 2021

jorgeorpinel commented Jan 23, 2021 •

edited

Loading

jorgeorpinel commented Feb 5, 2021 •

edited

Loading

jorgeorpinel commented Feb 5, 2021

efiop commented Feb 5, 2021

isidentical commented Feb 8, 2021

jorgeorpinel commented Feb 9, 2021

isidentical commented Feb 9, 2021

jorgeorpinel commented Feb 10, 2021

-        help="Download it directly to the remote",
+        help="Track an external target, but don't move it into the "
+             "workspace, nor cache it. Push it to remote storage "
+             "instead.",

Implement straight-to-remote functionality #5198

Implement straight-to-remote functionality #5198

Conversation

isidentical commented Jan 4, 2021 • edited by efiop Loading

efiop commented Jan 12, 2021

jorgeorpinel Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

This comment was marked as off-topic.

jorgeorpinel Jan 13, 2021

Choose a reason for hiding this comment

isidentical Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

shcheklein Jan 13, 2021

Choose a reason for hiding this comment

isidentical Jan 13, 2021

Choose a reason for hiding this comment

shcheklein Jan 13, 2021

Choose a reason for hiding this comment

jorgeorpinel Jan 13, 2021 • edited Loading

Choose a reason for hiding this comment

shcheklein Jan 13, 2021

Choose a reason for hiding this comment

jorgeorpinel commented Jan 17, 2021 • edited Loading

isidentical commented Jan 18, 2021 • edited by jorgeorpinel Loading

efiop left a comment

Choose a reason for hiding this comment

efiop Jan 19, 2021

Choose a reason for hiding this comment

jorgeorpinel commented Jan 23, 2021 • edited Loading

jorgeorpinel commented Feb 5, 2021 • edited Loading

jorgeorpinel commented Feb 5, 2021

efiop commented Feb 5, 2021

isidentical commented Feb 8, 2021

jorgeorpinel commented Feb 9, 2021

isidentical commented Feb 9, 2021

jorgeorpinel commented Feb 10, 2021

isidentical commented Jan 4, 2021 •

edited by efiop

Loading

jorgeorpinel Jan 13, 2021 •

edited

Loading

isidentical Jan 13, 2021 •

edited

Loading

jorgeorpinel Jan 13, 2021 •

edited

Loading

jorgeorpinel commented Jan 17, 2021 •

edited

Loading

isidentical commented Jan 18, 2021 •

edited by jorgeorpinel

Loading

jorgeorpinel commented Jan 23, 2021 •

edited

Loading

jorgeorpinel commented Feb 5, 2021 •

edited

Loading