FEA Callbacks base infrastructure + progress bars#27663
FEA Callbacks base infrastructure + progress bars#27663jeremiedbb wants to merge 57 commits intoscikit-learn:callbacksfrom
Conversation
sklearn/base.py
Outdated
| params_set = new_object.get_params(deep=False) | ||
|
|
||
| # attach callbacks to the new estimator | ||
| if hasattr(estimator, "_callbacks"): |
There was a problem hiding this comment.
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?
| try: | ||
| return fit_method(estimator, *args, **kwargs) | ||
| finally: | ||
| estimator._eval_callbacks_on_fit_end() |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
|
+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. |
|
Note that this PRis targeting the
I agree and we already discussed that with @glemaitre. I plan to do that in a follow up PR. |
|
I still need to figure out the issue with the CI though |
|
Any news on this being merged? |
We currently working on the design and need an agreement among core developers. |
|
Any news on this? It would be amazing would this be added. |
|
I think that the relevant PR is now #28760 |
|
@jeremiedbb @FrancoisPgm shall we close this? |
|
I was just keeping it open to compare 2 alternatives but I guess this one is completely outdated now. Let's close |
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
callbacksbranch 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.Managersand 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:
You add
sleepcalls 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...