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

log: reduce log, make it readable/easier #3834

Merged
merged 7 commits into from May 25, 2020

Conversation

skshetry
Copy link
Member

@skshetry skshetry commented May 20, 2020

  • ❗ 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. If the CLI API is changed, I have updated tab completion scripts.

  • ❌ I will check DeepSource, CodeClimate, and other sanity checks below. (We consider them recommendatory and don't expect everything to be addressed. Please fix things that actually improve code or fix bugs.)

Thank you for the contribution - we'll try to review it as soon as possible. πŸ™

What it looks like

➜ dvc run --name "generate-foo" --outs foo \
      "echo 'foo0\nfoo1' > foo"
Running callback stage 'dvc.yaml:generate-foo' with command:
	echo 'foo0\nfoo1' > foo
Creating 'dvc.yaml'
Adding stage 'generate-foo' to 'dvc.yaml'
Generating lock file 'dvc.lock'

To track the changes with git, run:

	git add dvc.lock dvc.yaml

➜ dvc run --name split-into-two --deps foo  \
        --outs foo0 \
        --outs-no-cache foo1 \
      "split -dn2 -a1 foo foo"
Running stage 'dvc.yaml:split-into-two' with command:
	split -dn2 -a1 foo foo
Adding stage 'split-into-two' to 'dvc.yaml'
Updating lock file 'dvc.lock'

To track the changes with git, run:

	git add dvc.lock dvc.yaml

➜ dvc run -n copy-foo-bar -d foo -o bar "cp foo bar"
Running stage 'dvc.yaml:copy-foo-bar' with command:
	cp foo bar
Adding stage 'copy-foo-bar' to 'dvc.yaml'
Updating lock file 'dvc.lock'

To track the changes with git, run:

	git add dvc.lock dvc.yaml

➜ dvc repro
Running callback stage 'dvc.yaml:generate-foo' with command:
	echo 'foo0\nfoo1' > foo

Stage 'dvc.yaml:split-into-two' didn't change, skipping
Stage 'dvc.yaml:copy-foo-bar' didn't change, skipping

➜ rm dvc.lock
➜ dvc repro
Running callback stage 'dvc.yaml:generate-foo' with command:
	echo 'foo0\nfoo1' > foo
Generating lock file 'dvc.lock'

Restored stage 'dvc.yaml:split-into-two' from run-cache
Skipping run, checking out outputs
Updating lock file 'dvc.lock'

Restored stage 'dvc.yaml:copy-foo-bar' from run-cache
Skipping run, checking out outputs
Updating lock file 'dvc.lock'

To track the changes with git, run:

	git add dvc.lock

@skshetry skshetry added the ui user interface / interaction label May 20, 2020
@skshetry skshetry self-assigned this May 20, 2020
@efiop
Copy link
Member

efiop commented May 20, 2020

'dvc.yaml' does not exist, creating…

Probably we don't need it. It is expected behavior.

I wonder if

Adding stage 'generate-foo' to 'dvc.yaml'…
Generating lock file…

is needed too. Seems a bit too verbose...

Same with

Updating lock file…

@dmpetrov what do you think?

@efiop efiop requested a review from dmpetrov May 20, 2020 17:28
@dmpetrov
Copy link
Member

Updating lock file…

@dmpetrov what do you think?

It worth keeping it if we can make it more informative like Lock file 'dvc.lock' was updated.

@skshetry thank you for the detailed description. It saves a lot of time for us!

My comments based on the description...

  1. callback stage What does callback mean? Can we skip the callback part?
  2. dvc.yaml:generate-foo - can we remove the file name if it's default? It applies to all the messages.
  3. 'dvc.yaml' does not exist, creating… it can be replaced to Creating 'dvc.yaml'. Minor.
  4. Adding stage 'generate-foo' to 'dvc.yaml'… --> Adding stage 'generate-foo' to 'dvc.yaml' Minor.
  5. Restored stage 'dvc.yaml:split-into-two' from run-cache, skipping… --> Stage 'split-into-two' was restored from run-cache.
  6. Do we need an empty line between these two:
Generating lock file…

Restored stage 'dvc.yaml:split-into-two' from run-cache, skipping…

I'm testing it now - I might bring more comments.

@skshetry
Copy link
Member Author

skshetry commented May 21, 2020

What does callback mean?

That's a term that's being used for too long in the warnings/log (not sure of docs). It essentially is a
stage that does not have any dependencies and hence will always run as a result.

can we remove the file name if it's default?

This should automatically resolve when we start using dvc repro <stage_name> addressing (i.e. without the colon :). This should happen within this week, so, I'll fix it there.

Restored stage 'dvc.yaml:split-into-two' from run-cache, skipping… --> Stage 'split-into-two' was restored from run-cache.

Skipping looks important to me. I want it to say what it's doing, rather than what's done. And was restored will be a bit lengthy than it already is.

Something like: Restoring stage 'dvc.yaml:split-into-two' from run-cache?

Do we need an empty line between these two:

Separating outputs between stages. Otherwise, when it's crammed, it looks awful.

@skshetry
Copy link
Member Author

skshetry commented May 21, 2020

Updating lock file…

I think this is important, especially when the stage is cached in run-cache but the lockfile is stale,
which happens quite a lot.

'dvc.yaml' does not exist, creating…
Adding stage 'generate-foo' to 'dvc.yaml'…
Generating lock file…

This will only appear on run and is informative. And, on run, only one stage is run. So, it's not too overwhelming.

@dmpetrov
Copy link
Member

What does callback mean?

That's a term that's being used for too long in the warnings/log (not sure of docs).

I didn't find it in docs. And it is not clear from the message. It looks like a technical detail. We should probably remove this.

This should automatically resolve when we start using dvc repro <stage_name> addressing

πŸ‘ It would be great to have that before 1.0 release.

Something like: Restoring stage 'dvc.yaml:split-into-two' from run-cache?

πŸ‘

Separating outputs between stages. Otherwise, when it's crammed, it looks awful.

πŸ‘

Updating lock file…
@dmpetrov what do you think?
It worth keeping it if we can make it more informative like Lock file 'dvc.lock' was updated.

I think I got @efiop point. This phrase appears at every stage, generating, updating, updating. It looks a bit too overwhelming. Is it possible to say that only once in the very end?

Also, if we change the lock file multiple times during a single repro, does it makes sense to create a temporary lock file and replace it at the very end if success. In that way, we preserve the old one if something failed.

@efiop efiop moved this from In progress to Review in progress in DVC Sprint 19 May - 2 June 2019 May 22, 2020
@skshetry skshetry requested a review from efiop May 22, 2020 02:22
@skshetry
Copy link
Member Author

This phrase appears at every stage, generating, updating, updating.

@dmpetrov, they only when appear when the lock data of that stage is stale which I think should be the case.

@shcheklein suggested to merge Restored from run-cache and checking out outputs to same line, but I think, it will be a bit lengthy. Maybe, it won't be the case after #3842 is implemented.
We'll of-course come to this later anyway.

@efiop, let's merge this one, remaining are minor concerns. We can address any of those later on.

@efiop
Copy link
Member

efiop commented May 24, 2020

@skshetry Could you please rebase? no_exec tests fail.

@efiop efiop merged commit 9198fa7 into iterative:master May 25, 2020
DVC Sprint 19 May - 2 June 2019 automation moved this from Review in progress to Done May 25, 2020
@skshetry skshetry deleted the repro-run-ui-nocolor branch May 25, 2020 02:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ui user interface / interaction
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

3 participants