Repository: scikit-learn/scikit-learn
Pull request: [MRG + 2] Fix cache of OpenML fetcher
Supposed to fix #12227 

Problem statement:
fetch_openml() does not use the local cache
#### Description
`fetch_openml()` does not use the local data cached in `~/scikit_learn_data`.

#### Steps/Code to Reproduce
Run the following code twice. The first time, with a working Internet connexion, the second time without an Internet connexion.
```python
from sklearn.datasets import fetch_openml
mnist = fetch_openml("mnist_784", version=1)
print(mnist.data.shape)
```

#### Expected Results
First run (with Internet access):
(70000, 784)

Second run (without Internet access):
(70000, 784)

#### Actual Results
First run (with Internet access):
(70000, 784)
Second run (without Internet access):
```
Traceback (most recent call last):
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/urllib/request.py", line 1318, in do_open
    encode_chunked=req.has_header('Transfer-encoding'))
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/http/client.py", line 1239, in request
    self._send_request(method, url, body, headers, encode_chunked)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/http/client.py", line 1285, in _send_request
    self.endheaders(body, encode_chunked=encode_chunked)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/http/client.py", line 1234, in endheaders
    self._send_output(message_body, encode_chunked=encode_chunked)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/http/client.py", line 1026, in _send_output
    self.send(msg)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/http/client.py", line 964, in send
    self.connect()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/http/client.py", line 1392, in connect
    super().connect()
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/http/client.py", line 936, in connect
    (self.host,self.port), self.timeout, self.source_address)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/socket.py", line 704, in create_connection
    for res in getaddrinfo(host, port, 0, SOCK_STREAM):
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/socket.py", line 745, in getaddrinfo
    for res in _socket.getaddrinfo(host, port, family, type, proto, flags):
socket.gaierror: [Errno 8] nodename nor servname provided, or not known

During handling of the above exception, another exception occurred:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/ageron/.virtualenvs/ml/lib/python3.6/site-packages/sklearn/datasets/openml.py", line 466, in fetch_openml
    data_info = _get_data_info_by_name(name, version, data_home)
  File "/Users/ageron/.virtualenvs/ml/lib/python3.6/site-packages/sklearn/datasets/openml.py", line 275, in _get_data_info_by_name
    data_home)
  File "/Users/ageron/.virtualenvs/ml/lib/python3.6/site-packages/sklearn/datasets/openml.py", line 119, in _get_json_content_from_openml_api
    response = _open_openml_url(url, data_home)
  File "/Users/ageron/.virtualenvs/ml/lib/python3.6/site-packages/sklearn/datasets/openml.py", line 55, in _open_openml_url
    fsrc = urlopen(req)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/urllib/request.py", line 223, in urlopen
    return opener.open(url, data, timeout)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/urllib/request.py", line 526, in open
    response = self._open(req, data)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/urllib/request.py", line 544, in _open
    '_open', req)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/urllib/request.py", line 504, in _call_chain
    result = func(*args)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/urllib/request.py", line 1361, in https_open
    context=self._context, check_hostname=self._check_hostname)
  File "/opt/local/Library/Frameworks/Python.framework/Versions/3.6/lib/python3.6/urllib/request.py", line 1320, in do_open
    raise URLError(err)
urllib.error.URLError: <urlopen error [Errno 8] nodename nor servname provided, or not known>
```

AFAICT, the data is correctly cached in the `~/scikit_learn_data` directory, but simply ignored by `fetch_openml()`:

```bash
$ tree ~/scikit_learn_data/openml/
/Users/ageron/scikit_learn_data/openml/
└── openml.org
    ├── api
    │   └── v1
    │       └── json
    │           └── data
    │               ├── 554.gz
    │               ├── features
    │               │   └── 554.gz
    │               └── list
    │                   └── data_name
    │                       └── mnist_784
    │                           └── limit
    │                               └── 2
    │                                   └── data_version
    │                                       └── 1.gz
    └── data
        └── v1
            └── download
                └── 52667.gz

15 directories, 4 files
```



