Skip to content

status: update cmd ref#416

Merged
shcheklein merged 35 commits into
treeverse:masterfrom
sanidhyamangal:master
Jul 27, 2019
Merged

status: update cmd ref#416
shcheklein merged 35 commits into
treeverse:masterfrom
sanidhyamangal:master

Conversation

@sanidhyamangal
Copy link
Copy Markdown
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?

Comment thread static/docs/commands-reference/status.md Outdated
Comment thread static/docs/commands-reference/status.md Outdated
Comment thread static/docs/commands-reference/status.md Outdated
Copy link
Copy Markdown
Contributor

@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.

Comment thread static/docs/commands-reference/status.md
@sanidhyamangal
Copy link
Copy Markdown
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
Copy Markdown
Contributor

@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
Copy Markdown
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
Copy Markdown
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.

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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
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

Comment thread static/docs/commands-reference/status.md Outdated
- _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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

DVC-file

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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.

Comment thread static/docs/commands-reference/status.md Outdated
- _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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

@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
Copy Markdown
Contributor Author

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

Comment thread static/docs/commands-reference/status.md Outdated
@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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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.

Comment thread static/docs/commands-reference/status.md Outdated
Comment thread static/docs/commands-reference/status.md Outdated
- _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
Copy Markdown
Contributor

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
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done!

Comment thread static/docs/commands-reference/status.md Outdated
- _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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
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
Copy Markdown
Contributor

@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

Comment thread static/docs/commands-reference/status.md Outdated
Comment thread static/docs/commands-reference/status.md
@shcheklein shcheklein temporarily deployed to dvc-org-pr-416 July 23, 2019 17:50 Inactive
@sanidhyamangal
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Fixed in d0bbaba

Comment thread static/docs/commands-reference/status.md Outdated
Comment thread static/docs/commands-reference/status.md Outdated
Comment thread static/docs/commands-reference/status.md Outdated
@shcheklein shcheklein merged commit 1502b45 into treeverse: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