Repository: scikit-learn/scikit-learn
Pull request: FIX: fetch covtype dataset concurrent-safe
<!--
Thanks for contributing a pull request! Please ensure you have taken a look at
the contribution guidelines: https://github.com/scikit-learn/scikit-learn/blob/main/CONTRIBUTING.md
-->

#### Reference Issues/PRs
<!--
Example: Fixes #1234. See also #3456.
Please use keywords (e.g., Fixes) to create link to the issues or pull requests
you resolved, so that they will automatically be closed when your pull request
is merged. See https://github.com/blog/1506-closing-issues-via-pull-requests
-->
Fixes #23048.


#### What does this implement/fix? Explain your changes.
When fetching the dataset, write the data files to a temporary first, then atomically rename to their target location. (similar to https://github.com/scikit-learn/scikit-learn/pull/21833)
This approach avoids trouble with concurrent runs and partially written data.

#### Any other comments?

It looks like most dataset fetchers don't currently implement a mechanism like this. Would it be worthwhile to apply the same procedure to the others? If so, would separate PRs be prefered or do I tack it on to this one?
<!--
Please be aware that we are a loose team of volunteers so patience is
necessary; assistance handling other issues is very welcome. We value
all user contributions, no matter how minor they are. If we are slow to
review, either the pull request needs some benchmarking, tinkering,
convincing, etc. or more likely the reviewers are simply busy. In either
case, we ask for your understanding during the review process.
For more information, see our FAQ on this topic:
http://scikit-learn.org/dev/faq.html#why-is-my-pull-request-not-getting-any-attention.

Thanks for contributing!
-->


Problem statement:
Covtype dataset raises error when fetching
### Describe the bug

When fetching the Covtype dataset
```python
from sklearn.datasets import fetch_covtype
fetch_covtype()
```

I am getting an

```python
error: Error -3 while decompressing data: invalid block type
```

Renaming the folder `$HOME/scikit_learn_data` and fetching the dataset again solves the problem, so the problem must be related to an old cached version of the folder.

### Steps/Code to Reproduce

```python
from sklearn.datasets import fetch_covtype

fetch_covtype()
```

### Expected Results

Dataset should be fetched without errors

### Actual Results

```python-traceback
UnboundLocalError                         Traceback (most recent call last)
File ~/scikit-learn/sklearn/datasets/_covtype.py:172, in fetch_covtype(data_home, download_if_missing, random_state, shuffle, return_X_y, as_frame)
    [171](file:///home/arturoamor/scikit-learn/sklearn/datasets/_covtype.py?line=170)[ try:
--> ]()[172](file:///home/arturoamor/scikit-learn/sklearn/datasets/_covtype.py?line=171)[     X, y
    ]()[173](file:///home/arturoamor/scikit-learn/sklearn/datasets/_covtype.py?line=172)[ except NameError:

UnboundLocalError: local variable 'X' referenced before assignment

During handling of the above exception, another exception occurred:

error                                     Traceback (most recent call last)
/home/arturoamor/scikit-learn/doc/developers/test_datasets.py in <module>
      ]()[94](file:///home/arturoamor/scikit-learn/doc/developers/test_datasets.py?line=93)[ from sklearn.datasets import fetch_covtype
----> ]()[96](file:///home/arturoamor/scikit-learn/doc/developers/test_datasets.py?line=95)[ fetch_covtype()

File ~/scikit-learn/sklearn/datasets/_covtype.py:174, in fetch_covtype(data_home, download_if_missing, random_state, shuffle, return_X_y, as_frame)
    ]()[172](file:///home/arturoamor/scikit-learn/sklearn/datasets/_covtype.py?line=171)[     X, y
    ]()[173](file:///home/arturoamor/scikit-learn/sklearn/datasets/_covtype.py?line=172)[ except NameError:
--> ]()[174](file:///home/arturoamor/scikit-learn/sklearn/datasets/_covtype.py?line=173)[     X = joblib.load(samples_path)
    ]()[175](file:///home/arturoamor/scikit-learn/sklearn/datasets/_covtype.py?line=174)[     y = joblib.load(targets_path)
    ]()[177](file:///home/arturoamor/scikit-learn/sklearn/datasets/_covtype.py?line=176)[ if shuffle:

File ~/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py:587, in load(filename, mmap_mode)
    ]()[581](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=580)[             if isinstance(fobj, str):
    ]()[582](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=581)[                 # if the returned file object is a string, this means we
    ]()[583](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=582)[                 # try to load a pickle file generated with an version of
    ]()[584](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=583)[                 # Joblib so we load it with joblib compatibility function.
    ]()[585](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=584)[                 return load_compatibility(fobj)
--> ]()[587](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=586)[             obj = _unpickle(fobj, filename, mmap_mode)
    ]()[588](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=587)[ return obj

File ~/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py:506, in _unpickle(fobj, filename, mmap_mode)
    ]()[504](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=503)[ obj = None
    ]()[505](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=504)[ try:
--> ]()[506](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=505)[     obj = unpickler.load()
    ]()[507](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=506)[     if unpickler.compat_mode:
    ]()[508](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=507)[         warnings.warn("The file '%s' has been generated with a "
    ]()[509](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=508)[                       "joblib version less than 0.10. "
    ]()[510](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=509)[                       "Please regenerate this pickle file."
    ]()[511](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=510)[                       % filename,
    ]()[512](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/numpy_pickle.py?line=511)[                       DeprecationWarning, stacklevel=3)

File ~/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py:1208, in _Unpickler.load(self)
   ]()[1206](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py?line=1205)[ try:
   ]()[1207](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py?line=1206)[     while True:
-> ]()[1208](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py?line=1207)[         key = read(1)
   ]()[1209](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py?line=1208)[         if not key:
   ]()[1210](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py?line=1209)[             raise EOFError

File ~/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py:298, in _Unframer.read(self, n)
    ]()[296](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py?line=295)[     return data
    ]()[297](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py?line=296)[ else:
--> ]()[298](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/pickle.py?line=297)[     return self.file_read(n)

File ~/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py:464, in BinaryZlibFile.readinto(self, b)
    ]()[459](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=458)[ """Read up to len(b) bytes into b.
    ]()[460](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=459)[ 
    ]()[461](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=460)[ Returns the number of bytes read (0 for EOF).
    ]()[462](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=461)[ """
    ]()[463](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=462)[ with self._lock:
--> ]()[464](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=463)[     return io.BufferedIOBase.readinto(self, b)

File ~/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py:456, in BinaryZlibFile.read(self, size)
    ]()[454](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=453)[     return self._read_all()
    ]()[455](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=454)[ else:
--> ]()[456](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=455)[     return self._read_block(size)

File ~/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py:429, in BinaryZlibFile._read_block(self, n_bytes, return_data)
    ]()[426](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=425)[ self._buffer_offset = 0
    ]()[428](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=427)[ blocks = []
--> ]()[429](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=428)[ while n_bytes > 0 and self._fill_buffer():
    ]()[430](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=429)[     if n_bytes < len(self._buffer):
    ]()[431](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=430)[         data = self._buffer[:n_bytes]

File ~/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py:393, in BinaryZlibFile._fill_buffer(self)
    ]()[391](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=390)[         return False
    ]()[392](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=391)[     else:
--> ]()[393](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=392)[         self._buffer = self._decompressor.decompress(rawblock)
    ]()[394](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=393)[     self._buffer_offset = 0
    ]()[395](file:///home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/joblib/compressor.py?line=394)[ return True

error: Error -3 while decompressing data: invalid block type]()
```

### Versions

```shell
System:
    python: 3.9.9 | packaged by conda-forge | (main, Dec 20 2021, 02:40:17)  [GCC 9.4.0]
executable: /home/arturoamor/miniforge3/envs/dev-scikit-learn/bin/python
   machine: Linux-5.14.0-1031-oem-x86_64-with-glibc2.31

Python dependencies:
      sklearn: 1.1.dev0
          pip: 21.3.1
   setuptools: 60.5.0
        numpy: 1.22.1
        scipy: 1.7.3
       Cython: 0.29.26
       pandas: 1.3.5
   matplotlib: 3.5.1
       joblib: 1.1.0
threadpoolctl: 3.0.0

Built with OpenMP: True

threadpoolctl info:
       user_api: blas
   internal_api: openblas
         prefix: libopenblas
       filepath: /home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/numpy.libs/libopenblas64_p-r0-2f7c42d4.3.18.so
        version: 0.3.18
threading_layer: pthreads
   architecture: SkylakeX
    num_threads: 8

       user_api: blas
   internal_api: openblas
         prefix: libopenblas
       filepath: /home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/python3.9/site-packages/scipy.libs/libopenblasp-r0-8b9e111f.3.17.so
        version: 0.3.17
threading_layer: pthreads
   architecture: SkylakeX
    num_threads: 8

       user_api: openmp
   internal_api: openmp
         prefix: libgomp
       filepath: /home/arturoamor/miniforge3/envs/dev-scikit-learn/lib/libgomp.so.1.0.0
        version: None
    num_threads: 8
```

Hints from issue discussion:
By looking at the source code of `fetch_covtype`, it appears that this code is not concurrent safe: it does not use the typical "download and save to a temporary file" followed by "atomic rename to the final filepath".

This can explain that we can get corrupted covtype data, for instance by running the test with pytest-xdist.

Something similar was implemented in #21833 to make `fetch_openml` concurrent-safe.

Commit message: add concurrent-safe covtype fetching

Patch to review:
diff --git a/sklearn/datasets/_covtype.py b/sklearn/datasets/_covtype.py
index c9f5291de153b..726cdebdac6af 100644
--- a/sklearn/datasets/_covtype.py
+++ b/sklearn/datasets/_covtype.py
@@ -17,7 +17,8 @@
 from gzip import GzipFile
 import logging
 from os.path import exists, join
-from os import remove, makedirs
+import os
+from tempfile import TemporaryDirectory
 
 import numpy as np
 import joblib
@@ -148,23 +149,29 @@ def fetch_covtype(
     covtype_dir = join(data_home, "covertype")
     samples_path = _pkl_filepath(covtype_dir, "samples")
     targets_path = _pkl_filepath(covtype_dir, "targets")
-    available = exists(samples_path)
+    available = exists(samples_path) and exists(targets_path)
 
     if download_if_missing and not available:
-        if not exists(covtype_dir):
-            makedirs(covtype_dir)
-        logger.info("Downloading %s" % ARCHIVE.url)
-
-        archive_path = _fetch_remote(ARCHIVE, dirname=covtype_dir)
-        Xy = np.genfromtxt(GzipFile(filename=archive_path), delimiter=",")
-        # delete archive
-        remove(archive_path)
-
-        X = Xy[:, :-1]
-        y = Xy[:, -1].astype(np.int32, copy=False)
-
-        joblib.dump(X, samples_path, compress=9)
-        joblib.dump(y, targets_path, compress=9)
+        os.makedirs(covtype_dir, exist_ok=True)
+
+        # Creating temp_dir as a direct subdirectory of the target directory
+        # guarantees that both reside on the same filesystem, so that we can use
+        # os.rename to atomically move the data files to their target location.
+        with TemporaryDirectory(dir=covtype_dir) as temp_dir:
+            logger.info("Downloading %s" % ARCHIVE.url)
+            archive_path = _fetch_remote(ARCHIVE, dirname=temp_dir)
+            Xy = np.genfromtxt(GzipFile(filename=archive_path), delimiter=",")
+
+            X = Xy[:, :-1]
+            y = Xy[:, -1].astype(np.int32, copy=False)
+
+            samples_tmp_path = _pkl_filepath(temp_dir, "samples")
+            joblib.dump(X, samples_tmp_path, compress=9)
+            os.rename(samples_tmp_path, samples_path)
+
+            targets_tmp_path = _pkl_filepath(temp_dir, "targets")
+            joblib.dump(y, targets_tmp_path, compress=9)
+            os.rename(targets_tmp_path, targets_path)
 
     elif not available and not download_if_missing:
         raise IOError("Data not found and `download_if_missing` is False")


Code review comments:
diff_hunk: @@ -148,23 +149,29 @@ def fetch_covtype(
     covtype_dir = join(data_home, "covertype")
     samples_path = _pkl_filepath(covtype_dir, "samples")
     targets_path = _pkl_filepath(covtype_dir, "targets")
-    available = exists(samples_path)
+    available = exists(samples_path) and exists(targets_path)
 
     if download_if_missing and not available:
-        if not exists(covtype_dir):
-            makedirs(covtype_dir)
-        logger.info("Downloading %s" % ARCHIVE.url)
-
-        archive_path = _fetch_remote(ARCHIVE, dirname=covtype_dir)
-        Xy = np.genfromtxt(GzipFile(filename=archive_path), delimiter=",")
-        # delete archive
-        remove(archive_path)
-
-        X = Xy[:, :-1]
-        y = Xy[:, -1].astype(np.int32, copy=False)
-
-        joblib.dump(X, samples_path, compress=9)
-        joblib.dump(y, targets_path, compress=9)
+        os.makedirs(covtype_dir, exist_ok=True)
+
+        # Creating temp_dir as a direct subdirectory of the target directory
+        # guarantees that both reside on the same filesystem, so that we can use
+        # os.rename to atomically move the data files to their target location.
+        with TemporaryDirectory(dir=covtype_dir) as temp_dir:
+            logger.info("Downloading %s" % ARCHIVE.url)
path: sklearn/datasets/_covtype.py
text: @user1:
```suggestion
            logger.info(f"Downloading {ARCHIVE.url}")
```