Repository: scikit-learn/scikit-learn
Pull request: Support cross 32bit/64bit pickles for decision tree
#### Reference Issues/PRs

Fix #19602.

#### What does this implement/fix? Explain your changes.

This makes it possible to save a decision tree pickle on a 64bit machine and load it on a 32bit machine (as well as the other way around although this is probably not that useful in practice).

The test uses a similar approach as #21539: it checks that a pickle containing arrays of a different bitness (e.g. 64bit arrays on a 32bit machine) can be loaded fine. This is not exactly what we want to test, i.e. generating a pickle on 64bit machine and load it on a 32 bit machine but is close enough. 

If we want, we could have an additional test that is exactly the use case we are aiming to support. We can generate pickle bytes on a 64 bit machine (we can put a comment indicating how to generate it if we need to regenerate it) and load it on the 32 bit CI.

#### Any other comments?

#21539 should be merged first, putting this one in draft for now.

- [x] once #21539 is merged, I think that the endianness test should pass with joblib < 1.1 so I can remove pytest.mark.skipif

Testing a similar thing for more/other estimators is left for a future PR.

Problem statement:
Tree pickle portability between 64bit and 32bit arch
`Decision Tree Classifier`

```
C:\Users\Aryan\Desktop\heart-host>python app.py
C:\Users\Aryan\AppData\Local\Programs\Python\Python38-32\lib\site-packages\sklearn\utils\deprecation.py:144: FutureWarning: The sklearn.tree.tree module is  deprecated in version 0.22 and will be removed in version 0.24. The corresponding classes / functions should instead be imported from sklearn.tree. Anything that cannot be imported from sklearn.tree is now part of the private API.
  warnings.warn(message, FutureWarning)
Traceback (most recent call last):
  File "app.py", line 5, in <module>
    heart_dtc = pickle.load(open('dtc.pkl', 'rb'))
  File "sklearn\tree\_tree.pyx", line 607, in sklearn.tree._tree.Tree.__cinit__
`ValueError: Buffer dtype mismatch, expected 'SIZE_t' but got 'long long'`
```

Hints from issue discussion:
Are you running the same version scikit-learn than the one used to create the `pkl`?
sklearn version : 0.22.2
YES
Could you as well give information about the OS and CPU architecture where the model was trained and where it is deployed?
From the type error, one could guess that the model was pickled with Python 32 bit on Windows and loaded on Python 64 bit (e.g. on a Linux server). Is this the case?

If this is confirmed, we should probably consider this a bug of scikit-learn as @rth recently hinted in another issue or PR (that I failed to find).

However we would need to make sure that this bug has not already been fixed. @aryanxk02, can you try to check if you can reproduce the problem with scikit-learn 0.24 or a even better a nightly build?

https://scikit-learn.org/stable/developers/advanced_installation.html#installing-nightly-builds
> Could you as well give information about the OS and CPU architecture where the model was trained and where it is deployed?

currently on Windows 64 bit Operating System
> From the type error, one could guess that the model was pickled with Python 32 bit on Windows and loaded on Python 64 bit (e.g. on a Linux server). Is this the case?
> 
> If this is confirmed, we should probably consider this a bug of scikit-learn as @rth recently hinted in another issue or PR (that I failed to find).
> 
> However we would need to make sure that this bug has not already been fixed. @aryanxk02, can you try to check if you can reproduce the problem with scikit-learn 0.24 or a even better a nightly build?
> 
> https://scikit-learn.org/stable/developers/advanced_installation.html#installing-nightly-builds

Okay, I developed the entire code (including ML model to pickle file) and ran it on python 32 bit through Jupyter Notebook on Windows.

Sure, regarding scikit-learn 0.24 I will try and will inform.
What version / bitness of Python and OS do you use to run the flask app?
> What version / bitness of Python and OS do you use to run the flask app?

1. Python 3.8 - 32 bit
2. Windows 10 - 64 bit

If you use the same version and bitness of Python both the training the model and running the flask app then I do not understand how you can get this error.

Can you please run `import sklearn; sklearn.show_versions()` both in your flask app and in your jupyter notebook to check this?
Also, can you check that you can run `pickle.load(open('dtc.pkl', 'rb'))` without error in your notebook right after saving the model to disk?

Commit message: Remove skipif since joblib < 1.1 loading in non-native endianness is now handled by astype in __cinit__

Patch to review:
diff --git a/doc/whats_new/v1.1.rst b/doc/whats_new/v1.1.rst
index 02982538ddd59..c478159523747 100644
--- a/doc/whats_new/v1.1.rst
+++ b/doc/whats_new/v1.1.rst
@@ -60,7 +60,7 @@ Changelog
   add this information to the plot.
   :pr:`21038` by :user:`Guillaume Lemaitre <glemaitre>`.
 
-- |Enhancement| :class:`cluster.SpectralClustering` and :func:`cluster.spectral` 
+- |Enhancement| :class:`cluster.SpectralClustering` and :func:`cluster.spectral`
   now include the new `'cluster_qr'` method from :func:`cluster.cluster_qr`
   that clusters samples in the embedding space as an alternative to the existing
   `'kmeans'` and `'discrete'` methods.
@@ -180,6 +180,14 @@ Changelog
   :class:`~sklearn.random_projection.SparseRandomProjection`. :pr:`21330` by
   :user:`Loïc Estève <lesteve>`.
 
+:mod:`sklearn.tree`
+...................
+
+- [Fix| Support loading pickles of decision tree models when the pickle has
+  been generated on a platform with a different bitness. A typical example is
+  to train and pickle the model on 64 bit machine and load the model on a 32
+  bit machine for prediction. :pr:`21552` by :user:`Loïc Estève <lesteve>`.
+
 Code and Documentation Contributors
 -----------------------------------
 
diff --git a/sklearn/tree/_tree.pyx b/sklearn/tree/_tree.pyx
index 84fb808318a49..e1bc44da7e554 100644
--- a/sklearn/tree/_tree.pyx
+++ b/sklearn/tree/_tree.pyx
@@ -583,13 +583,26 @@ cdef class Tree:
         def __get__(self):
             return self._get_value_ndarray()[:self.node_count]
 
-    def __cinit__(self, int n_features, np.ndarray[SIZE_t, ndim=1] n_classes,
-                  int n_outputs):
+    def __cinit__(self, int n_features, np.ndarray n_classes, int n_outputs):
         """Constructor."""
+        cdef SIZE_t dummy
+        size_t_dtype = np.array(dummy).dtype
+
+        if n_classes.ndim != 1:
+            raise ValueError(f"Wrong dimensions for n_classes: expected 1, got {n_classes.ndim}")
+
+        try:
+            n_classes = n_classes.astype(
+                dtype=size_t_dtype, casting='same_kind')
+        except Exception as exc:
+            raise ValueError(
+                f"Error converting n_classes: full exception was\n{exc}")
+
         # Input/Output layout
         self.n_features = n_features
         self.n_outputs = n_outputs
         self.n_classes = NULL
+
         safe_realloc(&self.n_classes, n_outputs)
 
         self.max_n_classes = np.max(n_classes)
@@ -645,17 +658,29 @@ cdef class Tree:
                        self.max_n_classes)
 
         if (node_ndarray.dtype != NODE_DTYPE):
-            # possible mismatch of big/little endian due to serialization
-            # on a different architecture. Try swapping the byte order.  
-            node_ndarray = node_ndarray.byteswap().newbyteorder()
-            if (node_ndarray.dtype != NODE_DTYPE):
-                raise ValueError('Did not recognise loaded array dytpe')
+            try:
+                node_ndarray = node_ndarray.astype(
+                    NODE_DTYPE, casting='same_kind')
+            except Exception as exc:
+                raise ValueError(
+                    "Error when converting node array "
+                    f"from dtype {node_ndarray.dtype} to {NODE_DTYPE}. "
+                    f"Exception was\n{exc}")
+
+        if (value_ndarray.dtype != np.float64):
+            try:
+                value_ndarray = value_ndarray.astype(
+                    np.float64, casting='same_kind')
+            except Exception as exc:
+                raise ValueError(
+                    "Error when converting value array "
+                    f" from dtype {value_ndarray.dtype} to float64. "
+                    f"Exception was\n{exc}")
 
         if (node_ndarray.ndim != 1 or
                 not node_ndarray.flags.c_contiguous or
                 value_ndarray.shape != value_shape or
-                not value_ndarray.flags.c_contiguous or
-                value_ndarray.dtype != np.float64):
+                not value_ndarray.flags.c_contiguous):
             raise ValueError('Did not recognise loaded array layout')
 
         self.capacity = node_ndarray.shape[0]
diff --git a/sklearn/tree/tests/test_tree.py b/sklearn/tree/tests/test_tree.py
index 90da2e04d6a9f..9c9420a99696d 100644
--- a/sklearn/tree/tests/test_tree.py
+++ b/sklearn/tree/tests/test_tree.py
@@ -7,6 +7,7 @@
 import struct
 import io
 import copyreg
