Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
ref: improve add/import* to-cache/remote info and examples #2302
ref: improve add/import* to-cache/remote info and examples #2302
Changes from 7 commits
3f1217e
bab95a9
69cbbb6
a1e609e
cd599c8
eb4af97
043db23
fa82c89
5729f49
c393212
b020b4e
86813ad
725aa92
f2350ab
14b62cc
d63b07f
94010d7
562b63c
f694719
d5e793e
0166b1f
696fa53
15097f1
31394de
0f2a2b1
3d10596
1082c85
b943df5
b25da5c
aa171d4
61f8806
d5f284a
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import
is not the right term hereThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would describe it as
when specified
dvc addcan accept path to an external data (e.g. S3 url, or path on a mounted volume, etc) and it copies data into remote storage w/o caching data into project's cache and w/o bringing it into the workspace. Besides that, it behaves in the same way - it creates a regular
.dvcfile, which can be used with
dvc pullto get data into workspace when and where it's needed. This options expects (?)
-oto be provided. It is useful to "bootstrap" the project, when data is too large for the disk the workspace is located on. See example ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch! Updated along those lines in d63b07f.
This seems like too many details for an option desc. It's already explained in the main command Description and I think it's assumed that everything still applies with every option except if otherwise noted.
Not required, it
should useuses./<basename>
by default (fromdvc add --to-remote /ext/path/basename
).Similarly, this idea is covered in the example (linked from here). Trying to keep the option desc. succinct.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
not related to this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did some general copy edits to the descriptions of all the commands related here (add, get, import, etc.) as I reviewed the -o/-to-remote stuff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it increases surfaces, please please let's try to avoid it as much as we can
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, will extract ⌛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, rolled back.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I wonder if we need a full to-cache example in
import-url
(like in https://dvc.org/doc/command-reference/add#example-transfer-to-the-cache).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import-url
doesn't have theto-cache
ability, but rather work on its own (like you just import an URL with it and it puts it to your cache, but not in a chunked way).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, good point. But then maybe we should explain about the chunking (transferring) a bit more... I'll review again ⌛
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But with
--to-cache
the chinked transfer does happen, right @isidentical ? Just double checking. Already updated in c393212, PTAL.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This was wrong right?
import-url
states the opposite.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import
andimport-url
have different behavior, the original docs for each command were correctThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interestinggggg 🤔 OK will roll back, thanks! ✔️
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't like
are more convenient.
but I guess we'll address it later. It's not about convenience to my mind. It's about different workflows around data. And we need to explain to people that in certain cases (unless you indeed write/read directly from/to remote location from code) you don't need this. If you are looking for a way to add data to DVC in regular way (remote storage, dvc pull, dvc push , whatever - find a way to define it ) there are better optionsThere was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. Changing to "better" for now.