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

Misc. updates #1978

Merged
merged 55 commits into from
Dec 21, 2020
Merged

Misc. updates #1978

merged 55 commits into from
Dec 21, 2020

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Nov 28, 2020

  • From my stash of changes done while, but unrelated to, recent PRs
  • Update commit and --no-commit options Extracted to 1989

Note for self:

@shcheklein shcheklein temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 November 28, 2020 21:54 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 November 28, 2020 21:55 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 November 28, 2020 21:57 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 November 28, 2020 21:58 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 November 28, 2020 23:09 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 November 28, 2020 23:36 Inactive
@jorgeorpinel jorgeorpinel marked this pull request as ready for review November 29, 2020 03:00
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 November 29, 2020 03:40 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 November 29, 2020 03:55 Inactive
Comment on lines 3 to 5
Get tracked files or directories from
[remote storage](/doc/command-reference/remote) into the <abbr>cache</abbr>.
Download <abbr>cache</abbr> files or directories from
[remote storage](/doc/command-reference/remote), based on the current `dvc.yaml`
and `.dvc` files.
Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Nov 29, 2020

Choose a reason for hiding this comment

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

per content/docs/command-reference/commit.md
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 November 30, 2020 02:41 Inactive
@jorgeorpinel jorgeorpinel temporarily deployed to dvc-landing-jorge-zct3dcllfnv7 December 6, 2020 19:55 Inactive
@jorgeorpinel
Copy link
Contributor Author

Oops I forgot about this PR! Addressing now...

import metadata is saved to the `.dvc` file. You can use `dvc commit` to
finish the operation.
- `--no-exec` - create the import `.dvc` file but don't download `url` (assumes
that the data source is valid). You can use `dvc commit` to finish the

This comment was marked as resolved.

This comment was marked as resolved.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Separately, I wonder why repro doesn't have --no-exec and why import* don't have --no-commit. Is it worth asking on the core repo?

This comment was marked as resolved.

@@ -25,8 +25,8 @@ next.
💡 For convenience, a pre-commit Git hook is available to remind you to
`dvc commit` when needed. See `dvc install` for more info.

Mainly, `dvc commit` provides a way to complete DVC commands that track data
(`dvc add`, `dvc repro`, `dvc import`, etc.), when they have been used with the
`dvc commit` provides a way to complete DVC commands that track data (`dvc add`,
Copy link
Member

Choose a reason for hiding this comment

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

Sometimes after executing a stage, we realize that not all of its dependencies
  or outputs are defined in `dvc.yaml`. It is possible to
  [add the missing deps/outs](/docs/user-guide/how-to/add-deps-or-outs-to-a-stage),
  and `dvc commit` may be needed to finalize the remedy (see link).

contradicts this. (and probably the previous version as well)

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Dec 19, 2020

Choose a reason for hiding this comment

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

I don't see the contradiction, but maybe the way this is written gives that impression? Here's what each use case means:

  1. Finish add/run/repro --no-commit/exec
  2. Cache missing outputs that you forgot to add to dvc.yaml before but now added (after executing the stage)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Clarified a little in ffff0a4 but it's not a big change.

Copy link
Member

Choose a reason for hiding this comment

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

Contradiction here is that you we state "Mainly" and then clarify "Specifically", but point is completely different from the one in the "Mainly" section. To my mind either introductory section should be completely removed, or generalized if we keep it this way. wdyt?

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Dec 20, 2020

Choose a reason for hiding this comment

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

Just to clarify, the the "Mainly" word was deleted in the lines above 🙂

either introductory section should be completely removed, or generalized

The intro is "Stores the current contents..." which is already a general explanation. That p ends in "We explore the scenarios next" and the desc goes on to the main scenario ("provides a way to complete add/repro --no-commit etc."). Then we have a list of other scenarios but we wanted to use the word "Specifically" there.

So I'm a bit confused, why delete the main scenario? Do you mean move it down to the list? How about reinstating "Other" instead of "Specific"? That was my originally intended structure for this Description (summarizing: intro, main scenario, list of secondary scenarios).

- `--no-exec` - write the stage to `dvc.yaml`, but do not execute the `command`.
DVC will still add the outputs to `.gitignore`, but they won't be cached or
recorded in `dvc.lock` (like with `--no-commit` below). You can use
`dvc repro` to finish the operation. This is useful if you need to define a
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 can omit now You can use dvc repro to finish the operation.

I wonder if we also simplify the long sentence further somehow ...

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Dec 20, 2020

Choose a reason for hiding this comment

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

we can omit now You can use dvc repro

True, it was repeated. Removed. And I did find a way to simplify the text (dce4cc8) so it's 20-30% shorter now. PTAL

TBH I would completely remove the mentions of --no-cache and dvc commit here too. It may be unnecessary (implied/obvious).

@jorgeorpinel jorgeorpinel merged commit ce23d01 into master Dec 21, 2020
efiop pushed a commit to iterative/dvc that referenced this pull request Dec 25, 2020
* cmd: matching help output to docs
per iterative/dvc.org#1971 (review)
and iterative/dvc.org#1978 (review)

* fetch: simplify help text
per iterative/dvc.org#1978 (review)

* commit: update help output to match iterative/dvc.org/pull/1978

* fetch: update help output to match iterative/dvc.org/pull/1978

* commmit: help output update (again)

* remove: simplify help output
per iterative/dvc.org#1971 (review)

* remove: simplify help output
per iterative/dvc.org#1971 (review)

* commit: update -h output
per iterative/dvc.org#1989 (review)
genarocoronel pushed a commit to genarocoronel/python-dvc that referenced this pull request Jun 15, 2021
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.

2 participants