FEAT SLEP006: metadata routing infrastructure #24027

adrinjalali · 2022-07-27T13:57:38Z

This PR is to merge the sample-props branch into main.

All contributions here are already reviewed in their corresponding PRs, but to make it easier, we can review this now directly here and do modifications directly on this branch rather than through PRs to this branch.

This feature is not enabled by default, and the userguide says that it's not ready, and the link to the user guide is also in an "under development" section.

EDIT (26.05.2023): calling set_{method}_request now raises if feature flag is not enabled, to prevent accidental behaviors for users thinking they're setting requests but those requests not having an effect.

* initial base implementation commit * fix test_props and the issue with attribute starting with __ * skip doctest in metadata_routing.rst for now * DOC explain why aliasing on sub-estimator of a consumer/router is useful * reduce diff * DOC add user guide link to method docstrings * DOC apply Thomas's suggestions to the rst file * CLN address a few comments in docs * ignore sentinel docstring check * handling backward compatibility and deprecation prototype * Update examples/plot_metadata_routing.py Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> * make __metadata_request__* format more intuitive and less redundant * metadata_request_factory always returns a copy * fix tests for the changed __metadata_request__* format * in example: foo->sample_weight, bar->groups * get_method_input->get_input * minor comments from Guillaume * fix estimator checks tests * Improved sample props developer API * fixes, updated doc, decorator * Add docstrings and some API cleanup * unify serialize/deserialize methods * Add more docstring to process_routing * fix MetadataRouter.get_params parameter mismatch * DOC add missing name to MethodMetadataRequest.deserialize docstring * DOC add MethodMapping.add docstring * DOC fix colons after versionadded * fix {method}_requests return type annotation * metadata_request_factory -> metadata_router_factory and docstring fixes * move 'me' out of the map in MetadataRouter * more docstring refinements * cleanup API addresses and create a utils.metadata_routing sub-folder * fix module import issue * more tests and a few bug fixes * Joel's comments * make process_routing a function * docstring fix * ^type -> $type * remove deserialize, return instance, and add type as an attribute * remove sentinels and use strings instead * make RequestType searchable and check for valid identifier * Route -> MethodPair * remove unnecessary sorted * clarification on usage of the process_routing func in the example * only print methods with non-empty requests * fix test_string_representations * remove source build cache from CircleCI (temporarily) * Trigger CI * Invalidate linux-arm64 ccache my changing the key * Trigger CI * method, used_in -> callee, caller * show RequestType instead of RequestType.value in _serialize() * more informative error messages * fix checking for conflicting keys * get_router_for_object -> get_routing_for_object * \{method\}_requests -> set_\{method\}_request * address metadata_routing.rst comments * some test enhancements * TypeError for extra arguments * add_request: prop -> param * original_names -> return_alias * add more tests for MetadataRouter and MethodMapping * more suggestions from Joel's review * fix return type * apply more suggestions from Joel's review * Christian\'s suggestions * more notes from Christian * test_get_routing_for_object returns empty requests on unknown objects * more notes from Christian * remove double line break * more notes from Christian * more notes from Christian * make type private * add more comments/docs * fix test * fix nits * add forgotten nit Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

* FEAT add metadata routing to splitters * forgot black * add end of file line * ignore mypy error * SIMPLE_SPLITTERS -> NO_GROUP_SPLITTERS * simplify comment * add notes on what the tests do * improve test and avoid using repr

