Repository: fatiando/pooch

Issue:
Delay cache folder creation to Pooch.fetch
**Description of the desired feature**

<!--
Please be as detailed as you can in your description. If possible, include an
example of how you would like to use this feature (even better if it's a code
example).
-->

`pooch.create` tries to ensure that the cache folder exists by creating it and trying to write to it through   `pooch.utils.make_local_storage`. This can cause problems if users call `create` to make a module global variable (since it will run at import time). If the import is run in parallel, this can lead to a race condition. #171 tried to fix this but it's not ideal since we shouldn't be writing to disk at import time anyway. A better approach would be to delay the folder creation to `Pooch.fetch` (which already does this https://github.com/fatiando/pooch/blob/master/pooch/core.py#L548). 

In this case, `make_local_storage` needs to be modified to only create the folder name (it would probably need to be renamed). This would also make #150 and comments in there unnecessary since we don't need to check for permission error if we're not writing to disk until `fetch` time. `retrieve` will also need to be modified to create the cache folder as well (like `fetch` does). We might want to wrap the folder creation so that we can check for permission error and add a message about the environment variable to the exception.

There might be unintended consequences to not checking if the cache folder is writable at import time. #22 is when we decided to do this in the first place.

See scikit-image/scikit-image#4660


**Are you willing to help implement and maintain this feature?** Yes

<!--
Every feature we add is code that we will have to maintain and keep updated.
This takes a lot of effort. If you are willing to be involved in the project
and help maintain your feature, it will make it easier for us to accept it.
-->


Discussion:
leouieda: This should fix the problems encountered in scikit-image/scikit-image#4660. I'd appreciate a review to see if I didn't do anything stupid here that would break other things :slightly_smiling_face:.
leouieda: Thanks for the comments @hmaarrfk and @hugovk! I'll merge this in and make a 1.1.1 release to get this out to the affected users.

@danshapero @andersy005 sorry for rushing this through and not waiting for your comments. 