#### Versions
Darwin-17.7.0-x86_64-i386-64bit
Python 3.6.6 (default, Jun 28 2018, 05:43:53)
[GCC 4.2.1 Compatible Apple LLVM 9.1.0 (clang-902.0.39.2)]
NumPy 1.15.2
SciPy 1.1.0
Scikit-Learn 0.20.0

Same results as well on:

Linux-4.15.0-36-generic-x86_64-with-Ubuntu-18.04-bionic
Python 3.6.5 (default, Apr  1 2018, 05:46:30)
[GCC 7.3.0]
NumPy 1.15.1
SciPy 1.1.0
Scikit-Learn 0.20.0

Hints from issue discussion:
https://github.com/scikit-learn/scikit-learn/blob/master/sklearn/datasets/openml.py

The first thing the function `fetch_openml` does is establish whether the data is cached or not:
```
data_home = get_data_home(data_home=data_home)
    data_home = join(data_home, 'openml')
    if cache is False:
        # no caching will be applied
        data_home = None
```
But then I was able to follow the function calls to line 55 in `_open_openml_url` where it tries to access a URL, so it seems to ignore the fact that the data exists and just proceeds to access information at some URL. There's a decent bit going on in this file though, so I haven't fully deduced why it's doing that.

I can take this in maybe the next day or two, but if someone else wants to take it first, go for it.
ping @janvanrijn :)
In `_open_openml_url`, before the request is made, there should be a check to see if the `local_path` exists. If the path exists, that file can be read, decompressed, and returned. 

There maybe an issue regarding when to invalidate the cache. For example, if the cached file was corrupted, the decompression would most likely fail. The user would be left with manually going through the file structure to remove the cache file.
Sloppy, my fault. I will create a PR. 
> Sloppy, my fault. 

I take this back. 

I think this went wrong with the enhancement #11830, where gzip functionality was added to the fetcher. The urlopen was moved to a position where it was able to be invoked, also when cache was available. 

>  I will create a PR.

Including a more extensive testcase :)

Commit message: fixes openml fetcher

Patch to review:
diff --git a/sklearn/datasets/openml.py b/sklearn/datasets/openml.py
index d667cb3699b28..7f2538f622757 100644
--- a/sklearn/datasets/openml.py
+++ b/sklearn/datasets/openml.py
@@ -31,6 +31,10 @@
 _DATA_FILE = "data/v1/download/{}"
 
 
+def _get_local_path(openml_path, data_home):
+    return os.path.join(data_home, 'openml.org', openml_path + ".gz")
+
+
 def _open_openml_url(openml_path, data_home):
     """
     Returns a resource from OpenML.org. Caches it to data_home if required.
@@ -50,20 +54,23 @@ def _open_openml_url(openml_path, data_home):
     result : stream
         A stream to the OpenML resource
     """
+    def is_gzip(_fsrc):
+        return _fsrc.info().get('Content-Encoding', '') == 'gzip'
+
     req = Request(_OPENML_PREFIX + openml_path)
     req.add_header('Accept-encoding', 'gzip')
-    fsrc = urlopen(req)
-    is_gzip = fsrc.info().get('Content-Encoding', '') == 'gzip'
 
     if data_home is None:
-        if is_gzip:
+        fsrc = urlopen(req)
+        if is_gzip(fsrc):
             if PY2:
                 fsrc = BytesIO(fsrc.read())
             return gzip.GzipFile(fileobj=fsrc, mode='rb')
         return fsrc
 
-    local_path = os.path.join(data_home, 'openml.org', openml_path + ".gz")
+    local_path = _get_local_path(openml_path, data_home)
     if not os.path.exists(local_path):
+        fsrc = urlopen(req)
         try:
             os.makedirs(os.path.dirname(local_path))
         except OSError:
@@ -77,10 +84,10 @@ def _open_openml_url(openml_path, data_home):
         except Exception:
             os.unlink(local_path)
             raise
