Repository: dask/dask-ml

Issue:
BaseEstimator tokenization may cause issues for fitted models
In https://github.com/dask/dask-ml/blob/master/dask_ml/model_selection/_normalize.py#L18-L24, we register a tokenizer for scikit-learn models that inherit from BaseEstimator. This can cause issues when we have multiple instances of the same model that have been fitted to different pieces of data (as in https://github.com/dask/dask-ml/pull/657).

```python
In [8]: import dask
   ...: import sklearn.datasets
   ...: import sklearn.linear_model
   ...: import dask_ml.model_selection._normalize
   ...:
   ...: m1 = sklearn.linear_model.LogisticRegression(random_state=0)
   ...: m2 = sklearn.linear_model.LogisticRegression(random_state=0)
   ...:
   ...: # Not fitted, fine for these to match
   ...: assert dask.base.tokenize(m1) == dask.base.tokenize(m2)
   ...:
   ...: X1, y1 = sklearn.datasets.make_classification()
   ...: X2, y2 = sklearn.datasets.make_classification()
   ...: m1.fit(X1, y1)
   ...: m2.fit(X2, y2)
   ...:
   ...: # After fitting, these should be different!
   ...: assert dask.base.tokenize(m1) != dask.base.tokenize(m2)
   ...:
   ...:
---------------------------------------------------------------------------
AssertionError                            Traceback (most recent call last)
<ipython-input-8-590215541e34> in <module>
     16
     17 # After fitting, these should be different!
---> 18 assert dask.base.tokenize(m1) != dask.base.tokenize(m2)
     19

AssertionError:
```

I think that `normalize_estimator` should also (try to) hash things like `coef_`. Since this is supposed to work for arbitrary estimators, the safest thing to do is tokenize any attribute ending in `_`. We might exclude selected attributes like `GridSearchCV.cv_results_`, since those include things that are incidental (like the time it took to train a specific split).

Discussion:
TomAugspurger: Only failure is the known hang on incremental `test_basic` .

Patch:
diff --git a/dask_ml/__init__.py b/dask_ml/__init__.py
index df4c4253..ab311357 100644
--- a/dask_ml/__init__.py
+++ b/dask_ml/__init__.py
@@ -1,7 +1,18 @@
 from pkg_resources import DistributionNotFound, get_distribution
 
+# Ensure we always register tokenizers
+from dask_ml.model_selection import _normalize
+
+__all__ = []
+
 try:
     __version__ = get_distribution(__name__).version
+    __all__.append("__version__")
 except DistributionNotFound:
     # package is not installed
     pass
+
+
+del DistributionNotFound
+del get_distribution
+del _normalize
diff --git a/dask_ml/model_selection/_normalize.py b/dask_ml/model_selection/_normalize.py
index 6cac6894..9591ade8 100644
--- a/dask_ml/model_selection/_normalize.py
+++ b/dask_ml/model_selection/_normalize.py
@@ -1,6 +1,9 @@
 from __future__ import absolute_import, division, print_function
 
+import warnings
+
 import numpy as np
+import sklearn.exceptions
 from dask.base import normalize_token
 from sklearn.base import BaseEstimator
 from sklearn.model_selection._split import (
@@ -21,7 +24,22 @@ def normalize_estimator(est):
 
     Note: Since scikit-learn requires duck-typing, but not sub-typing from
     ``BaseEstimator``, we sometimes need to call this function directly."""
-    return type(est).__name__, normalize_token(est.get_params())
+    base = [type(est).__name__, normalize_token(est.get_params())]
+    # fitted attributes: https://github.com/dask/dask-ml/issues/658
+    attrs = [x for x in dir(est) if x.endswith("_") and not x.startswith("_")]
+    exclude = {"cv_results_", "model_history_", "history_", "refit_time_"}
+
+    with warnings.catch_warnings():
+        warnings.simplefilter("ignore", FutureWarning)
+        for attr in attrs:
+            if attr in exclude:
+                continue
+            try:
+                val = getattr(est, attr)
+            except (sklearn.exceptions.NotFittedError, AttributeError):
+                continue
+            base.append(val)
+    return tuple(base)
 
 
 def normalize_random_state(random_state):
diff --git a/setup.cfg b/setup.cfg
index dffd5ec8..e55bcc71 100644
--- a/setup.cfg
+++ b/setup.cfg
@@ -34,6 +34,9 @@ strict_equality=True
 [mypy-dask_ml.metrics]
 check_untyped_defs=False
 
+[mypy-dask_ml.model_selection.*]
+follow_imports=skip
+
 [tool:pytest]
 addopts = -rsx -v --durations=10
 minversion = 3.2


Test patch:
diff --git a/tests/test_normalize.py b/tests/test_normalize.py
new file mode 100644
index 00000000..7eb5fd14
--- /dev/null
+++ b/tests/test_normalize.py
@@ -0,0 +1,38 @@
+import dask
+import sklearn.datasets
+import sklearn.linear_model
+import sklearn.model_selection
+
+import dask_ml  # noqa
+from dask_ml._compat import SK_022
+
+if SK_022:
+    # deprecated in 0.22
+    iid = {}
+else:
+    iid = {"iid": True}
+
+
+def test_normalize_estimator():
+    m1 = sklearn.linear_model.LogisticRegression(solver="lbfgs")
+    m2 = sklearn.linear_model.LogisticRegression(solver="lbfgs")
+
+    assert dask.base.tokenize(m1) == dask.base.tokenize(m2)
+    m1.fit(*sklearn.datasets.make_classification())
+    m2.fit(*sklearn.datasets.make_classification())
+
+    assert dask.base.tokenize(m1) != dask.base.tokenize(m2)
+
+
+def test_normalize_estimator_cv():
+    param_grid = {"C": [0.01]}
+    a = sklearn.linear_model.LogisticRegression(random_state=0, solver="lbfgs")
+    m1 = sklearn.model_selection.GridSearchCV(a, param_grid, cv=3, **iid)
+    m2 = sklearn.model_selection.GridSearchCV(a, param_grid, cv=3, **iid)
+
+    assert dask.base.tokenize(m1) == dask.base.tokenize(m2)
+    X, y = sklearn.datasets.make_classification()
+    m1.fit(X, y)
+    m2.fit(X, y)
+
+    assert dask.base.tokenize(m1) == dask.base.tokenize(m2)