* initial base implementation commit * fix test_props and the issue with attribute starting with __ * skip doctest in metadata_routing.rst for now * DOC explain why aliasing on sub-estimator of a consumer/router is useful * reduce diff * DOC add user guide link to method docstrings * DOC apply Thomas's suggestions to the rst file * CLN address a few comments in docs * ignore sentinel docstring check * handling backward compatibility and deprecation prototype * Update examples/plot_metadata_routing.py Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> * make __metadata_request__* format more intuitive and less redundant * metadata_request_factory always returns a copy * fix tests for the changed __metadata_request__* format * in example: foo->sample_weight, bar->groups * get_method_input->get_input * minor comments from Guillaume * fix estimator checks tests * Improved sample props developer API * fixes, updated doc, decorator * Add docstrings and some API cleanup * unify serialize/deserialize methods * Add more docstring to process_routing * fix MetadataRouter.get_params parameter mismatch * DOC add missing name to MethodMetadataRequest.deserialize docstring * DOC add MethodMapping.add docstring * DOC fix colons after versionadded * fix {method}_requests return type annotation * metadata_request_factory -> metadata_router_factory and docstring fixes * move 'me' out of the map in MetadataRouter * more docstring refinements * cleanup API addresses and create a utils.metadata_routing sub-folder * fix module import issue * more tests and a few bug fixes * Joel's comments * make process_routing a function * docstring fix * ^type -> $type * remove deserialize, return instance, and add type as an attribute * remove sentinels and use strings instead * make RequestType searchable and check for valid identifier * Route -> MethodPair * remove unnecessary sorted * clarification on usage of the process_routing func in the example * only print methods with non-empty requests * fix test_string_representations * remove source build cache from CircleCI (temporarily) * Trigger CI * Invalidate linux-arm64 ccache my changing the key * Trigger CI * method, used_in -> callee, caller * show RequestType instead of RequestType.value in _serialize() * more informative error messages * fix checking for conflicting keys * get_router_for_object -> get_routing_for_object * \{method\}_requests -> set_\{method\}_request * address metadata_routing.rst comments * some test enhancements * TypeError for extra arguments * add_request: prop -> param * original_names -> return_alias * add more tests for MetadataRouter and MethodMapping * more suggestions from Joel's review * fix return type * apply more suggestions from Joel's review * Christian\'s suggestions * more notes from Christian * test_get_routing_for_object returns empty requests on unknown objects * more notes from Christian * remove double line break * more notes from Christian * more notes from Christian * make type private * add more comments/docs * fix test * fix nits * add forgotten nit Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org>

* FEAT add metadata routing to splitters * forgot black * add end of file line * ignore mypy error * SIMPLE_SPLITTERS -> NO_GROUP_SPLITTERS * simplify comment * add notes on what the tests do * improve test and avoid using repr

…nto sample-props

…data request (#23342)

…nto sample-props

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: adrinjalali <adrin.jalali@gmail.com>

…nto sample-props

adrinjalali

@thomasjpfan I added a documentation section to _metadata_requests.py, and reordered the file into sections. It should make it much easier to read / maintain the file.

I wonder if we should move sklearn.utils.metadata_routing to sklearn.metadata_routing. I don't have a strong preference.

sklearn/utils/_metadata_requests.py

adrinjalali · 2023-05-31T09:46:58Z

sklearn/utils/_metadata_requests.py

+        - ``str``: metadata should be passed to the meta-estimator with \
+this given alias instead of the original name.
+
+        The default (``UNCHANGED``) retains the existing request. This allows


changed it to sklearn.utils.metadata_routing.UNCHANGED.

…imator

thomasjpfan · 2023-06-01T15:51:40Z

sklearn/utils/_metadata_requests.py

+from .. import get_config
+from ..exceptions import UnsetMetadataPassedError
+from ._bunch import Bunch


These relative imports will make it harder to vendor. I'm okay with leaving this as is and fixing it as a follow up.

sklearn/utils/_metadata_requests.py

thomasjpfan · 2023-06-02T02:28:12Z

sklearn/utils/_metadata_requests.py

+        self._check_warnings(params=params)
+        unrequested = dict()
+        params = {} if params is None else params
+        args = {arg: value for arg, value in params.items() if value is not None}


The tests passed without needing to filter out if value is not None. Is it required to filter them out?

the tests pass because we use process_routing which applies the same filter. I removed the filter in process_routing and kept the one here now, and added a test to explicitly test passing None has no effect.

sklearn/utils/_metadata_requests.py

thomasjpfan · 2023-06-02T02:33:19Z

sklearn/utils/_metadata_requests.py

+            A string representing the mapping, it can be:
+
+              - `"one-to-one"`: a one to one mapping for all methods.
+              - `"method"`: the name of a single method.


A developer can be confused and think the string "method" is a valid route. We can dynamically list out the METHODS to make it clear.

Expanded the description to make it more clear.

sklearn/utils/_metadata_requests.py

thomasjpfan · 2023-06-02T02:56:02Z

sklearn/utils/_metadata_requests.py

+
+    validate_keys : bool, default=True
+        Whether to check if the requested parameters fit the actual parameters
+        of the method.


I think this kwarg is only used for testing:

Suggested change

of the method.

of the method. Only used for testing.

Not really, in the mocking there's the only place we have validate_keys=False but that kind of usecase can happen in the wild too, where you want to accept any kind of request w/o having them pre-defined.

thomasjpfan · 2023-06-02T02:58:28Z

sklearn/utils/_metadata_requests.py

+                if "__metadata_request__" in attr
+            }
+            defaults.update(base_defaults)
+        defaults = dict(sorted(defaults.items()))