-    # XXX: unnecessary decompression on first access
-    if is_gzip:
-        return gzip.GzipFile(local_path, 'rb')
-    return fsrc
+
+    # XXX: First time, decompression will not be necessary (by using fsrc), but
+    # it will happen nonetheless
+    return gzip.GzipFile(local_path, 'rb')
 
 
 def _get_json_content_from_openml_api(url, error_message, raise_if_error,
diff --git a/sklearn/datasets/tests/test_openml.py b/sklearn/datasets/tests/test_openml.py
index cf9cfcdc81ede..884211c38e369 100644
--- a/sklearn/datasets/tests/test_openml.py
+++ b/sklearn/datasets/tests/test_openml.py
@@ -459,17 +459,39 @@ def test_open_openml_url_cache(monkeypatch, gzip_response):
     _monkey_patch_webbased_functions(
         monkeypatch, data_id, gzip_response)
     openml_path = sklearn.datasets.openml._DATA_FILE.format(data_id)
-    test_directory = os.path.join(os.path.expanduser('~'), 'scikit_learn_data')
+    cache_directory = os.path.join(os.path.expanduser('~'), 'scikit_learn_data')
     # first fill the cache
-    response1 = _open_openml_url(openml_path, test_directory)
+    response1 = _open_openml_url(openml_path, cache_directory)
     # assert file exists
-    location = os.path.join(test_directory, 'openml.org', openml_path + '.gz')
+    location = os.path.join(cache_directory, 'openml.org', openml_path + '.gz')
     assert os.path.isfile(location)
     # redownload, to utilize cache
-    response2 = _open_openml_url(openml_path, test_directory)
+    response2 = _open_openml_url(openml_path, cache_directory)
     assert response1.read() == response2.read()
 
 
+@pytest.mark.parametrize('gzip_response', [True, False])
+def test_fetch_openml_cache(monkeypatch, gzip_response):
+    def _mock_urlopen_raise(request):
+        raise ValueError('This mechanism intends to test correct cache'
+                         'handling. As such, urlopen should never be '
+                         'accessed. URL: %s' % request.get_full_url())
+    data_id = 2
+    cache_directory = os.path.join(os.path.expanduser('~'), 'scikit_learn_data')
+    _monkey_patch_webbased_functions(
+        monkeypatch, data_id, gzip_response)
+    X_f, y_f = fetch_openml(data_id=data_id, cache=True,
+                            data_home=cache_directory, return_X_y=True)
+
+    monkeypatch.setattr(sklearn.datasets.openml, 'urlopen',
+                        _mock_urlopen_raise)
+
+    X_c, y_c = fetch_openml(data_id=data_id, cache=True,
+                            data_home=cache_directory, return_X_y=True)
+    np.testing.assert_array_equal(X_f, X_c)
+    np.testing.assert_array_equal(y_f, y_c)
+
+
 @pytest.mark.parametrize('gzip_response', [True, False])
 def test_fetch_openml_notarget(monkeypatch, gzip_response):
     data_id = 61


Code review comments:
diff_hunk: @@ -459,17 +459,39 @@ def test_open_openml_url_cache(monkeypatch, gzip_response):
     _monkey_patch_webbased_functions(
         monkeypatch, data_id, gzip_response)
     openml_path = sklearn.datasets.openml._DATA_FILE.format(data_id)
-    test_directory = os.path.join(os.path.expanduser('~'), 'scikit_learn_data')
+    cache_directory = os.path.join(os.path.expanduser('~'), 'scikit_learn_data')
path: sklearn/datasets/tests/test_openml.py
text: @user1:
What do you think about using a tempdir for the cache directory during testing:

```python
def test_open_openml_url_cache(monkeypatch, gzip_response, tmpdir):
    ...
    cache_directory = str(tmpdir.mkdir('scikit_learn_data'))
```

This can also be used in `test_fetch_openml_cache`.

@author:
The longer I think about it, the more important I think it is. Due to using the same data ids as the main server, yet not using the same files, developers that run unit tests can obtain files in their cache that are inconsistent with the real datasets. 

I will try to add a sensible fix

@author:
Actually, I wasn't able to fix this. What is the tmpdir library? Everything I find in the Pydocs points to the tempfile library. The following code leads to the following error:
code
```
cache_directory = os.path.join(tempfile.mkdtemp(),
                                   'scikit_learn_test_data')
```


error
```
monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7fd5898692e8>, gzip_response = False

    @user2.mark.parametrize('gzip_response', [True, False])
    def test_open_openml_url_cache(monkeypatch, gzip_response):
        data_id = 61
    
        _monkey_patch_webbased_functions(
            monkeypatch, data_id, gzip_response)
        openml_path = sklearn.datasets.openml._DATA_FILE.format(data_id)
        cache_directory = os.path.join(tempfile.mkdtemp(),
                                       'scikit_learn_test_data')
        # first fill the cache
        response1 = _open_openml_url(openml_path, cache_directory)
        # assert file exists
        location = _get_local_path(openml_path, cache_directory)
        assert os.path.isfile(location)
        # redownload, to utilize cache
        response2 = _open_openml_url(openml_path, cache_directory)
>       assert response1.read() == response2.read()

test_openml.py:478: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../../../../anaconda3/envs/scikit-learn34/lib/python3.4/gzip.py:360: in read
    while self._read(readsize):
../../../../../anaconda3/envs/scikit-learn34/lib/python3.4/gzip.py:433: in _read
    if not self._read_gzip_header():
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <gzip _io.BufferedReader name='/tmp/tmp80braqj1/scikit_learn_test_data/openml.org/data/v1/download/61.gz' 0x7fd589869d68>

    def _read_gzip_header(self):
        magic = self.fileobj.read(2)
        if magic == b'':
            return False
    
        if magic != b'\037\213':
>           raise OSError('Not a gzipped file')
E           OSError: Not a gzipped file
```

@user1:
`tmpdir` is a pytest fixture: https://docs.pytest.org/en/latest/tmpdir.html. It is included when pytest is installed. For example, you can use it as follows:

```python
@user2.mark.parametrize('gzip_response', [True, False])
 def test_fetch_openml_cache(monkeypatch, gzip_response, tmpdir):
     def _mock_urlopen_raise(request):
         raise ValueError('This mechanism intends to test correct cache'
                          'handling. As such, urlopen should never be '
                          'accessed. URL: %s' % request.get_full_url())
     data_id = 2
     cache_directory = str(tmpdir.mkdir('scikit_learn_data'))
     _monkey_patch_webbased_functions(
         monkeypatch, data_id, gzip_response)
     X_f, y_f = fetch_openml(data_id=data_id, cache=True,
                             data_home=cache_directory, return_X_y=True)

     monkeypatch.setattr(sklearn.datasets.openml, 'urlopen',
                         _mock_urlopen_raise)

     X_c, y_c = fetch_openml(data_id=data_id, cache=True,
                             data_home=cache_directory, return_X_y=True)
     np.testing.assert_array_equal(X_f, X_c)
     np.testing.assert_array_equal(y_f, y_c)
```

`tmpdir` just needs to be included as a parameter to `test_fetch_openml_cache`.

@author:
Cool, thanks. But still the same error:

```
================================================================================================= FAILURES =================================================================================================
____________________________________________________________________________________ test_open_openml_url_cache[False] _____________________________________________________________________________________

monkeypatch = <_pytest.monkeypatch.MonkeyPatch object at 0x7f84fdbd0fd0>, gzip_response = False, tmpdir = local('/tmp/pytest-of-janvanrijn/pytest-0/test_open_openml_url_cache_Fal0')

    @user2.mark.parametrize('gzip_response', [True, False])
    def test_open_openml_url_cache(monkeypatch, gzip_response, tmpdir):
        data_id = 61
    
        _monkey_patch_webbased_functions(
            monkeypatch, data_id, gzip_response)
        openml_path = sklearn.datasets.openml._DATA_FILE.format(data_id)
        cache_directory = str(tmpdir.mkdir('scikit_learn_data'))
        # first fill the cache
        response1 = _open_openml_url(openml_path, cache_directory)
        # assert file exists
        location = _get_local_path(openml_path, cache_directory)
        assert os.path.isfile(location)
        # redownload, to utilize cache
        response2 = _open_openml_url(openml_path, cache_directory)
>       assert response1.read() == response2.read()

test_openml.py:476: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
../../../../../anaconda3/envs/scikit-learn34/lib/python3.4/gzip.py:360: in read
    while self._read(readsize):
../../../../../anaconda3/envs/scikit-learn34/lib/python3.4/gzip.py:433: in _read
    if not self._read_gzip_header():
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

self = <gzip _io.BufferedReader name='/tmp/pytest-of-janvanrijn/pytest-0/test_open_openml_url_cache_Fal0/scikit_learn_data/openml.org/data/v1/download/61.gz' 0x7f84fdbdbc18>

    def _read_gzip_header(self):
        magic = self.fileobj.read(2)
        if magic == b'':
            return False
    
        if magic != b'\037\213':
>           raise OSError('Not a gzipped file')
E           OSError: Not a gzipped file

../../../../../anaconda3/envs/scikit-learn34/lib/python3.4/gzip.py:297: OSError
```
Is there any logical reason why calling to repsonse.read() in a temp dir results in an error, whereas calling to it in a regular dir doesn't? 

@user1:
This error is happening when `gzip=False`. When the fsrc object is returned as a non gzipped binary, it is saved as a gzip file anyways. This can be fixed by adjusting `_open_openml_url`:

```python
    if not os.path.exists(local_path):
        fsrc = urlopen(req)
        try:
            os.makedirs(os.path.dirname(local_path))
        except OSError:
            # potentially, the directory has been created already
            pass
        try:
            if is_gzip(fsrc):
                with open(local_path, 'wb') as fdst:
                    shutil.copyfileobj(fsrc, fdst)
                    fsrc.close()
            else:
                with gzip.GzipFile(local_path, 'wb') as fdst:
                    shutil.copyfileobj(fsrc, fdst)
                    fsrc.close()
```

If `fsrc` is a gzipped file, it is cached directly. If `fsrc` is not gzipped, it will be gzipped locally.

The strange thing is how the test passed when with the `os.path.expanduser('~')` directory.

@author:
My current guess is a little bug in the `_monkey_patch_webbased_functions`. It seems to work with two notions of gzip: 1) gzip_response and 2) has_gzip_header. 

