Repository: joblib/joblib

Issue:
Writable output numpy arrays hashed to the same memmap
I'm using `Parallel` to run jobs that all write to (large) output numpy arrays. I allocate these in advance (as zeros), then pass into the functions that are executing in parallel to be populated.

In my case, these are distinct arrays of zeros of the same shape and dtype; for that reason they are all hashed to the same value by the Joblib hashers (https://github.com/joblib/joblib/blob/e82b5d6684fcb61b0f778df3e29d2985a1c54208/joblib/hashing.py#L52), and so end up sharing the memory map objects Joblib creates. As a consequence my jobs, instead of producing distinct outputs, end up writing over each other.

This behaviour is perfectly fine when the memmap mode is read-only, but is wrong when the memmap mode allows writing.

One solution is to include the object identity in the hash whenever the memmap mode allows writing.

Discussion:
elsander: Possible considerations: 
- if we're hashing the object id, do we need to hash any of the other properties at all?
- does the object id need to be added for non-numpy-array objects as well? (I could not replicate this problem for a list of lists, for example, but there might be other cases that have this same issue)
codecov[bot]: # [Codecov](https://codecov.io/gh/joblib/joblib/pull/565?src=pr&el=h1) Report
> Merging [#565](https://codecov.io/gh/joblib/joblib/pull/565?src=pr&el=desc) into [master](https://codecov.io/gh/joblib/joblib/commit/8892839cb883b95fd2de3656bf719488eacb851f?src=pr&el=desc) will **decrease** coverage by `0.04%`.
> The diff coverage is `100%`.

[![Impacted file tree graph](https://codecov.io/gh/joblib/joblib/pull/565/graphs/tree.svg?token=gA6LF5DGTW&height=150&width=650&src=pr)](https://codecov.io/gh/joblib/joblib/pull/565?src=pr&el=tree)

```diff
@@            Coverage Diff             @@
##           master     #565      +/-   ##
==========================================
- Coverage   94.28%   94.24%   -0.05%     
==========================================
  Files          39       39              
  Lines        5008     5004       -4     
==========================================
- Hits         4722     4716       -6     
- Misses        286      288       +2
```


| [Impacted Files](https://codecov.io/gh/joblib/joblib/pull/565?src=pr&el=tree) | Coverage Δ | |
|---|---|---|
| [joblib/test/test\_hashing.py](https://codecov.io/gh/joblib/joblib/pull/565?src=pr&el=tree#diff-am9ibGliL3Rlc3QvdGVzdF9oYXNoaW5nLnB5) | `98.85% <100%> (-0.03%)` | :arrow_down: |
| [joblib/hashing.py](https://codecov.io/gh/joblib/joblib/pull/565?src=pr&el=tree#diff-am9ibGliL2hhc2hpbmcucHk=) | `92.98% <100%> (ø)` | :arrow_up: |
| [joblib/\_parallel\_backends.py](https://codecov.io/gh/joblib/joblib/pull/565?src=pr&el=tree#diff-am9ibGliL19wYXJhbGxlbF9iYWNrZW5kcy5weQ==) | `94.85% <0%> (-1.41%)` | :arrow_down: |
| [joblib/test/test\_parallel.py](https://codecov.io/gh/joblib/joblib/pull/565?src=pr&el=tree#diff-am9ibGliL3Rlc3QvdGVzdF9wYXJhbGxlbC5weQ==) | `95.92% <0%> (+0.21%)` | :arrow_up: |

------

[Continue to review full report at Codecov](https://codecov.io/gh/joblib/joblib/pull/565?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/565?src=pr&el=footer). Last update [8892839...c5f300c](https://codecov.io/gh/joblib/joblib/pull/565?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).

lesteve: Thanks a lot for your PR, I am afraid this is not the right fix. What you need to change is not the hashing but the auto memmapping mechanism. You need to change the lines around here: https://github.com/joblib/joblib/blob/8892839cb883b95fd2de3656bf719488eacb851f/joblib/_memmapping_reducer.py#L252-L256 and add the id of the object to the filename. You probably want to add a test too (the original bug report may provide a good test).
elsander: Thanks for the feedback-- this fix is much better, and didn't involve updating any other tests.
lesteve: The Windows failure looks like a glitch ... it would be great to push an empty commit and see whether it goes away.

The Python 3.4 failure looks weird ... probably you need to use @with_numpy for your test. But why the error only happens with Python 3.4, no clue ...
ogrisel: numpy is not a mandatory dependency of joblib and tests that use numpy should be marked as such (see other examples in the test suite). This is the cause of the failed test on travis.

Please also add an entry in the changelog and +1 for merge.
elsander: I did add an entry in the changelog, unless there's another place to add the change other than `CHANGES.rst`. Just added the decorator, hopefully that will resolve the testing issue.
elsander: I'm not sure how to proceed with the Windows error. It failed after pushing a commit, so if it's a glitch, it's a consistent one. It seems to be failing on the Parallel call itself, not on assertion. Unfortunately I know very little about development for Windows... does anyone have thoughts on how to resolve the error?
lesteve: I can reproduce the Windows problem so it needs some investigation. You don't happen to have a Windows machine easily accessible with Python installed? Otherwise I'll try to look at it, but I can not promise it will happen before next week.

Maybe @ogrisel has a suggestion off the top of his head, because I seem to remember he fixed similar issues about memmap permissions errors on Windows in the past.
elsander: Unfortunately I don't, but I won't have time to work on this more until sometime next week anyway.
ogrisel: Unfortunately, I don't know why it fails in this case and why it did not fail before. Maybe we can ignore the error under windows and only rely on the `atexit` callback.
ogrisel: In particular I do not understand why this new test fails while `test_auto_memmap_on_arrays_from_generator` does not.
elsander: Agreed, I don't understand why this test fails on Windows. Is there a flag I can add to ignore the error? I can add that if the maintainers are comfortable with ignoring the error, otherwise I'm not sure how to move forward with this PR, unfortunately.
lesteve: I think this is fine to skip the tests on Windows (add a comment explaining why we are skipping it). There is an example of how do this in test_hashing.py:

```py
@skipif(sys.platform == 'win32', reason='This test is not stable under windows'
                                        ' for some reason')
def test_hash_numpy_performance():
    ...
```
elsander: Got it! I'll implement this either later this week or early next week.
elsander: Sorry for the long delay on this. This fix should do it.
GaelVaroquaux: This seems like a PR that we should merge.

@lesteve : any specific reason that we did not?
GaelVaroquaux: Cc @ogrisel, @tomMoral 
lesteve: The reason we did not merge it is: https://github.com/joblib/joblib/pull/565#discussion_r157519117. Basically if we pass `X` and `X.copy()` as arguments, we want them to be memmapped to the same array when `mmap_mode='r'`, which is what is used in 99% of the cases.  

Also IIRC,  talking with @ogrisel, it seems like a bad idea in general to allow mmap in write mode or at least there was not an obvious real use case for it.

To sum up this is a bug fix in an edge case of a use case we are not entirely sure whether it actually exists for a good reason ...
GaelVaroquaux: Cc @ogrisel, @tomMoral 
GaelVaroquaux: OK, so I now understand the logic: if mmap_mode="r" or mmap_mode="c", the data on the disk is never modified, so the bug cannot happen. Hence it is useful to have the original behavior to save resources.

So, the right fix is probably to add "id" in the filename, as @elsander did, but only if not mmap_mode in ('r', 'c').

We should probably also update the test to convince ourselves that the solution is the correct one.

@elsander, does this make sense? And do you want to go ahead with the proposed solution?
elsander: @GaelVaroquaux Yeah, this makes more sense to me. I'll make a fix today or tomorrow.
GaelVaroquaux: Thanks Elizabeth. This is terribly useful. And sorry for the
communication delay that left this PR pending.

elsander: Ok, I added the logic to special-case `r+` and `w+`. I updated the test, but I couldn't think of a good way to test the behavior of `r` and `c` in the test. It's trivially true that the bug can't show up in these modes, since we don't write to disk, so I couldn't think of a good test to write that was actually testing anything relevant (as in, testing joblib behavior rather than numpy's memmapping). How does this look?
GaelVaroquaux: This looks great. Thank you so much. Merging!

Patch:
diff --git a/CHANGES.rst b/CHANGES.rst
index 278586b..ffc1c0c 100644
--- a/CHANGES.rst
+++ b/CHANGES.rst
@@ -4,6 +4,11 @@ Latest changes
 Development
 -----------
 
+Elizabeth Sander
+    Prevent numpy arrays with the same shape and data from hashing to
+    the same memmap, to prevent jobs with preallocated arrays from
+    writing over each other.
+
 Olivier Grisel
 
     Make ``Memory.cache`` robust to ``PermissionError (errno 13)`` under
diff --git a/joblib/_memmapping_reducer.py b/joblib/_memmapping_reducer.py
index 6153c67..d825b4a 100644
--- a/joblib/_memmapping_reducer.py
+++ b/joblib/_memmapping_reducer.py
@@ -261,8 +261,13 @@ class ArrayMemmapReducer(object):
 
             # Find a unique, concurrent safe filename for writing the
             # content of this array only once.
-            basename = "{}-{}-{}.pkl".format(
-                os.getpid(), id(threading.current_thread()), hash(a))
+            if self._mmap_mode in ['r', 'c']:
+                basename = "{}-{}-{}.pkl".format(
+                    os.getpid(), id(threading.current_thread()), hash(a))
+            else:
+                basename = "{}-{}-{}-{}.pkl".format(
+                    os.getpid(), id(threading.current_thread()),
+                    hash(a), id(a))
             filename = os.path.join(self._temp_folder, basename)
 
             # In case the same array with the same content is passed several


Test patch:
diff --git a/joblib/test/test_memmapping.py b/joblib/test/test_memmapping.py
index df50b4a..16f7954 100644
--- a/joblib/test/test_memmapping.py
+++ b/joblib/test/test_memmapping.py
@@ -7,8 +7,9 @@ from joblib.test.common import setup_autokill
 from joblib.test.common import teardown_autokill
 from joblib.test.common import with_multiprocessing
 from joblib.test.common import with_dev_shm
-from joblib.testing import raises, parametrize
+from joblib.testing import raises, parametrize, skipif
 from joblib.backports import make_memmap
+from joblib.parallel import Parallel, delayed
 
 from joblib.pool import MemmappingPool
 from joblib.executor import _TestingMemmappingExecutor
@@ -545,3 +546,21 @@ def test_pool_get_temp_dir(tmpdir):
     if sys.platform.startswith('win'):
         assert shared_mem is False
     assert pool_folder.endswith(pool_folder_name)
+
+
+@with_numpy
+@skipif(sys.platform == 'win32', reason='This test fails with a '
+        'PermissionError on Windows')
+@parametrize("mmap_mode", ["r+", "w+"])
+def test_numpy_arrays_use_different_memory(mmap_mode):
+    def func(arr, value):
+        arr[:] = value
+        return arr
+
+    arrays = [np.zeros((10, 10), dtype='float64') for i in range(10)]
+
+    results = Parallel(mmap_mode=mmap_mode, max_nbytes=0, n_jobs=2)(
+        delayed(func)(arr, i) for i, arr in enumerate(arrays))
+
+    for i, arr in enumerate(results):
+        np.testing.assert_array_equal(arr, i)