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

status: no warnings for missing state #4470

Merged

Conversation

nik123
Copy link
Contributor

@nik123 nik123 commented Aug 25, 2020

Fixes #4383 (again)

After PR #4398 it occured, that dvc status -c output is bloated and informs about "missing" files twice (took from #4398 (comment)):

$ dvc status -c -R .
WARNING: Some of the cache files do not exist neither locally nor on remote. Missing cache files:
name: data-in-cache2-not-in-remote.txt, md5: 599b9eea7a2c4f97be18c077d2df4789
	new:                data-in-cache-1-not-in-remote.txt
	missing:            data-in-cache-2-not-in-remote.txt

If missing is now an expected status, the WARNING is probably no longer needed. So it could just output something like:

$ dvc status -c  # Assumes default remote exists, otherwise -r myremote
new:                data-in-cache-1-not-in-remote.txt
missing:            data-in-cache2-not-in-remote.txt

The WARNING shouldn't shown for dvc status -c. However it should be shown for all other commands (fetch, gc, etc.). This PR implements such behavior.

P.R.: I didn't come up with anything better than adding log_missing=True param into dvc.data_cloud.DataCloud.status and passing it all the way down dvc.cache.local._make_status. It's ugly. If there any better way please inform me about it.

issue in dvc.or repo already exist: iterative/dvc.org#1701

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

@pared pared self-requested a review August 25, 2020 19:37
@pared
Copy link
Contributor

pared commented Aug 26, 2020

@nik123 the change looks good. I agree we should probably avoid passing down the flag, though I do not see any alternative as of now. Maybe its time we revisit #2957

@@ -114,6 +114,7 @@ def status(
jobs=None,
show_checksums=False,
download=False,
log_missing=True,
Copy link
Member

Choose a reason for hiding this comment

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

We've been thinking about revisit logging/info interface for a long time, since passing a chain of such flags is not the prettiest thing ever πŸ™ But there is no simple better way for now, so this will work for now πŸ‘

Copy link
Member

@efiop efiop left a comment

Choose a reason for hiding this comment

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

Thank you @nik123 !

@efiop efiop merged commit 1d9acdc into iterative:master Aug 26, 2020
@nik123 nik123 deleted the i4383-remove-missing-warning-in-status branch August 26, 2020 14:59
@skshetry skshetry added the enhancement Enhances DVC label Aug 26, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Enhances DVC
Projects
None yet
Development

Successfully merging this pull request may close these issues.

status: ignoring cache that is not in local cache nor in remote
4 participants