Repository: scikit-learn/scikit-learn
Pull request: Impose shared memory when fitting a SGDClassifier
Fixes #12518 

Previously, fitting a `SGDClassifier` instance in a multi-class setting involved modifying in-place its `coef_` attribute in the workers created by a `joblib.Parallel` call. However, this `Parallel` call was not requiring shared memory. For this reason, calling `SGDClassifier.fit` from a context manager with a `loky` or a `multiprocessing` backend (where workers are processes, and memory is not shared)  was causing a segmentation fault in the worker processes.

This PR fixes it by changing the soft constraint `prefer='threads'` into the hard constraint `require='sharedmem'` when instantiating the `Parallel` instance.

We also introduce a non-regression test that makes sure that fitting a `SGDClassifier` in a parallel fashion yields coefficients identical to those obtained from a sequential fit, and this for each backend (`loky`, `threading`, `multiprocessing`)

Problem statement:
BUG SGDClassifier.fit segfaulting under loky and multiprocessing backend
Fitting a `SGDClassifier` instance in a multi-class setting involves modifying in-place its`coef_` attribute in the workers created by a `joblib.Parallel` call. However, this Parallel call is not requiring shared memory. For this reason, calling `SGDClassifier.fit` from a context manager with a `loky` or a multiprocessing backend (where workers are processes, and memory is not shared) is causing a segmentation fault in the worker processes.

Hints from issue discussion:

Commit message: TST skip test if no scipy.sparse.random

