FIX Deep copy criterion in trees to fix concurrency bug (#19580)

samdbrice · Samuel Brice · glemaitre · commit 142115d13527 · 2021-04-28T09:40:11.000+02:00
Co-authored-by: Samuel Brice &lt;samuel.brice@twosigma.com&gt;
diff --git a/doc/whats_new/v0.24.rst b/doc/whats_new/v0.24.rst
@@ -48,6 +48,16 @@ Changelog
   :class:`~sklearn.semi_supervised.LabelPropagation`.
   :pr:`19271` by :user:`Zhaowei Wang <ThuWangzw>`.
 
+:mod:`sklearn.tree`
+.......................
+
+- |Fix| Fix a bug in `fit` of :class:`tree.BaseDecisionTree` that caused
+  segmentation faults under certain conditions. `fit` now deep copies the
+  `Criterion` object to prevent shared concurrent accesses.
+  :pr:`19580` by :user:`Samuel Brice <samdbrice>` and
+  :user:`Alex Adamson <aadamson>` and
+  :user:`Wil Yegelwel <wyegelwel>`.
+
 :mod:`sklearn.utils`
 ....................
 
diff --git a/sklearn/ensemble/tests/test_forest.py b/sklearn/ensemble/tests/test_forest.py
@@ -1377,3 +1377,21 @@ def test_little_tree_with_small_max_samples(ForestClass):
 
     msg = "Tree without `max_samples` restriction should have more nodes"
     assert tree1.node_count > tree2.node_count, msg
+
+
+@pytest.mark.parametrize('Forest', FOREST_REGRESSORS)
+def test_mse_criterion_object_segfault_smoke_test(Forest):
+    # This is a smoke test to ensure that passing a mutable criterion
+    # does not cause a segfault when fitting with concurrent threads.
+    # Non-regression test for:
+    # https://github.com/scikit-learn/scikit-learn/issues/12623
+    from sklearn.tree._criterion import MSE
+
+    y = y_reg.reshape(-1, 1)
+    n_samples, n_outputs = y.shape
+    mse_criterion = MSE(n_outputs, n_samples)
+    est = FOREST_REGRESSORS[Forest](
+        n_estimators=2, n_jobs=2, criterion=mse_criterion
+    )
+
+    est.fit(X_reg, y)
diff --git a/sklearn/tree/_classes.py b/sklearn/tree/_classes.py
@@ -16,6 +16,7 @@
 
 import numbers
 import warnings
+import copy
 from abc import ABCMeta
 from abc import abstractmethod
 from math import ceil
@@ -349,6 +350,10 @@ def fit(self, X, y, sample_weight=None, check_input=True,
             else:
                 criterion = CRITERIA_REG[self.criterion](self.n_outputs_,
                                                          n_samples)
+        else:
+            # Make a deepcopy in case the criterion has mutable attributes that
+            # might be shared and modified concurrently during parallel fitting
+            criterion = copy.deepcopy(criterion)
 
         SPLITTERS = SPARSE_SPLITTERS if issparse(X) else DENSE_SPLITTERS
 
