-
Notifications
You must be signed in to change notification settings - Fork 394
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
import-url/update: add --no-download flag #3773
Conversation
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.
Let's reorg/rewarp this under 69 cols (to avoid horizontal scrolling) and to match the order in the Options section. Committing:
finish the operation(s)); or if the target data already exist locally and you | ||
want to "DVCfy" this state of the project (see also `dvc commit`). |
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 directly related to the change in question but the description of --no-exec
could be better. For starters, maybe say something like "create a value-less import .dvc file"? (for contrast with --no-download
which includes *some* values)
This last sentence (seen above) especially is pretty hard to understand. What is "to DVCfy"? 🤔
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.
Also instead of "download everything later" it should be "import the data later" now, I think. Since there's a separate --no-download
option now (so let's only use term download there). BUT it could state "implies --no-download
".
p.s. this could be done in a separate PR but I think it's related enough to this change to address now.
p.p.s. should also apply to dvc import --no-exec
.
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 would get rid of the second part ("DVCfy..."). It's not clear what it's supposed to do, and the command will fail if a file with the same of what is being imported already exists locally.
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.
Suggestion on actual change
Hey @dtrifiro another note for future reference: please open PRs to this repo directly on the upstream (no fork). See https://github.com/iterative/dvc.org/wiki/2.-Pull-requests-for-core-DVC-changes for context. Thanks! |
@dtrifiro Do you think it's realistic to get this drafted, reviewed, and merged this sprint? |
7e0d255
to
904c474
Compare
cd68df1
to
d146a87
Compare
d146a87
to
7faae7c
Compare
In case @jorgeorpinel didn't already mention it, please open future docs PRs from this repo instead of a fork. That way, the changes will be deployed into an example app when you open the PR and it's easy for people to see what the final docs will look like. 🙏 No need to bother for 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.
@dtrifiro LGTM except for 2 minor comments.
@jorgeorpinel Do you want to take another look or leave it to me to approve/merge?
7faae7c
to
6061d63
Compare
@jorgeorpinel I merged, but feel free to leave comments if you have them |
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.
feel free to leave comments
A few, minor:
- `--no-exec` - create the import `.dvc` file but don't download `url` or get | ||
checksums (assumes that the data source is valid). This is useful if you need |
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.
checksums - no context for this term/idea. Not mentioned anywhere in the Description. Not even "hashes" are mentioned.
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.
Idea: could at least l'ink to https://dvc.org/doc/user-guide/project-structure/dvc-files#output-entries. And I'd prefer the term "file hash" but up to you.
to define the project imports quickly, and import the data later (use | ||
`dvc update` to finish the operation(s)). | ||
|
||
- `--no-download` - create the import `.dvc` with data checksums but without |
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 .dvc
file
- `--no-exec` - create the import `.dvc` file but don't download `url` (assumes | ||
that the data source is valid). This is useful if you need to define the |
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.
Checksums not mentioned here (inconsistent?)
usage: dvc import-url [-h] [-q | -v] [-j <number>] [--file <filename>] | ||
[--no-exec] [--to-remote] [-r <name>] | ||
[--no-exec | --no-download] [--to-remote] [-r <name>] | ||
[--desc <text>] | ||
url [out] |
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 is now too long when rendered. See https://dvc.org/doc/command-reference/import-url
💅🏼 but let's regroup the args? Same for the other 2 files.
- `--no-download` - create the import `.dvc` with data checksums but without | ||
downloading the associated data. This is useful if you need track changes in | ||
remote data but do not (yet) need to download data to the local workspace. | ||
Data can be later downloaded using `dvc pull`. |
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.
Should we mention update --no-download
in these?
with `-r`). Use `dvc pull` to get the data locally. | ||
- `--no-download` - Update data checksums in the `.dvc` file (`md5`, `etag`, or | ||
`checksum` fields) without actually downloading the latest data. See | ||
`dvc import-url --no-download`/`dvc import --no-download` for more context. |
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.
💅🏼
`dvc import-url --no-download`/`dvc import --no-download` for more context. | |
`dvc import-url --no-download` or `dvc import --no-download` for more context. |
Thanks for the feedback @jorgeorpinel, here's a new PR: main...import-url-no-download-fixes we can move the discussion there |
Add documentation for new
dvc import-url
dvc update
flags (--no-download
)related: iterative/dvc#8024 (per iterative/dvc#7918)