+import re
 
 import pytest
 import numpy as np
@@ -37,7 +38,7 @@
 
 from sklearn.utils.estimator_checks import check_sample_weights_invariance
 from sklearn.utils.validation import check_random_state
-from sklearn.utils import parse_version
+from sklearn.utils import _IS_32BIT
 
 from sklearn.exceptions import NotFittedError
 
@@ -2212,10 +2213,6 @@ def get_pickle_non_native_endianness():
     assert np.isclose(score, new_score)
 
 
-@pytest.mark.skipif(
-    parse_version(joblib.__version__) < parse_version("1.1"),
-    reason="joblib >= 1.1 is needed to load numpy arrays in native endianness",
-)
 def test_different_endianness_joblib_pickle():
     X, y = datasets.make_classification(random_state=0)
 
@@ -2240,3 +2237,89 @@ def get_joblib_pickle_non_native_endianness():
     new_clf = joblib.load(get_joblib_pickle_non_native_endianness())
     new_score = new_clf.score(X, y)
     assert np.isclose(score, new_score)
+
+
+def get_different_bitness_dtype_descr_single(dtype_descr_element):
+    dtype_name, dtype_str, *rest = dtype_descr_element
+
+    if _IS_32BIT:
+        new_dtype_str = re.sub(r"([iufc])4", r"\g<1>8", dtype_str)
+    else:
+        new_dtype_str = re.sub(r"([iufc])8", r"\g<1>4", dtype_str)
+
+    return (dtype_name, new_dtype_str, *rest)
+
+
+def get_different_bitness_array(arr):
+    descr = arr.dtype.descr
+
+    if arr.dtype.names is not None:
+        # Remove potential padding fields
+        descr = [
+            (field_name, *rest)
+            for field_name, *rest in descr
+            if field_name in arr.dtype.names
+        ]
+
+    new_descr = [get_different_bitness_dtype_descr_single(each) for each in descr]
+    # avoid creating dummy field names in simple dtype case e.g.
+    # dtype(np.float64)
+    if len(new_descr) == 1:
+        new_descr = new_descr[0][1]
+
+    return arr.astype(new_descr, casting="same_kind")
+
+
+def reduce_different_bitness_ndarray(arr):
+    return get_different_bitness_array(arr).__reduce__()
+
+
+def test_different_bitness_pickle():
+    X, y = datasets.make_classification(random_state=0)
+
+    clf = DecisionTreeClassifier(random_state=0, max_depth=3)
+    clf.fit(X, y)
+    score = clf.score(X, y)
+
+    def reduce_ndarray(arr):
+        return arr.byteswap().newbyteorder().__reduce__()
+
+    def get_different_bitness_pickle():
+        f = io.BytesIO()
+        p = pickle.Pickler(f)
+        p.dispatch_table = copyreg.dispatch_table.copy()
+        p.dispatch_table[np.ndarray] = reduce_different_bitness_ndarray
+
+        p.dump(clf)
+        f.seek(0)
+        return f
+
+    new_clf = pickle.load(get_different_bitness_pickle())
+    new_score = new_clf.score(X, y)
+    assert np.isclose(score, new_score)
+
+
+def test_different_bitness_joblib_pickle():
+    X, y = datasets.make_classification(random_state=0)
+
+    clf = DecisionTreeClassifier(random_state=0, max_depth=3)
+    clf.fit(X, y)
+    score = clf.score(X, y)
+
+    class DifferentBitnessNumpyPickler(NumpyPickler):
+        def save(self, obj):
+            if isinstance(obj, np.ndarray):
+                obj = get_different_bitness_array(obj)
+            super().save(obj)
+
+    def get_different_bitness_joblib_pickle():
+        f = io.BytesIO()
+        p = DifferentBitnessNumpyPickler(f)
+
+        p.dump(clf)
+        f.seek(0)
+        return f
+
+    new_clf = joblib.load(get_different_bitness_joblib_pickle())
+    new_score = new_clf.score(X, y)
+    assert np.isclose(score, new_score)


Code review comments:
diff_hunk: @@ -645,17 +658,29 @@ cdef class Tree:
                        self.max_n_classes)
 
         if (node_ndarray.dtype != NODE_DTYPE):
