Support adding directories in google cloud storage remote #2853

skshetry · 2019-11-26T13:37:57Z

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

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

Description

I have reused test code from unit/test_s3.py by moving it to functional tests, as, I argue, moto server can be assumed as functional as real s3 (which is a selling point of moto itself). The same tests are run with different remotes (i.e. s3 and gs).

And, I have stolen most of the code from #2619.

The logic behind s3 and gs is almost similar, as they both are object storages. So, there is an opportunity for abstraction for ObjectStorageRemote, but, I think that it'll be over-engineering for now (what about azure?).

Todo

Reuse code instead of copy-pasting.
Check for empty_dir. Will that work?
Use prefix for the test, or custom bucket (might not be possible locally)
Check if tests for s3 and gs can be combined with different remotes.
Rebase with current master.
Run some manual tests with directories.
Run all tests locally 2-3 times with DVC_TEST_GCP set and make it pass.
Make this PR ready for review.

How was this tested?

Upload folder in structure similar to the following:

$ gsutil ls -r gs://dvc-test/skshetry-test
gs://dvc-test/skshetry-test/:
gs://dvc-test/skshetry-test/data1.txt

gs://dvc-test/skshetry-test/data/:
gs://dvc-test/skshetry-test/data/data2.txt
gs://dvc-test/skshetry-test/data/data3.txt
gs://dvc-test/skshetry-test/data/data5.txt

gs://dvc-test/skshetry-test/data/subdir/:
gs://dvc-test/skshetry-test/data/subdir/data4.txt

gs://dvc-test/skshetry-test/data/subdir2/:
gs://dvc-test/skshetry-test/data/subdir2/data6.txt
gs://dvc-test/skshetry-test/data/subdir2/data7.txt

gs://dvc-test/skshetry-test/data/subdir3/:

gs://dvc-test/skshetry-test/data/subdir3/nested/:
gs://dvc-test/skshetry-test/data/subdir3/nested/data6.txt

$ git init
$ dvc init
$ dvc remote add gscache gs://dvc-test/skshetry-test
$ dvc config cache.gs gscache

dvc add gs://dvc-test/skshetry-test/data (check gs://dvc-test/skshetry-test/data1.txt should not get uploaded).
Reset cache, and dvc add gs://dvc-test/skshetry-test/data/subdir.
Similar to step 4 with dvc add gs://dvc-test/skshetry-test/data/subdir2.
Similar to step 4 with dvc add gs://dvc-test/skshetry-test/data/subdir3.
Similar to step 4 with dvc add gs://dvc-test/skshetry-test and dvc add gs://dvc-test/skshetry-test/.
Reset cache and dvc run -d remote://gs/data 'echo hello world'.

Fixes #2814

tests/func/test_gs.py

Suor · 2019-11-26T14:44:42Z

Am I missing something? We agreed to not add this for now. The ticket #1654 is closed.

skshetry · 2019-11-26T14:46:48Z

@Suor, there is another issue for gs only: #2814. Forgot to mention in the PR.

efiop · 2019-11-27T15:35:27Z

FYI: added [WIP] prefix to make it more obvious ("draft" is fine, but prefix seems to be a bit easier to grasp, no biggie) 🙂

…s/isdir

dvc/remote/gs.py

efiop · 2019-11-28T11:04:01Z

@skshetry About empty dirs, have you seen #2678 ? Does gs have a concept of empty dirs? 🙂

Suor

Ok, good start.

We need to unify this and some existing code both in remotes and tests, see below.

dvc/remote/base.py

dvc/remote/gs.py

tests/func/test_remote_dir.py

skshetry · 2019-11-28T13:52:18Z

@skshetry About empty dirs, have you seen #2678 ? Does gs have a concept of empty dirs?

Yep, I saw that, and this also suffers from same issue.

➜ dvc add gs://dvc-test/skshetry-test/empty-dir/
100%|██████████|Add                                                                                                 1.00/1.00 [00:21<00:00,  21.1s/file]
ERROR: 'skshetry-test/empty-dir/.' doesn't exist in the cloud

gsutil does not support creating empty directories, but Google Cloud Console seems to support this which I can't confirm as I don't have access to it. And, we can emulate this by creating blob with trailing slash (gsutil provides an illusion to this.)

I didn't act on it because of #2683 (comment), as I am not really sure what you mean by solving it properly. I guess, I was just waiting for how it will be solved for s3. But, what @MrOutis has done in the PR, same can be applied here char-by-char.

cc @efiop

EDIT: talked with efiop on chat

ignore empty dirs inside of the directory we are adding,
but not ignore it if the whole directory is empty.
That way it will have the same behaviour as in local empty dirs.

efiop · 2019-11-28T16:49:53Z

For the record, had an additional discussion on discord: https://discordapp.com/channels/485586884165107732/565699007037571084/649634041993101333

tests/func/test_remote_dir.py

dvc/remote/gs.py

@efiop

* master: address @efiop's suggestions s3: add support for top level empty directories remote: http: calculate length basing on response and not head call test: remote: http: clear test file name HTTPError: reformat error message Update dvc/remote/http.py remote: http: rename http error, refactor test remote: http: raise exception when download response with error status code Support GDrive as remote setup: don't forget contextlib2 remote: refactor ssh ask password code remote: protect all remote client/session creation code with locks test: refactor & remove redundant test fixtures NoRemoteInExternalRepoError: dont pass cause of exception perf: optimize cache listing for local, ssh and hdfs remote: small .save_info()/.get_checksum() cleanup

Suor · 2019-11-30T12:51:46Z

Wet should list all, not only first one indeed. Good catch. сб, 30 нояб. 2019 г., 13:43 saugat pachhai <[email protected]>:

…

***@***.**** commented on this pull request. ------------------------------ In dvc/remote/gs.py <#2853 (comment)>: > + dir_path = path_info / "" + fname = next(self._list_paths(path_info, max_items=1), "") + return path_info.path == fname or fname.startswith(dir_path.path) Yes, sorry, I did not explain it correctly. The next() could provide you subdir-file.txt which won't match, therefore, it returning False, resulting in 'data/subdir' does not exist message. There's no short-circuiting for checking if a given file/folder exists. You have to first check if the folder exists, and then, loop through it until you find exact match. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2853?email_source=notifications&email_token=AACFLR7SOU4VYZ4WN34NH2LQWJNVZA5CNFSM4JRYD7T2YY3PNVWWK3TUL52HS4DFWFIHK3DMKJSXC5LFON2FEZLWNFSXPKTDN5WW2ZLOORPWSZGOCNOXC2I#discussion_r352283639>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AACFLR3FR5LN3P2JKUXT24LQWJNVZANCNFSM4JRYD7TQ> .

efiop · 2019-11-30T18:20:55Z

For the record: windows tests are failing due to unrelated problems with chocolatey packages. The rest of the tests are fine. ;)

EDIT: sorry, accidentally closed 🙁

Suor · 2019-11-30T19:25:28Z

Wrote about listing for exists here - #2873 (comment). TLDR, the reason why we do listing instead of some Blob.exists() call is unknown. If we may do the latter then we won't have all those issues.

BTW, we will still need to list to check if dir exists, which is not good for cache batch exists, quoting from there:

Additional consideration about cache, we don't really want to check .isdir() for every checksum, but we want .exists() to work for both files and dirs for remotes supporting that. This contradiction might be resolved by calling .isfile() instead of .exists() inside .cache_exists(). We will need to make isfile() work properly though, now it is just a stab in RemoteBASE.

skshetry · 2019-12-01T03:12:43Z

Thanks @Suor. I was too pinned on checking for the directory, that I missed it. I implemented isfile, but for the .cache_exists(), we could implement that in #2873 or in a separate PR.

* master: travis: windows: use python 3.7.5 test: skip non supported remotes fast in api tests

dvc/remote/gs.py

tests/func/test_api.py

Suor · 2019-12-01T08:57:15Z

tests/remotes.py

@@ -0,0 +1,337 @@
+import os


If you postpone, then create an issue, add a link here and resolve this thread :)

tests/remotes.py

Suor · 2019-12-01T09:42:16Z

tests/unit/remote/test_remote_dir.py

+@pytest.mark.parametrize("remote", [S3Mocked], indirect=True)
 def test_copy_preserve_etag_across_buckets(remote):


We don't need to create all those blobs for this test and parametrize looks a bit stretched. I suggest using @mock_s3 and creating both remotes simply with RemoteS3(...) and not using S3Mocked here.

Current approach is also fine, I guess.

Keeping it this way for now, then. :)

skshetry · 2019-12-01T11:08:21Z

tests/unit/remote/test_remote_dir.py

+@pytest.mark.parametrize("remote", [GCP], indirect=True)
+def test_isfile(remote):
+    test_cases = [
+        (False, "empty_dir/"),
+        (True, "empty_file"),
+        (True, "foo"),
+        (True, "data/alice"),
+        (True, "data/alpha"),
+        (True, "data/subdir/1"),
+        (True, "data/subdir/2"),
+        (True, "data/subdir/3"),
+        (False, "data/subdir/empty_dir/"),
+        (True, "data/subdir/empty_file"),
+        (False, "something-that-does-not-exist"),
+        (False, "data/subdir/empty-file/"),
+        (False, "empty_dir"),
+    ]
+
+    for expected, path in test_cases:
+        assert remote.isfile(remote.path_info / path) == expected


This test will need adjustment if #2873 gets merged.

Yeah, let's merge #2873 first, once we get one more approval 😉 And then we'll adjust and merge this one.

@skshetry #2873 is merged, please adjust this PR accordingly 🙂

Suor

It looks way cleaner than average dvc code now) And thanks for starting a remote fixtures refactor.

Suor · 2019-12-01T11:20:16Z

dvc/remote/gs.py

+
+        eg: if `data/file.txt` exists, check for `data` should return True
+        """
+        return self.isfile(path_info) or self.isdir(path_info / "")


Suggested change

return self.isfile(path_info) or self.isdir(path_info / "")

return self.isfile(path_info) or self.isdir(path_info)

No need to make it twice.

Suor · 2019-12-01T11:33:03Z

BTW, need to create .cache_exists()/isfile() issue before closing it.

skshetry · 2019-12-01T11:55:29Z

@Suor, I have created an issue: #2877. Regarding the suggestion #2853 (comment), I'll make a issue after this gets merged.

efiop · 2019-12-01T12:50:20Z

Great stuff, thank you so much @skshetry ! 🚀

For the record: windows tests are failing due to temporary problems with chocolatey. Ignoring that for now.

skshetry commented Nov 26, 2019

View reviewed changes

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

efiop changed the title ~~Support adding directories in google cloud storage remote~~ WIP Support adding directories in google cloud storage remote Nov 27, 2019

efiop changed the title ~~WIP Support adding directories in google cloud storage remote~~ [WIP] Support adding directories in google cloud storage remote Nov 27, 2019

skshetry force-pushed the gs-external-deps branch 3 times, most recently from 73d6102 to 87ee935 Compare November 27, 2019 17:08

skshetry added 4 commits November 28, 2019 00:07

remote/gs: support adding directories

e750496

remote/gs: set max_results to improve performance when checking exist…

5e2150d

…s/isdir

dvc: append slash at the end for dir path_info

0c08c09

dvc: use replace instead of posixpath

fe248aa

skshetry force-pushed the gs-external-deps branch from 87ee935 to fe248aa Compare November 28, 2019 00:17

skshetry marked this pull request as ready for review November 28, 2019 00:17

skshetry changed the title ~~[WIP] Support adding directories in google cloud storage remote~~ Support adding directories in google cloud storage remote Nov 28, 2019

remove extra args

db12416

shcheklein requested a review from a user November 28, 2019 00:28

skshetry commented Nov 28, 2019

View reviewed changes

dvc/remote/gs.py Outdated Show resolved Hide resolved

gs: return relative path in list_cache_paths

54dd989

Suor suggested changes Nov 28, 2019

View reviewed changes

gs: make it work with empty dir

ee69ac8

skshetry commented Nov 29, 2019

View reviewed changes

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

skshetry commented Nov 29, 2019

View reviewed changes

dvc/remote/gs.py Show resolved Hide resolved

skshetry added 3 commits November 30, 2019 08:22

dvc: refactor tests

caa67c7

Move to tests/remotes.py

63cf117

test: remove nested contextmanagers, return False on exists()

433d2f8

efiop requested a review from Suor November 30, 2019 18:17

efiop closed this Nov 30, 2019

efiop reopened this Nov 30, 2019

skshetry added 2 commits December 1, 2019 08:25

gs: use Blob.exists() instead of _list_paths to check for blob

25f0e07

gs: implement isfile

7e62a7d

Merge branch 'master' into gs-external-deps

f4f836e

* master: travis: windows: use python 3.7.5 test: skip non supported remotes fast in api tests

weekly-digest bot mentioned this pull request Dec 1, 2019

Weekly Digest (24 November, 2019 - 1 December, 2019) #2876

Closed

Suor suggested changes Dec 1, 2019

View reviewed changes

skshetry added 2 commits December 1, 2019 16:08

address @efiop's suggestions

22c0a64

use yield and staticmethod

4f011f6

efiop approved these changes Dec 1, 2019

View reviewed changes

efiop requested a review from Suor December 1, 2019 11:02

skshetry commented Dec 1, 2019

View reviewed changes

Suor approved these changes Dec 1, 2019

View reviewed changes

gs: adjust tests

6ba7b1d

skshetry mentioned this pull request Dec 1, 2019

Do not check for directory when looking for cache files #2877

Closed

skshetry requested a review from efiop December 1, 2019 12:12

efiop merged commit 3d70a1e into iterative:master Dec 1, 2019

skshetry deleted the gs-external-deps branch December 1, 2019 12:50

skshetry mentioned this pull request Dec 1, 2019

Refactor test/remotes.py by merging inside remote classes #2878

Closed

ghost mentioned this pull request Dec 5, 2019

tests: use stubbers for unit tests #907

Closed

2 tasks

weekly-digest bot mentioned this pull request Dec 8, 2019

Weekly Digest (1 December, 2019 - 8 December, 2019) #2919

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support adding directories in google cloud storage remote #2853

Support adding directories in google cloud storage remote #2853

skshetry commented Nov 26, 2019 •

edited

Loading

Suor commented Nov 26, 2019

skshetry commented Nov 26, 2019

efiop commented Nov 27, 2019

efiop commented Nov 28, 2019

Suor left a comment

skshetry commented Nov 28, 2019 •

edited

Loading

efiop commented Nov 28, 2019

Suor commented Nov 30, 2019 via email

efiop commented Nov 30, 2019 •

edited

Loading

Suor commented Nov 30, 2019

skshetry commented Dec 1, 2019

Suor Dec 1, 2019

Suor Dec 1, 2019

Suor Dec 1, 2019

skshetry Dec 1, 2019

skshetry Dec 1, 2019

efiop Dec 1, 2019 •

edited

Loading

efiop Dec 1, 2019

Suor left a comment

Suor Dec 1, 2019

Suor commented Dec 1, 2019

skshetry commented Dec 1, 2019

efiop commented Dec 1, 2019

		@pytest.mark.parametrize("remote", [S3Mocked], indirect=True)
		def test_copy_preserve_etag_across_buckets(remote):

	return self.isfile(path_info) or self.isdir(path_info / "")
	return self.isfile(path_info) or self.isdir(path_info)

Support adding directories in google cloud storage remote #2853

Support adding directories in google cloud storage remote #2853

Conversation

skshetry commented Nov 26, 2019 • edited Loading

Description

Todo

How was this tested?

Suor commented Nov 26, 2019

skshetry commented Nov 26, 2019

efiop commented Nov 27, 2019

efiop commented Nov 28, 2019

Suor left a comment

Choose a reason for hiding this comment

skshetry commented Nov 28, 2019 • edited Loading

efiop commented Nov 28, 2019

Suor commented Nov 30, 2019 via email

efiop commented Nov 30, 2019 • edited Loading

Suor commented Nov 30, 2019

skshetry commented Dec 1, 2019

Suor Dec 1, 2019

Choose a reason for hiding this comment

Suor Dec 1, 2019

Choose a reason for hiding this comment

Suor Dec 1, 2019

Choose a reason for hiding this comment

skshetry Dec 1, 2019

Choose a reason for hiding this comment

skshetry Dec 1, 2019

Choose a reason for hiding this comment

efiop Dec 1, 2019 • edited Loading

Choose a reason for hiding this comment

efiop Dec 1, 2019

Choose a reason for hiding this comment

Suor left a comment

Choose a reason for hiding this comment

Suor Dec 1, 2019

Choose a reason for hiding this comment

Suor commented Dec 1, 2019

skshetry commented Dec 1, 2019

efiop commented Dec 1, 2019

skshetry commented Nov 26, 2019 •

edited

Loading

skshetry commented Nov 28, 2019 •

edited

Loading

efiop commented Nov 30, 2019 •

edited

Loading

efiop Dec 1, 2019 •

edited

Loading