RFC: Breaking Changes for Version 2

A while ago we talked about the possibility of a version 2 with breaking changes. Specifically, this came up in the context of SLEP006: metadata routing, but there are other things we have wanted to break which we can do all in a single release. In this issue we can talk about the possibility of such a release, and maybe a few things we'd like to include in it.

I'll try to summarize the challenges of keeping backward compatibility (BC) in the context of SLEP006:

- `MetaEstimator(Estimator()).fit(X, y, sample_weight)` will raise according to SLEP006 since `sample_weight` is not requested by any object. For backward compatibility, during the transition period, it'll only warn and assume `sample_weight` is requested by `Estimator()`.
- What should the behavior of `GridSearchCV(Estimator(), scorer=scorer).fit(X, y, sample_weight)` be during the transition period? If we keep backward compatibility, it should warn (whatever the warning be), route `sample_weight` to `Estimator`, but not to the `scorer`. And that's only the case because we're keeping BC. From the user's perspective, it's very weird.
- `Pipeline` ATM in certain methods like `fit`, `fit_transform`, `predict`, routes `*_params` to the `fit/etc` of the last estimator only. And `decision_function` and `transform`, `score_samples` have no routing at all. Keeping this BC, proves to be challenging, and a bit nasty. ref: #24270
- In quite a few meta-estimators, we check if the sub-estimator has `sample_weight` in the signature of `fit`, and we pass the given sample weight to it if that's the case, and not if that's not the case. With routing, we would have a much better idea of when to pass sample weight and when not, but if we're keeping BC, it's really challenging to see if we should forward sample weights or not. Both AdaBoost (#24026) and Bagging (#24250) have been very challenging, to the point that we might end up having to check if a sub-estimator is a meta-estimator itself or not, and check signature of sub-estimator's `fit`, and check the given routing info to keep BC.

I'm pretty sure there are other issues we haven't foreseen, and they'll come up as we work on this project. We thought it'd be easy once we get to this point, but BC has proven to make it very tricky and has stalled the progress.

All of the above is adding substantial complexity which is probably error prone, and buggy. We have had to spend quite a bit of time every time we go back to review those bits to understand why we've done what we've done. Even as a temporary solution they're not very maintainable.

My proposal here is to have a version 2 which is not BC with previous releases in certain ways, and target a few breaking changes for it. We can aim to have the release in October 2023 or April/May 2024 as the version 2.

cc @scikit-learn/core-devs

COMMENT:
I wasn't part of the team when the decision was made to go with SLEP6, so I will make general comments.

In general having a breaking release (without deprecation cycles) is a major pain for everyone involved. Including the maintainers, though they get a bit of a benefit because implementation is often less complex. A major source of the pain is that the breakage will catch lots of people by surprise. For example a third part library will usually only specify `scikit-learn` or `scikit-learn>0.19` as a dependency. Hardly any add the `<2` specifier. You can have opinions on whether they should add this upper limit or not, but in practice today nearly no one has it. This means once scikit-learn v2 exists, all these packages will break until there is a new release of that package (with an upper version specified). And even then you will still get failures in cases where people install a third party scikit-learn package that is a dependency of some other package, as some of those packages will say "we depend on `third-party-sklearn=1.2.3`", where that `third-party-sklearn` release is one that doesn't contain an upper version limit in its dependencies (You and I have opinions on whether you should have dependencies like this, but that doesn't matter because people do this :-/).

Given enough time, this will sort itself out. But in the mean time people will arrive in the scikit-learn repo with questions and complaints about things not working. These need dealing with, and this is how the maintainers that hoped to save themselves work by making a breaking release without deprecation cycle end up having additional work/pain.

How long is the time frame? How many people will be effected? I don't know. From my experience with Jupyter and mybinder.org makes me say "much longer than you think and many more (vocal) people than you think".

This means I think it is worth trying to estimate the number of impacted packages based on code search/inspection on GitHub. Yes, opening an issue in third-party repos doesn't scale, but maybe we are lucky and there aren't that many? Similarly dealing with complaints from users of third party packages doesn't scale either :-/

I like the idea of feature flags. It is a bit fiddly and for a few releases will lead to duplicated code, but it can be a good way to transition. Python is flexible enough so that you can have `from sklearn.experimental import sleep6` modify what gets imported when the user runs `from sklearn.foo import BarClassifier` (to address the function signature concern).

TL;DR: IMHO we need some data on how many people will be surprised with breakage by this particular backwards incompatible change without deprecation cycles.

---

As a alternative for Christians "your alternative here" list: just because a SLEP has been accepted doesn't mean it has to be implemented. If since accepting a SLEP people realised that implementing it will be much more costly than they thought, it can be wise to just stop working on it/reconsider. Otherwise known as the oh so popular sunk cost fallacy.

Labels: RFC, Breaking Change