ENH: Quantile Forest Support by reidjohnson · Pull Request #384 · skops-dev/skops

reidjohnson · 2023-07-22T11:17:18Z

Fixes #383.

The quantile-forest package extends sklearn random forest regressors to implement quantile regression forests. Without these changes, the Cython-based model object is unable to be loaded by skops due to its use of the __reduce__ method.

I understand adding support for another package may be outside the current scope of this project. As the primary maintainer of the quantile-forest package, I'd also be interested in better understanding if there's a recommended alternative to the use of __reduce__ that would enable the model to be compatible with this project's existing secure persistence feature.

BenjaminBossan

Thanks so much for providing this fix. I haven't done a proper review yet. Would you mind adding a test for quantile forest estimators? It could be similar to the tests here. If you need help with that, let us know.

-Adds base unit tests -Patches constructor visualization error

Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>

reidjohnson · 2023-07-23T09:32:10Z

            "here: https://github.com/skops-dev/skops/issues"
        )

+    if node_name == "constructor":


Suggested resolution for #385.

I would include that in a separate PR.

reidjohnson · 2023-07-23T09:33:00Z

@BenjaminBossan: Sure, I took a pass at adding a test for the quantile forest estimators. This also uncovered a bug in the visualization of sklearn DecisionTreeRegressors reported as #385, with a suggested resolution included in these changes.

adrinjalali

Thanks for the nice PR. Other than the small nits, I think this looks good.

Might be time for us to start working on a better way to handle C extension types.

adrinjalali · 2023-07-24T11:28:58Z

+        self.trusted = self._get_trusted(
+            trusted,
+            [get_module(QuantileForest) + ".QuantileForest"],
+        )


I'd be happier if we don't trust them by default.

adrinjalali · 2023-07-24T11:30:20Z

            "here: https://github.com/skops-dev/skops/issues"
        )

+    if node_name == "constructor":


I would include that in a separate PR.

adrinjalali · 2023-07-24T11:32:59Z

+        tree_methods = [
+            quantile_forest.RandomForestQuantileRegressor,
+            quantile_forest.ExtraTreesQuantileRegressor,
+        ]


move to @pytest.mark.parametrize?

In favor of separate PR.

Uses pytest.mark.parametrize

reidjohnson · 2023-07-24T17:59:48Z

Thanks for the review. Updated based on suggestions. Removed fix for #385 in favor of PR #386.

adrinjalali · 2023-07-25T19:22:41Z

@reidjohnson tests failing

reidjohnson · 2023-07-25T19:26:51Z

Failing due to #385.

adrinjalali · 2023-07-28T12:41:01Z

+from __future__ import annotations
+
+from typing import Any, Sequence
+
+from ._protocol import PROTOCOL
+from ._sklearn import ReduceNode, reduce_get_state
+from ._utils import LoadContext, SaveContext
+
+
+def _lazy_import():
+    from quantile_forest._quantile_forest_fast import QuantileForest
+
+    def quantile_forest_get_state(
+        obj: Any,
+        save_context: SaveContext,
+    ) -> dict[str, Any]:
+        state = reduce_get_state(obj, save_context)
+        state["__loader__"] = "QuantileForestNode"
+        return state
+
+    class QuantileForestNode(ReduceNode):
+        def __init__(
+            self,
+            state: dict[str, Any],
+            load_context: LoadContext,
+            trusted: bool | Sequence[str] = False,
+        ) -> None:
+            super().__init__(
+                state,
+                load_context,
+                constructor=QuantileForest,
+                trusted=trusted,
+            )
+            self.trusted = self._get_trusted(trusted, [])
+
+    return QuantileForest, QuantileForestNode, quantile_forest_get_state
+
+
+try:
+    QuantileForest, QuantileForestNode, quantile_forest_get_state = _lazy_import()
+
+    # tuples of type and function that gets the state of that type
+    GET_STATE_DISPATCH_FUNCTIONS = [(QuantileForest, quantile_forest_get_state)]
+
+    # tuples of type and function that creates the instance of that type
+    NODE_TYPE_MAPPING = {("QuantileForestNode", PROTOCOL): QuantileForestNode}
+except ModuleNotFoundError:
+    GET_STATE_DISPATCH_FUNCTIONS = []
+    NODE_TYPE_MAPPING = {}


What do you think of this instead? (slightly different)

from __future__ import annotations from typing import Any, Sequence from ._protocol import PROTOCOL from ._sklearn import ReduceNode, reduce_get_state from ._utils import LoadContext, SaveContext try: from quantile_forest._quantile_forest_fast import QuantileForest except ImportError: QuantileForest = None def quantile_forest_get_state( obj: Any, save_context: SaveContext, ) -> dict[str, Any]: state = reduce_get_state(obj, save_context) state["__loader__"] = "QuantileForestNode" return state class QuantileForestNode(ReduceNode): def __init__( self, state: dict[str, Any], load_context: LoadContext, trusted: bool | Sequence[str] = False, ) -> None: if QuantileForest is None: raise ImportError( "`quantile_forest` is missing and needs to be installed in order to" " load this model." ) super().__init__( state, load_context, constructor=QuantileForest, trusted=trusted, ) self.trusted = self._get_trusted(trusted, []) # tuples of type and function that gets the state of that type if QuantileForest is not None: GET_STATE_DISPATCH_FUNCTIONS = [(QuantileForest, quantile_forest_get_state)] # tuples of type and function that creates the instance of that type NODE_TYPE_MAPPING = {("QuantileForestNode", PROTOCOL): QuantileForestNode}

