dvc: update gc to remove unpacked dir #3054

sharidas · 2020-01-04T13:03:04Z

In local remote its found that gc does not
remove the unpacked dir. This change set
helps to remove it.

Signed-off-by: Sujith H [email protected]

❗ 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.
❌ 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.

Fixes dvc gc does not remove files under dir.unpacked #2946

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

pared · 2020-01-04T13:23:23Z

dvc/remote/local.py

@@ -152,6 +152,9 @@ def remove(self, path_info):

        if self.exists(path_info):
            remove(path_info.fspath)
+            # Remove unpacked if dir checksum is true


I don't think that this comment is necessary, the code is pretty self-explanatory :)

Removed the comment.

pared · 2020-01-04T13:59:05Z

Ok, @sharidas, as you probably noticed, the tests do not pass. The reason for that is that you are trying to check is_dir_checksum(path_to_checksum) which will not work. I think this is a problem with naming on our side, but path_to_checksum works only for cache paths. What you could do is:

use self.get_checksum to obtain checksum for path_info
check if checksum is dir checksum using self.is_dir_checksum
get unpacked dir path info using _get_unpacked_dir_path_info
and then remove it
Also, that will have to be done before actually removing path_info.

sharidas · 2020-01-04T15:11:17Z

@pared Thanks for the support. I did implemented the way suggested here #3054 (comment). Unfortunately the tests are still failing. Need to look into it.

skshetry · 2020-01-04T15:45:42Z

dvc/remote/local.py

@@ -151,6 +151,9 @@ def remove(self, path_info):
            raise NotImplementedError

        if self.exists(path_info):
+            checksum = self.get_checksum(path_info)


@sharidas, looks like on some tests, the state does not get load and get_checksum fails?

@pared, I wonder if this could be handled here:

dvc/dvc/remote/base.py

Lines 699 to 700 in 85c35f8

path_info = self.checksum_to_path_info(checksum)

self.remove(path_info)

@skshetry Not sure. I was looking into one of the test, basically looking into the stack trace and found https://github.com/iterative/dvc/blob/master/tests/func/test_checkout.py#L320. And this test does not parse the gc code if I noticed the flow correctly.

The updated .remove() has .get_checksum() which gets checksum from .dvc/state AFAIK, but the test does not load the state and hence the exception (i assume as the state.cursor is None). You can load the state via with self.dvc.state: though. But, I'd try moving the logic after this line (though, a check if it's local needs to be added), but @iterative/dvc developers might have a different suggestion.

@skshetry is right and that is also what we meant in the solution description in #2946 (comment) . This PR is trying to make a change to a generic method that doesn't really need to know anything about checksums and is often used in other places where it is not expected to have such a behaviour.

@efiop Thanks for the feedback. I was trying to make the solution close to the remote local. Now I have updated the pr again by making change in base.py. Initially I made the change in the base.py as suggested in the issue. Later I thought it would be better to make the solution close to the local.

@sharidas Makes sense, I guess my comment about "local" was confusing, sorry about that 🙂

efiop

Thanks for the PR! Please see a comment above. Also, please don't forget to include issue number in the PR or commit desc. #2946 🙂

sharidas · 2020-01-05T09:55:47Z

@efiop I have added Fixes tag to the pr description with the issue number. Hope this sounds ok.

efiop

Looks good! Please see a few minor comments.

efiop · 2020-01-05T21:48:16Z

dvc/remote/base.py

@@ -697,6 +697,9 @@ def gc(self, named_cache):
            if checksum in used:
                continue
            path_info = self.checksum_to_path_info(checksum)
+            if self.is_dir_checksum(checksum):
+                if callable(getattr(self, "_get_unpacked_dir_path_info")):


Probably don't need callable 🙂 Also, this getattr will keep working normally even if we remove _get_unpacked_dir_path_info at some point. So your implementation is correct, but maybe we could clean it up a bit. How about we mimic the other unpacked_dir logic that we have in the code and define _remove_unpacked_dir method in base(as a noop) and in local, so that we could simply:

if self.is_dir_checksum(checksum): self._remove_unpacked_dir(checksum)

here, and won't have to do getattr tricks?

Also, please add a test for this to tests/func/test_gc.py. Something similar to the reproduction script in the issue would be just right 🙂

tests/func/test_gc.py

sharidas · 2020-01-06T16:52:52Z

Updated the test as per the suggestion from #3054 (comment)

pared · 2020-01-06T17:09:49Z

tests/func/test_gc.py

@@ -204,3 +205,15 @@ def test_gc_no_dir_cache(tmp_dir, dvc, repo_template):

 def _count_files(path):
    return sum(len(files) for _, _, files in os.walk(path))
+
+def test_gc_no_unpacked_dir(tmp_dir, dvc, repo_template):
+    dir_stages = dvc.add("dir")


you can use stage, = tmp_dir.dvc_gen({"dir": {"file": "file_content"}})

