FEA Callbacks base infrastructure + progress bars

[jeremiedbb]

Extracted from #22000

This PR implements a smaller portion of #22000, with only the base infrastructure for callbacks and the implementation of a single callback (progress bars). It targets the callbacks branch and the goal is to have the full callback implementation done in several smaller PRs merged in this branch before we merge it in main.

This PR proposes significant changes compared to #22000 that should imo improve a lot its change of getting merged 😄
The main improvement is that it no longer requires writing on the disk, but instead relies on multiprocessing.Managers and queues. It simplifies the code a lot.

In #22000 I adapted some estimators to work with the callbacks which I did not include here to keep the PR as light as possible. You can however experiment the callbacks on estimators that I wrote for testing purpose:

from sklearn.callback import ProgressBar
from sklearn.callback.tests._utils import Estimator, MetaEstimator
est = Estimator()
meta_est = MetaEstimator(est, n_jobs=2)
meta_est._set_callbacks(ProgressBar())
meta_est.fit(None, None)

You add sleep calls in these testing estimators to simulate how it changes when the computations take longer.

The plan is to then have several PRs to implement the other callbacks, adapt a few estimators to work with the callbacks, add some documentation and examples, add more tests...

[glemaitre]

Is there a way to cover this one?

[jeremiedbb]

It will be covered with the EarlyStopping callback

[jeremiedbb]

Hi @rth, I think this is getting ready for a second review :)
This PR introduces the base infrastructure for callbacks and a first callback for progress bars. Next steps are detailed in #27676

[glemaitre]

LGTM in my side. @rth please ignore all the lock file changes. this does not require any attention.

[rth]

Very sorry for the response time @jeremiedbb . Hopefully I fixed my emails filters to see mentions from scikit-learn .

I read it though once, and the main thing I don't understand is how do you compute levels dict. I would have expected the build_computation_tree to build the computation tree, but actually it takes levels as parameters which is already some form of that tree.

Say you have a ColumnTransformer with 2 pipelines inside first with 2 steps, the other one with 3 steps. How do you compute the computation tree in that case?

[rth]

BTW, this might break something for https://github.com/microsoft/FLAML/blob/0415638dd1e1d3149fb17fb8760520af975d16f6/flaml/automl/model.py#L1587 which also adds this attribute to scikit-learn's base estimator in their library. But there is no reserved private namespaces, so probably they could adapt if it does.

[adrinjalali]

This can also break quite easily if the callback object keeps references to attributes of the old estimator. Why aren't we creating a copy here?

[jeremiedbb]

BTW, this might break something for https://github.com/microsoft/FLAML/blob/0415638dd1e1d3149fb17fb8760520af975d16f6/flaml/automl/model.py#L1587 which also adds this attribute to scikit-learn's base estimator in their library

I can change the name to _sk_callbacks or any derivative of that.

[jeremiedbb]

Why aren't we creating a copy here?

The motivation is a use case like this

monitoring = Monitoring()
lr = LogisitcRegression()._set_callbacks(monitoring)
GridSearchCV(lr, param_grid).fit(X,y)
monitoring.plot()

The monitoring callback will gather information across all copies of logistic regression made in the grid search. If we made a copy of the callback in clone, then we couldn't retrieve any information once the grid search is finished.

[adrinjalali]

The object itself can disable copy by implementing __copy__ and __deep_copy__, and then they would be in the space of "we know what we're doing" and we don't need to worry about it.

I think the kind of state you're referring to here, is something which can be outside the callback object, like a file / a database / an external singleton object, and the callback method just writes into that storage, and at the end one can use that data to plot/investigate/etc.

[jeremiedbb]

I think the kind of state you're referring to here, is something which can be outside the callback object, like a file / a database / an external singleton object

good point and actually in my latest design of the callbacks I didn't rely on a shared state so you can ignore my previous comment :) And we can't get around copies anyway since the clones can happen in subprocesses (in a grid search for instance). I updated to clone the callbacks as any other param

[rth]

What happens if you call this method twice? Wouldn't this be raised on the second run. If so it feels a bit fragile. What's the harm of not checking this?

[jeremiedbb]

It should never be called twice. The meta estimator only propagates its callbacks once to its sub-estimator(s).

This error is important to have an informative error message if a user tries something like

lr = LogisiticRegression()._set_callbacks(ProgressBar)
GridSearchCV(lr, param_grid)

It would crash without telling the user what's the right way to do it.

[rth]

Wait so max_iter has nothing to do with the number of iterations of an algorithm. Why is it called that way then.

Also if it's expected to have only a few keys, typing wise might have been better to make it a dataclass or something rather than a dict, but no strong opinion.

[adrinjalali]

A very shallow review.

[adrinjalali]

This can also break quite easily if the callback object keeps references to attributes of the old estimator. Why aren't we creating a copy here?

[adrinjalali]

this is becoming larger than just a validation wrapper. We can simplify debugging and magic by having a BaseEstimator.fit which calls self._fit(...) and does all the common stuff before and after. That seems a lot better to understand and debug.

[jeremiedbb]

The motivation when I introduced _fit_context was not only for validation but to have a generic context manager to handle everything we need to do before and after fit. That's why I gave it this generic name.

Although having a consistent framework where we'd have a BaseEstimator.fit and every child estimator implements _fit is appealing, I think it goes far beyond the scope of this PR and requires rewriting a lot of estimators.

[jeremiedbb]

Btw do you why BaseEstimator does not implement fit in the first place ?

Also, note that _fit_context also handles partial_fit, but I don't think we want BaseEstimator to implement partial_fit

[adrinjalali]

BaseEstimator doesn't implement fit cause we don't generally have methods which raise NotImplementedError. They're simply not there. But now that we have all this work, we can certainly have it in BaseEstimator, and children only implement a __sklearn_fit__ kind of method instead.

[jeremiedbb]

I still think it's outside the scope of this PR. Using the existing context manager is just 1 line addition whereas implementing __sklearn_fit__ means countless PRs :)

[rth]

+1 to merge this. I personally find that it would be more valuable to mark it as experimental (it's private anyway so far) let the users use this for a version or two, aggregate their feedback and iterate in future details if needed.

Overall the API sounds reasonable to me. Rather than to approve a SLEP on this, only then to realize that users would have preferred something else, or that there are some weird edge cases for some estimator that need special handling.

[jeremiedbb]

Note that this PRis targeting the callbacks branch, not main so merging this is not a big commitment anyway 😄
And it would allow me to implement the rest that I did not include in this PR to keep it as small as possible.

I personally find that it would be more valuable to mark it as experimental

I agree and we already discussed that with @glemaitre. I plan to do that in a follow up PR.

[jeremiedbb]

I still need to figure out the issue with the CI though
EDIT: good now