Repository: iterative/dvc

Issue:
Possible bug related to re-computing md5 for directories
DVC 0.29.0 / ubuntu / pip

I believe you recently fixed a bug related to re-computing md5s for large files.  There might be something similar happening again — or maybe I just need to better understand what triggers md5 to be computed.

$ dvc status
Computing md5 for a large directory project/data/images. This is only done once.
[##############################] 100% project/data/images

This happens not every time I run dvc status, but at least every time I reboot the machine — I haven't 100% narrowed down what triggers it.  Is this expected?

It's not super high priority — it only takes ~30 seconds to re-compute md5s for these directories, which is kind of surprisingly fast.  Could it be caching file-level (image-level) md5s and then simply recomputing the directory-level md5?



Discussion:
Hi @colllin. Thanks for reporting, it is not desired behaviour, there is PR trying to tackle this one:
#1526 

> it only takes ~30 seconds to re-compute md5s for these directories, which is kind of surprisingly fast. >Could it be caching file-level (image-level) md5s and then simply recomputing the directory-level md5?

Currently, besides md5 we store modification time and size of file, and, for given inode, we check if file mtime or size has been changed. If it has not, we assume that we do not need to recompute md5. So yes, we are caching file level md5s.
For the record, looks like it can be reproduced this way:
```
#!/bin/bash                                                         
                                                                    
set -x                                                              
set -e                                                              
                                                                    
rm -rf myrepo                                                       
mkdir myrepo                                                        
cd myrepo                                                           
git init                                                            
dvc init                                                            
                                                                    
git commit -m"init"                                                 
                                                                    
mkdir dir                                                           
for i in $(seq 1 1000); do                                          
    echo $i > dir/$i                                                
done                                                                
dvc add dir                                                         
dvc status                                                          
dvc status                                                          
```

which produces

```
+ dvc add dir                                                   
Computing md5 for a large directory dir. This is only done once.
[##############################] 100% dir                       
Adding 'dir' to '.gitignore'.                                   
Saving 'dir' to cache '.dvc/cache'.                             
Linking directory 'dir'.                                        
[##############################] 100% dir                       
Saving information to 'dir.dvc'.                                
                                                                
To track the changes with git run:                              
                                                                
        git add .gitignore dir.dvc                              
+ dvc status                                                    
Computing md5 for a large directory dir. This is only done once.
[##############################] 100% dir                       
Pipeline is up to date. Nothing to reproduce.                   
+ dvc status                                                    
Pipeline is up to date. Nothing to reproduce.
```

We don't print a progress bar when verifying cache for a directory, so looks like there is something else that we've forgotten to update, which makes first `dvc status` actually compute something once again.
@colllin sorry, I made a mistake, it seems there is something more to this case. 
Thanks @efiop, Ill look into that.

Patch:
diff --git a/dvc/remote/local.py b/dvc/remote/local.py
--- a/dvc/remote/local.py
+++ b/dvc/remote/local.py
@@ -422,6 +422,7 @@ def _save_dir(self, path, md5):
 
         cache = self.get(md5)
         self.state.update(cache)
+        self.state.update(path, md5)
 
         if bar:
             progress.finish_target(dir_relpath)


Test patch:
diff --git a/tests/test_add.py b/tests/test_add.py
--- a/tests/test_add.py
+++ b/tests/test_add.py
@@ -299,3 +299,35 @@ def test(self):
             ret = main(["status", "{}.dvc".format(self.DATA_DIR)])
             self.assertEqual(ret, 0)
             self.assertEqual(1, remote_local_loader_spy.mock.call_count)
+
+
+class TestShouldCollectDirCacheOnlyOnce(TestDvc):
+    NEW_LARGE_DIR_SIZE = 1
+
+    @patch("dvc.remote.local.LARGE_DIR_SIZE", NEW_LARGE_DIR_SIZE)
+    def test(self):
+        from dvc.remote.local import RemoteLOCAL
+
+        collect_dir_counter = spy(RemoteLOCAL.collect_dir_cache)
+        with patch.object(
+            RemoteLOCAL, "collect_dir_cache", collect_dir_counter
+        ):
+
+            LARGE_DIR_FILES_NUM = self.NEW_LARGE_DIR_SIZE + 1
+            data_dir = "dir"
+
+            os.makedirs(data_dir)
+
+            for i in range(LARGE_DIR_FILES_NUM):
+                with open(os.path.join(data_dir, str(i)), "w+") as f:
+                    f.write(str(i))
+
+            ret = main(["add", data_dir])
+            self.assertEqual(0, ret)
+
+            ret = main(["status"])
+            self.assertEqual(0, ret)
+
+            ret = main(["status"])
+            self.assertEqual(0, ret)
+        self.assertEqual(1, collect_dir_counter.mock.call_count)