Patch to review:
diff --git a/sklearn/linear_model/stochastic_gradient.py b/sklearn/linear_model/stochastic_gradient.py
index 146d9623f22e7..843f9252f3426 100644
--- a/sklearn/linear_model/stochastic_gradient.py
+++ b/sklearn/linear_model/stochastic_gradient.py
@@ -639,7 +639,7 @@ def _fit_multiclass(self, X, y, alpha, C, learning_rate,
         validation_mask = self._make_validation_split(y)
 
         # Use joblib to fit OvA in parallel.
-        result = Parallel(n_jobs=self.n_jobs, prefer="threads",
+        result = Parallel(n_jobs=self.n_jobs, require="sharedmem",
                           verbose=self.verbose)(
             delayed(fit_binary)(self, i, X, y, alpha, C, learning_rate,
                                 max_iter, self._expanded_class_weight[i],
diff --git a/sklearn/linear_model/tests/test_sgd.py b/sklearn/linear_model/tests/test_sgd.py
index 9ca05b3254706..ff20d3cd7375a 100644
--- a/sklearn/linear_model/tests/test_sgd.py
+++ b/sklearn/linear_model/tests/test_sgd.py
@@ -1516,3 +1516,48 @@ def test_multi_core_gridsearch_and_early_stopping():
                                 random_state=0)
     search.fit(iris.data, iris.target)
     assert search.best_score_ > 0.8
+
+
+@pytest.mark.skipif(
+        not hasattr(sp, 'random'),
+        reason="this test uses scipy.random, that was introduced in version  "
+        "0.17. This skip condition can be dropped as soon as we drop support "
+        "for scipy versions older than 0.17")
+def test_SGDClassifier_fit_for_all_backends():
+    # This is a non-regression smoke test. In the multi-class case,
+    # SGDClassifier.fit fits each class in a one-versus-all fashion using
+    # joblib.Parallel.  However, each OvA step updates the coef_ attribute of
+    # the estimator in-place. Internally, SGDClassifier calls Parallel using
+    # require='sharedmem'. This test makes sure SGDClassifier.fit works
+    # consistently even when the user asks for a backend that does not provide
+    # sharedmem semantics.
+
+    # We further test a case where memmapping would have been used if
+    # SGDClassifier.fit was called from a loky or multiprocessing backend. In
+    # this specific case, in-place modiciation of clf.coef_ would have caused a
+    # segmentation fault
+
+    random_state = 42
+
+    # Create a classification problem with 50000 features and 20 classes. Using
+    # loky or multiprocessing this make the clf.coef_ exceed the threshold
+    # above which memmaping is used in joblib and loky (1MB as of 2018/11/1).
+    sparse_X = sp.random(1000, 50000, density=0.01, format='csr',
+                         random_state=random_state)
+    y = np.random.choice(20, 1000)
+
+    # begin by fitting a SGD classifier sequentially
+    clf = SGDClassifier(tol=1e-3, max_iter=1000, n_jobs=1,
+                        random_state=random_state)
+    clf.fit(sparse_X, y=y)
+    coef_sequential_fitting = clf.coef_
+
+    # fit a SGDClassifier for each available backend, and make sure the
+    # coefficients are equal to those obtained using a sequential fit
+    from sklearn.externals.joblib import parallel_backend
+    for backend in ['threading', 'multiprocessing', 'loky']:
+        clf = SGDClassifier(tol=1e-3, max_iter=1000, n_jobs=4,
+                            random_state=random_state)
+        with parallel_backend(backend=backend):
+            clf.fit(sparse_X, y=y)
+        assert_array_equal(coef_sequential_fitting, clf.coef_)


Code review comments:
diff_hunk: @@ -1516,3 +1516,48 @@ def test_multi_core_gridsearch_and_early_stopping():
                                 random_state=0)
     search.fit(iris.data, iris.target)
     assert search.best_score_ > 0.8
+
+
+@pytest.mark.skipif(
+        not hasattr(sp, 'random'),
+        reason="this test uses scipy.random, that was introduced in version  "
+        "0.17. This skip condition can be dropped as soon as we drop support "
+        "for scipy versions older than 0.17")
+def test_SGDClassifier_fit_for_all_backends():
+    # This is a non-regression smoke test. In the multi-class case,
+    # SGDClassifier.fit fits each class in a one-versus-all fashion using
+    # joblib.Parallel.  However, each OvA step updates the coef_ attribute of
+    # the estimator in-place. Internally, SGDClassifier calls Parallel using
+    # require='sharedmem'. This test makes sure SGDClassifier.fit works
+    # consistently even when the user asks for a backend that does not provide
+    # sharedmem semantics.
+
+    # We further test a case where memmapping would have been used if
+    # SGDClassifier.fit was called from a loky or multiprocessing backend. In
+    # this specific case, in-place modiciation of clf.coef_ would have caused a
+    # segmentation fault
+
+    random_state = 42
+
+    # Create a classification problem with 50000 features and 20 classes. Using
+    # loky or multiprocessing this make the clf.coef_ exceed the threshold
+    # above which memmaping is used in joblib and loky (1MB as of 2018/11/1).
+    sparse_X = sp.random(1000, 50000, density=0.01, format='csr',
path: sklearn/linear_model/tests/test_sgd.py
text: @user1:
style: `X` is enough.

diff_hunk: @@ -1516,3 +1516,48 @@ def test_multi_core_gridsearch_and_early_stopping():
                                 random_state=0)
     search.fit(iris.data, iris.target)
     assert search.best_score_ > 0.8
+
+
+@pytest.mark.skipif(
+        not hasattr(sp, 'random'),
+        reason="this test uses scipy.random, that was introduced in version  "
+        "0.17. This skip condition can be dropped as soon as we drop support "
+        "for scipy versions older than 0.17")
+def test_SGDClassifier_fit_for_all_backends():
+    # This is a non-regression smoke test. In the multi-class case,
+    # SGDClassifier.fit fits each class in a one-versus-all fashion using
+    # joblib.Parallel.  However, each OvA step updates the coef_ attribute of
+    # the estimator in-place. Internally, SGDClassifier calls Parallel using
+    # require='sharedmem'. This test makes sure SGDClassifier.fit works
+    # consistently even when the user asks for a backend that does not provide
+    # sharedmem semantics.
+
+    # We further test a case where memmapping would have been used if
+    # SGDClassifier.fit was called from a loky or multiprocessing backend. In
+    # this specific case, in-place modiciation of clf.coef_ would have caused a
+    # segmentation fault
+
+    random_state = 42
+
+    # Create a classification problem with 50000 features and 20 classes. Using
+    # loky or multiprocessing this make the clf.coef_ exceed the threshold
+    # above which memmaping is used in joblib and loky (1MB as of 2018/11/1).
+    sparse_X = sp.random(1000, 50000, density=0.01, format='csr',
+                         random_state=random_state)
+    y = np.random.choice(20, 1000)
+
+    # begin by fitting a SGD classifier sequentially
+    clf = SGDClassifier(tol=1e-3, max_iter=1000, n_jobs=1,
+                        random_state=random_state)
+    clf.fit(sparse_X, y=y)
+    coef_sequential_fitting = clf.coef_
+
+    # fit a SGDClassifier for each available backend, and make sure the
+    # coefficients are equal to those obtained using a sequential fit
+    from sklearn.externals.joblib import parallel_backend
path: sklearn/linear_model/tests/test_sgd.py
text: @user1:
Please move the import to the top and import from `sklearn.utils` instead.

diff_hunk: @@ -1516,3 +1516,48 @@ def test_multi_core_gridsearch_and_early_stopping():
                                 random_state=0)
     search.fit(iris.data, iris.target)
     assert search.best_score_ > 0.8
+
+
+@pytest.mark.skipif(
+        not hasattr(sp, 'random'),
+        reason="this test uses scipy.random, that was introduced in version  "
+        "0.17. This skip condition can be dropped as soon as we drop support "
+        "for scipy versions older than 0.17")
+def test_SGDClassifier_fit_for_all_backends():
+    # This is a non-regression smoke test. In the multi-class case,
+    # SGDClassifier.fit fits each class in a one-versus-all fashion using
+    # joblib.Parallel.  However, each OvA step updates the coef_ attribute of
+    # the estimator in-place. Internally, SGDClassifier calls Parallel using
+    # require='sharedmem'. This test makes sure SGDClassifier.fit works
+    # consistently even when the user asks for a backend that does not provide
+    # sharedmem semantics.
+
+    # We further test a case where memmapping would have been used if
+    # SGDClassifier.fit was called from a loky or multiprocessing backend. In
+    # this specific case, in-place modiciation of clf.coef_ would have caused a
path: sklearn/linear_model/tests/test_sgd.py
text: @user1:
modification

diff_hunk: @@ -1516,3 +1516,48 @@ def test_multi_core_gridsearch_and_early_stopping():
                                 random_state=0)
     search.fit(iris.data, iris.target)
     assert search.best_score_ > 0.8
