Talip7/scikit-learn-issues-multilabel huggingface dataset:
RFC deprecation warnings only when user is affected

While reviewing https://github.com/scikit-learn/scikit-learn/pull/29288, I realised we're raising deprecation warnings to users, while most of them are not affected by the change, since the change only occurs when a division by zero is happenings.

So I was wondering about our deprecation warning policy.

In many cases, most users might not be affected at all, and we'd be asking them to set the value of a parameter explicitly while the parameter doesn't affect their code at all, and make their code more verbose unnecessarily. So not raising the warning for them, would be nice in this case.

The down side is that we might be changing some behavior, which although not affecting the user, the user might rely on it, or they might only be affected very close to the deprecation cycle ends, not giving them much time to react. But if this is the case, they still will have time to react since they get the warning.

WDYT?

cc @StefanieSenger @scikit-learn/core-devs

COMMENT:
For the division by zero issues it might be odd, if users see the warning only for certain input data `y_true`, `y_pred` (that would cause division by zero), not for other data, while probably 2 releases down the road they'll train on different data again that might or might not cause division by zero.

I see the `FutureWarning` as a medium to communicate possible future problems in general, not just on this specific data. So, I would rather warn in vain than not warn.

Labels: API, RFC
RFC deprecation warnings only when user is affected

While reviewing https://github.com/scikit-learn/scikit-learn/pull/29288, I realised we're raising deprecation warnings to users, while most of them are not affected by the change, since the change only occurs when a division by zero is happenings.

So I was wondering about our deprecation warning policy.

In many cases, most users might not be affected at all, and we'd be asking them to set the value of a parameter explicitly while the parameter doesn't affect their code at all, and make their code more verbose unnecessarily. So not raising the warning for them, would be nice in this case.

The down side is that we might be changing some behavior, which although not affecting the user, the user might rely on it, or they might only be affected very close to the deprecation cycle ends, not giving them much time to react. But if this is the case, they still will have time to react since they get the warning.

WDYT?

cc @StefanieSenger @scikit-learn/core-devs

COMMENT:
In the past, we already implemented the policy where the problematic behaviour is triggered and only trigger the `FutureWarning` at this time. For instance, we recently introduce a deprecation of the behaviour of `keep_empty_feature` with constant strategy (https://github.com/scikit-learn/scikit-learn/blob/f54c98ea3/sklearn/impute/_base.py#L465-L477).

Thinking about a wider policy:

- I would prefer to warn as little as possible. People potentially ignore warnings when exposing them. If too many warnings are shown, this is more likely that they will not trigger attention.
- Then, when it comes to always warn of just when the behaviour is triggered, I would argue that we should assess whether we are on a case where only a few people will be impacted by the change.

So specifically for the zero-division case, I would only raise when the zero-division happen. My reasoning is that the use case at hand is specific enough that the warning would be raised on the different dataset on which the user trained the models (e.g. object detection for large number of classes). Another possibility would be that this is an uncommon corner case and I would also be fine to be less noisy because it should affect only a limited number of users.

Labels: API, RFC
RFC deprecation warnings only when user is affected

While reviewing https://github.com/scikit-learn/scikit-learn/pull/29288, I realised we're raising deprecation warnings to users, while most of them are not affected by the change, since the change only occurs when a division by zero is happenings.

So I was wondering about our deprecation warning policy.

In many cases, most users might not be affected at all, and we'd be asking them to set the value of a parameter explicitly while the parameter doesn't affect their code at all, and make their code more verbose unnecessarily. So not raising the warning for them, would be nice in this case.

The down side is that we might be changing some behavior, which although not affecting the user, the user might rely on it, or they might only be affected very close to the deprecation cycle ends, not giving them much time to react. But if this is the case, they still will have time to react since they get the warning.

WDYT?

cc @StefanieSenger @scikit-learn/core-devs

COMMENT:
Cool, I'd agree. Then we can do the same for the deprecations related to the metrics we're doing.

Labels: API, RFC
RFC deprecation warnings only when user is affected

While reviewing https://github.com/scikit-learn/scikit-learn/pull/29288, I realised we're raising deprecation warnings to users, while most of them are not affected by the change, since the change only occurs when a division by zero is happenings.

So I was wondering about our deprecation warning policy.

In many cases, most users might not be affected at all, and we'd be asking them to set the value of a parameter explicitly while the parameter doesn't affect their code at all, and make their code more verbose unnecessarily. So not raising the warning for them, would be nice in this case.

The down side is that we might be changing some behavior, which although not affecting the user, the user might rely on it, or they might only be affected very close to the deprecation cycle ends, not giving them much time to react. But if this is the case, they still will have time to react since they get the warning.

WDYT?

cc @StefanieSenger @scikit-learn/core-devs

COMMENT:
> while probably 2 releases down the road they'll train on different data again that might or might not cause division by zero.

Maybe this is okay? If the user has not explicitly used the zero division parameter, it may not be important to their case? And in the future, post-deprecation, if this parameter becomes useful to them, they can read the docs, which will show the new parameter. Just a thought.

Labels: API, RFC
RFC deprecation warnings only when user is affected

While reviewing https://github.com/scikit-learn/scikit-learn/pull/29288, I realised we're raising deprecation warnings to users, while most of them are not affected by the change, since the change only occurs when a division by zero is happenings.

So I was wondering about our deprecation warning policy.

In many cases, most users might not be affected at all, and we'd be asking them to set the value of a parameter explicitly while the parameter doesn't affect their code at all, and make their code more verbose unnecessarily. So not raising the warning for them, would be nice in this case.

The down side is that we might be changing some behavior, which although not affecting the user, the user might rely on it, or they might only be affected very close to the deprecation cycle ends, not giving them much time to react. But if this is the case, they still will have time to react since they get the warning.

WDYT?

cc @StefanieSenger @scikit-learn/core-devs

COMMENT:
> Maybe this is okay? If the user has not explicitly used the zero division parameter, it may not be important to their case? 

Oh, but I was referring to a scenario where the user does set the zero_division parameter but never sees the FutureWarning, because the incoming data doesn't trigger a division by zero.

Labels: API, RFC
RFC deprecation warnings only when user is affected

While reviewing https://github.com/scikit-learn/scikit-learn/pull/29288, I realised we're raising deprecation warnings to users, while most of them are not affected by the change, since the change only occurs when a division by zero is happenings.

So I was wondering about our deprecation warning policy.

In many cases, most users might not be affected at all, and we'd be asking them to set the value of a parameter explicitly while the parameter doesn't affect their code at all, and make their code more verbose unnecessarily. So not raising the warning for them, would be nice in this case.

The down side is that we might be changing some behavior, which although not affecting the user, the user might rely on it, or they might only be affected very close to the deprecation cycle ends, not giving them much time to react. But if this is the case, they still will have time to react since they get the warning.

WDYT?

cc @StefanieSenger @scikit-learn/core-devs

COMMENT:
Ah yeah, did not think that through. Ignore that comment then.

Labels: API, RFC
Factor out `EmptyRequest`

### Describe the workflow you want to enable

At the moment, SKL creates the `EmptyRequest` at run time [here](https://github.com/scikit-learn/scikit-learn/blob/4adafd9ceb8e67467b81654c3632cd99c203df40/sklearn/utils/_metadata_requests.py#L1565). That makes it difficult to test properly if an an object is an instance of `EmptyRequest`.

### Describe your proposed solution

Could we factor this class definition out of the `process_routing` function?

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

`EmptyRequest` doesn't use local variables, so it doesn't benefit from the closure within `process_routing`.

COMMENT:
You should be able to test with assertion function (`assert_request_is_empty`): https://github.com/scikit-learn/scikit-learn/blob/main/sklearn/tests/metadata_routing_common.py#L98-L124

Maybe @adrinjalali has more insights?

Labels: New Feature, Needs Info
Factor out `EmptyRequest`

### Describe the workflow you want to enable

At the moment, SKL creates the `EmptyRequest` at run time [here](https://github.com/scikit-learn/scikit-learn/blob/4adafd9ceb8e67467b81654c3632cd99c203df40/sklearn/utils/_metadata_requests.py#L1565). That makes it difficult to test properly if an an object is an instance of `EmptyRequest`.

### Describe your proposed solution

Could we factor this class definition out of the `process_routing` function?

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

`EmptyRequest` doesn't use local variables, so it doesn't benefit from the closure within `process_routing`.

COMMENT:
I don't mind factoring it out, but I'd like to first understand what the usecase is and why you ended up on that part of the code 😅

Labels: New Feature, Needs Info
Factor out `EmptyRequest`

### Describe the workflow you want to enable

At the moment, SKL creates the `EmptyRequest` at run time [here](https://github.com/scikit-learn/scikit-learn/blob/4adafd9ceb8e67467b81654c3632cd99c203df40/sklearn/utils/_metadata_requests.py#L1565). That makes it difficult to test properly if an an object is an instance of `EmptyRequest`.

### Describe your proposed solution

Could we factor this class definition out of the `process_routing` function?

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

`EmptyRequest` doesn't use local variables, so it doesn't benefit from the closure within `process_routing`.

COMMENT:
Thanks for the quick response!

We have a custom pipeline class that splits data into different samples (train, validation, etc.). It's convenient when you want to pass evaluation samples to LightGBM for early stopping, say. It's easier for us to split the data after processing the metadata. We do so by iterating over the keys of the routed parameters, which works fine when the routed parameters take the form of a `Bunch`, but it fails when we have an `EmptyRequest`, which doesn't have `keys`. Hence the need to catch `EmptyRequest`s.

We could obviously have a `try/except` block, but it isn't as clean and I figured that factoring out the `EmptyRequest` class would be a small change.

Labels: New Feature, Needs Info
Factor out `EmptyRequest`

### Describe the workflow you want to enable

At the moment, SKL creates the `EmptyRequest` at run time [here](https://github.com/scikit-learn/scikit-learn/blob/4adafd9ceb8e67467b81654c3632cd99c203df40/sklearn/utils/_metadata_requests.py#L1565). That makes it difficult to test properly if an an object is an instance of `EmptyRequest`.

### Describe your proposed solution

Could we factor this class definition out of the `process_routing` function?

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

`EmptyRequest` doesn't use local variables, so it doesn't benefit from the closure within `process_routing`.

COMMENT:
That sounds a lot like https://github.com/scikit-learn/scikit-learn/pull/28901, and there we didn't need to check the type of the request object returned. Please have a look at that PR, and I think it should solve your issue. If not, happy to reopen this issue.

Labels: New Feature, Needs Info
Factor out `EmptyRequest`

### Describe the workflow you want to enable

At the moment, SKL creates the `EmptyRequest` at run time [here](https://github.com/scikit-learn/scikit-learn/blob/4adafd9ceb8e67467b81654c3632cd99c203df40/sklearn/utils/_metadata_requests.py#L1565). That makes it difficult to test properly if an an object is an instance of `EmptyRequest`.

### Describe your proposed solution

Could we factor this class definition out of the `process_routing` function?

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

`EmptyRequest` doesn't use local variables, so it doesn't benefit from the closure within `process_routing`.

COMMENT:
I took a look at the PR, but it doesn't quite work for us because we manipulate the props at a different point in the pipeline.

As an alternative to spinning `EmptyRequest` off, would it be feasible to add a  `__bool__` method to it? I think that it'd be make sense for an empty request to be falsy, just like an empty dictionary.

Labels: New Feature, Needs Info
Factor out `EmptyRequest`

### Describe the workflow you want to enable

At the moment, SKL creates the `EmptyRequest` at run time [here](https://github.com/scikit-learn/scikit-learn/blob/4adafd9ceb8e67467b81654c3632cd99c203df40/sklearn/utils/_metadata_requests.py#L1565). That makes it difficult to test properly if an an object is an instance of `EmptyRequest`.

### Describe your proposed solution

Could we factor this class definition out of the `process_routing` function?

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

`EmptyRequest` doesn't use local variables, so it doesn't benefit from the closure within `process_routing`.

COMMENT:
that doesn't sound like a good idea. I'm still wondering what exactly it is that you're trying to do. Whether or not a request is empty, should never be the question as far as all implementations we've done are concerned. So I suspect there's something we're missing here. I'd like to have some sort of a minimal code here to see what we can do.

Labels: New Feature, Needs Info
Factor out `EmptyRequest`

### Describe the workflow you want to enable

At the moment, SKL creates the `EmptyRequest` at run time [here](https://github.com/scikit-learn/scikit-learn/blob/4adafd9ceb8e67467b81654c3632cd99c203df40/sklearn/utils/_metadata_requests.py#L1565). That makes it difficult to test properly if an an object is an instance of `EmptyRequest`.

### Describe your proposed solution

Could we factor this class definition out of the `process_routing` function?

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

`EmptyRequest` doesn't use local variables, so it doesn't benefit from the closure within `process_routing`.

COMMENT:
Here's some pseudo code:

We have a `SampleSplitter` class:

```
class SampleSplitter:

    ...

    def split(self, X, y, props):

        X_train, X_rest = self.split_X(X)
        y_train, y_rest = self.split_y(y)
        props_train, props_rest = self.split_props(props)

        return (X_train, y_train, props_train), (X_rest, y_rest, props_rest)

    def split_props(self, props):

        props_train = Bunch() 
        props_test = Bunch()

        for key, value in props.items():
            ...
```

And we have a `Pipeline` class:

```
class Pipeline(sklearn.pipeline.Pipeline):

    ...

    def fit(self. X, y, **kwargs):

        props = self.process_routing(self, method="fit", **params)
        data_train, data_rest = SampleSplitter().split(X, y, props)

        ... # call `super()._fit` on `data_train`, etc.
```

The problem here is that we can't call `items` on an empty request and that it's also very difficult to know whether we have an empty request.

There are alternative solutions: a `try/except` block, for example, or splitting the data before routing. However, they're either less safe or much more cumbersome and repetitive.

Labels: New Feature, Needs Info
Factor out `EmptyRequest`

### Describe the workflow you want to enable

At the moment, SKL creates the `EmptyRequest` at run time [here](https://github.com/scikit-learn/scikit-learn/blob/4adafd9ceb8e67467b81654c3632cd99c203df40/sklearn/utils/_metadata_requests.py#L1565). That makes it difficult to test properly if an an object is an instance of `EmptyRequest`.

### Describe your proposed solution

Could we factor this class definition out of the `process_routing` function?

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

`EmptyRequest` doesn't use local variables, so it doesn't benefit from the closure within `process_routing`.

COMMENT:
To check which metadata are consumed by your sub estimators, you can simply use the `.consumes` method, as is done in these places:

https://github.com/scikit-learn/scikit-learn/blob/fa5d7275ba4dd2627b6522e1ec4eaf0f3a2e3c05/sklearn/base.py#L899-L901

https://github.com/scikit-learn/scikit-learn/blob/fa5d7275ba4dd2627b6522e1ec4eaf0f3a2e3c05/sklearn/pipeline.py#L520-L522

You shouldn't need to iterate through items requested in the routing. You probably should change the perspective and always iterate over given/provided metadata, and see if they're requested or not.

Also, as the pattern of splitting metadata itself, what you probably want to do is similar to what's done here:

https://github.com/scikit-learn/scikit-learn/blob/fa5d7275ba4dd2627b6522e1ec4eaf0f3a2e3c05/sklearn/model_selection/_validation.py#L842-L846

or here:

https://github.com/scikit-learn/scikit-learn/blob/fa5d7275ba4dd2627b6522e1ec4eaf0f3a2e3c05/sklearn/model_selection/_validation.py#L1321-L1325

Labels: New Feature, Needs Info
Factor out `EmptyRequest`

### Describe the workflow you want to enable

At the moment, SKL creates the `EmptyRequest` at run time [here](https://github.com/scikit-learn/scikit-learn/blob/4adafd9ceb8e67467b81654c3632cd99c203df40/sklearn/utils/_metadata_requests.py#L1565). That makes it difficult to test properly if an an object is an instance of `EmptyRequest`.

### Describe your proposed solution

Could we factor this class definition out of the `process_routing` function?

### Describe alternatives you've considered, if relevant

_No response_

### Additional context

`EmptyRequest` doesn't use local variables, so it doesn't benefit from the closure within `process_routing`.

COMMENT:
Thanks for the quick response.

> To check which metadata are consumed by your sub estimators, you can simply use the .consumes method

Unfortunately, the `SampleSplitter` class doesn't have that information, because it doesn't have sub-estimators.

> as the pattern of splitting metadata itself, what you probably want to do is similar to what's done here

These patterns correspond to subsetting before routing, which results in a lot more redundancy in our code base.

For what it's worth, I found an alternative solution: implementing a wrapper `sklearn.utils.metadata_routing.process_routing` that returns a falsy empty request. :)

Labels: New Feature, Needs Info
Make transition from 1.5 to 1.6 easier for third-party library using scikit-learn utilities

In 1.6, we introduced several breaking changes:

- `self._validate_data` became `sklearn.utils.validation.validate_data`
- `self._check_n_features` became `sklearn.utils.validation.check_n_features`
- `self._check_feature_names` became `sklearn.utils.validation.check_feature_names`
- a complete revamp for the tag infrastructure

While that all these changes are intended to improve the quality of life of third-party libraries by providing public utilities, it is going to break estimators and will require some boilerplate code to be compatible across scikit-learn version.

While those tools are private, it seems that we should still make a deprecation cycle such that we warn about the changes and start to raise error in future version (1.8).

COMMENT:
So https://github.com/scikit-learn/scikit-learn/pull/30327 seems to resolve some issue regarding the tag with a way forward.

I also want to point that I'm planning to make a small package in order to support the 4 latest version of scikit-learn with minimal pain (or at least the pain in centralized in one place).

I'll use `imbalanced-learn` and `skrub` to collect what tools are needed and centralize them here: https://github.com/glemaitre/sklearn_compat. I plan to move this package in the `scikit-learn-contrib` if we don't plan to take the maintenance hit in `scikit-learn` itself.

Labels: Blocker
⚠️ CI failed on macOS.pylatest_conda_forge_mkl (last failure: Nov 18, 2024) ⚠️

**CI failed on [macOS.pylatest_conda_forge_mkl](https://dev.azure.com/scikit-learn/scikit-learn/_build/results?buildId=72102&view=logs&j=97641769-79fb-5590-9088-a30ce9b850b9)** (Nov 18, 2024)
Unable to find junit file. Please see link for details.

COMMENT:
This should only be transient: the issue is related to internet connection.

Labels: Needs Triage
⚠️ CI failed on Wheel builder (last failure: Nov 16, 2024) ⚠️

**CI failed on [Wheel builder](https://github.com/scikit-learn/scikit-learn/actions/runs/11866940607)** (Nov 16, 2024)

COMMENT:
## CI is no longer failing! ✅

[Successful run](https://github.com/scikit-learn/scikit-learn/actions/runs/11875699003) on Nov 17, 2024

Labels: Needs Triage