Is this line required?

Suggested change

defaults = dict(sorted(defaults.items()))

This is to make the output deterministic, otherwise I saw cases where the output would change and the repr would not give a consistent result.

adrinjalali

@thomasjpfan should be good now.

adrinjalali · 2023-06-02T10:05:56Z

sklearn/utils/_metadata_requests.py

+        self._check_warnings(params=params)
+        unrequested = dict()
+        params = {} if params is None else params
+        args = {arg: value for arg, value in params.items() if value is not None}


the tests pass because we use process_routing which applies the same filter. I removed the filter in process_routing and kept the one here now, and added a test to explicitly test passing None has no effect.

sklearn/utils/_metadata_requests.py

adrinjalali · 2023-06-02T10:11:44Z

sklearn/utils/_metadata_requests.py

+        route : str
+            A string representing the mapping, it can be:
+
+              - `"one-to-one"`: a one to one mapping for all methods.


To me a one to one mapping is a bijection, doesn't really need all in the name.

adrinjalali · 2023-06-02T10:12:33Z

sklearn/utils/_metadata_requests.py

+            A string representing the mapping, it can be:
+
+              - `"one-to-one"`: a one to one mapping for all methods.
+              - `"method"`: the name of a single method.


Expanded the description to make it more clear.

adrinjalali · 2023-06-02T11:00:51Z

sklearn/utils/_metadata_requests.py

+
+    validate_keys : bool, default=True
+        Whether to check if the requested parameters fit the actual parameters
+        of the method.


Not really, in the mocking there's the only place we have validate_keys=False but that kind of usecase can happen in the wild too, where you want to accept any kind of request w/o having them pre-defined.

adrinjalali · 2023-06-02T11:01:55Z

sklearn/utils/_metadata_requests.py

+                if "__metadata_request__" in attr
+            }
+            defaults.update(base_defaults)
+        defaults = dict(sorted(defaults.items()))


This is to make the output deterministic, otherwise I saw cases where the output would change and the repr would not give a consistent result.

sklearn/utils/_metadata_requests.py

adrinjalali · 2023-06-02T11:24:24Z

sklearn/utils/_metadata_requests.py

+In routers, e.g. meta-estimators and a multi metric scorer,
+``get_metadata_routing`` returns a ``MetadataRouter`` object.


lorentzenchr · 2023-06-02T13:06:48Z

Changelog reflects changes that users should care about, but we kinda don't want end users to start using this immediately, while we want third party developers to care about it.

I could think to advertise it inside an "Experimental" section.

What about a dedicated new section: "For 3rd party maintainers" in the usual changelog.

thomasjpfan

One last concern, otherwise this looks ready to go.

sklearn/utils/_metadata_requests.py

thomasjpfan

I think further improvements to the docs and code can be done in follow up PRs.

thomasjpfan · 2023-06-02T14:06:35Z

sklearn/multioutput.py

+                routed_params = Bunch(
+                    estimator=Bunch(partial_fit=Bunch(sample_weight=sample_weight))
+                )


Not a blocker: Nesting Bunches like this is a little ugly. I'm considering not using Bunch at all and going with dicts. It would make the _metadata_request.py slightly easier to vendor. Then developers would need to do:

routed_params["estimator"]["partial_fit"]

(I'm +0.25 on using regular dicts)

adrinjalali · 2023-06-02T15:15:31Z

One last concern, otherwise this looks ready to go.

@thomasjpfan done.

thomasjpfan

LGTM

thomasjpfan · 2023-06-02T20:44:06Z

I added a comment to the meta-issue on SLEP006: #22893 (comment)

agramfort · 2023-06-03T09:54:40Z

❤️ @adrinjalali ❤️

jjerphan · 2023-06-04T15:51:19Z

Thanks, @adrinjalali and everyone involved for this massive contribution. 🥳 👏 🤝

lorentzenchr · 2023-06-08T15:18:08Z

How about a blog post like like the one for pandas in-out?

Co-authored-by: Christian Lorentzen <lorentzen.ch@gmail.com> Co-authored-by: Olivier Grisel <olivier.grisel@ensta.org> Co-authored-by: Benjamin Bossan <BenjaminBossan@users.noreply.github.com> Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: Omar Salman <omar.salman@arbisoft.com>

adrinjalali and others added 16 commits March 10, 2022 15:39

FEAT add metadata routing to splitters (#22765)

1dc6f5f

* FEAT add metadata routing to splitters * forgot black * add end of file line * ignore mypy error * SIMPLE_SPLITTERS -> NO_GROUP_SPLITTERS * simplify comment * add notes on what the tests do * improve test and avoid using repr

FEAT add metadata routing to splitters (#22765)

6a0fe13

* FEAT add metadata routing to splitters * forgot black * add end of file line * ignore mypy error * SIMPLE_SPLITTERS -> NO_GROUP_SPLITTERS * simplify comment * add notes on what the tests do * improve test and avoid using repr

Merge branch 'sample-props' of github.com:scikit-learn/scikit-learn i…

9183f5f

…nto sample-props

FEAT SLEP6: scorers (#22757)

c963068

Merge remote-tracking branch 'origin/main' into sample-props

29dc4ab

Merge branch 'sample-props' of github.com:scikit-learn/scikit-learn i…

d9105f6

…nto sample-props

Merge remote-tracking branch 'origin/main' into sample-props

4225a75

Merge remote-tracking branch 'origin/main' into sample-props

bbd96ba

MNT change versions to 1.2 (#23341)

6ae2052

Merge remote-tracking branch 'origin/main' into sample-props

e80be0b

ENH allow all main top level methods to have a corresponding set meta…

0511d26

…data request (#23342)

Merge remote-tracking branch 'origin/main' into sample-props

29f354e

Merge branch 'sample-props' of github.com:scikit-learn/scikit-learn i…

e9f5676

…nto sample-props

FEAT multioutput routes metadata (#22986)

b93a9e6

adrinjalali mentioned this pull request Jul 27, 2022

SLEP006 - Metadata Routing task list #22893

Open

28 tasks

ENH make sure warn_on errors on invalid child (#24023)

9bd9c12

lorentzenchr mentioned this pull request Aug 6, 2022

Add sample_weight fit param for Pipeline #18159

Closed

lorentzenchr linked an issue Aug 6, 2022 that may be closed by this pull request

Add sample_weight fit param for Pipeline #18159

Closed

adrinjalali and others added 4 commits August 8, 2022 11:24

Merge remote-tracking branch 'origin/main' into sample-props

50f9b13

Merge branch 'sample-props' of github.com:scikit-learn/scikit-learn i…

4d062fb

…nto sample-props

MNT fix linter issues related to multioutput (#24143)

6b678c0

SLEP006: CalibratedClassifierCV (#24126)

0afaa63

Co-authored-by: Thomas J. Fan <thomasjpfan@gmail.com> Co-authored-by: adrinjalali <adrin.jalali@gmail.com>

adrinjalali mentioned this pull request Aug 18, 2022

fit_params for Adaboost estimator #21706

Open

adrinjalali added 3 commits September 12, 2022 10:17

Merge remote-tracking branch 'origin/main' into sample-props

b064a11

Merge branch 'sample-props' of github.com:scikit-learn/scikit-learn i…

b541b4f

…nto sample-props

Merge remote-tracking branch 'origin/main' into sample-props

2e62f3b

This was referenced Sep 26, 2022

Support usage of predict_params and predict_proba_params in cross validation #24507

Closed

add **fit_params to sklearn.compose.ColumnTransformer().fit() #24490

Closed

adrinjalali commented May 31, 2023

View reviewed changes

adrinjalali added 3 commits May 31, 2023 16:27

DOC fix links

764ae8b

DOC fix another wrong link due to moved example

39d0f3b

Merge remote-tracking branch 'origin/main' into sample-props

8857a5c

adrinjalali mentioned this pull request Jun 1, 2023

Cross validation error of a Gaussian process with noisy target #26328

Closed

DOC add a note on methods not being relevant if not inside a meta-est…

5fc6a70

…imator

thomasjpfan reviewed Jun 1, 2023

View reviewed changes

thomasjpfan reviewed Jun 2, 2023

View reviewed changes

adrinjalali added 2 commits June 2, 2023 13:32

MNT add_self -> add_self_request and more nits

3199db0

Merge remote-tracking branch 'origin/main' into sample-props

1359e3c

adrinjalali commented Jun 2, 2023

View reviewed changes

thomasjpfan reviewed Jun 2, 2023

View reviewed changes

sklearn/utils/_metadata_requests.py Outdated Show resolved Hide resolved

sklearn/utils/_metadata_requests.py Outdated Show resolved Hide resolved

thomasjpfan reviewed Jun 2, 2023

View reviewed changes

_self -> _self_request

1348869

thomasjpfan approved these changes Jun 2, 2023

View reviewed changes

thomasjpfan merged commit 62671a7 into main Jun 2, 2023

thomasjpfan deleted the sample-props branch June 2, 2023 20:43

This was referenced Jun 3, 2023

SLEP006 - Add Metadata Routing to LogisticRegressionCV #24498

Closed

SLEP006: Metadata routing for bagging #24250

Closed

plcrodrigues mentioned this pull request Jun 28, 2023

Update to Python-3.8 pyRiemann/pyRiemann#254

Merged

jameslamb mentioned this pull request Jul 2, 2023

DOC Remove scikit-learn-only docs references related to metadata routing #26747

Closed

peytondmurray mentioned this pull request Jul 27, 2023

[Core] Pass logs through if sphinx-doctest is running ray-project/ray#36306

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

FEAT SLEP006: metadata routing infrastructure #24027

FEAT SLEP006: metadata routing infrastructure #24027

adrinjalali commented Jul 27, 2022 •

edited

Loading

adrinjalali left a comment

adrinjalali May 31, 2023

thomasjpfan Jun 1, 2023

thomasjpfan Jun 2, 2023

adrinjalali Jun 2, 2023

thomasjpfan Jun 2, 2023

adrinjalali Jun 2, 2023

thomasjpfan Jun 2, 2023

adrinjalali Jun 2, 2023

thomasjpfan Jun 2, 2023 •

edited

Loading

adrinjalali Jun 2, 2023

adrinjalali left a comment

adrinjalali Jun 2, 2023

adrinjalali Jun 2, 2023

adrinjalali Jun 2, 2023

adrinjalali Jun 2, 2023

adrinjalali Jun 2, 2023

adrinjalali Jun 2, 2023

lorentzenchr commented Jun 2, 2023 •

edited

Loading

thomasjpfan left a comment

thomasjpfan left a comment

thomasjpfan Jun 2, 2023

adrinjalali commented Jun 2, 2023

thomasjpfan left a comment

thomasjpfan commented Jun 2, 2023

agramfort commented Jun 3, 2023

jjerphan commented Jun 4, 2023 •

edited by adrinjalali

Loading

lorentzenchr commented Jun 8, 2023

		In routers, e.g. meta-estimators and a multi metric scorer,
		``get_metadata_routing`` returns a ``MetadataRouter`` object.

FEAT SLEP006: metadata routing infrastructure #24027

FEAT SLEP006: metadata routing infrastructure #24027

Conversation

adrinjalali commented Jul 27, 2022 • edited Loading

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasjpfan Jun 2, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lorentzenchr commented Jun 2, 2023 • edited Loading

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

adrinjalali commented Jun 2, 2023

thomasjpfan left a comment

Choose a reason for hiding this comment

thomasjpfan commented Jun 2, 2023

agramfort commented Jun 3, 2023

jjerphan commented Jun 4, 2023 • edited by adrinjalali Loading

lorentzenchr commented Jun 8, 2023

adrinjalali commented Jul 27, 2022 •

edited

Loading

thomasjpfan Jun 2, 2023 •

edited

Loading

lorentzenchr commented Jun 2, 2023 •

edited

Loading

jjerphan commented Jun 4, 2023 •

edited by adrinjalali

Loading