Repository: iterative/dvc

Issue:
Unable to dvc add more than 1 file at a time in s3 bucket
** Note: DVC version 0.66.1, pip install, Ubuntu 16.04

I am having difficulty configuring DVC to track files on an s3 bucket with the structure

```
ellesdatabucket
       ├── data
       │     ├── img1.png
       │     ├── img2.png
       │     ├── ...
       └── cache
```

Specifically, I want to use DVC to version control *.png files stored in the data folder, and use the cache folder as DVC's cache.

Based on the docs provided [here](https://dvc.org/doc/user-guide/managing-external-data#amazon-s-3), I believe I've replicated exactly the provided steps. But I hit an error when I run `dvc add`:
```
$ git init
$ dvc init
$ dvc remote add myremote s3://ellesdatabucket/data
$ dvc remote add s3cache s3://ellesdatabucket/cache
$ dvc config cache.s3 s3cache
$ dvc add s3://ellesdatabucket/data
```
The output looks initially encouraging,
``` 0%|          |Add                                      0/1 [00:00<?,     ?file/s]
 29%|██▊       |Computing hashes (only done once691/2424 [00:03<00:07,    223md5/s]
```

But then I get this error message:

```100%|██████████|Add                                  1/1 [00:07<00:00,  7.56s/file]
ERROR: s3://ellesdatabucket/data/. does not exist: An error occurred (404) when calling the HeadObject operation: Not Found
```

I'm positive that there are files in the data folder and can view them by `aws s3 ls s3://ellesdatabucket/data`. And, if I try to run `dvc add` with only a single file at a time instead of a whole directory, the command completes successfully. Although a bash script could `dvc add` each file in a loop, I want to make sure there's not a better way. [Issue 2647](https://github.com/iterative/dvc/issues/2647) seems to be discussing a similar problem but I can't figure out how to apply that code to my own example here. 

Thank you for any help!

**More context**:

https://discordapp.com/channels/485586884165107732/485596304961962003/637836708184064010



Discussion:
mroutis: Had an offline discussion with @efiop where we argued if DVC should track empty directories or not.
I agreed on submitting a patch that supports empty directories for S3 but it is not straight forward.

`git` ignores empty directories, it is convention to add an empty stub file (e.g. `.keep`, `.gitkeep`) if you want to track it.

What do you think, @Suor , @efiop , sholud we track empty dirs?
efiop: @mroutis It is not an empty dir, it is a special object that is used that way. I think we have to support it, as someone might have those outputs as empty dirs and have logic built on the assumption that that prefix already exists.
mroutis: @efiop, what kind of logic?
mroutis: @efiop , depending on existing directories breaks reproducibility (preferred way: `dvc run -o data 'mkdir data && fill_data.sh'`) .  In case you are depending on a directory it would cause overlapping stages:

```console
$ dvc init --no-scm
$ mkdir data
$ echo 'foo' > data/foo
$ dvc add data
$ echo 'bar' > data/bar
$ dvc add data/bar

ERROR: Paths for outs:
'data'('data.dvc')
'data/bar'('data/bar.dvc')
overlap. To avoid unpredictable behaviour, rerun command with non overlapping outs paths.

Having any troubles? Hit us up at https://dvc.org/support, we are always happy to help!
```
efiop: @mroutis The logic of something assuming that the prefix exists. For example your first stage is creating that directory that way by doing `aws s3api put-object --bucket bucket --key empty_dir/` and then doesn't have anything to put in there. Next stage expects that prefix to exist, but won't find it if we don't cache and checkout it properly.

> changes done

This patch still doesn't handle `aws s3api put-object --bucket bucket --key empty_dir/`.

mroutis: > Next stage expects that prefix to exist, but won't find it if we don't cache and checkout it properly.
@efiop , but why would the next stage expect that such prefix exist?
efiop: @mroutis I have no idea, something created that empty dir in users example, so something is using it and I won't be surprised if something else would be expecting it to exist. I would think about it as creating an empty dir in local fs, but just adapted to s3 caveats.
Suor: I believe the original issue - "an empty dir" created by user making a mess - is fixed. The supposition that some later user code will rely on that "empty dir" to exist is a separate thing. 

I am not sure it's actually a thing, the issue might, however, come up if a user is using "a dir file" as an actual file with contents. And this can't be fixed without giving up on dir notion for S3 and use prefix notion instead. 
efiop: @Suor This PR is hiding the issue and we still don't support "empty dirs" here. We should solve it properly.

Patch:
diff --git a/dvc/remote/s3.py b/dvc/remote/s3.py
index 5594806f0..8013f9bb9 100644
--- a/dvc/remote/s3.py
+++ b/dvc/remote/s3.py
@@ -212,6 +212,15 @@ class RemoteS3(RemoteBASE):
         fname = next(self._list_paths(path_info, max_items=1), "")
         return path_info.path == fname or fname.startswith(dir_path.path)
 
+    def makedirs(self, path_info):
+        # We need to support creating empty directories, which means
+        # creating an object with an empty body and a trailing slash `/`.
+        #
+        # We are not creating directory objects for every parent prefix,
+        # as it is not required.
+        dir_path = path_info / ""
+        self.s3.put_object(Bucket=path_info.bucket, Key=dir_path.path, Body="")
+
     def isdir(self, path_info):
         # S3 doesn't have a concept for directories.
         #
@@ -271,4 +280,7 @@ class RemoteS3(RemoteBASE):
 
     def walk_files(self, path_info, max_items=None):
         for fname in self._list_paths(path_info, max_items):
+            if fname.endswith("/"):
+                continue
+
             yield path_info.replace(path=fname)


Test patch:
diff --git a/tests/unit/remote/test_s3.py b/tests/unit/remote/test_s3.py
index a82d005c4..49fc3dbb8 100644
--- a/tests/unit/remote/test_s3.py
+++ b/tests/unit/remote/test_s3.py
@@ -79,9 +79,11 @@ def test_walk_files(remote):
         remote.path_info / "data/subdir/1",
         remote.path_info / "data/subdir/2",
         remote.path_info / "data/subdir/3",
+        remote.path_info / "empty_file",
+        remote.path_info / "foo",
     ]
 
-    assert list(remote.walk_files(remote.path_info / "data")) == files
+    assert list(remote.walk_files(remote.path_info)) == files
 
 
 def test_copy_preserve_etag_across_buckets(remote):
@@ -99,3 +101,11 @@ def test_copy_preserve_etag_across_buckets(remote):
     to_etag = RemoteS3.get_etag(s3, "another", "foo")
 
     assert from_etag == to_etag
+
+
+def test_makedirs(remote):
+    empty_dir = remote.path_info / "empty_dir" / ""
+    remote.remove(empty_dir)
+    assert not remote.exists(empty_dir)
+    remote.makedirs(empty_dir)
+    assert remote.exists(empty_dir)