+
+
+@pytest.mark.skipif(
+        not hasattr(sp, 'random'),
+        reason="this test uses scipy.random, that was introduced in version  "
+        "0.17. This skip condition can be dropped as soon as we drop support "
+        "for scipy versions older than 0.17")
+def test_SGDClassifier_fit_for_all_backends():
+    # This is a non-regression smoke test. In the multi-class case,
+    # SGDClassifier.fit fits each class in a one-versus-all fashion using
+    # joblib.Parallel.  However, each OvA step updates the coef_ attribute of
+    # the estimator in-place. Internally, SGDClassifier calls Parallel using
+    # require='sharedmem'. This test makes sure SGDClassifier.fit works
+    # consistently even when the user asks for a backend that does not provide
+    # sharedmem semantics.
+
+    # We further test a case where memmapping would have been used if
+    # SGDClassifier.fit was called from a loky or multiprocessing backend. In
+    # this specific case, in-place modiciation of clf.coef_ would have caused a
+    # segmentation fault
+
+    random_state = 42
+
+    # Create a classification problem with 50000 features and 20 classes. Using
+    # loky or multiprocessing this make the clf.coef_ exceed the threshold
+    # above which memmaping is used in joblib and loky (1MB as of 2018/11/1).
+    sparse_X = sp.random(1000, 50000, density=0.01, format='csr',
+                         random_state=random_state)
+    y = np.random.choice(20, 1000)
+
+    # begin by fitting a SGD classifier sequentially
+    clf = SGDClassifier(tol=1e-3, max_iter=1000, n_jobs=1,
+                        random_state=random_state)
+    clf.fit(sparse_X, y=y)
path: sklearn/linear_model/tests/test_sgd.py
text: @user1:
style: no need for `y=`, just call `clf.fit(X, y)`.