checkout: show summary by default and introduce --show-changes flag #3401

skshetry · 2020-02-25T11:44:30Z

❗ Have you followed the guidelines in the Contributing to DVC list?
📖 Check this box if this PR does not require documentation updates, or if it does and you have created a separate PR in dvc.org with such updates (or at least opened an issue about it in that repo). Please link below to your PR (or issue) in the dvc.org repo.
cmd-ref: document checkout displaying changes dvc.org#1054
❌ Have you checked DeepSource, CodeClimate, and other sanity checks below? We consider their findings recommendatory and don't expect everything to be addressed. Please review them carefully and fix those that actually improve code or fix bugs.

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

Resolves #2979.

TODO

Add few tests.
CheckoutError: go with the consensus.
Make changes to flags if required. (make default?)
Handle --relink and modified case.
Show --relink successfully message. (Separate PR)
Show how many files were downloaded/fetched. (Separate PR)
Remove warnings on checkout failure
Fix pull/fetch outputs
Autocompletion scripts and help messages

dvc/remote/base.py

Suor

This makes our checkout tech debt even bigger. But I am fine for now as long as this works.

dvc/command/checkout.py

dvc/repo/checkout.py

dvc/stage.py

dvc/state.py

tests/func/test_state.py

dvc/command/checkout.py

skshetry · 2020-02-26T06:02:41Z

@efiop @Suor, the plan was to just show stats/summary. More verbose checkout changes did not seem too hard with existing stats/summary implementation so I quickly prototyped on it with --show-changes. I've asked @dmpetrov for feedback.

pared

One comment.
I find handling of added, modified, and no_change as False, True, None respectively a bit confusing at least in 2 places in code, one is base._checkout_dir and another one is pointed by @Suor
here
Boolean has 2 values and I think it is easy to omit the information passed from within checkout.

dvc/remote/base.py

shcheklein · 2020-02-26T22:45:37Z

dvc/command/checkout.py

+logger = logging.getLogger(__name__)
+
+
+def _humanize_wordseries(words=None):


move it to the utils? there should be some libraries that do different "humanization" stuff ... not sure if we want to utilize them.

I'd like to keep it near to where it's used for now. If we later need it elsewhere, we can do so at that time.

👍 up to you, absolutely minor thing.

Let me share my intuition behind this, though :)

Readability - any boilerplate code that makes file longer and absolutely not relevant/generic makes it harder to see the "bigger picture" (tools like CodeClimate complain about file being too long, about too many methods, etc for a good reason);

Reusability - the knowledge about this generic "text" util is only in your head. Low chances that someone will be able to find it when it's needed again. Means some probability of a duplicate later.

Testing - you can unit test this simple things, would be strange to write a unit test for a private method though.

It's not like humanizing, it's joining and not word series but some items in a list, so the name might be better, e.g. human_join() or smth.

On keeping it here - I am with @skshetry here, move it later if that is used elsewhere. Spreading code is the big source of errors. Also impairs readability.

dvc/command/checkout.py

dvc/remote/base.py

dvc/stage.py

dvc/repo/checkout.py

dvc/stage.py

gurobokum · 2020-02-28T03:43:45Z

dvc/stage.py

+        def _fspath_dir(path):
+            """Return string with "/" at the end if it's a dir."""
+            p = os.path.join(path, "") if os.path.isdir(path) else fspath(path)
+            return relpath(p, self.repo.root_dir)


What do you think about it? (if I got the function purpose right)

rel = relpath(p, self.repo.root_dir) return "{}{}".format(rel, os.path.sep) if os.path.isdir(path) else rel

shcheklein · 2020-03-10T20:00:08Z

dvc/exceptions.py

-    def __init__(self, target_infos):
+    def __init__(self, target_infos, stats=None):
+        self.target_infos = target_infos
+        self.stats = stats
        targets = [str(t) for t in target_infos]
        m = (
            "Checkout failed for following targets:\n {}\nDid you "


sorry, really minor request (feel free to nor change this), came out as I was reviewing:

ERROR: unexpected error - Checkout failed for following targets: data/prepared data/features data/data.xml

there is an extra space that is not needed before data/prepared. I think the change is \n {}\n -> \n{}\n

shcheklein · 2020-03-10T21:55:13Z

@skshetry going back to the requirements in the #2979 (you mention in the description) , I think it does not resolve them for the failed case:

WARNING: Cache '6836f797f3924fb46fcfd6b9f6aa6416.dir' not found. File 'data/prepared' won't be created.
WARNING: Cache 'a304afb96060aad90176268345e10355' not found. File 'data/data.xml' won't be created.
WARNING: Cache '42c7025fc0edeb174069280d17add2d4.dir' not found. File 'data/features' won't be created.
1 added
ERROR: unexpected error - Checkout failed for following targets:
 data/prepared
data/data.xml
data/features
Did you forget to fetch?

Having any troubles? Hit us up at https://dvc.org/support, we are always happy to help!

we still have excessive WARNINGS . We can create a separate PR for this or keep the ticket open. I think a summary is enough:

1 added, 3 failed

ERROR: unexpected error - Checkout failed for following targets:
 data/prepared
data/data.xml
data/features
Did you forget to fetch?

Again, happy to just keep that ticket open.

shcheklein · 2020-03-10T22:57:47Z

It still affects dvc pull in some unexpected (?) ways:

(.env) [ivan@ivan ~/Projects/example-get-started]$ dvc pull
2 added
Everything is up to date.

and sometimes:

(.env) [ivan@ivan ~/Projects/example-get-started]$ dvc pull
2 modified

shcheklein · 2020-03-10T23:01:20Z

(Also, going through the example get started I have more and more reasons for the --show-changes option by default. One more reason - we mix directories and files when we count added, modified, etc. E.g. when it prints 1 added but actually it's a directory with two preprocessed files it make it confusing if dvc checkout model.dvc prints the same. Just a thought to consider.)

skshetry · 2020-03-11T09:01:32Z

it's a directory with two preprocessed files it make it confusing if dvc checkout model.dvc prints the same. Just a thought to consider.)

I'm also more inclined to make this the default, even more so with making output formats of git and dvc checkouts similar (helpful with post-checkout). So, to not make anymore changes, I'll swap the defaults, and introduce --summary flag.

Later, we could decide if we really need --summary, and then either remove or even rename those as --shortstat/ --short/--stat/--numstat, etc.

It still affects dvc pull in some unexpected (?) ways:

Looks like on second example, pull did fetch something and did checkout (checkout looks to be working fine), but pull did not report anything (same in-line with previous behavior that it didn't print anything if it fetched something). I'll fix fetch and pull behavior on separate PRs.

we still have excessive WARNINGS . We can create a separate PR for this or keep the ticket open. I think a summary is enough:

It'd be better to just reduce the level of loggings here to DEBUG. Again, I'll handle it in a separate PRs (added to TODO).

Thanks Ivan for checking out!

Suor · 2020-03-11T09:57:27Z

@skshetry I don't think you can simply make those warnings debug, this will affect more things than just checkout, e.g. Stage.run() does not check for those checkouts to succeed.

dvc/command/checkout.py

dvc/external_repo.py

dvc/remote/base.py

dvc/repo/checkout.py

Suor · 2020-03-11T10:19:54Z

dvc/repo/checkout.py

+        unused = _get_unused_links(self)
+
+    stats["deleted"] = [_fspath_dir(u, self.root_dir) for u in unused]
+    self.state.remove_links(unused)


Why is this not under if? Looks fragile this way, we shouldn't call remove_links() at all if a particular target is specified.

It's empty if there's no argument.

Also, I need to check if something is a dir before deleting.

Checkout in dvc:

checks out particular thing(s) if that things (target) is specified

if no target is specified it checks out everything and removes anything previously checked out and having no references now.

I.e. if there is target we don't try to change the rest of the workspace, so .remove_links() should not be called. The logic of your code is different.

If targets are specified, unused will be empty and hence, nothing will get removed. If your concern is regarding calling .remove_links() at all, I don't think there's any reason to be defensive.

…w summary

skshetry · 2020-03-13T06:54:21Z

@shcheklein, I have changed the defaults. There's a --summary flag for showing summary instead of detaliled changelog. Other stuffs will be fixed in a separate PR.

shcheklein

Great stuff, thanks @skshetry !

(please, create a ticket of address docs and bash/zsh scripts? I assume the plan is to do a separate PR, right?)

shcheklein · 2020-03-14T03:48:36Z

sorry, wanted to approve initially, missed the button )

dvc/command/checkout.py

Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

skshetry · 2020-03-16T14:22:49Z

@shcheklein, @efiop, I have created an issue #3483 for handling autocompletion scripts.

skshetry self-assigned this Feb 25, 2020

skshetry force-pushed the checkout-summary-changes branch from 63de201 to 23256a5 Compare February 25, 2020 11:48

skshetry commented Feb 25, 2020

View reviewed changes

dvc/remote/base.py Show resolved Hide resolved

skshetry marked this pull request as ready for review February 25, 2020 12:58

skshetry requested review from efiop, Suor, pared and gurobokum February 25, 2020 12:58

skshetry changed the title ~~[WIP] checkout: introduce summary and changes flag~~ checkout: introduce summary and changes flag Feb 25, 2020

Suor suggested changes Feb 26, 2020

View reviewed changes

efiop reviewed Feb 26, 2020

View reviewed changes

dvc/command/checkout.py Outdated Show resolved Hide resolved

pared suggested changes Feb 26, 2020

View reviewed changes

dvc/remote/base.py Outdated Show resolved Hide resolved

shcheklein reviewed Feb 26, 2020

View reviewed changes

dvc/command/checkout.py Outdated Show resolved Hide resolved

gurobokum reviewed Feb 27, 2020

View reviewed changes

dvc/remote/base.py Show resolved Hide resolved

gurobokum reviewed Feb 27, 2020

View reviewed changes

dvc/stage.py Outdated Show resolved Hide resolved

skshetry changed the title ~~checkout: introduce summary and changes flag~~ [WIp] checkout: introduce summary and changes flag Feb 27, 2020

skshetry changed the title ~~[WIp] checkout: introduce summary and changes flag~~ [WIP] checkout: introduce summary and changes flag Feb 27, 2020

skshetry commented Feb 27, 2020

View reviewed changes

dvc/repo/checkout.py Show resolved Hide resolved

gurobokum reviewed Feb 28, 2020

View reviewed changes

dvc/stage.py Outdated Show resolved Hide resolved

gurobokum reviewed Feb 28, 2020

View reviewed changes

skshetry force-pushed the checkout-summary-changes branch from 30c9e5d to 1f4fbf0 Compare February 28, 2020 12:37

skshetry changed the title ~~[WIP] checkout: introduce summary and changes flag~~ checkout: introduce summary and changes flag Feb 28, 2020

skshetry requested review from Suor, shcheklein, efiop and gurobokum February 28, 2020 12:38

pared approved these changes Mar 10, 2020

View reviewed changes

shcheklein reviewed Mar 10, 2020

View reviewed changes

fix tests

f2cf62e

Suor suggested changes Mar 11, 2020

View reviewed changes

skshetry added 3 commits March 12, 2020 14:02

checkout: make show changes default and introduce summary flag to sho…

104b450

…w summary

address suggestions

5bcd1bf

exceptions: remove extra spacing on erorr

5fb1c90

skshetry requested review from shcheklein, pared and Suor March 14, 2020 03:24

shcheklein requested changes Mar 14, 2020

View reviewed changes

shcheklein approved these changes Mar 14, 2020

View reviewed changes

jorgeorpinel reviewed Mar 15, 2020

View reviewed changes

dvc/command/checkout.py Outdated Show resolved Hide resolved

jorgeorpinel approved these changes Mar 15, 2020

View reviewed changes

Update dvc/command/checkout.py

b80a44b

Co-Authored-By: Jorge Orpinel <jorgeorpinel@users.noreply.github.com>

This was referenced Mar 16, 2020

cmd-ref: document checkout displaying changes iterative/dvc.org#1054

Merged

checkout: autocompletion scripts and help message for summary #3483

Closed

efiop merged commit 066ccea into iterative:master Mar 16, 2020

skshetry deleted the checkout-summary-changes branch March 16, 2020 14:30

This was referenced Mar 16, 2020

checkout: --relink show helpful message on completion #3484

Closed

pull/fetch: show how many files were downloaded #3485

Closed

checkout: remove warnings on checkout failure #3486

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

checkout: show summary by default and introduce --show-changes flag #3401

checkout: show summary by default and introduce --show-changes flag #3401

skshetry commented Feb 25, 2020 •

edited

Loading

Suor left a comment

skshetry commented Feb 26, 2020

pared left a comment •

edited

Loading

shcheklein Feb 26, 2020

skshetry Feb 27, 2020

shcheklein Feb 27, 2020

Suor Feb 27, 2020

Suor Feb 27, 2020

gurobokum Feb 28, 2020

shcheklein Mar 10, 2020

shcheklein commented Mar 10, 2020

shcheklein commented Mar 10, 2020 •

edited

Loading

shcheklein commented Mar 10, 2020

skshetry commented Mar 11, 2020

Suor commented Mar 11, 2020

Suor Mar 11, 2020

skshetry Mar 11, 2020

skshetry Mar 11, 2020

Suor Mar 12, 2020

skshetry Mar 13, 2020 •

edited

Loading

skshetry commented Mar 13, 2020

shcheklein left a comment

shcheklein commented Mar 14, 2020

skshetry commented Mar 16, 2020

		logger = logging.getLogger(__name__)


		def _humanize_wordseries(words=None):

checkout: show summary by default and introduce --show-changes flag #3401

checkout: show summary by default and introduce --show-changes flag #3401

Conversation

skshetry commented Feb 25, 2020 • edited Loading

Suor left a comment

Choose a reason for hiding this comment

skshetry commented Feb 26, 2020

pared left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

shcheklein commented Mar 10, 2020

shcheklein commented Mar 10, 2020 • edited Loading

shcheklein commented Mar 10, 2020

skshetry commented Mar 11, 2020

Suor commented Mar 11, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry Mar 13, 2020 • edited Loading

Choose a reason for hiding this comment

skshetry commented Mar 13, 2020

shcheklein left a comment

Choose a reason for hiding this comment

shcheklein commented Mar 14, 2020

skshetry commented Mar 16, 2020

skshetry commented Feb 25, 2020 •

edited

Loading

pared left a comment •

edited

Loading

shcheklein commented Mar 10, 2020 •

edited

Loading

skshetry Mar 13, 2020 •

edited

Loading