Allow use of cloud stores for intermediate zarr files #9

tomwhite · 2020-09-07T11:15:44Z

Fixes #7

jeromekelleher

Looks good @tomwhite, but we're pretty light on testing here. There's a lot of corner cases that can go wrong. I think we need tests for

Make sure that the dir=file:// works as expected
Verify sensible behaviour when dir is a local file but doesn't exist
Check that well-formed URLs that are not local files get processes as expected (I'm guessing a mock is the right way to do this: we mock out fspec.filesystem() and check that it's (a) getting called with the right args, and (b) fs.mkdir is getting called with the right args later.
Verify that name collisions are handled correctly. This should be possible to do by calling tempdir twice with random.seed() set to the same value immediately before calling.

sgkit_vcf/tests/test_utils.py

sgkit_vcf/utils.py

tomwhite · 2020-09-07T16:39:11Z

@jeromekelleher thanks for enumerating those cases. I think most of them are straightforward.

Name collisions are tricky for cloud object stores, since they don't have a first-class concept of directory. Indeed, the doc for fsspec mkdir states:

For systems that don’t have true directories, may create an [entry?] for this instance only and not touch the real filesystem

So we could easily get into race conditions and eventual consistency issues.

Instead, I think we should use a UUID (rather than the 8 character random string I used in the first PR) to ensure uniqueness.

sgkit_vcf/utils.py

jeromekelleher

LGTM, thanks @tomwhite. UUID is much better - although I wonder if it's better to go all the way with uuid4.

Fixes sgkit-dev#7

tomwhite · 2020-09-09T16:58:11Z

It turns out that this needed a bit more work to get it to handle non-local filesystems properly, after I tried using it on GCS.

Pass storage_options through API calls.
Fix (and test) tempdir URL construction (using pytest-mock, and callee).

As a result of these changes I discovered that file: URLs didn't work, and on closer inspection it looks like fsspec doesn't support them. More accurately, fsspec will get a local filesystem for a file: URL, but it won't open/mkdir a file/dir for a for a file: URL. I propose that we don't support file: URLs - at least for the moment.

tomwhite · 2020-09-09T17:01:42Z

(BTW the reason that file: URLs were working before was because I was converting them to strings using str(Path(url)). But this isn't safe for non-local URLs, e.g. s3:// or gs://. As a rule, I think we should do as little path manipulation as possible.)

hammer · 2020-09-09T17:22:00Z

I'm out of my lane here but is there a reason to manipulate URLs as strings rather than use urllib.parse?

hammer · 2020-09-09T17:26:14Z

I discovered that file: URLs didn't work, and on closer inspection it looks like fsspec doesn't support them.

Should we file an issue upstream?

tomwhite · 2020-09-10T07:28:28Z

I'm out of my lane here but is there a reason to manipulate URLs as strings rather than use urllib.parse?

Good question. Actually, urllib.parse is being used to extract the protocol from the URL string. However, urllib.parse.urljoin doesn't work for arbitrary protocols (like s3).

I wondered if we should use a URI library like uritools. But even that would require additional utility functions to extract a filename, and combine a parent directory with a child directory or file.

I discovered that file: URLs didn't work

This turned out to be a bug in the test, and file:/// URLs do work, so I've added a test for that case.

tomwhite · 2020-09-10T16:43:46Z

Hopefully final update - use yarl for URL parsing (simple API, more popular library). This is ready for review again now.

jeromekelleher

LGTM @tomwhite, I think we just need to do a bit more testing on build_url. There's a lot of different corner cases here, so we need to be reasonably thorough here I think.

jeromekelleher · 2020-09-10T16:42:20Z

sgkit_vcf/tests/test_utils.py

+
+def test_build_url():
+    assert build_url("http://host/path", "subpath") == "http://host/path/subpath"
+    assert build_url("http://host/path/", "subpath") == "http://host/path/subpath"


Be good to test this with more awkard examples:

Non http scheme (s3, say)

No scheme, just a local file path like "relative_path/stuff" and "/absolute_path/stuff/"

A URL with a query string http://host/path?x=1 -> http://host/path/subpath?x= presumably?

A URL with some url encoded entities http://host/a%20path/, say

A URL that has some things in it that need to be URL encoded, say, "http://host/!#_%" - presumably, we should always return a properly URL encoded string?

Thanks @jeromekelleher! Fixed in e2de445

jeromekelleher

LGTM, reapproving.

ravwojdyla · 2020-09-10T16:04:46Z

sgkit_vcf/utils.py

+    prefix: Optional[str] = None,
+    dir: Optional[PathType] = None,
+    storage_options: Optional[Dict[str, str]] = None,
+) -> Generator[str, None, None]:


Instead of Generator[str, None, None] you can say Iterator[str].

Thanks. Fixed in 5671652

ravwojdyla · 2020-09-10T16:07:42Z

sgkit_vcf/vcf_reader.py


 DEFAULT_ALT_NUMBER = 3  # see vcf_read.py in scikit_allel


 @contextmanager
-def open_vcf(path: PathType) -> VCF:
+def open_vcf(path: PathType) -> Generator[VCF, None, None]:


No need to use Generator, see above

ravwojdyla · 2020-09-10T17:18:07Z

sgkit_vcf/utils.py

+        yield tempdir
+    finally:
+        # Remove the temporary directory on exiting the context manager
+        fs.rm(tempdir, recursive=True)


I wonder if we should allow to control whether temp files are deleted via a flag (default = True)?

That could be useful. I've opened #10 for it.

ravwojdyla · 2020-09-10T17:34:38Z

sgkit_vcf/utils.py

+    suffix: Optional[str] = None,
+    prefix: Optional[str] = None,
+    dir: Optional[PathType] = None,
+    storage_options: Optional[Dict[str, str]] = None,


Are these storage options tested anywhere?

ravwojdyla · 2020-09-10T17:35:31Z

sgkit_vcf/utils.py

+
+    # Construct a random directory name
+    tempdir = build_url(dir, prefix + str(uuid.uuid4()) + suffix)
+    fs.mkdir(tempdir)


I wonder if this fs.mkdir call should be inside the try/finally?

jeromekelleher reviewed Sep 7, 2020

View reviewed changes

sgkit_vcf/tests/test_utils.py Outdated Show resolved Hide resolved

sgkit_vcf/utils.py Outdated Show resolved Hide resolved

sgkit_vcf/utils.py Outdated Show resolved Hide resolved

jeromekelleher reviewed Sep 7, 2020

View reviewed changes

sgkit_vcf/utils.py Outdated Show resolved Hide resolved

jeromekelleher approved these changes Sep 7, 2020

View reviewed changes

tomwhite added 8 commits September 9, 2020 14:33

Allow use of cloud stores for intermediate zarr files

9858d52

Fixes sgkit-dev#7

Remove duplicated expression

13c1395

Test missing parent directories are created.

70ca693

Test PermissionError is thrown

bdec60e

Test file://

058fcd5

Use a UUID for the random component of the directory name

09eb2a9

Fix coverage

d552d49

Use UUID4

a3cd98a

tomwhite force-pushed the cloud-tempdir branch from 0f67185 to a3cd98a Compare September 9, 2020 13:34

tomwhite added 2 commits September 9, 2020 14:43

Fix conflicts

3e169df

Handle non-local filesystem URLs.

950effa

Add back file URL test

214db9d

tomwhite added 2 commits September 10, 2020 08:43

Use uritools to manipulate filesystem URLs

7d692af

Switch out uritools for yarl

53ee391

jeromekelleher reviewed Sep 10, 2020

View reviewed changes

Add more tests for build_url

e2de445

jeromekelleher approved these changes Sep 10, 2020

View reviewed changes

ravwojdyla approved these changes Sep 10, 2020

View reviewed changes

tomwhite added 2 commits September 11, 2020 09:49

Replace Generator type with Iterator

5671652

Move mkdir into try/finally

b43f491

Test storage options

7b873b9

tomwhite mentioned this pull request Sep 14, 2020

Add option delete_tempdir=True #10

Open

tomwhite merged commit 8c8faaf into sgkit-dev:master Sep 14, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow use of cloud stores for intermediate zarr files #9

Allow use of cloud stores for intermediate zarr files #9

tomwhite commented Sep 7, 2020

jeromekelleher left a comment

tomwhite commented Sep 7, 2020

jeromekelleher left a comment

tomwhite commented Sep 9, 2020

tomwhite commented Sep 9, 2020

hammer commented Sep 9, 2020

hammer commented Sep 9, 2020

tomwhite commented Sep 10, 2020

tomwhite commented Sep 10, 2020

jeromekelleher left a comment

jeromekelleher Sep 10, 2020

tomwhite Sep 10, 2020

jeromekelleher left a comment

ravwojdyla Sep 10, 2020

tomwhite Sep 14, 2020

ravwojdyla Sep 10, 2020

ravwojdyla Sep 10, 2020

tomwhite Sep 14, 2020

ravwojdyla Sep 10, 2020

tomwhite Sep 14, 2020

ravwojdyla Sep 10, 2020

tomwhite Sep 14, 2020

Allow use of cloud stores for intermediate zarr files #9

Allow use of cloud stores for intermediate zarr files #9

Conversation

tomwhite commented Sep 7, 2020

jeromekelleher left a comment

Choose a reason for hiding this comment

tomwhite commented Sep 7, 2020

jeromekelleher left a comment

Choose a reason for hiding this comment

tomwhite commented Sep 9, 2020

tomwhite commented Sep 9, 2020

hammer commented Sep 9, 2020

hammer commented Sep 9, 2020

tomwhite commented Sep 10, 2020

tomwhite commented Sep 10, 2020

jeromekelleher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeromekelleher left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment