FIX get config from dispatcher thread in delayed by default

[glemaitre]

closes #25239

delayed function wrap the function and fetch the global configuration. However, this "global" dictionary is local to the specific thread that dispatches the job. Therefore, we can end-up with a default "global" config instead of the configuration defined in the main thread.

The solution here is to add a parameter to delayed, defaulting to the main thread, to retrieve the configuration associated with the main thread.

[glemaitre]

The failure with ARM shows that we register more threads than expected because we run the test in parallel. It is probably best to rely upon that the weakref dictionary does the job when joblib kill the thread.

[thomasjpfan]

Thank you for the fix!

I'm trying to avoid adding more API so we can introduce this as a bug fix into 1.2.1

[thomasjpfan]

Overall, I think this solution works and do not foresee any issues with it.

[thomasjpfan]

If the default working_memory on the main thread were to change this test would fail:

sklearn.set_config(working_memory=140)
# the following fails
assert results == [get_working_memory()] * n_iter

The less fragile assertion would be check that the local_thread uses the default global config:

from sklearn._config import _global_config_default
assert results == [_global_config_default["working_memory"]] * n_iter

[ogrisel]

Reviewing this test, it seems that I have found a bug in the way the default value of the thread argument of delayed is defined. I am working on a PR against this PR.

[ogrisel]

Capturing the current thread here is problematic because it makes the behavior of scikit-learn dependent on which thread first imported scikit-learn and scikit-learn's behavior is no longer thread-symmetric.

I tried changing this to:

def _delayed(func, thread=None):
    if thread is None:
        thread = threading.current_thread()

but this does not work either. Thread inspection does not work as intended (too late) when calling Parallel on a generator expression which is the canonical way to use joblib. Instead we should capture the state of the config of the thread that calls Parallel just before the call happens and ship it to all the dispatched tasks.

We just had a live pair-debugging / programming session with @glemaitre on discord and I think we came up with a better solution that is a bit more verbose but also much more explicit (and correct ;). He will open a PR soon and we will be able to have a more informed technical discussion there.

For the longer term we could expose a hook in joblib to better handle this kind of configuration propagation but having a stopgap fix in scikit-learn makes it possible to decouple scikit-learn from the version of joblib.

[glemaitre]

Refer to #25290 for the better solution

[fcharras]

👍 I was afraid it would come to this kind of more verbose solution. Maybe at the same time this is merge to enable the fix, a separate issue could be opened to discuss the in and outs of the per-thread config ? unless the behavior that is enforced and supported is clear already but that didn't seem to be (the PR where the behavior was enabled does not discuss much)

[glemaitre]

This is true that we did not discuss this here but we did during the pair programming session.

@ogrisel agreed that we should keep the current behavior where you don't want a thread modifying the config during that other threads may use it. It is a bit counter-intuitive if we rely on the fact that threads should share memory but the side-effect within scikit-learn would be potentially bad. For instance, you can potentially get different random errors that is not reproducible because it would depend on the config state at a particular moment.

[ogrisel]

I think we should discuss the two options at the next dev meeting:

• option 1: revert sklearn.get/set_config to always use a single shared config without per-thread isolation as was the case prior in 1.0.
• option 2: make thread isolation work correctly at the cost of a bit of verbosity as implemented in FIX pass explicit configuration to delayed #25290

I think I am in favor of option 2 but I think it worth discussing this with others.

[thomasjpfan]

Option 1 would resolve the issue for multi-threading, but I think the issue will remain for multiprocessing or loky.

I am okay with Option 2. Most of my concern is how third party developers using joblib need to update their code to use utils.fixes.delayed to work with scikit-learn's config.

[thomasjpfan]

From the developer call, joblib uses another thread to get jobs from a generator, which means Option 1 with a thread local configuration would resolve the current issue.

[ogrisel]

Closing in favor of #25363.