Outputs as target supporting for `dvc status` #4433

karajan1001 · 2020-08-20T12:01:31Z

Add a related test which would fail on current version.

❗ I have followed the Contributing to DVC checklist.
📖 If this PR requires documentation updates, I have created a separate PR (or issue, at least) in dvc.org and linked it here.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

fix iterative#4191 1. Add a related test which would fail on current version.

jorgeorpinel · 2020-08-20T16:27:11Z

I marked the documentation checkbox as this is already supposed to be the case and mentioned in https://dvc.org/doc/command-reference/status. Thanks

tests/func/test_status.py

1. add deps to the tests. 2. make change to pass the tests.

karajan1001 · 2020-08-24T10:59:06Z

Only show outputs in a rough way, need to be rearranged and summarized in a new PR related to #2180.

pared

LGTM, thanks @karajan1001!

pared · 2020-08-24T11:04:26Z

tests/func/test_status.py

+    assert main(["status", "alice_bob"]) == 0
+    assert "alice_bob:" in caplog.text
+    assert "changed outs:" in caplog.text
+    assert "modified:           alice" in caplog.text
+    assert "modified:           bob" in caplog.text
+    caplog.clear()
+
+    assert main(["status", "alice"]) == 0
+    assert "modified:           alice" in caplog.text
+    assert "modified:           bob" not in caplog.text


Maybe it would be easier to check the status dictionary (like in test above that one, test_status_recursive), instead of caplog messages?

@pared
Already Done.

skshetry

Thanks @karajan1001 for the PR. Looks good to me, although, have a few suggestions.

skshetry · 2020-08-24T11:37:45Z

dvc/repo/status.py

+        if not filter_info:
+            status_info.update(stage.status(check_updates=True))
+        else:
+            for out in stage.filter_outs(filter_info):
+                status_info.update(out.status())


Should we push this to Stage::status() or Stage::_status_outs?

Also, we should keep {stage_name: stage_status} format, to make it in line with --show-json.

Thanks @karajan1001 for the PR. Looks good to me, although, have a few suggestions.
@skshetry , Thanks for your suggestions.

Also, we should keep {stage_name: stage_status} format, to make it in line with --show-json.
Here, I just follow the format of dvc status -c [outputs]. Besides, if we show

"alice_bob": "changed outs": "alice": "modified"

We give misinformation that there is only one output "alice" in stage "alice_bob", here we need more emphasis on outputs than stages.

Should we push this to Stage::status() or Stage::_status_outs?
It has a different format from the current Stage::status() and Stage::_status_outs now. So we can't reuse them here.

Maybe we can discuss the output format more in #2180?

@karajan1001, better to be consistent here. If we need better CLI output, we can handle that in dvc/commands. But, let's wait for others, let's see what they'll say.

It has a different format from the current Stage::status() and Stage::_status_outs now

Regarding status, we could add filter_info to it so that you could just do

Suggested change

if not filter_info:

status_info.update(stage.status(check_updates=True))

else:

for out in stage.filter_outs(filter_info):

status_info.update(out.status())

status_info.update(stage.status(filter_outs=filter_info))

But this would make sense if we made result consistent.

@karajan1001, better to be consistent here. If we need better CLI output, we can handle that in dvc/commands. But, let's wait for others, let's see what they'll say.

It has a different format from the current Stage::status() and Stage::_status_outs now

Regarding status, we could add filter_info to it so that you could just do

But this would make sense if we made result consistent.

@skshetry
The question is dvc/commands didn't know targets are stages or outputs.

Actually I think the most elegant way is that collect_granular returns a list of outputs or stages or files, and they share the same API .status() which returns the result. But we need another object FilePathSlot which is at a finer granularity than outputs to get rid of filter_info.

I would agree with @skshetry here,

"alice_bob": "changed outs": "alice": "modified"

In that case we inform user that only one out has changed, but that does not mean that this particular stage has only this output. I would say that status primary function is to inform about changes, and there is no need to list all of its outputs, if they did not change.

I would agree with @skshetry here,

"alice_bob": "changed outs": "alice": "modified"

In that case we inform user that only one out has changed, but that does not mean that this particular stage has only this output. I would say that status primary function is to inform about changes, and there is no need to list all of its outputs, if they did not change.

@pared Thank you.
How about the status -c? Do they also need to follow this format, or just keep the current one?

@karajan1001 This is a good question.
So in case of our current test, if we commit modified alice we will get Data and pipelines up to date on dvc status
and we will get new: alice on dvc status -c. As this issue is about filtering the status I would keep output of dvc status -c as is and create issue discussing whether we should have consistent output format for both dvc status and dvc status -c.

tests/func/test_status.py

Co-authored-by: Saugat Pachhai <[email protected]>

