MNT little refactor and doc improvement for metadata routing consumes() methods #31703

StefanieSenger · 2025-07-04T15:58:32Z

What does this implement/fix? Explain your changes.

This PR aims to make metadata routing more intuitive and maintainable by a little refactoring and doc improvements:

simplifies the code in MetadataRequest.consumes() to use set notation (which I think is easier to read)
improves the docstrings of all three consumes methods
renames variables names to make them more specific
corrects a mistake in test_metadata_request_consumes_method

…() methods

github-actions · 2025-07-04T15:59:24Z

✔️ Linting Passed

All linting checks passed. Your pull request is in excellent shape! ☀️

_{Generated for commit: 7c849ce. Link to the linter CI: here}

StefanieSenger · 2025-07-04T16:05:17Z

sklearn/utils/_metadata_requests.py


        Returns
        -------
-        consumed : set of str
-            A set of parameters which are consumed by the given method.
+        method_consumes : set of str


Also renaming consumed into method_consumes to prevent the expectation of a boolean.

It could also be consumes?

consumed_params ?

Yes, I think that reads well. :)

StefanieSenger · 2025-07-04T16:16:21Z

sklearn/utils/_metadata_requests.py

+                    method_consumes.update(
+                        route_mapping.router.consumes(method=callee, params=params)
                    )


I've replaced res = res | ... with res.update(...) for readability.

The bitwise OR feels like a lower-level tool and using the set notation instead makes it easier for anyone reading this and they are not required to interpret a bitwise operator.

Do you agree with that, @adrinjalali?

I 💯 disagree and I cringe when I see the nice one-liner being made a long method call and spanning 3 lines. But I know @glemaitre also prefers your way. I think I've lost that battle 😅 😆 . If y'all think this version reads better, then that's what's important. I'll keep my operators to my own code.

To me personally, with this longer variable names and removing operators, the code reads less easy than before, but I can still read it and live with it.

It is just a proposal and I'm fine with leaving it as is, too.

reasoning:
I have now learned that we can operate on a bit-wise level on immutable objects. I was thinking that might be an additional stretch we could spare others to do. I would prefer short variable names too if they are expressive (but res?) though I find this is hard to find often.
Also, it was already a three-liner before, so in that sense, the change maybe isn't too bad?

meta talk:
I really value you recognising my need for explicitness. ❤️ I don't want this to feel like a battle at all.

Edit: Ah true, above this change turns a one-liner into a three-liner, unfortunately.

I'm okay to keep the code as you have it here. But res is quite a common and (to me at least) a very self explanatory name. You can grep for res = in the codebase and see many other places with the same pattern. It saves space and it's clear what it is.

StefanieSenger · 2025-07-04T18:42:10Z

sklearn/utils/_metadata_requests.py

@@ -571,22 +572,27 @@ def __init__(self, owner):
            )

    def consumes(self, method, params):
-        """Check whether the given metadata are consumed by the given method.
+        """Return params consumed as metadata in a :term:`consumer`.


Several times it happened to me that at first sight I expected consumes() would return a boolean. Now this summary line hopefully prevents contributors and developers (and myself) to fall into this trap again.

StefanieSenger · 2025-07-04T19:11:00Z

sklearn/tests/test_metadata_routing.py

@@ -638,14 +638,14 @@ class Consumer(BaseEstimator):
 @config_context(enable_metadata_routing=True)
 def test_metadata_request_consumes_method():
    """Test that MetadataRequest().consumes() method works as expected."""
-    request = MetadataRouter(owner="test")
+    request = MetadataRequest(owner="test_method")


Corrected wrong class name.

StefanieSenger · 2025-07-05T19:02:21Z

That's ready for a review, @adrinjalali. Would you have a look?

lucyleeow

I like the changes, only some nitpicks on wording

sklearn/utils/_metadata_requests.py

lucyleeow · 2025-07-07T04:26:11Z

sklearn/utils/_metadata_requests.py


        Returns
        -------
-        consumed : set of str
-            A set of parameters which are consumed by the given method.
+        method_consumes : set of str


consumed_params ?

sklearn/utils/_metadata_requests.py

adrinjalali · 2025-07-07T08:23:00Z

sklearn/tests/test_metadata_routing.py

    assert request.consumes(method="fit", params={"foo"}) == set()

-    request = MetadataRequest(owner="test")
+    request = MetadataRequest(owner="test_method")


I don't understand why the diff here. The owner in the whole test file is mostly "test", and it doesn't really matter what it is. I don't mind it either way, but it's not something I'd go out of my way to change.

Right, I have reverted this change.

My intend had been to make this test more intuitive to read for future contributors or third party developers, but I see my change was not quite correct and and I should have done it in all the places at the same time anyways.

adrinjalali · 2025-07-07T08:28:36Z

sklearn/utils/_metadata_requests.py

