Repository: joblib/joblib
Pull request #511 (2017-03-21 18:29:39)

Move temp_dir path generation to a separate private function
This PR addresses issue #510 and moves the code used to compute the path to the `temp_folder` from `joblib.pool.MemmapingPool` to a separate private function, so that it could be potentially (carefully) reused with `joblib.Memory`. 

codecov[bot]: # [Codecov](https://codecov.io/gh/joblib/joblib/pull/511?src=pr&el=h1) Report
> Merging [#511](https://codecov.io/gh/joblib/joblib/pull/511?src=pr&el=desc) into [master](https://codecov.io/gh/joblib/joblib/commit/a9cec693fce171d97a6ba54b8645e2952b786cbc?src=pr&el=desc) will **increase** coverage by `0.08%`.
> The diff coverage is `93.54%`.


```diff
@@            Coverage Diff             @@
##           master     #511      +/-   ##
==========================================
+ Coverage   93.66%   93.74%   +0.08%     
==========================================
  Files          37       37              
  Lines        4765     4779      +14     
==========================================
+ Hits         4463     4480      +17     
+ Misses        302      299       -3
```


| [Impacted Files](https://codecov.io/gh/joblib/joblib/pull/511?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [joblib/test/test\_pool.py](https://codecov.io/gh/joblib/joblib/compare/a9cec693fce171d97a6ba54b8645e2952b786cbc...2b68d33c89a0044529a4efb7784340bb499ff9da?src=pr&el=tree#diff-am9ibGliL3Rlc3QvdGVzdF9wb29sLnB5) | `98.83% <100%> (+0.05%)` | :arrow_up: |
| [joblib/pool.py](https://codecov.io/gh/joblib/joblib/compare/a9cec693fce171d97a6ba54b8645e2952b786cbc...2b68d33c89a0044529a4efb7784340bb499ff9da?src=pr&el=tree#diff-am9ibGliL3Bvb2wucHk=) | `91.63% <90%> (+0.1%)` | :arrow_up: |
| [joblib/parallel.py](https://codecov.io/gh/joblib/joblib/compare/a9cec693fce171d97a6ba54b8645e2952b786cbc...2b68d33c89a0044529a4efb7784340bb499ff9da?src=pr&el=tree#diff-am9ibGliL3BhcmFsbGVsLnB5) | `97.4% <0%> (+0.37%)` | :arrow_up: |
| [joblib/test/test\_memory.py](https://codecov.io/gh/joblib/joblib/compare/a9cec693fce171d97a6ba54b8645e2952b786cbc...2b68d33c89a0044529a4efb7784340bb499ff9da?src=pr&el=tree#diff-am9ibGliL3Rlc3QvdGVzdF9tZW1vcnkucHk=) | `98.52% <0%> (+0.42%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/joblib/joblib/pull/511?src=pr&el=continue).
> **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
> `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
> Powered by [Codecov](https://codecov.io/gh/joblib/joblib/pull/511?src=pr&el=footer). Last update [a9cec69...c3c522d](https://codecov.io/gh/joblib/joblib/compare/a9cec693fce171d97a6ba54b8645e2952b786cbc...c3c522d026f1f1153ade55aa320dcb0ab76f9ac9?el=footer&src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
rth: The codecov settings might be a bit too drastic: having the overall CI status failing  because the patch coverage is 93.54% instead of the expected 93.66% (all the while the overall project coverage increases with a PR) is perplexing. Not saying, that adding more tests here would not be useful though..
lesteve: I would look at it in a slightly different way and wonder why the coverage is not 100% for the function you created ;-). More seriously, I think the diff coverage is actually quite good to prevent us from being lazy and taking shortcuts.

Quickly looking at the [report](https://codecov.io/gh/joblib/joblib/compare/a9cec693fce171d97a6ba54b8645e2952b786cbc...c3c522d026f1f1153ade55aa320dcb0ab76f9ac9/diff), it seems like the line not covered is when `/dev/shm` is not writable. Not sure how to the codecov red status can be ignored.

rth: @lesteve Thanks for the review! I removed the paragraph in the docstring.

> I would look at it in a slightly different way and wonder why the coverage is not 100% for the function you created ;-). More seriously, I think the diff coverage is actually quite good to prevent us from being lazy and taking shortcuts.

Well, coverage is definitely a useful indicator (and the automatic comment is quite helpful), but using it to automatically validate PRs is counterproductive IMO because it shifts the discussion from whether the tests are good and sufficient to the quirks of coverage calculation (as in [here](https://github.com/lemurheavy/coveralls-public/issues/565) and  [here](https://github.com/libwilliam/flask-compress/pull/23))  and how one can hack it to get a green codecov status. The alternative is to merge PRs with a failed CI status which is also bad...

> Quickly looking at the report, it seems like the line not covered is when /dev/shm is not writable.

Yes, so here I would have to mock `os.makedirs` to raise an `IOError` in tests just to make codecov happy, not because I deemed it necessary, or you requested it during review.. ^^

> Not sure how to the codecov red status can be ignored.

Apparently there is a [`threashold` parameter](https://docs.codecov.io/docs/commit-status) that could be set, but I'm not sure where is the joblib's codecov yaml configuration file... Should I add one?

BTW, something stange is happening in `test_dispatch_multiprocessing` where it failed in this [build](https://travis-ci.org/joblib/joblib/jobs/214156864) but passed in the [next one](https://travis-ci.org/joblib/joblib/builds/214165445) without any changes to the code base. Should I remove back the `py.test -v` option? Thanks.

rth: Also you would notice that adding the `-v` option to `py.test` changes the `codecov/project` status from passing (previous commit) to failing (this commit); I wonder why ^^
lesteve: Yeah codecov is no silver bullet I agree but I would keep the config as it is right now and maybe revisit in a couple of months if the requirement turns out to be too stringent. At the moment I am fine with understanding why codecov is red and merging a PR if it is deemed acceptable.

I would add that the diff coverage (or alternatively the browser extension to see the coverage in the PR diff) is more useful than the global coverage number which has to be taken with a pinch of salt.

> Also you would notice that adding the -v option to py.test changes the codecov/project status from passing (previous commit) to failing (this commit); I wonder why ^^

It is not related to the `-v` option I reckon. There is some non-deterministic noise in joblib at the moment due to coverage of subprocesses that may or may not have time to write their coverage to a file before they are killed. If you want to know more about this, I may have commented about this in another issue. Otherwise https://bitbucket.org/ned/coveragepy/issues/458/instrumenting-of-multiprocessingpool is where the root cause lies I believe. 

We have seen `test_dispatch_multiprocessing` fail from time to time and I don't think I have managed to reproduce.

> Yes, so here I would have to mock os.makedirs to raise an IOError in tests just to make codecov happy, not because I deemed it necessary, or you requested it during review.. ^^

You could actually use the `monkeypatch` functionality of pytest to do this, your call.
rth: OK I removed the `-v` pytest flag.  

> You could actually use the monkeypatch functionality of pytest to do this, your call.

Interesting I didn't know about pytest's `monkeypatch`. Well, in this case, I don't think that testing for the unwritable `/dev/shm` is useful (I don't see under what circumstances such test could fail) so I would just keep the submitted tests as they are now, if you are OK with it..

> There is some non-deterministic noise in joblib at the moment due to coverage of subprocesses that may or may not have time to write their coverage to a file before they are killed. If you want to know more about this, I may have commented about this in another issue. Otherwise https://bitbucket.org/ned/coveragepy/issues/458/instrumenting-of-multiprocessingpool is where the root cause lies I believe.

Sounds complicated indeed.
> We have seen test_dispatch_multiprocessing fail from time to time and I don't think I have managed to reproduce

This happened again for the last build with PY2.7..

lesteve: > I would just keep the submitted tests as they are now, if you are OK with it..

Fine with me, let's try not to spend too much time on bikeshedding ..

Patch:
diff --git a/joblib/pool.py b/joblib/pool.py
index c53a12d..aca8db5 100644
--- a/joblib/pool.py
+++ b/joblib/pool.py
@@ -93,6 +93,58 @@ def _get_backing_memmap(a):
         return _get_backing_memmap(b)
 
 
+def _get_temp_dir(pool_folder_name, temp_folder=None):
+    """Get the full path to a subfolder inside the temporary folder.
+
+    Parameters
+    ----------
+    pool_folder_name : str
+        Sub-folder name used for the serialization of a pool instance.
+
+    temp_folder: str, optional
+        Folder to be used by the pool for memmaping large arrays
+        for sharing memory with worker processes. If None, this will try in
+        order:
+
+        - a folder pointed by the JOBLIB_TEMP_FOLDER environment
+          variable,
+        - /dev/shm if the folder exists and is writable: this is a
+          RAMdisk filesystem available by default on modern Linux
+          distributions,
+        - the default system temporary folder that can be
+          overridden with TMP, TMPDIR or TEMP environment
+          variables, typically /tmp under Unix operating systems.
+
+    Returns
+    -------
+    pool_folder : str
+       full path to the temporary folder
+    use_shared_mem : bool
+       whether the temporary folder is written to tmpfs
+    """
+    use_shared_mem = False
+    if temp_folder is None:
+        temp_folder = os.environ.get('JOBLIB_TEMP_FOLDER', None)
+    if temp_folder is None:
+        if os.path.exists(SYSTEM_SHARED_MEM_FS):
+            try:
+                temp_folder = SYSTEM_SHARED_MEM_FS
+                pool_folder = os.path.join(temp_folder, pool_folder_name)
+                if not os.path.exists(pool_folder):
+                    os.makedirs(pool_folder)
+                use_shared_mem = True
+            except IOError:
+                # Missing rights in the the /dev/shm partition,
+                # fallback to regular temp folder.
+                temp_folder = None
+    if temp_folder is None:
+        # Fallback to the default tmp folder, typically /tmp
+        temp_folder = tempfile.gettempdir()
+    temp_folder = os.path.abspath(os.path.expanduser(temp_folder))
+    pool_folder = os.path.join(temp_folder, pool_folder_name)
+    return pool_folder, use_shared_mem
+
+
 def has_shareable_memory(a):
     """Return True if a is backed by some mmap buffer directly or not."""
     return _get_backing_memmap(a) is not None
@@ -525,28 +577,10 @@ class MemmapingPool(PicklingPool):
         # Prepare a sub-folder name for the serialization of this particular
         # pool instance (do not create in advance to spare FS write access if
         # no array is to be dumped):
-        use_shared_mem = False
         pool_folder_name = "joblib_memmaping_pool_%d_%d" % (
             os.getpid(), id(self))
-        if temp_folder is None:
-            temp_folder = os.environ.get('JOBLIB_TEMP_FOLDER', None)
-        if temp_folder is None:
-            if os.path.exists(SYSTEM_SHARED_MEM_FS):
-                try:
-                    temp_folder = SYSTEM_SHARED_MEM_FS
-                    pool_folder = os.path.join(temp_folder, pool_folder_name)
-                    if not os.path.exists(pool_folder):
-                        os.makedirs(pool_folder)
-                    use_shared_mem = True
-                except IOError:
-                    # Missing rights in the the /dev/shm partition,
-                    # fallback to regular temp folder.
-                    temp_folder = None
-        if temp_folder is None:
-            # Fallback to the default tmp folder, typically /tmp
-            temp_folder = tempfile.gettempdir()
-        temp_folder = os.path.abspath(os.path.expanduser(temp_folder))
-        pool_folder = os.path.join(temp_folder, pool_folder_name)
+        pool_folder, use_shared_mem = _get_temp_dir(pool_folder_name,
+                                                    temp_folder)
         self._temp_folder = pool_folder
 
         # Register the garbage collector at program exit in case caller forgets


Test patch:
diff --git a/joblib/test/test_pool.py b/joblib/test/test_pool.py
index a24648d..ad0bfb0 100644
--- a/joblib/test/test_pool.py
+++ b/joblib/test/test_pool.py
@@ -1,5 +1,6 @@
 import os
 import mmap
+import sys
 
 from joblib.test.common import with_numpy, np
 from joblib.test.common import setup_autokill
@@ -15,6 +16,7 @@ from joblib.pool import ArrayMemmapReducer
 from joblib.pool import reduce_memmap
 from joblib.pool import _strided_from_memmap
 from joblib.pool import _get_backing_memmap
+from joblib.pool import _get_temp_dir
 
 
 def setup_module():
@@ -478,3 +480,15 @@ def test_pool_memmap_with_big_offset(tmpdir):
     assert isinstance(result, np.memmap)
     assert result.offset == offset
     np.testing.assert_array_equal(obj, result)
+
+
+def test_pool_get_temp_dir(tmpdir):
+    pool_folder_name = 'test.tmpdir'
+    pool_folder, shared_mem = _get_temp_dir(pool_folder_name, tmpdir.strpath)
+    assert shared_mem is False
+    assert pool_folder == tmpdir.join('test.tmpdir').strpath
+
+    pool_folder, shared_mem = _get_temp_dir(pool_folder_name, temp_folder=None)
+    if sys.platform.startswith('win'):
+        assert shared_mem is False
+    assert pool_folder.endswith(pool_folder_name)