On the top of the head, I would expect callee to be fit because this is the step that requires metadata for this specific selector.

However, I'm always confused with the routing/caller/callee :).

@StefanieSenger @adrinjalali would you mind to have a look.

we shouldn't have to add these composite methods here, they should be auto generated from the simple methods. This might be an issue in the _metadata_requests.py that I need to debug to see where the issue is.

I was also curious about @glemaitre's question. It actually doesn't matter whether you use callee="fit_transform" or callee="fit" for the fix. Both values will result in successful metadata routing.

I think this is because the Feature Selector's fit_transform function passes the **fit_params to self.fit() without explicitly using the metadata routing functionality (because technically it doesn't have to). Code snippet below (source) shows that its only using the metadata routing for validation and has no impact on the **fit_params.

    def fit_transform(self, X, y=None, **fit_params):
        if _routing_enabled():
            transform_params = self.get_metadata_routing().consumes(
                method="transform", params=fit_params.keys()
            )
            if transform_params:
                warnings.warn(
                    (
                        f"This object ({self.__class__.__name__}) has a `transform`"
                        " method which consumes metadata, but `fit_transform` does not"
                        " forward metadata to `transform`. Please implement a custom"
                        " `fit_transform` method to forward metadata to `transform` as"
                        " well. Alternatively, you can explicitly do"
                        " `set_transform_request`and set all values to `False` to"
                        " disable metadata routed to `transform`, if that's an option."
                    ),
                    UserWarning,
                )
        if y is None:
            # fit method of arity 1 (unsupervised transformation)
            return self.fit(X, **fit_params).transform(X)
        else:
            # fit method of arity 2 (supervised transformation)
            return self.fit(X, y, **fit_params).transform(X)

To illustrate the function calls, in order:

1. pipeline.fit():
   • First calls process_routing(_method='fit') to request the metadata to pass forward
   • Then calls feature_selector.fit_transform() and includes the routed metadata
2. feature_selector.fit_transform():
   • does NOT call process_routing()
   • calls self.fit(X, y, **fit_params)
3. feature_selector.fit():
   • First calls process_routing(_method='fit') to request the metadata to pass forward
   • Then calls estimator.fit() and includes the routed metadata

Because step 2 doesn't participate in the metadata routing, it doesn't matter what the callee value is for the feature_selector. It just has to be one of the generically valid values defined by the class (e.g., fit, transform, fit_transform, etc).

I'm sure you've done a great job going through this bit of code figuring it out, but from the change in the docstring, it's not clear to me exactly what the issue is. Could you please add a test specifically for the change here, to make it clear for future developers?

Should I actually add a test in the sklearn/tests/test_metaestimators_metadata_routing.py, mark it with @pytest.mark.skip since it is expected to fail, then just add a note in the docstring referencing the test?

Since I don't really understand this comment, seeing that test helps.

My question was more around the logistics of adding a test that I know is going to fail, without it messing up any CI pipelines. For example, at work we enforce 100% of our tests to pass in order for PRs to get merged, so I just haven't encountered this situation before. Wasn't sure if that would be a problem here and if it is, is @pytest.mark.skip the right way to handle it?

We won't be merging w/o CI being green. Once you write a test, I'd know what needs to be done to fix it. Here I just don't understand what the comment is saying.

got it, will write up a test shortly