s3: ignore empty directories while walking files

[ghost]

Fix #2678

https://docs.aws.amazon.com/AmazonS3/latest/user-guide/using-folders.html

[ghost]

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.

[ghost]

@efiop, what kind of logic?

[ghost]

@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:

$ 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!

changes done

[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/.

[efiop]

The issue is not fixed yet.

[ghost]

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 the dir notion for S3 and using a 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.

[efiop]

So there are 2 parts to what we've discussed here in terms of the implementation.

1. collection
   Same as with local directories, walk_files should ignore empty directories. But the top level directory shouldn't be ignored, and that can be implemented by making isdir return True on empty directories. That way get_checksum will also be able to properly call get_dir_checksum instead of calling get_file_checksum on an "empty_dir" object.

2. checkout
   We shouldn't create embedded empty dirs, hence why we didn't collect them in 1. But we should create a top level empty dir. For the sake of implementation simplicity, we could simply implement makedirs for s3 to create empty_dir objects.

[ghost]

For the record, dvc add remote://s3/empty_dir already worked @{30.days.ago}.
It was just a matter of implementing makedirs

[efiop]

Thanks! 🎉

[Suor]

Sorry guys to revive this, but I still don't get the logic behind this change. If we state that an s3 dir is a collection of files with common prefix ending with /, say:

s3://bucket/path/dir/a
s3://bucket/path/dir/b
s3://bucket/path/dir/c

Or

s3://bucket/path/dir/
s3://bucket/path/dir/a
s3://bucket/path/dir/b
s3://bucket/path/dir/c

Then why do we actively creating dir files - the ones ending with /?

If we require dir file to be present then will dvc break when someone will try to dvc add the first list of files?

[efiop]

@Suor The / creation is just for a simplicity of implementation, this is how s3 creates them through web console, for example. dvc add won't break, we don't require / on collection, we just simplify the checkout implementation by creating /, so that if someone would to add dvc add s3://bucket/empty_dir, then we would be able to properly re-create it on checkout.