pared · 2020-08-25T10:48:19Z

@karajan1001 is there anything that you would like to add here? Or can we drop the [WIP]? :)

karajan1001 · 2020-08-25T11:10:56Z

@karajan1001 is there anything that you would like to add here? Or can we drop the [WIP]? :)

@pared
I'm sorry, I don't know what is [WIP] and googled it a moment ago. Now I can say yes.

pared · 2020-08-25T11:48:28Z

@karajan1001 no problem, it just felt like the PR is complete, thats why I asked :)

skshetry · 2020-08-25T12:03:17Z

Would have been better to have a consistent format: #4433 (comment). 🤷‍♂️

pared · 2020-08-25T12:13:55Z

@skshetry sorry, my bad. Let's finish the discussion here and prepare follow up PR?

Outputs as target supporting for dvc status

e29f99d

fix iterative#4191 1. Add a related test which would fail on current version.

efiop changed the title ~~Outputs as target supporting for dvc status~~ [WIP] Outputs as target supporting for dvc status Aug 20, 2020

pared reviewed Aug 21, 2020

View reviewed changes

tests/func/test_status.py Outdated Show resolved Hide resolved

Pass the tests

6f63e66

1. add deps to the tests. 2. make change to pass the tests.

karajan1001 requested a review from pared August 24, 2020 10:59

pared approved these changes Aug 24, 2020

View reviewed changes

skshetry approved these changes Aug 24, 2020

View reviewed changes

karajan1001 and others added 2 commits August 25, 2020 09:08

Update tests/func/test_status.py

1fcc1bf

Co-authored-by: Saugat Pachhai <[email protected]>

Solve some change request.

ed07261

pared changed the title ~~[WIP] Outputs as target supporting for dvc status~~ Outputs as target supporting for dvc status Aug 25, 2020

pared merged commit fcca636 into iterative:master Aug 25, 2020

skshetry added enhancement Enhances DVC feature is a feature labels Aug 26, 2020

karajan1001 mentioned this pull request Aug 27, 2020

Discuss whether we should have consistent output format for both dvc status and dvc status -c. #4482

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Outputs as target supporting for `dvc status` #4433

Outputs as target supporting for `dvc status` #4433

karajan1001 commented Aug 20, 2020 •

edited by jorgeorpinel

Loading

jorgeorpinel commented Aug 20, 2020

karajan1001 commented Aug 24, 2020

pared left a comment

pared Aug 24, 2020

karajan1001 Aug 25, 2020

skshetry left a comment

skshetry Aug 24, 2020

skshetry Aug 24, 2020

karajan1001 Aug 25, 2020 •

edited

Loading

skshetry Aug 25, 2020

karajan1001 Aug 25, 2020 •

edited

Loading

pared Aug 25, 2020

karajan1001 Aug 26, 2020 •

edited

Loading

pared Aug 26, 2020

pared commented Aug 25, 2020

karajan1001 commented Aug 25, 2020

pared commented Aug 25, 2020

skshetry commented Aug 25, 2020 •

edited

Loading

pared commented Aug 25, 2020

Outputs as target supporting for dvc status #4433

Outputs as target supporting for dvc status #4433

Conversation

karajan1001 commented Aug 20, 2020 • edited by jorgeorpinel Loading

jorgeorpinel commented Aug 20, 2020

karajan1001 commented Aug 24, 2020

pared left a comment

Choose a reason for hiding this comment

pared Aug 24, 2020

Choose a reason for hiding this comment

karajan1001 Aug 25, 2020

Choose a reason for hiding this comment

skshetry left a comment

Choose a reason for hiding this comment

skshetry Aug 24, 2020

Choose a reason for hiding this comment

skshetry Aug 24, 2020

Choose a reason for hiding this comment

karajan1001 Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

skshetry Aug 25, 2020

Choose a reason for hiding this comment

karajan1001 Aug 25, 2020 • edited Loading

Choose a reason for hiding this comment

pared Aug 25, 2020

Choose a reason for hiding this comment

karajan1001 Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

pared Aug 26, 2020

Choose a reason for hiding this comment

pared commented Aug 25, 2020

karajan1001 commented Aug 25, 2020

pared commented Aug 25, 2020

skshetry commented Aug 25, 2020 • edited Loading

pared commented Aug 25, 2020

Outputs as target supporting for `dvc status` #4433

Outputs as target supporting for `dvc status` #4433

karajan1001 commented Aug 20, 2020 •

edited by jorgeorpinel

Loading

karajan1001 Aug 25, 2020 •

edited

Loading

karajan1001 Aug 25, 2020 •

edited

Loading

karajan1001 Aug 26, 2020 •

edited

Loading

skshetry commented Aug 25, 2020 •

edited

Loading