Skip to content

FIX nested backend not changed by SequentialBackend#792

Merged
tomMoral merged 2 commits intojoblib:masterfrom
tomMoral:FIX_nested_backend_sequential
Oct 16, 2018
Merged

FIX nested backend not changed by SequentialBackend#792
tomMoral merged 2 commits intojoblib:masterfrom
tomMoral:FIX_nested_backend_sequential

Conversation

@tomMoral
Copy link
Copy Markdown
Contributor

The nested backend in SequentialBackend should not be changed as we do not got deeper in the nesting. This causes issue for libraries that relies on Parallel even when n_jobs=1, with subsequent nested calls (see eg scikit-learn/scikit-learn#12389 ).

This PR should fix this and test the behavior.

@codecov
Copy link
Copy Markdown

codecov bot commented Oct 15, 2018

Codecov Report

Merging #792 into master will decrease coverage by <.01%.
The diff coverage is 95%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #792      +/-   ##
==========================================
- Coverage   95.33%   95.33%   -0.01%     
==========================================
  Files          44       44              
  Lines        6307     6319      +12     
==========================================
+ Hits         6013     6024      +11     
- Misses        294      295       +1
Impacted Files Coverage Δ
joblib/_parallel_backends.py 96% <100%> (ø) ⬆️
joblib/test/test_parallel.py 96.82% <94.44%> (-0.1%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 2f252e3...a366d01. Read the comment docs.

Copy link
Copy Markdown
Contributor

@ogrisel ogrisel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

return SequentialBackend(nesting_level=nested_level), None
# SequentialBackend should neither change the nesting level, the
# default backend or the number of jobs. Just return the current one.
from .parallel import get_active_backend
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Explain that the local import is to prevent cyclic import issues.

def check_nested_backend(expected_backend_type, expected_n_job):
# Assert that the sequential backend at top level, does not change
if expected_backend_type is None:
expected_backend_type = 'loky'
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This if block can be removed as the expected_backend_type is never None.

@tomMoral tomMoral merged commit acca6b9 into joblib:master Oct 16, 2018
@tomMoral tomMoral deleted the FIX_nested_backend_sequential branch October 16, 2018 08:50
yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Apr 12, 2019
* tag '0.13.0':
  Release 0.13.0
  Use https (joblib#805)
  MTN bump loky to 2.4.2 (joblib#804)
  DOC: provide some useful variables for Makefile (joblib#794)
  DOC serialization and processes (joblib#803)
  ENH update loky 2.4.0 (joblib#802)
  FIX backward compatibility for nested backend (joblib#789)
  enable python 3.7 (joblib#795)
  memory: add test for call_and_shelve performance (joblib#791)
  FIX nested backend not changed by SequentialBackend (joblib#792)
  cloudpickle 0.6.0 (joblib#788)
  FIX nested backend setting n_jobs=-1 (joblib#785)
  Raises a more explicit exception when a corrupted MemorizedResult is loaded (joblib#768)
  Back to dev mode
yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Apr 12, 2019
* releases:
  Release 0.13.0
  Use https (joblib#805)
  MTN bump loky to 2.4.2 (joblib#804)
  DOC: provide some useful variables for Makefile (joblib#794)
  DOC serialization and processes (joblib#803)
  ENH update loky 2.4.0 (joblib#802)
  FIX backward compatibility for nested backend (joblib#789)
  enable python 3.7 (joblib#795)
  memory: add test for call_and_shelve performance (joblib#791)
  FIX nested backend not changed by SequentialBackend (joblib#792)
  cloudpickle 0.6.0 (joblib#788)
  FIX nested backend setting n_jobs=-1 (joblib#785)
  Raises a more explicit exception when a corrupted MemorizedResult is loaded (joblib#768)
  Back to dev mode
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants