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

cmd: update commit-related info. #1989

Merged
merged 23 commits into from
Dec 13, 2020
Merged

cmd: update commit-related info. #1989

merged 23 commits into from
Dec 13, 2020

Conversation

jorgeorpinel
Copy link
Contributor

@jorgeorpinel jorgeorpinel commented Dec 1, 2020

Extracted from #1978 (comment)

Also rel. iterative/dvc/issues/5000

Note for self:

@shcheklein shcheklein temporarily deployed to dvc-landing-cmd-commit-1arj7hp December 1, 2020 05:26 Inactive
@jorgeorpinel jorgeorpinel mentioned this pull request Dec 1, 2020
1 task
jorgeorpinel added a commit that referenced this pull request Dec 1, 2020
@jorgeorpinel
Copy link
Contributor Author

PTAL @shcheklein

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

Other scenarios include:
Copy link
Member

Choose a reason for hiding this comment

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

Not sure Other applies here? it's more like a detailed explanation?

Copy link
Contributor Author

@jorgeorpinel jorgeorpinel Dec 12, 2020

Choose a reason for hiding this comment

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

Hmmm they're meant as different scenarios. The part before this is about add/run/repro --no-commit + dvc commit — main use case. The bullets in this list are

  • dvc add *
  • force-accepting cosmetic changes to dependencies
  • adding missing deps/outs
  • executing commands manually (this one I guess is pretty similar to the main case)

Copy link
Member

Choose a reason for hiding this comment

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

some of them (all of them?) are part of the main use case in this terminology.

add/run/repro --no-commit is not even a use case by itself, right? it doesn't explain a specific need.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I thought the main reason commit exists as a stand-alone command is to complement the --no-commit options of add/run/repro?

In terms of the "story", its explained as "forces DVC to accept the contents of tracked data currently in the workspace" in the first p of the description. So do you mean they're all different flavors of that explanation and that there should be a single bullet list (including add/run/repro --no-commit)?

Copy link
Member

Choose a reason for hiding this comment

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

I thought the main reason commit exists as a stand-alone command is to complement the --no-commit options of add/run/repro?

probably initially it was the case, now we reference it in other places? (like --no-exec?) So we might need to revisit, generalize? (not 100% sure, just asking)

So do you mean they're all different flavors of that explanation

it seems so (not sure about all)

that there should be a single bullet list

ah, not necessarily. Just removing Other might help? or rephrasing it a bit if the first paragraph is already general.

again, not very constructive feedback here - just highlighting stuff as I read it, a think that seemed strange

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Gotcha. I iterated on this to make clear that the first p is a generalization, then a main scenario is explained, and finally a list with other scenarios. PTAL.

Copy link
Member

@shcheklein shcheklein left a comment

Choose a reason for hiding this comment

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

Overall it's great, just a few minor comments to iterate on

`dvc commit` to store it in the cache and record its hash value in the `.dvc`
file.

This is useful if, for example, you need to build a project that will use
Copy link
Member

Choose a reason for hiding this comment

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

you have repetition useful ... useful

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hm yeah this sentence wasn't great, sorry... Updated in d3ed553.

Copy link
Member

Choose a reason for hiding this comment

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

it's repetitive with the last sentence of the previous paragraph, essentially it reads:

something. It is useful when A.

This is useful when B.

is it intentional?

and overall it got complicated again. Is there a way to merge A and B?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

True. I guess this is why we didn't have so much motivation in the --no-exec options before... I tried to merge it now but it's still a little long.

Maybe we should remove the part about "if the file/directory already exists locally"? It seems like a very edge case to download something manually and THEN register it with import.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should remove the part about "if the file/directory already exists locally"

Yeah, I removed that to simplify.

jorgeorpinel added a commit that referenced this pull request Dec 13, 2020
@jorgeorpinel
Copy link
Contributor Author

I suggest we don't obsess over the --no-exec text for now and instead open/reactivate core tickets to make that the default behavior, at least for run (which would need to be renamed probably) — since it's supposed to be a helper for writing dvc.yaml files.

It's also an interesting question what to do with the import commands. I think that .dvc files could be phased and all tracked data could be included in a section of dvc.yaml (before stages, see iterative/dvc#3936 (comment)) so in that case --no-exec should also be the default behavior.

@shcheklein shcheklein merged commit c3b1ba9 into master Dec 13, 2020
@jorgeorpinel jorgeorpinel deleted the cmd/commit branch December 21, 2020 23:11
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)
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.

cmd ref: add a few lines about cache and no-commit in dvc run
2 participants