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.

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?

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.

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.

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.

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

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?

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.

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.

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?

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.

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.

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

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.

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 :)