-            # possible mismatch of big/little endian due to serialization
-            # on a different architecture. Try swapping the byte order.  
-            node_ndarray = node_ndarray.byteswap().newbyteorder()
-            if (node_ndarray.dtype != NODE_DTYPE):
-                raise ValueError('Did not recognise loaded array dytpe')
+            try:
+                node_ndarray = node_ndarray.astype(
+                    NODE_DTYPE, casting='same_kind')
+            except Exception as exc:
+                raise ValueError(
+                    "Error when converting node array "
+                    f"from dtype {node_ndarray.dtype} to {NODE_DTYPE}. "
+                    f"Exception was\n{exc}")
path: sklearn/tree/_tree.pyx
text: @user1:
I would prefer to use the raise / from construct to get the full details of the original exception instead of string formatting in the ValueError message.

```suggestion
                raise ValueError(
                    "Error when converting node array "
                    f"from dtype {node_ndarray.dtype} to {NODE_DTYPE}."
                ) from exc
```

diff_hunk: @@ -645,17 +658,29 @@ cdef class Tree:
                        self.max_n_classes)
 
         if (node_ndarray.dtype != NODE_DTYPE):
-            # possible mismatch of big/little endian due to serialization
-            # on a different architecture. Try swapping the byte order.  
-            node_ndarray = node_ndarray.byteswap().newbyteorder()
-            if (node_ndarray.dtype != NODE_DTYPE):
-                raise ValueError('Did not recognise loaded array dytpe')
+            try:
+                node_ndarray = node_ndarray.astype(
+                    NODE_DTYPE, casting='same_kind')
+            except Exception as exc:
+                raise ValueError(
+                    "Error when converting node array "
+                    f"from dtype {node_ndarray.dtype} to {NODE_DTYPE}. "
+                    f"Exception was\n{exc}")
+
+        if (value_ndarray.dtype != np.float64):
+            try:
+                value_ndarray = value_ndarray.astype(
+                    np.float64, casting='same_kind')
path: sklearn/tree/_tree.pyx
text: @user1:
Do we really need to convert the value array? I thought the dtype of that array would be np.float64 on any platform. I don't see any reason why the values would would be platform dependent.

@author:
- this `.astype` is also tackling different endianness right now (althouth 'same_kind' is a bit loose we probably want to use 'equiv' if we want to tackle only endianness)
- I guess the 64bit to 32bit conversion is linked to being more specific in what we are accepting as you said in https://github.com/scikit-learn/scikit-learn/pull/21552/files#r752062913. I need to think a bit more about this ...


@user1:
But cross-endianess pickling already works, no? `test_different_endianness_pickle` already passes on `main`.

diff_hunk: @@ -2240,3 +2237,89 @@ def get_joblib_pickle_non_native_endianness():
     new_clf = joblib.load(get_joblib_pickle_non_native_endianness())
     new_score = new_clf.score(X, y)
     assert np.isclose(score, new_score)
+
+
+def get_different_bitness_dtype_descr_single(dtype_descr_element):
+    dtype_name, dtype_str, *rest = dtype_descr_element
+
+    if _IS_32BIT:
+        new_dtype_str = re.sub(r"([iufc])4", r"\g<1>8", dtype_str)
+    else:
+        new_dtype_str = re.sub(r"([iufc])8", r"\g<1>4", dtype_str)
+
+    return (dtype_name, new_dtype_str, *rest)
+
+
+def get_different_bitness_array(arr):
+    descr = arr.dtype.descr
+
+    if arr.dtype.names is not None:
+        # Remove potential padding fields
+        descr = [
+            (field_name, *rest)
+            for field_name, *rest in descr
+            if field_name in arr.dtype.names
+        ]
+
+    new_descr = [get_different_bitness_dtype_descr_single(each) for each in descr]
+    # avoid creating dummy field names in simple dtype case e.g.
+    # dtype(np.float64)
+    if len(new_descr) == 1:
+        new_descr = new_descr[0][1]
+
+    return arr.astype(new_descr, casting="same_kind")
+
+
+def reduce_different_bitness_ndarray(arr):
+    return get_different_bitness_array(arr).__reduce__()
+
+
+def test_different_bitness_pickle():
+    X, y = datasets.make_classification(random_state=0)
+
+    clf = DecisionTreeClassifier(random_state=0, max_depth=3)
+    clf.fit(X, y)
+    score = clf.score(X, y)
+
+    def reduce_ndarray(arr):
+        return arr.byteswap().newbyteorder().__reduce__()
path: sklearn/tree/tests/test_tree.py
text: @user1:
`reduce_ndarray` is never called. I think this is a copy/paste left over of the `test_different_endianness_pickle` test.

I am in favor of keeping the matters of cross-endianess pickling and cross-bitness pickling covered in independent tests to make it easier to debug in case of a future regression.

diff_hunk: @@ -645,17 +658,29 @@ cdef class Tree:
                        self.max_n_classes)
 
         if (node_ndarray.dtype != NODE_DTYPE):
path: sklearn/tree/_tree.pyx
text: @user1:
I think we should be stricter about the specific fields that we expect to be potentially require a cast. We should not blindly accept other casts that we don't anticipate to happen in nominal scikit-learn usage scenarios.

In the original report #19602, only the `SIZE_t` fields are expected to be platform specific. It means that we should only map i4 to i8 or vice versa (big or little endian). All other mappings should be rejected with a more specific error message.

```
cdef struct Node:
    # Base storage structure for the nodes in a Tree object

    SIZE_t left_child                    # id of the left child of the node
    SIZE_t right_child                   # id of the right child of the node
    SIZE_t feature                       # Feature used for splitting the node
    DOUBLE_t threshold                   # Threshold value at the node
    DOUBLE_t impurity                    # Impurity of the node (i.e., the value of the criterion)
    SIZE_t n_node_samples                # Number of samples at the node
    DOUBLE_t weighted_n_node_samples     # Weighted number of samples at the node
```



@author:
Fair point, that means that the test would need to be more specific, I need to look at how much harder it is. Right now I am converting all 64bit dtypes into 32bit dtypes when generating the "pseudo 32bit pickle" on a 64bit machine.

The check in the code as well would need to be more precise.

With the longer-term idea of having a common test for 32bit/64bit pickling, generating a more realistic "pseudo 32bit pickle" will need to be on a estimator by estimator basis and seems a lot more tricky to do.

@user1:
Yes I agree. I think it's ok to keep those tests estimator specific and only targets those who have a custom `__setstate__` or `__reduce__` method.

diff_hunk: @@ -583,13 +583,26 @@ cdef class Tree:
         def __get__(self):
             return self._get_value_ndarray()[:self.node_count]
 
-    def __cinit__(self, int n_features, np.ndarray[SIZE_t, ndim=1] n_classes,
-                  int n_outputs):
+    def __cinit__(self, int n_features, np.ndarray n_classes, int n_outputs):
         """Constructor."""
+        cdef SIZE_t dummy
+        size_t_dtype = np.array(dummy).dtype
+
+        if n_classes.ndim != 1:
+            raise ValueError(f"Wrong dimensions for n_classes: expected 1, got {n_classes.ndim}")
+
+        try:
+            n_classes = n_classes.astype(
+                dtype=size_t_dtype, casting='same_kind')
path: sklearn/tree/_tree.pyx
text: @user1:
embrace black 88 in your mind :)

```suggestion
            n_classes = n_classes.astype(dtype=size_t_dtype, casting='same_kind')
```

diff_hunk: @@ -645,17 +658,29 @@ cdef class Tree:
                        self.max_n_classes)
 
         if (node_ndarray.dtype != NODE_DTYPE):
-            # possible mismatch of big/little endian due to serialization
-            # on a different architecture. Try swapping the byte order.  
-            node_ndarray = node_ndarray.byteswap().newbyteorder()
-            if (node_ndarray.dtype != NODE_DTYPE):
-                raise ValueError('Did not recognise loaded array dytpe')
+            try:
+                node_ndarray = node_ndarray.astype(
+                    NODE_DTYPE, casting='same_kind')
+            except Exception as exc:
+                raise ValueError(
+                    "Error when converting node array "
+                    f"from dtype {node_ndarray.dtype} to {NODE_DTYPE}. "
+                    f"Exception was\n{exc}")
+
+        if (value_ndarray.dtype != np.float64):
+            try:
+                value_ndarray = value_ndarray.astype(
+                    np.float64, casting='same_kind')
+            except Exception as exc:
+                raise ValueError(
+                    "Error when converting value array "
+                    f" from dtype {value_ndarray.dtype} to float64. "
+                    f"Exception was\n{exc}")
path: sklearn/tree/_tree.pyx
text: @user1:
Same comment here if needed but I don't think we need to cast the `value_ndarray` in the first place.

```suggestion
                raise ValueError(
                    "Error when converting value array "
                    f" from dtype {value_ndarray.dtype} to float64."
                ) from exc
```