What is exactly the meaning of either? 

@user1:
`has_gzip_header` is set to true when the request header includes `Accept-encoding=gzip`. `gzip_response` is provided by the tests to determine if a gzip response should be returned by the mocked urlopen. This test the case where the client request for `Accept-encoding=gzip`, but the server still decides to return a uncompressed version.

I tested this version of `_open_openml_url` local which passes your new tests:

```python
def _open_openml_url(openml_path, data_home):
    def is_gzip(_fsrc):
        return _fsrc.info().get('Content-Encoding', '') == 'gzip' 

    req = Request(_OPENML_PREFIX + openml_path)
    req.add_header('Accept-encoding', 'gzip')

    if data_home is None:
        fsrc = urlopen(req)
        if is_gzip(fsrc):
            if PY2:
                fsrc = BytesIO(fsrc.read())
            return gzip.GzipFile(fileobj=fsrc, mode='rb')
        return fsrc

    local_path = _get_local_path(openml_path, data_home)
    if not os.path.exists(local_path):
        fsrc = urlopen(req)
        try:
            os.makedirs(os.path.dirname(local_path))
        except OSError:
            # potentially, the directory has been created already
            pass

        try:
            if is_gzip(fsrc):
                with open(local_path, 'wb') as fdst:
                    shutil.copyfileobj(fsrc, fdst)
                    fsrc.close()
            else:
                with gzip.GzipFile(local_path, 'wb') as fdst:
                    shutil.copyfileobj(fsrc, fdst)
                    fsrc.close()
        except Exception:
            os.unlink(local_path)
            raise
    # XXX: First time, decompression will not be necessary (by using fsrc), but
    # it will happen nonetheless
    return gzip.GzipFile(local_path, 'rb')
```

@author:
I just incorporated your fix and it seems to solve the problem. Thanks a lot!