FIX parse pre-dispatch with AST instead of calling eval

[adrinjalali]

I realized #1321 doesn't actually fix the issue, since eval("exec('import os')", {}, {}) would still work.

This PR uses AST and limits the operations to the very basic ones.

This should be an actual fix for #1128

cc @ogrisel

[codecov]

Codecov Report

Merging #1327 (8a5c364) into master (1f00a1c) will decrease coverage by 0.07%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #1327      +/-   ##
==========================================
- Coverage   93.91%   93.83%   -0.08%     
==========================================
  Files          50       52       +2     
  Lines        7275     7301      +26     
==========================================
+ Hits         6832     6851      +19     
- Misses        443      450       +7     

Impacted Files	Coverage Δ
joblib/_utils.py	100.00% <100.00%> (ø)
joblib/parallel.py	96.03% <100.00%> (-0.52%)	⬇️
joblib/test/test_utils.py	100.00% <100.00%> (ø)
joblib/_parallel_backends.py	91.54% <0.00%> (-1.48%)	⬇️
joblib/pool.py	87.80% <0.00%> (-0.82%)	⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[ogrisel]

Thanks for the follow-up PR @adrinjalali! Here is some feedback:

[ogrisel]

Could you please add a new test for a pre_dispatch expression which includes the n_jobs placeholder in it?

Other than that and the linting issue, LGTM!

[adrinjalali]

Could you please add a new test for a pre_dispatch expression which includes the n_jobs placeholder in it?

Doesn't this already cover it?

@parametrize('n_tasks, n_jobs, pre_dispatch, batch_size',
             [(2, 2, 'all', 'auto'),
              (2, 2, 'n_jobs', 'auto'),
              (10, 2, 'n_jobs', 'auto'),
              (517, 2, 'n_jobs', 'auto'),
              (10, 2, 'n_jobs', 'auto'),
              (10, 4, 'n_jobs', 'auto'),
              (200, 12, 'n_jobs', 'auto'),
              (25, 12, '2 * n_jobs', 1),
              (250, 12, 'all', 1),
              (250, 12, '2 * n_jobs', 7),
              (200, 12, '2 * n_jobs', 'auto')])
def test_dispatch_race_condition(n_tasks, n_jobs, pre_dispatch, batch_size):
    # Check that using (async-)dispatch does not yield a race condition on the
    # iterable generator that is not thread-safe natively.
    # This is a non-regression test for the "Pool seems closed" class of error
    params = {'n_jobs': n_jobs, 'pre_dispatch': pre_dispatch,
              'batch_size': batch_size}
    expected = [square(i) for i in range(n_tasks)]
    results = Parallel(**params)(delayed(square)(i) for i in range(n_tasks))
    assert results == expected

[ogrisel]

Indeed, my git grep was too restrictive.

[ogrisel]

Thanks for the fix @adrinjalali !

[miraculixx]

I'd like to vote for reverting this, as it does not address a real issue, as noted in #1128 (comment)