BenjaminBossan · 2023-08-02T10:00:53Z

+
+
+class TestQuantileForest:
+    """Tests for RandomForestQuantileRegressor and ExtraTreesQuantileRegressor"""


Is this not missing QuantileForest?

These are the user-facing classes which are explicitly tested, QuantileForest is the underlying Cython object shared by the user-facing classes and isn't explicitly tested here (but is serialized by the loading/dumping).

BenjaminBossan

LGTM, thanks a lot for adding the support. I have an extremely minor comment, but this is good to be merged from my point of view.

adrinjalali · 2023-08-03T09:16:22Z

I just noticed this is missing a changelog entry.

reidjohnson · 2023-08-03T16:37:12Z

Updated, thanks for flagging.

brentonmallen1 · 2023-08-03T18:02:52Z

awesome work everyone, y'all are amazing! I really appreciate it!

brentonmallen1 · 2023-09-14T19:35:39Z

I finally got back to this effort and it seems like, even with the changes, I'm still seeing the following error when attempting to load a quantile forest model:

TypeError: __cinit__() takes at least 2 positional arguments (0 given)```

Did this come up during testing? Am I missing something?

brentonmallen1 · 2023-09-14T19:40:16Z

more specifically: File "quantile_forest/_quantile_forest_fast.pyx", line 568, in quantile_forest._quantile_forest_fast.QuantileForest.__cinit__

reidjohnson · 2023-09-14T19:58:57Z

@brentonmallen1 Just checking, but are you using the development build? I don't believe there's yet been a public release since these changes were merged.

brentonmallen1 · 2023-09-14T20:08:29Z

I was using the main release for 0.8.0 because the changelog had mentioned QuantileForest support, but I could have misread. I can try using the dev release and give it another go and report back. thank you

brentonmallen1 · 2023-09-14T20:20:12Z

well, I don't see the quantile changes in my site-packages so it does seem like I haven't installed what I was expecting. I also don't see a dev branch/build, how would I go about installing that directly?

reidjohnson · 2023-09-14T20:39:43Z

@brentonmallen1 I'll defer to the maintainers, but a viable route is to follow the dev setup described in the contributing doc.

adrinjalali · 2023-10-23T12:21:47Z

We just released 0.9.0, which includes this change.

brentonmallen1 · 2023-10-23T14:32:06Z

y'all are wonderful! thank you!

reidjohnson added 2 commits July 22, 2023 02:55

Initial commit

ee7bde5

Update dependencies

522fc4b

BenjaminBossan reviewed Jul 22, 2023

View reviewed changes

Comment thread skops/_min_dependencies.py Outdated

reidjohnson and others added 2 commits July 23, 2023 02:25

Add unit tests for quantile-forest

425f8b9

-Adds base unit tests -Patches constructor visualization error

Update skops/_min_dependencies.py

d9d022b

Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com>

reidjohnson commented Jul 23, 2023

View reviewed changes

adrinjalali reviewed Jul 24, 2023

View reviewed changes

reidjohnson added 4 commits July 24, 2023 10:14

Linting

f39816b

Remove Visualization Fix

e0d59a8

In favor of separate PR.

Remove Default Trusted

27fa532

Update test

fd4dfff

Uses pytest.mark.parametrize

BenjaminBossan reviewed Jul 27, 2023

View reviewed changes

Comment thread skops/io/_persist.py

Lazy import quantile-forest

e9b0afb

adrinjalali reviewed Jul 28, 2023

View reviewed changes

reidjohnson added 2 commits July 28, 2023 08:18

Update lazy loading

c19c7ff

Merge branch 'main' into quantile-forest-support

6482190

reidjohnson marked this pull request as ready for review August 1, 2023 16:36

adrinjalali approved these changes Aug 2, 2023

View reviewed changes

adrinjalali requested a review from BenjaminBossan August 2, 2023 09:45

BenjaminBossan reviewed Aug 2, 2023

View reviewed changes

BenjaminBossan approved these changes Aug 2, 2023

View reviewed changes

Add changelog

0512fab

adrinjalali merged commit 61aa534 into skops-dev:main Aug 3, 2023

reidjohnson deleted the quantile-forest-support branch August 4, 2023 01:13



		class TestQuantileForest:
		"""Tests for RandomForestQuantileRegressor and ExtraTreesQuantileRegressor"""

Conversation

reidjohnson commented Jul 22, 2023

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reidjohnson commented Jul 23, 2023

Uh oh!

adrinjalali left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

reidjohnson commented Jul 24, 2023

Uh oh!

adrinjalali commented Jul 25, 2023

Uh oh!

reidjohnson commented Jul 25, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BenjaminBossan left a comment

Choose a reason for hiding this comment

Uh oh!

adrinjalali commented Aug 3, 2023

Uh oh!

reidjohnson commented Aug 3, 2023

Uh oh!

brentonmallen1 commented Aug 3, 2023

Uh oh!

brentonmallen1 commented Sep 14, 2023

Uh oh!

brentonmallen1 commented Sep 14, 2023

Uh oh!

reidjohnson commented Sep 14, 2023

Uh oh!

brentonmallen1 commented Sep 14, 2023

Uh oh!

brentonmallen1 commented Sep 14, 2023

Uh oh!

reidjohnson commented Sep 14, 2023

Uh oh!

adrinjalali commented Oct 23, 2023

Uh oh!

brentonmallen1 commented Oct 23, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants