Dvc: checkout/add: change warning messages to regular/info if behavior is expected

[dmpetrov]

$ dvc -V
0.75.0

$ dvc checkout
WARNING: data 'newfile.txt' exists. Removing before checkout.
WARNING: data 'ttt.txt' exists. Removing before checkout.

EDITED 12/18/19: The message is actually wrong! The files were not removed - they were replaced by a new version. It has to be fixed as well.

$ echo hello >> newfile.txt
$ dvc add newfile.txt
WARNING: Output 'newfile.txt' of 'newfile.txt.dvc' changed because it is 'modified'
100% Add|███████████████████████████████████████████|1.00/1.00 [00:00<00:00,  2.45file/s]

To track the changes with git, run:

    git add newfile.txt.dvc

Based on my understanding warnings are needed when you need to alert a user. It is not needed for routine operations with expected results. In both of these cases, the priority needs to be reduced to regular/info.

PS: Are there any other warnings like these?

[shcheklein]

I think it's a duplicate of this https://github.com/iterative/dvc/issues/2329 ?

Also, yet another evidence (as many others) that loggers are not meant to be used as a mechanism to built UI/UX on top - https://github.com/iterative/dvc/issues/1930. In this specific case, for example, the priority is already set to INFO. It's just happened that in the "loggers world" INFO includes WARNING and above, not other way around.

[dmpetrov]

I think it's a duplicate of this #2329 ?

This is a discussion about the mechanism in general. It won't change the verbosity level of these particular messages.

loggers are not meant to be used as a mechanism to built UI/UX on top

Logging is just not the right term. We should use output and verbosity level instead (like in #2329).

Separation is definitely needed if we decide to output logs into a separate space while keeping printing messages to output (where verbosity level still be needed).

In this specific case, for example, the priority is already set to INFO. It's just happened that in the "loggers world" INFO includes WARNING and above, not other way around.

Sorry, I'm not following this. Could you please clarify? Where is it set to INFO and why it is a problem in this specific case? My understanding - by default we should print all info messages and above.

[shcheklein]

It won't change the verbosity level of these particular messages.

probably I was confused by PS: Are there any other warnings like these? and the generic title.

change it to be "add: remove output modified warning message"?

My understanding - by default we should print all info messages and above.

hmm, may be I didn't understand the intention for the ticket ... Technical details aside, what is the outcome you'd like to get - remove the message completely, remove the WARNING prefix?

[dmpetrov]

remove the WARNING prefix?

Yes

[shcheklein]

Gotcha! Yep, disregard my comments then! (I would probably still change title, this one feels generic, at least specific add/checkout: remove unnecessary warning prefixes - would make it way more explicit :) )

[ghost]

@dmpetrov , @shcheklein , what about removing those messages at all?

WARNING: data 'newfile.txt' exists. Removing before checkout.
WARNING: data 'ttt.txt' exists. Removing before checkout.

I think it was a _warning_ because DVC is "removing" data.

echo "foo" > foo
dvc add foo
echo "something else" > foo
dvc checkout
# user will lost "something else"

In my opinion, having those messages just clutters the UI. This is related to #2498 .

_by the way, I'm +1 for preventing the user to checkout when it has a "dirty" workspace_

[dmpetrov]

@mroutis it is a good point we can definitely consider this.

To my mind, we should NOT make checkout silent. It should output ALL the changes (and what exactly happened: remove/changed/added) or at least a summary. I'd prefer to see all the changes.

Re checkout... I've just noticed that the message is wrong (I'll modify the description) - in this scenario, the files were not removed - they were replaced by a new version. So, the message needs to be changed. Also, we need to make sure that the message will be ".. removing" if there is nothing to replacement.

I don't see a clear connection to prompt-issue #2498. That issue was not about outputs, it was about asking vs termination.

[dmpetrov]

This misleading warning makes this issue p1 or even p0 :(
Assigning a priority.

[ghost]

I don't see a clear connection to prompt-issue #2498. That issue was not about outputs, it was about asking vs termination.

@dmpetrov , we could terminate the checkout operation if there's a non-cached version of that file.

[dmpetrov]

@dmpetrov , we could terminate the checkout operation if there's a non-cached version of that file.

Right. But this is part of #2498, not the current one.

[efiop]

To my mind, we should NOT make checkout silent. It should output ALL the changes (and what exactly happened: remove/changed/added) or at least a summary. I'd prefer to see all the changes.

@dmpetrov So say you have data and you checkout to another branch. Are you saying that we should say that we are data will be replaced? That seems too verbose to me. For example git checkout does that silently. I think we should do the same there. Or, maybe you really want some summary instead? If so, we might be confusing current issue - "misleading bad warnings" and "checkout should not be silent and show tell what happened in a form of summary"(e.g. "successfully checked out 10 files").

[dmpetrov]

@efiop you are right. We should probably be silent as Git.
As an option, we can output a single message (not a warning) per changed data file or data dir (no need to write about each data file in a data-dir).

Any of these is much better than the current misleading warning message.

[skshetry]

As an option, we can output a single message (not a warning) per changed data file or data dir (no need to write about each data file in a data-dir).

@dmpetrov, how about we keep it on -v flag only? Do we really need to introduce new option, considering that this is a day-to-day used command?

Also, there's not much happening different/unsafe compared to git checkout anyway. But, :+1: for fixing misleading warning.

[efiop]

@skshetry I think @dmpetrov means that it might be nice to have a summary or something that would mark a successful run. This is a reoccurring theme in our UI discussion :slightly_smiling_face:

[skshetry]

@skshetry I think @dmpetrov means that it might be nice to have a summary or something that would mark a successful run. This is a reoccurring theme in our UI discussion

So, something similar to this?

$ dvc checkout
300 files added, 30 files replaced and 3 files removed

Or, more verbose?

[dmpetrov]

@skshetry, yes, @efiop is right. A summary would be great. Please include only the changes in the summary.

-v is too verbose. It is mostly for debugging purposes.