Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add docs regaring --to-remote option for add/import-url #2091

Merged
merged 40 commits into from
Feb 9, 2021

Conversation

isidentical
Copy link
Contributor

@shcheklein shcheklein temporarily deployed to dvc-landing-straight-to-i3tkaz January 12, 2021 11:47 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-straight-to-i3tkaz January 12, 2021 11:58 Inactive
@jorgeorpinel

This comment has been minimized.

@shcheklein shcheklein temporarily deployed to dvc-landing-straight-to-i3tkaz January 14, 2021 08:03 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-straight-to-i3tkaz January 14, 2021 08:16 Inactive
@@ -1,8 +1,9 @@
# import-url

Download a file or directory from a supported URL (for example `s3://`,
`ssh://`, and other protocols) into the <abbr>workspace</abbr>, and track it (an
import `.dvc` file is created).
`ssh://`, and other protocols) into the <abbr>workspace</abbr> (or to the
Copy link
Member

Choose a reason for hiding this comment

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

I think we need to work on this a bit more ... probably get rid of Download?

brackets look too complicated for the very beginning of the doc

cc @jorgeorpinel

This comment was marked as resolved.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jan 15, 2021

Choose a reason for hiding this comment

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

But OK, if we completely rewrite this anyway, we can address the parenthesis situation along the way.

  • Maybe something like

Track a file or directory found in an external location (`s3://`, `/local/path`, etc.), and download it to the local project, or make a copy in [remote storage](/doc/command-reference/remote).

Copy link
Contributor

@jorgeorpinel jorgeorpinel Jan 15, 2021

Choose a reason for hiding this comment

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

Please notice similar suggestion (for add --to-remote) in iterative/dvc#5198 (review) @isidentical:

Track an external target, but don't move it into the workspace, nor cache it. Transfer it to remote storage instead.

@shcheklein shcheklein temporarily deployed to dvc-landing-straight-to-i3tkaz January 15, 2021 08:33 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-straight-to-i3tkaz January 15, 2021 11:13 Inactive
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Left a detailed review on add for now (some can be applied to import-url too, probably). Getting there, but this change is tricky and may result larger than it seemed at first.

I left specific, mergeable suggestions to make it easier but at some point we can take this over (after merged in core and most discussions are resolved, hopefully). Thanks @isidentical

@shcheklein shcheklein temporarily deployed to dvc-landing-straight-to-i3tkaz January 18, 2021 07:24 Inactive
@shcheklein shcheklein temporarily deployed to dvc-landing-straight-to-i3tkaz January 18, 2021 07:31 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-straight-to-i3tkaz January 19, 2021 09:17 Inactive
Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Almost finished. A few details left for the import-url Example, and then copy/adapt it over to the add ref. That should do it. Thanks!

Note that I committed most of my recent suggestions, please see the ones that are still unresolved directly on GH.

usage: dvc get-url [-h] [-q | -v] [-j <number>] url [out]
usage: dvc get-url [-h] [-q | -v] url [out]
Copy link
Contributor

@jorgeorpinel jorgeorpinel Feb 9, 2021

Choose a reason for hiding this comment

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

Oops @isidentical I'm seeing lots of -j related changes here. Maybe this got contaminated from another one of your docs branches?

Copy link
Contributor

Choose a reason for hiding this comment

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

This file (content/docs/command-reference/get-url.md), content/docs/command-reference/get.md, and content/docs/command-reference/import.md to be precise.

Copy link
Contributor

@jorgeorpinel jorgeorpinel left a comment

Choose a reason for hiding this comment

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

Thanks @isidentical, I've finished dogin copy edits to the import-url example. One last thing pending (for consistency), suggested by Ivan earlier I believe:

Comment on lines 42 to 46
When you don't want to store the target data in your local system, you can still
create an import `.dvc` file while transferring a file or directory directly to
remote storage, by using the `--to-remote` option. See the
[Import straight to remote](#example-transfer-to-remote-storage) example for
more details.
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we put something like this in the add reference Description (instead of the Transferring data directly to remote storage section) ?

Comment on lines +361 to +365
## Example: Transfer to remote storage

When you have a large dataset in an external location, you may want to import it
to you project without downloading it to the local file system (for using it
later/elsewhere). The `--to-remote` option lets you skip the download, while
Copy link
Contributor

Choose a reason for hiding this comment

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

And copy over the Example as well (will need some some adapting). Thanks

@shcheklein
Copy link
Member

@jorgeorpinel let's merge it and create a ticket to address the issue left. Probably simplify description, use an example instead, move info organically into Description - e.g. as a hint.

@jorgeorpinel
Copy link
Contributor

jorgeorpinel commented Feb 9, 2021

Sounds good. Thanks again @isidentical

@jorgeorpinel
Copy link
Contributor

Oops wait. We merged the --jobs changes that shouldn't be in this PR (see #2091 (review)). Should we revert the merge or is that change also needed (for some other PR)?

@jorgeorpinel
Copy link
Contributor

BTW extracted the pending #2091 (review) to #2161.

@shcheklein shcheklein deleted the straight-to-remote branch February 28, 2021 19:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants