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

Commit cmdref updates per 1.x #1626

Merged
merged 22 commits into from
Aug 18, 2020
Merged

Conversation

imhardikj
Copy link
Contributor

@imhardikj imhardikj commented Jul 25, 2020

Partially addresses some of the tasks from #1255

  • Replace instances of term "DVC-file(s)" with dvc.yaml
  • Update all other command references

This PR updates commit cmd ref.


Pending after this PR:
#1626 (review) and
#1626 (review)

Comment on lines 248 to 240
deleted: model.pkl
deleted: data/features/test.pkl
deleted: data/features/train.pkl
deleted: data/data.xml
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jorgeorpinel When I replicated this example it didn't show data/data.xml.

Copy link
Contributor

Choose a reason for hiding this comment

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

OK

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.

@imhardikj the changes look good but it's a little strange that you would focus only on updating the examples in 2 command references, instead of the entire ref. even if it's just one command at a time... Can you please finish updating the entire ref of any one of these, and leave the other one for another PR? Thanks

@imhardikj imhardikj changed the title Example updates per 1.x Commit cmdref updates per 1.x Jul 28, 2020
@imhardikj imhardikj requested a review from jorgeorpinel July 28, 2020 19:55
@jorgeorpinel
Copy link
Contributor

@imhardikj imhardikj requested a review from jorgeorpinel yesterday

There are conflicting files in the PR. Please merge master into your branch.

Record changes to DVC-tracked files in the <abbr>project</abbr>, by updating
[DVC-files](/doc/user-guide/dvc-files-and-directories) and saving
Record changes to DVC-tracked files in the <abbr>project</abbr>, by updating the
[stages](/doc/command-reference/run) (or `.dvc` files) and saving
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't change stages, can you check what it does and try again? 🙂
And no need for parenthesis around "and .dvc files".

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 think commit only saves data to cache, it doesn't update dvc.lock or .dvc files. But the previous definition had the updating part so I didn't change it.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hey sorry, I was out a few days. I'm back, let me review this again.

Copy link
Contributor

Choose a reason for hiding this comment

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

Continued this conversation in #1626 (review).

Comment on lines 48 to 49
just use `dvc commit` to force update the related DVC-files and cache.
just use `dvc commit` to force update the related stage (or `.dvc` file) and
cache.
Copy link
Contributor

Choose a reason for hiding this comment

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

Same

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed this to save updated DVC-tracked files to the cache.

Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 4, 2020

Choose a reason for hiding this comment

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

Same. Continued in #1626 (review)

Comment on lines 52 to 54
<abbr>cache</abbr> after creating a DVC-file. What _commit_ means is that DVC:
<abbr>cache</abbr> after creating or updating the `dvc.yaml` (or `.dvc` file).
What _commit_ means is that DVC:
Copy link
Contributor

Choose a reason for hiding this comment

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

OK, this is closer but still not quite right. Is dvc.yaml the relevant special file to mention?

Copy link
Contributor Author

@imhardikj imhardikj Jul 30, 2020

Choose a reason for hiding this comment

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

Actually dvc.lock and .dvc will be updated if we run repro after doing changes in some files(deps).
And dvc.yaml will be updated in case we run a stage for first time.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here I think instead of mentioning all three(which I did currently), it can be updating the stage or .dvc file😅

Copy link
Contributor

Choose a reason for hiding this comment

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

"updating the stage" would be incorrect. Again, commit does not change the stage at all. A stage = a command, its dependencies (paths), and outputs (paths).

Copy link
Contributor

Choose a reason for hiding this comment

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

Continued in #1626 (review).

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.

Only reviewed the Description and it has the same problem in multiple places. I noted several of them specifically ☝️

@imhardikj imhardikj requested a review from jorgeorpinel July 30, 2020 20:12
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.

Good work with checking and updating the full commit doc, but still there are some issues here and there. Please address ☝️

@imhardikj imhardikj requested a review from jorgeorpinel August 9, 2020 13:40
content/docs/command-reference/commit.md Outdated Show resolved Hide resolved
Comment on lines 24 to 25
manually editing or generating DVC <abbr>outputs</abbr>; or to force the
`dvc.lock` or `.dvc` updates without reproducing stages or pipelines. These
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure about the grammar here. "the .dvc updates"? Please review the phrase between force...updates here.

content/docs/command-reference/commit.md Outdated Show resolved Hide resolved
content/docs/command-reference/commit.md Outdated Show resolved Hide resolved
content/docs/command-reference/commit.md Outdated Show resolved Hide resolved
content/docs/command-reference/commit.md Show resolved Hide resolved
force-update the [DVC-files](/doc/user-guide/dvc-files-and-directories) and save
data to cache. They are still useful, but keep in mind that DVC can't guarantee
reproducibility in those cases.
force-update the `dvc.lock` or `.dvc` files and save data to cache. They are
Copy link
Contributor

Choose a reason for hiding this comment

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

Perfect 🙏

It was just a simple replacement of "DVC-file" -> "dvc.lock or/and .dvc files" in most places but I wanted you to figure it out 🙂

content/docs/command-reference/commit.md Outdated Show resolved Hide resolved
content/docs/command-reference/commit.md Outdated Show resolved Hide resolved
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.

Hopefully last round, still trying to nail the writing. Thanks ☝️

Comment on lines 78 to +82
- `-d`, `--with-deps` - determines files to commit by tracking dependencies to
the target DVC-files (stages). If no `targets` are provided, this option is
ignored. By traversing all stage dependencies, DVC searches backward from the
target stages in the corresponding pipelines. This means DVC will not commit
files referenced in later stages than the `targets`.
the target stages or `.dvc` files. If no `targets` are provided, this option
is ignored. By traversing all stage dependencies, DVC searches backward from
the target stages in the corresponding pipelines. This means DVC will not
commit files referenced in later stages than the `targets`.
Copy link
Contributor

Choose a reason for hiding this comment

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

OK for now but

  • We need to update this option desc. in several cmd refs.

(out of scope for this PR)

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.

The file tracked by .dvc is its dependency. So the text is correct.

Copy link
Contributor

Choose a reason for hiding this comment

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

That part is correct but there's no mention of dvc.yaml and the text could probably be simplified a little. It's OK for now.

Comment on lines 84 to +86
- `-R`, `--recursive` - determines the files to commit by searching each target
directory and its subdirectories for DVC-files to inspect. If there are no
directories among the `targets`, this option is ignored.
directory and its subdirectories for stages or `.dvc` files to inspect. If
there are no directories among the `targets`, this option is ignored.
Copy link
Contributor

Choose a reason for hiding this comment

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

  • Should probably use stages (in `dvc.yaml`) or `.dvc` files

But let's leave it for another PR since this option is also in several refs.

@imhardikj imhardikj requested a review from jorgeorpinel August 11, 2020 19:53
Comment on lines 23 to 24
manually editing or generating DVC <abbr>outputs</abbr>; or to force the
`dvc.lock` or the `.dvc` updates without reproducing stages or pipelines. These
Copy link
Contributor

Choose a reason for hiding this comment

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

Still weird grammar 😢

Copy link
Contributor

@jorgeorpinel jorgeorpinel Aug 12, 2020

Choose a reason for hiding this comment

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

Adding "the" everywhere is not a solution for all grammar issues 😄 need to read it and see if there are missing words, etc... 🙏

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.

A few things are still pending.

@jorgeorpinel

This comment has been minimized.

@imhardikj imhardikj requested a review from jorgeorpinel August 12, 2020 22:19
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.

Just one last detail left ☝️

Almost...

@imhardikj imhardikj requested a review from jorgeorpinel August 16, 2020 17:48
@jorgeorpinel jorgeorpinel merged commit d3ad810 into iterative:master Aug 18, 2020
@imhardikj imhardikj deleted the examples branch August 20, 2020 17:14
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