Emotional roller-coaster 😅

making sure there are no threads left open after a fit is done

We can add a test for that. Right now the existing tests make sure that all teardowns are called (and threads are closed in those teardowns), so it's already something.

the need for a singleton main root node kept at the callback module level instead of the parent node creating it

That (was discussed between Adrin and I during a call) was not a necessity after all and that's good because it would have added a lot of complexity to make make work accross processes.

Even though I opened this PR, my preference is still that clone shouldn't keep callbacks. I'd even not raise a warning in clone and instead document that if you want callbacks to work, you need to implement it in all the nested estimators.
That being said, it's just a preference but I'm not opposed to the option presented in this PR. Let's find consensus during tomorrow's meeting :)

Actually everything's not as simple as I stated in the PR description. Reusing the cross-val example but running the cross-val in parallel this time we only see 1 bar progressing at once and then new bars appears and are already finished. This is because we spawn threads in the sub-processes and their stdout is not directly forwarded to the parent process. So the rendering in parallel setting is quite bad.

It also requires some small changes in #33494, but nothing too bad (still a bit annoying like the clean setup/teardown around fit and setting the context as attribute of the estimator)

Thank you for being open for revision, @jeremiedbb and @FrancoisPgm.

So it seems that a common expectation is that callbacks should work even when set on sub-estimators of meta-estimators or functions that don't implement callback support.

For me, that was not the expectation. I only now see that your main intention behind not cloning callbacks by default was to prevent undefined callback behaviour, whereas fixing the pickling error in the clone (making cloning work) and other issues were only side quests. There was some information loss as to what for we want to define the cloning behaviour, at least for me.

1. I think we could document that callback behaviour is undefined / not guaranteed to work if users set callbacks, but combine a non-callback supporting object. That seems a reasonable and not surprising limit of a new broad feature.

2. For preventing undefined callback behaviour I can see that cutting the chain after the first non supporting element is a great idea. I wonder if there is a better place to do so than in clone.

   Alternatively, sub-estimators could check if their parents are a CallbackSupportMixin instance or have a set_callback attribute (not sure how to do this for functions, but probably there are tricks to attach attributes to functions?).

fc833ab shows the necessary changes to ProgressBar to make it work.

The main change is that now the callback must be able to handle:

• set on a single estimator in the main process and pickled in child processes (so a managed queue)
• single instance set on several estimators directly in sub-processes (so process-local monitor threads)

It complexifies a bit implementing propagated callbacks properly.

Alternatively, sub-estimators could check if their parents are a CallbackSupportMixin instance or have a set_callback attribute (not sure how to do this for functions, but probably there are tricks to attach attributes to functions?).

I have tried to do that before, to raise a warning when a sub-estimator that supports callbacks is used in a meta-estimator that doesn't. My approach was inspecting the traceback during runtime, identifying if it contains a call of a fit from a non CallbackSupportMixin estimator, but it was a bit clunky, not very robust, and would not work in parallelized settings, so we gave up on that idea. Maybe there is a smarter way to perform the check though.

Actually everything's not as simple as I stated in the PR description. Reusing the cross-val example but running the cross-val in parallel this time we only see 1 bar progressing at once and then new bars appears and are already finished.

Just to be sure, do you have some code just so we get an idea how bad the behaviour is. From what you are saying, this doesn't sound horribly bad, but maybe with other callbacks it could be worse?

I tried something like:

from sklearn.base import clone
from joblib import delayed, Parallel
from sklearn.callback.tests._utils import MaxIterEstimator
from sklearn.callback import ProgressBar

n_jobs = 4
backend = "loky"

if __name__ == "__main__":
    def clone_and_fit(estimator):
        clone(estimator).fit()

    def func(estimator, n_fits, n_jobs, backend):
        Parallel(n_jobs=n_jobs, backend=backend)(
            delayed(clone_and_fit)(estimator) for _ in range(n_fits)
        )

    n_fits, max_iter = 5, 7
    callback = ProgressBar()
    estimator = MaxIterEstimator(max_iter=max_iter).set_callbacks(callback)

    func(estimator, n_fits, n_jobs, backend)

and I don't see any major issue personally.

Just to be sure, do you have some code just so we get an idea how bad the behaviour is. From what you are saying, this doesn't sound horribly bad, but maybe with other callbacks it could be worse?

Your snippet illustrates what I mean. It's just that you need to set a higher number of iterations and fits to actually see something (otherwise it's too fast). Say n_fits=12 and max_iter=100.
Then you'll only see 1 bar progressing and when it's finished, 3 more bars appear and are finished (we didn't see them progress). Then a new bar progresses and the same behavior repeats.
It's because we don't get right away the stdout from the other 3 sub-processes.

The final rendering is fine and shows all bars finished, but that's not what we're interested in with progress bars, we want to see progress.

EDIT: Actually it looks like all bars from subprocesses are stacked during progress and un-stacked when finished.

As far as I can tell, this looks good to me now

LGTM thanks :)

I addressed the review comments. I'm merging this one with 2 approvals. Thanks !