if you choose to go this way there will be no need for tepo_template :)

@pared repo_template is fine too though.

efiop · 2020-01-06T17:39:30Z

@sharidas Looks like you didn't install pre-commit hooks described in our contribution guide, hence why the styling checks are failing.

In local remote its found that gc does not remove the unpacked dir. This change set helps to remove it. Signed-off-by: Sujith H <[email protected]>

sharidas · 2020-01-06T18:29:30Z

@sharidas Looks like you didn't install pre-commit hooks described in our contribution guide, hence why the styling checks are failing.

@efiop Yes pre-commit hooks were not installed in my env. Thanks for the pointer. Now I have installed it and rebased the PR again.

efiop

Thanks!

sharidas mentioned this pull request Jan 4, 2020

dvc gc does not remove files under dir.unpacked #2946

Closed

pared reviewed Jan 4, 2020

View reviewed changes

sharidas force-pushed the fix-gc-local branch from 3c165b2 to 784977c Compare January 4, 2020 13:38

sharidas force-pushed the fix-gc-local branch from 784977c to 91679a7 Compare January 4, 2020 14:27

skshetry reviewed Jan 4, 2020

View reviewed changes

efiop suggested changes Jan 4, 2020

View reviewed changes

sharidas force-pushed the fix-gc-local branch from 91679a7 to 1ebdede Compare January 5, 2020 09:29

efiop suggested changes Jan 5, 2020

View reviewed changes

sharidas force-pushed the fix-gc-local branch from 1ebdede to 16ca803 Compare January 6, 2020 12:05

restyled-io bot mentioned this pull request Jan 6, 2020

Restyle dvc: update gc to remove unpacked dir #3065

Closed

sharidas force-pushed the fix-gc-local branch from 16ca803 to 8b40b9e Compare January 6, 2020 12:19

efiop reviewed Jan 6, 2020

View reviewed changes

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

sharidas force-pushed the fix-gc-local branch from 8b40b9e to cf982ae Compare January 6, 2020 15:52

skshetry reviewed Jan 6, 2020

View reviewed changes

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

sharidas force-pushed the fix-gc-local branch from cf982ae to b46e2fb Compare January 6, 2020 16:38

pared reviewed Jan 6, 2020

View reviewed changes

pared approved these changes Jan 6, 2020

View reviewed changes

dvc: update gc to remove unpacked dir

1e3abe2

In local remote its found that gc does not remove the unpacked dir. This change set helps to remove it. Signed-off-by: Sujith H <[email protected]>

sharidas force-pushed the fix-gc-local branch from b46e2fb to 1e3abe2 Compare January 6, 2020 18:28

efiop approved these changes Jan 6, 2020

View reviewed changes

efiop merged commit 498dcab into iterative:master Jan 6, 2020

weekly-digest bot mentioned this pull request Jan 12, 2020

Weekly Digest (5 January, 2020 - 12 January, 2020) #3119

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

dvc: update gc to remove unpacked dir #3054

dvc: update gc to remove unpacked dir #3054

sharidas commented Jan 4, 2020 •

edited by efiop

Loading

pared Jan 4, 2020

sharidas Jan 4, 2020

pared commented Jan 4, 2020

sharidas commented Jan 4, 2020

skshetry Jan 4, 2020

sharidas Jan 4, 2020

skshetry Jan 4, 2020 •

edited

Loading

efiop Jan 4, 2020 •

edited

Loading

sharidas Jan 5, 2020

efiop Jan 5, 2020

efiop left a comment •

edited

Loading

sharidas commented Jan 5, 2020

efiop left a comment

efiop Jan 5, 2020

sharidas commented Jan 6, 2020

pared Jan 6, 2020

pared Jan 6, 2020

efiop Jan 6, 2020

efiop commented Jan 6, 2020 •

edited

Loading

sharidas commented Jan 6, 2020

efiop left a comment

	path_info = self.checksum_to_path_info(checksum)
	self.remove(path_info)

dvc: update gc to remove unpacked dir #3054

dvc: update gc to remove unpacked dir #3054

Conversation

sharidas commented Jan 4, 2020 • edited by efiop Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pared commented Jan 4, 2020

sharidas commented Jan 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

skshetry Jan 4, 2020 • edited Loading

Choose a reason for hiding this comment

efiop Jan 4, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop left a comment • edited Loading

Choose a reason for hiding this comment

sharidas commented Jan 5, 2020

efiop left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sharidas commented Jan 6, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

efiop commented Jan 6, 2020 • edited Loading

sharidas commented Jan 6, 2020

efiop left a comment

Choose a reason for hiding this comment

sharidas commented Jan 4, 2020 •

edited by efiop

Loading

skshetry Jan 4, 2020 •

edited

Loading

efiop Jan 4, 2020 •

edited

Loading

efiop left a comment •

edited

Loading

efiop commented Jan 6, 2020 •

edited

Loading