Patch:
diff --git a/pooch/core.py b/pooch/core.py
index 8a09770..9b3ea19 100644
--- a/pooch/core.py
+++ b/pooch/core.py
@@ -13,6 +13,7 @@ from .utils import (
     parse_url,
     get_logger,
     make_local_storage,
+    cache_location,
     hash_matches,
     temporary_file,
     os_cache,
@@ -200,9 +201,10 @@ def retrieve(url, known_hash, fname=None, path=None, processor=None, downloader=
         fname = unique_file_name(url)
     # Create the local data directory if it doesn't already exist and make the
     # path absolute.
-    path = make_local_storage(path, env=None, version=None).resolve()
+    path = cache_location(path, env=None, version=None)
+    make_local_storage(path)
 
-    full_path = path / fname
+    full_path = path.resolve() / fname
     action, verb = download_action(full_path, known_hash)
 
     if action in ("download", "update"):
@@ -253,6 +255,12 @@ def create(
     ``https://github.com/fatiando/pooch/raw/v0.1/data``). If the version string
     contains ``+XX.XXXXX``, it will be interpreted as a development version.
 
+    Does **not** create the local data storage folder. The folder will only be
+    created the first time a download is attempted with
+    :meth:`pooch.Pooch.fetch`. This makes it safe to use this function at the
+    module level (so it's executed on ``import`` and the resulting
+    :class:`~pooch.Pooch` is a global variable).
+
     Parameters
     ----------
     path : str, PathLike, list or tuple
@@ -307,6 +315,9 @@ def create(
     ...              registry={"data.txt": "9081wo2eb2gc0u..."})
     >>> print(pup.path.parts)  # The path is a pathlib.Path
     ('myproject', 'v0.1')
+    >>> # The local folder is only created when a dataset is first downloaded
+    >>> print(pup.path.exists())
+    False
     >>> print(pup.base_url)
     http://some.link.com/v0.1/
     >>> print(pup.registry)
@@ -368,7 +379,12 @@ def create(
     if version is not None:
         version = check_version(version, fallback=version_dev)
         base_url = base_url.format(version=version)
-    path = make_local_storage(path, env, version)
+    # Don't create the cache folder here! This function is usually called in
+    # the module context (at import time), so touching the file system is not
+    # recommended. It could cause crashes when multiple processes/threads try
+    # to import at the same time (which would try to create the folder several
+    # times at once).
+    path = cache_location(path, env, version)
     pup = Pooch(path=path, base_url=base_url, registry=registry, urls=urls)
     return pup
 
@@ -545,7 +561,7 @@ class Pooch:
         self._assert_file_in_registry(fname)
 
         # Create the local data directory if it doesn't already exist
-        os.makedirs(str(self.abspath), exist_ok=True)
+        make_local_storage(str(self.abspath))
 
         url = self.get_url(fname)
         full_path = self.abspath / fname
diff --git a/pooch/utils.py b/pooch/utils.py
index fa88114..93e03c9 100644
--- a/pooch/utils.py
+++ b/pooch/utils.py
@@ -216,11 +216,12 @@ def parse_url(url):
     return {"protocol": protocol, "netloc": parsed_url.netloc, "path": parsed_url.path}
 
 
-def make_local_storage(path, env=None, version=None):
+def cache_location(path, env=None, version=None):
     """
-    Create the local cache directory and make sure it's writable.
+    Location of the cache given a base path and optional configuration.
 
-    If the directory doesn't exist, it will be created.
+    Checks for the environment variable to overwrite the path of the local
+    cache. Optionally add *version* to the path if given.
 
     Parameters
     ----------
@@ -249,6 +250,22 @@ def make_local_storage(path, env=None, version=None):
     if version is not None:
         path = os.path.join(str(path), version)
     path = os.path.expanduser(str(path))
+    return Path(path)
+
+
+def make_local_storage(path, env=None):
+    """
+    Create the local cache directory and make sure it's writable.
+
+    Parameters
+    ----------
+    path : str or PathLike
+        The path to the local data storage folder.
+    env : str or None
+        An environment variable that can be used to overwrite *path*. Only used
+        in the error message in case the folder is not writable.
+    """
+    path = str(path)
     # Check that the data directory is writable
     try:
         if not os.path.exists(path):
@@ -261,24 +278,19 @@ def make_local_storage(path, env=None, version=None):
             action = "write to"
             with tempfile.NamedTemporaryFile(dir=path):
                 pass
-    except PermissionError:
-        # Only log an error message instead of raising an exception. The cache
-        # is usually created at import time, so raising an exception here would
-        # cause packages to crash immediately, even if users aren't using the
-        # sample data at all. So issue a warning here just in case and only
-        # crash with an exception when the user actually tries to download
-        # data (Pooch.fetch or retrieve).
-        message = (
-            "Cannot %s data cache folder '%s'. "
-            "Will not be able to download remote data files. "
-        )
-        args = [action, path]
+    except PermissionError as error:
+        message = [
+            str(error),
+            "| Pooch could not {} data cache folder '{}'.".format(action, path),
+            "Will not be able to download data files.",
+        ]
         if env is not None:
-            message += "Use environment variable '%s' to specify another directory."
-            args += [env]
-
-        get_logger().warning(message, *args)
-    return Path(path)
+            message.append(
+                "Use environment variable '{}' to specify a different location.".format(
+                    env
+                )
+            )
+        raise PermissionError(" ".join(message)) from error
 
 
 def hash_algorithm(hash_string):


Test patch:
diff --git a/pooch/tests/test_integration.py b/pooch/tests/test_integration.py
index 7bfbc13..c693625 100644
--- a/pooch/tests/test_integration.py
+++ b/pooch/tests/test_integration.py
@@ -6,35 +6,27 @@ import os
 import shutil
 from pathlib import Path
 
-import pytest
-
 from .. import create, os_cache
 from ..version import full_version
 from .utils import check_tiny_data, capture_log
 
 
-@pytest.fixture
-def pup():
-    "Create a pooch the way most projects would."
-    doggo = create(
-        path=os_cache("pooch"),
+def test_create_and_fetch():
+    "Fetch a data file from the local storage"
+    path = os_cache("pooch-testing")
+    if path.exists():
+        shutil.rmtree(str(path))
+    pup = create(
+        path=path,
         base_url="https://github.com/fatiando/pooch/raw/{version}/data/",
         version=full_version,
         version_dev="master",
         env="POOCH_DATA_DIR",
     )
-    # The str conversion is needed in Python 3.5
-    doggo.load_registry(str(Path(os.path.dirname(__file__), "data", "registry.txt")))
-    if os.path.exists(str(doggo.abspath)):
-        shutil.rmtree(str(doggo.abspath))
-    yield doggo
-    shutil.rmtree(str(doggo.abspath))
-
-
-def test_fetch(pup):
-    "Fetch a data file from the local storage"
-    # Make sure the storage has been cleaned up before running the tests
+    # Make sure the storage isn't created until a download is required
     assert not pup.abspath.exists()
+    # The str conversion is needed in Python 3.5
+    pup.load_registry(str(Path(os.path.dirname(__file__), "data", "registry.txt")))
     for target in ["tiny-data.txt", "subdir/tiny-data.txt"]:
         with capture_log() as log_file:
             fname = pup.fetch(target)
diff --git a/pooch/tests/test_utils.py b/pooch/tests/test_utils.py
index 536db26..5ffe7cb 100644
--- a/pooch/tests/test_utils.py
+++ b/pooch/tests/test_utils.py
@@ -22,7 +22,7 @@ from ..utils import (
     temporary_file,
     unique_file_name,
 )
-from .utils import check_tiny_data, capture_log
+from .utils import check_tiny_data
 
 DATA_DIR = str(Path(__file__).parent / "data" / "store")
 REGISTRY = (
@@ -77,7 +77,8 @@ def test_make_local_storage_parallel(pool, monkeypatch):
                 executor.submit(make_local_storage, data_cache) for i in range(4)
             ]
             for future in futures:
-                assert os.path.exists(str(future.result()))
+                future.result()
+            assert os.path.exists(data_cache)
     finally:
         if os.path.exists(data_cache):
             shutil.rmtree(data_cache)
@@ -95,13 +96,12 @@ def test_local_storage_makedirs_permissionerror(monkeypatch):
 
     monkeypatch.setattr(os, "makedirs", mockmakedirs)
 
-    with capture_log() as log_file:
+    with pytest.raises(PermissionError) as error:
         make_local_storage(
-            path=data_cache, version="1.0", env="SOME_VARIABLE",
+            path=data_cache, env="SOME_VARIABLE",
         )
-        logs = log_file.getvalue()
-        assert logs.startswith("Cannot create data cache")
-        assert "'SOME_VARIABLE'" in logs
+        assert "Pooch could not create data cache" in str(error)
+        assert "'SOME_VARIABLE'" in str(error)
 
 
 def test_local_storage_newfile_permissionerror(monkeypatch):
@@ -119,13 +119,12 @@ def test_local_storage_newfile_permissionerror(monkeypatch):
 
         monkeypatch.setattr(tempfile, "NamedTemporaryFile", mocktempfile)
 
-        with capture_log() as log_file:
+        with pytest.raises(PermissionError) as error:
             make_local_storage(
-                path=data_cache, version="1.0", env="SOME_VARIABLE",
+                path=data_cache, env="SOME_VARIABLE",
             )
-            logs = log_file.getvalue()
-            assert logs.startswith("Cannot write to data cache")
-            assert "'SOME_VARIABLE'" in logs
+            assert "Pooch could not write to data cache" in str(error)
+            assert "'SOME_VARIABLE'" in str(error)
 
 
 def test_registry_builder():