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

status: update cmd ref #416

Merged
merged 35 commits into from
Jul 27, 2019
Merged

status: update cmd ref #416

merged 35 commits into from
Jul 27, 2019

Conversation

sanidhyamangal
Copy link
Contributor

@sanidhyamangal sanidhyamangal commented Jun 6, 2019

This pull request updates \static\docs\commands-reference\status.md for new options, usage examples and synopsis to fix #185.

@shcheklein kindly check the changes, I missed optional arguments in the synopsis section so shall I add it in docs?

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.

It looks great, thanks @sanidhyamangal ! A put a few comments to address. Among other things:

  1. There are a few new values for the dvc status in the local mode (w/o --cloud or --remote):
        if self.changed_md5():
            ret.append("changed checksum")

        if self.is_callback:
            ret.append("always changed")

description should be updated to include those. Please ask guys on dvc.org/chat #dev-general to explain those values.

  1. In the description changed should be split now into changed outs and changed deps.

  2. Would be great to update the dvc status help message in the dvc repo to make it something like:
    An optional list of .dvc files to limit the scope. With -R accepts directories to search .dvc files in.

@sanidhyamangal
Copy link
Contributor Author

sanidhyamangal commented Jun 9, 2019

It looks great, thanks @sanidhyamangal ! A put a few comments to address. Among other things:

  1. There are a few new values for the dvc status in the local mode (w/o --cloud or --remote):
        if self.changed_md5():
            ret.append("changed checksum")

        if self.is_callback:
            ret.append("always changed")

description should be updated to include those. Please ask guys on dvc.org/chat #dev-general to explain those values.

  1. In the description changed should be split now into changed outs and changed deps.
  2. Would be great to update the dvc status help message in the dvc repo to make it something like:
    An optional list of .dvc files to limit the scope. With -R accepts directories to search .dvc files in.

@shcheklein I couldn't get last part, so can you please throw some light on it.

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.

@sanidhyamangal please, read carefully my previous "request for changes" and the comments in this PR. They are not addressed still. If you have any questions, please, ask me here or on Discord.

@sanidhyamangal
Copy link
Contributor Author

@shcheklein I am processing these changes still, need some to figure out exact functionality and reference that can be added into the description. If still, I have some problems I will address them to you on discord

@sanidhyamangal
Copy link
Contributor Author

@shcheklein, I have changed the description for the local mode as per your request so kindly review them.

I have some questions regarding implementation -R flag, posted a query regarding same on discord kindly check that also.

@@ -38,7 +38,10 @@ stages that affect the target stage.

In the `local` mode, changes are detected through the checksum of every file
listed in every stage file in the pipeline against the corresponding file in the
file system. The output indicates the detected changes, if any. If no
file system, using two modes `changed checksum` when actual checksum of data
Copy link
Member

Choose a reason for hiding this comment

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

these are not modes. These are different outputs/statuses. The paragraph that starts with "If instead differences ..." should be updated. Check the list of different outputs that starts line 57-60.

@shcheklein
Copy link
Member

@sanidhyamangal re the 3 - I meant more or less the change you made here iterative/dvc#2111 .

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.

please, check the latest comments

@sanidhyamangal
Copy link
Contributor Author

@shcheklein, I have successfully updated the requested changes please go through them. I have also updated the changes from iterative/dvc#2111 and created a pr for the same, so kindly go through it as well iterative/dvc#2121

- _changed outs_ means the named file has changed outputs
- _changed checksum_ means actual check sum of data dosen't match specified in
dvc files
- _always checksum_ means special dvc file with no dependencies, considered
Copy link
Member

Choose a reason for hiding this comment

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

DVC-file

Copy link
Member

Choose a reason for hiding this comment

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

is it always checksum or something else? cc @efiop

Copy link
Contributor

@efiop efiop Jun 13, 2019

Choose a reason for hiding this comment

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

It should be always changed.

- _changed deps_ means the named file has changed dependencies
- _changed outs_ means the named file has changed outputs
- _changed checksum_ means actual check sum of data dosen't match specified in
dvc files
Copy link
Member

Choose a reason for hiding this comment

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

in the DVC-file

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.

@sanidhyamangal please, see the comments.

@sanidhyamangal
Copy link
Contributor Author

@shcheklein, I have successfully updated all the typo's in the description section. Kindly review them now.

@shcheklein shcheklein temporarily deployed to dvc-org-pr-416 July 14, 2019 17:53 Inactive
@sanidhyamangal

This comment has been minimized.

not reflected in the DVC-file;
- _deleted_ : when an exsisting outputs are removed from the workspace but
still referred in the DVC-file;
- _not in cache_ : when outputs mentioned in DVC-file no longer exsists in
Copy link
Member

Choose a reason for hiding this comment

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

exists -> exist

(orphans), which is considered always changed;
- _changed deps_ means that there are some changes in dependencies that are
incorporated in the DVC-file, some of the states are:
- _new_ : when reference of new dependency files are added into DVC-file;
Copy link
Member

Choose a reason for hiding this comment

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

@efiop just to double check. is it a correct description of this status? can you take a brief look at other statuses as well please.

- _changed outs_ means that there are some changes in output states that are
incorporated in the DVC-file, some of the states are:
- _new_ : when reference of new output files are are added into DVC-file;
- _modified_ : when md5 of an exsisting outputs are changed in workspace but
Copy link
Member

Choose a reason for hiding this comment

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

if you mention checksum everywhere above, let's use checksum here as well, instead of md5

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

- _always changed_ means that this is a special DVC-file with no dependencies
(orphans), which is considered always changed;
- _changed deps_ means that there are some changes in dependencies that are
incorporated in the DVC-file, some of the states are:
Copy link
Member

Choose a reason for hiding this comment

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

some - it's not some, it should be a complete list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paraphrased the given line, please review them.

- _not in cache_ : when dependencies mentioned in DVC-file no longer exsists
in local cache;
- _changed outs_ means that there are some changes in output states that are
incorporated in the DVC-file, some of the states are:
Copy link
Member

Choose a reason for hiding this comment

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

some - not some, it should be a complete list

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Paraphrased for this line as well kindly review the changes.

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.

it's getting better and better :) please, see the comments

@shcheklein shcheklein temporarily deployed to dvc-org-pr-416 July 23, 2019 17:50 Inactive
@sanidhyamangal
Copy link
Contributor Author

@shcheklein I have addressed all the issues and reformed the file based on requested changes. Kindly review the updated version.

@jorgeorpinel jorgeorpinel changed the title updated: status docs for new outputs status: update cmd ref Jul 24, 2019
the checksum is shown, and additionally a status word is shown describing the
changes. This changes list provides a reference to both the status of a
DVC-file, as well as the changes to individual dependencies and outputs
described in it:
Copy link
Contributor

Choose a reason for hiding this comment

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

Please end this paragraph in period . not column :

Copy link
Contributor

Choose a reason for hiding this comment

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

Fixed in d0bbaba

@shcheklein shcheklein merged commit 1502b45 into iterative:master Jul 27, 2019
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.

update status command output description
4 participants