Skip to content

FIX error when deserialzing a Tree instance from a read only buffer #25585

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
Feb 13, 2023
Merged

FIX error when deserialzing a Tree instance from a read only buffer #25585

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
b5e5ddd
FIX error when deserialzing a Tree from a read only buffer
OmarManzoor Feb 10, 2023
b3e01d2
Improve the test and remove the partial display plot
OmarManzoor Feb 10, 2023
417a801
Fix windows compile error by using cnp.PyArray_DATA instead of const …
OmarManzoor Feb 10, 2023
79b7c57
Improve docstring comment of added test
OmarManzoor Feb 11, 2023
077ab59
Test that the classifier trees are equal
OmarManzoor Feb 11, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions sklearn/tree/_tree.pyx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -708,11 +708,9 @@ cdef class Tree:
if self._resize_c(self.capacity) != 0:
raise MemoryError("resizing tree to %d" % self.capacity)

cdef Node[::1] node_memory_view = node_ndarray
cdef DOUBLE_t[:, :, ::1] value_memory_view = value_ndarray
nodes = memcpy(self.nodes, &node_memory_view[0],
nodes = memcpy(self.nodes, cnp.PyArray_DATA(node_ndarray),
self.capacity * sizeof(Node))
value = memcpy(self.value, &value_memory_view[0, 0, 0],
value = memcpy(self.value, cnp.PyArray_DATA(value_ndarray),
self.capacity * self.value_stride * sizeof(double))

cdef int _resize(self, SIZE_t capacity) nogil except -1:
Expand Down
19 changes: 19 additions & 0 deletions sklearn/tree/tests/test_tree.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2406,3 +2406,22 @@ def test_splitter_serializable(Splitter):
splitter_back = pickle.loads(splitter_serialize)
assert splitter_back.max_features == max_features
assert isinstance(splitter_back, Splitter)


def test_tree_deserialization_from_read_only_buffer(tmpdir):
"""Check that Trees can be deserialized with read only buffers.

Non-regression test for gh-25584.
"""
pickle_path = str(tmpdir.join("clf.joblib"))
clf = DecisionTreeClassifier(random_state=0)
clf.fit(X_small, y_small)

joblib.dump(clf, pickle_path)
loaded_clf = joblib.load(pickle_path, mmap_mode="r")

assert_tree_equal(
loaded_clf.tree_,
clf.tree_,
"The trees of the original and loaded classifiers are not equal.",
)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Instead of indirectly relying on joblib parallelism via partial dependence computation, it would be better to write a non-regression test based on unpickling from a readonly buffer. This can be done as explained in this comment #25584 (comment).

To do this in a test, you could use the tmpdir fixture from pytest.

Also this would be great to write this test as a new estimator check to be run by test_common.py for any scikit-learn estimator, not just decision trees.

I thought we already had such a common test but apparently this is not the case.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed.

I need to improve my knowledge of common tests; ideally we must assert Cython implementations' correct behavior on (memmapped) readonly datasets.

@OmarManzoor: do you see how you can perform what Olivier has mentioned?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did check the common tests and from what I understand we need to replicate the behavior in this PR to all estimators in the common tests.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, for the scope of this PR, you can stick to the remarks in the two first paragraph of #25585 (comment).

Then, the general coverage explained in the third and fourth paragraphs of #25585 (comment) better be treated in another pull request IMO. I think you can author another PR for it if you are interested but you need not to for this current PR to be accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay sounds good. I think this PR can be merged then?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I let @ogrisel merge when this LGTH (I am not 100% sure that the current state of the test satisfies what he indicated in this thread first comment).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as it is. Let's do the common test in a follow-up PR.