+                    method_consumes.update(
+                        route_mapping.router.consumes(method=callee, params=params)
                    )


I 💯 disagree and I cringe when I see the nice one-liner being made a long method call and spanning 3 lines. But I know @glemaitre also prefers your way. I think I've lost that battle 😅 😆 . If y'all think this version reads better, then that's what's important. I'll keep my operators to my own code.

To me personally, with this longer variable names and removing operators, the code reads less easy than before, but I can still read it and live with it.

Co-authored-by: Lucy Liu <jliu176@gmail.com>

StefanieSenger

Thank you two for your fast reviews, @lucyleeow and @adrinjalali! I have made a few updates that you had suggested and on the question of bit-wise comparison or set notation, I am open to change back and will wait for where the discussion goes.

StefanieSenger · 2025-07-07T09:06:21Z

sklearn/tests/test_metadata_routing.py

    assert request.consumes(method="fit", params={"foo"}) == set()

-    request = MetadataRequest(owner="test")
+    request = MetadataRequest(owner="test_method")


Right, I have reverted this change.

My intend had been to make this test more intuitive to read for future contributors or third party developers, but I see my change was not quite correct and and I should have done it in all the places at the same time anyways.

StefanieSenger · 2025-07-07T09:16:23Z

sklearn/utils/_metadata_requests.py


        Returns
        -------
-        consumed : set of str
-            A set of parameters which are consumed by the given method.
+        method_consumes : set of str


Yes, I think that reads well. :)

StefanieSenger · 2025-07-07T09:43:36Z

sklearn/utils/_metadata_requests.py

+                    method_consumes.update(
+                        route_mapping.router.consumes(method=callee, params=params)
                    )


It is just a proposal and I'm fine with leaving it as is, too.

reasoning:
I have now learned that we can operate on a bit-wise level on immutable objects. I was thinking that might be an additional stretch we could spare others to do. I would prefer short variable names too if they are expressive (but res?) though I find this is hard to find often.
Also, it was already a three-liner before, so in that sense, the change maybe isn't too bad?

meta talk:
I really value you recognising my need for explicitness. ❤️ I don't want this to feel like a battle at all.

Edit: Ah true, above this change turns a one-liner into a three-liner, unfortunately.

lucyleeow

LGTM thanks!

MNT little refactor and doc improvement for metadata routing consumes…

0099dbf

…() methods

github-actions bot added the module:utils label Jul 4, 2025

StefanieSenger added No Changelog Needed Metadata Routing all issues related to metadata routing, slep006, sample props labels Jul 4, 2025

StefanieSenger commented Jul 4, 2025

View reviewed changes

StefanieSenger added the Documentation label Jul 4, 2025

improve docstrings

e761f6e

StefanieSenger commented Jul 4, 2025

View reviewed changes

wording

6d6c327

lucyleeow reviewed Jul 7, 2025

View reviewed changes

adrinjalali reviewed Jul 7, 2025

View reviewed changes

StefanieSenger and others added 2 commits July 7, 2025 11:08

Apply suggestions from code review

7d25fa9

Co-authored-by: Lucy Liu <jliu176@gmail.com>

updates after reviews

7c849ce

StefanieSenger commented Jul 7, 2025

View reviewed changes

adrinjalali approved these changes Jul 7, 2025

View reviewed changes

lucyleeow approved these changes Jul 11, 2025

View reviewed changes

lucyleeow merged commit 4206d14 into scikit-learn:main Jul 11, 2025
36 checks passed

jeremiedbb mentioned this pull request Jul 15, 2025

Release 1.7.1 #31762

Merged

13 tasks

StefanieSenger deleted the metadata_routing_consumes branch July 18, 2025 13:55

Uh oh!

MNT little refactor and doc improvement for metadata routing consumes() methods #31703

MNT little refactor and doc improvement for metadata routing consumes() methods #31703

Uh oh!

Conversation

StefanieSenger commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What does this implement/fix? Explain your changes.

Uh oh!

github-actions bot commented Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

✔️ Linting Passed

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!

StefanieSenger Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adrinjalali Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanieSenger Jul 4, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanieSenger commented Jul 5, 2025

Uh oh!

lucyleeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

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!

adrinjalali Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

StefanieSenger 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!

StefanieSenger Jul 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

lucyleeow left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

StefanieSenger commented Jul 4, 2025 •

edited

Loading

github-actions bot commented Jul 4, 2025 •

edited

Loading

StefanieSenger Jul 4, 2025 •

edited

Loading

adrinjalali Jul 7, 2025 •

edited

Loading

StefanieSenger Jul 7, 2025 •

edited

Loading

StefanieSenger Jul 4, 2025 •

edited

Loading

StefanieSenger Jul 4, 2025 •

edited

Loading

adrinjalali Jul 7, 2025 •

edited

Loading

StefanieSenger Jul 7, 2025 •

edited

Loading