BUG Passes global configuration when spawning joblib jobs

[thomasjpfan]

On master the global configuration flag would not be passed joblib jobs with a multiprocess backend:

from joblib import Parallel, delayed
from sklearn import get_config, config_context

def get_working_memory():
    return get_config()['working_memory']

with config_context(working_memory=123):
    results = Parallel(n_jobs=2)(
        delayed(get_working_memory)() for _ in range(2)
    )

results
# [1024, 1024]

XREF: joblib/joblib#1071

[rth]

Thanks for detecting this and the issue at joblib! Let's see if there is a way to fix it on the joblib side (or maybe our config mechanism needs to be revisited). Using a parallel wrapper means that external users of joblib + scikit-learn would still run into this issue..

[adrinjalali]

@rth do you have a proposal for a revised config handling?

[thomasjpfan]

I have a "fun" proposal here: joblib/joblib#1071 (comment)

(Global config + doing things in parallel = fun)

[jnothman]

We could just use os.environ to store the config?

[thomasjpfan]

Storing them in the environment may not work: joblib/joblib#1064

[adrinjalali]

Doesn't that depend on the backend? It should work as long as all those processes are on the same machine, doesn't it?

[ogrisel]

Maybe we could pass it as environment and make joblib / loky detect that environment has changed and re-spawn workers accordingly. That might be a bit expensive though.

[ogrisel]

Doesn't that depend on the backend? It should work as long as all those processes are on the same machine, doesn't it?

The threading backend should have no problem. multiprocessing + fork should work out of the box but it's broken in other ways and not available on windows.

loky and dask will need explicit config init.

[adrinjalali]

I think I'm okay with this, but I'd probably move the code to fixes.py to have them all in the same place, and I think adding a linter to fail on importing joblib.delayed would be an overkill?

[jnothman]

Apart from not being comfortable with the use of utils.fixes for something other than a backport, this looks great

[jnothman]

Is this still needed?

[thomasjpfan]

Apart from not being comfortable with the use of utils.fixes for something other than a backport, this looks great

Should we use sklearn.utls._parallel like we had before?

[jnothman]

I think I can come to accept fixes :)

[adrinjalali]

Apart from not being comfortable with the use of utils.fixes for something other than a backport, this looks great

This needs to be fixed in joblib upstream, and then it'll be removed, from that perspective, fixes.py is the right place for it I think.

Otherwise this LGTM. I'm happy to have it as a temporary workaround.

[adrinjalali]

maybe add a comment like

remove when https://github.com/joblib/joblib/issues/1071 is fixed

?

[rth]

This looks reasonable but would one among @lesteve @ogrisel or @tomMoral be also be able to review this, following discussion in joblib/joblib#1071?

[rth]

Could you please also check that the vendored version of delayed, hasn't significantly changed for the supported versions of joblib?

[thomasjpfan]

I looked at:

https://github.com/joblib/joblib/blob/19db4f58ac502a4bd01ab07aa31b2fc8756bcc3a/joblib/parallel.py#L302

from tags 0.11 to 0.16 and the function does the same thing.

After reading its implementation, we can use a simplified version in our utils.fixes for now:

def delayed(function):
    """Decorator used to capture the arguments of a function."""
    @functools.wraps(function)
    def delayed_function(*args, **kwargs):
        return _FuncWrapper(function), args, kwargs
    return delayed_function

This way we do not need to depend on a vendored version.

[cmarmo]

Hi @scikit-learn/core-devs, two approvals here: is something left? Or is it good to merge?

[adrinjalali]

Since the last commit and hence the CI runs were from almost 3 weeks ago, I'm happy to merge once we do another merge with the current master. Would you mind updating @thomasjpfan ? I feel like we agreed to having this in the meantime.

[thomasjpfan]

Merged with master and it still passes.

[thomasjpfan]

Merged with master and it still passes.

[adrinjalali]

woohoo 🥳 let's do this then 👍