[MRG+1] Integrate loky as default backend by tomMoral · Pull Request #516 · joblib/joblib

tomMoral · 2017-04-13T17:38:58Z

This PR aims to integrate loky as a backend in joblib. This PR features:

Vendoring of loky as joblib.externals.loky
Vendoring of cloudpickle as joblib.externals.cloudpickle
Fixes the integration of loky.get_reusable executor as a parallel backend.

codecov · 2017-04-13T17:45:28Z

Codecov Report

Merging #516 into master will increase coverage by 0.43%.
The diff coverage is 96.22%.

@@            Coverage Diff             @@
##           master     #516      +/-   ##
==========================================
+ Coverage   93.73%   94.16%   +0.43%     
==========================================
  Files          37       39       +2     
  Lines        4785     4953     +168     
==========================================
+ Hits         4485     4664     +179     
+ Misses        300      289      -11

Impacted Files	Coverage Δ
joblib/numpy_pickle.py	`98.47% <ø> (ø)`	⬆️
joblib/test/test_numpy_pickle.py	`99.04% <100%> (-0.01%)`	⬇️
joblib/test/test_parallel.py	`95.52% <100%> (+0.3%)`	⬆️
joblib/pool.py	`91.37% <100%> (-0.26%)`	⬇️
joblib/test/test_memory.py	`99.36% <100%> (+0.84%)`	⬆️
joblib/testing.py	`87.5% <100%> (+0.65%)`	⬆️
joblib/executor.py	`100% <100%> (ø)`
joblib/test/test_memmapping.py	`100% <100%> (ø)`
joblib/parallel.py	`98.2% <100%> (+1.16%)`	⬆️
joblib/disk.py	`80.35% <65%> (-0.04%)`	⬇️
... and 9 more

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 5b6e44b...7502017. Read the comment docs.

ogrisel · 2017-04-14T08:58:52Z

This is WIP because we need to deal with the dependencies (loky and cloudpickle). @tomMoral will work on a second PR that sits on top of this one to vendor those two dependencies into a joblib/externals or joblib/vendor folder.

tomMoral · 2017-05-14T08:07:23Z

I finished vendoring loky and cloudpickle as part of joblib but I did it in the same PR.
I can open 2 separate PR is you want.

aabadie

Even if this PR is marked WIP, I wanted to have a look at it.
For the moment, I only made a code review and have a few comments. Since the CI passes, it should work well.
Do you have some benchmarks compared to multiprocessing/threading ?

aabadie · 2017-05-17T14:08:48Z

Makefile


 test:
-	pytest joblib
+


This empty line is not necessary

aabadie · 2017-05-17T14:09:45Z

joblib/_memmapping_reducer.py

@@ -0,0 +1,357 @@
+# -*- coding: utf-8 -*-


A module docstring is missing. This is not the case in other internal modules.

aabadie · 2017-05-17T14:16:05Z

joblib/_memmapping_reducer.py

+            # done processing this data.
+            if not os.path.exists(filename):
+                if self.verbose > 0:
+                    print("Memmapping (shape=%r, dtype=%s) to new file %s" % (


I prefer using .format() with string. This needs to be changed in other places as well.

aabadie · 2017-05-17T14:17:37Z

joblib/_memmapping_reducer.py

+            return (loads, (dumps(a, protocol=HIGHEST_PROTOCOL),))
+
+
+def delete_folder(folder_path):


This is the kind if function that already exists in the disk.py module. Maybe it could be factorize here ?

aabadie · 2017-05-17T14:18:47Z

joblib/_memmapping_reducer.py

+
+    if np is not None:
+        # Register smart numpy.ndarray reducers that detects memmap backed
+        # arrays and that is alse able to dump to memmap large in-memory


aabadie · 2017-05-17T14:46:17Z

joblib/test/test_parallel.py

    # 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}
+              'batch_size': batch_size, 'verbose': 100}


is this change really required ?

aabadie · 2017-05-17T14:47:50Z

joblib/test/test_parallel.py

+        try:
+            import cloudpickle  # noqa
+        except ImportError:
+            # This test pass only when fork is the process start method or


s/pass/passes/

aabadie · 2017-05-17T14:48:55Z

joblib/test/test_parallel.py

+
+    # Make sure that the shared memory is cleaned at the end when we exit
+    # the context
+    assert len(os.listdir(tmpdir)) == 0


assert not os.listdir(tmpdir)

aabadie · 2017-05-17T14:49:20Z

joblib/test/test_parallel.py

+    p = Parallel(n_jobs=2, max_nbytes=1, backend=backend)
+    p(delayed(check_memmap)(a) for a in [np.random.random(10)] * 2)
+
+    assert len(os.listdir(tmpdir)) == 0


assert not os.listdir(tmpdir)

aabadie · 2017-05-17T14:49:55Z

setup.cfg

    --doctest-glob="doc/*.rst"
    --doctest-modules
-testpaths = joblib
+testpaths = joblib/test


not sure we want to change this.

aabadie · 2017-05-26T16:12:03Z

joblib/disk.py

+                    err_count += 1
+                    if err_count > RM_SUBDIRS_N_RETRY:
+                        warnings.warn(
+                            "Enable to delete folder {} after {} tentatives."


s/Enable/Unable/

aabadie · 2017-05-26T16:15:21Z

There's still an issue when removing a directory on Windows. It seems that there's still a file open in the directory and Windows prevents that kind of things...

ogrisel · 2017-06-01T13:53:02Z

Do you have some benchmarks compared to multiprocessing/threading ?

I ran various tests on a big rackspace machine (sklearn grid searches) and observed no performance regression over multiprocessing.

ogrisel

LGTM, the [WIP] should be updated to [MRG].

ogrisel · 2017-06-02T11:59:11Z

Makefile


 test-no-multiprocessing:
-	export JOBLIB_MULTIPROCESSING=0 && pytest joblib
+	export JOBLIB_MULTIPROCESSING=0 && pytest joblib -c setup.cfg


The "-c setup.cfg" is no longer necessary.

ogrisel · 2017-06-02T12:07:58Z

Please re-run the vendor_cloudpickle.sh script as part of this PR to get the 0.3.1 release that was released yesterday or the day before.

ogrisel · 2017-06-02T13:44:00Z

@tomMoral Please add a new test to check that the (default) locky backend accepts nested parallelism (without warnings).

tomMoral · 2017-06-02T13:57:34Z

@ogrisel : test nested loky with no warning added.
@aabadie : The error with the windows memmaps release have been fixed.
Let me know if you see other parts that need fixes.

aabadie · 2017-06-02T14:01:32Z

Fine with me. Merge when you want :)

- This causes crashes with the new backend because some constants gets lost. - It also makes sense to avoid mixing this 2 backends has the results might be unexepected.

to leave this file unmodified

when running tests from the joblib root foler and joblib is not installed

since it not used anywhere

ogrisel · 2017-08-11T11:06:47Z

@lesteve I addressed the remaining comments and rebased to benefit from the merge of #541.

lesteve · 2017-08-12T06:09:39Z

OK LGTM, merging, great stuff!

GaelVaroquaux · 2017-09-04T12:47:51Z

Just saw that. Awesome!!

ogrisel added the in progress label Apr 13, 2017

ogrisel mentioned this pull request Apr 13, 2017

[WIP] PR to debug loky master integration in joblib #503

Closed

5 tasks

ogrisel changed the title ~~Integrate loky as default backend~~ [WIP] Integrate loky as default backend Apr 14, 2017

tomMoral force-pushed the loky_integration branch 7 times, most recently from 804255e to 50953d0 Compare May 13, 2017 21:26

aabadie suggested changes May 17, 2017

View reviewed changes

aabadie reviewed May 26, 2017

View reviewed changes

tomMoral force-pushed the loky_integration branch 3 times, most recently from 3353076 to 653ffc2 Compare June 2, 2017 11:56

ogrisel approved these changes Jun 2, 2017

View reviewed changes

tomMoral force-pushed the loky_integration branch from 653ffc2 to 99158ef Compare June 2, 2017 12:05

tomMoral force-pushed the loky_integration branch from 8b9d967 to ad7d4e1 Compare June 2, 2017 12:40

ogrisel changed the title ~~[WIP] Integrate loky as default backend~~ [MRG] Integrate loky as default backend Jun 2, 2017

ogrisel changed the title ~~[MRG] Integrate loky as default backend~~ [MRG+1] Integrate loky as default backend Jun 2, 2017

aabadie approved these changes Jun 2, 2017

View reviewed changes

tomMoral and others added 16 commits August 11, 2017 11:40

FIX disable nested multiprocessing in loky

eaec888

- This causes crashes with the new backend because some constants gets lost. - It also makes sense to avoid mixing this 2 backends has the results might be unexepected.

TST improve nested warning tests

1f0ed88

FIX threading warning handeling

1f2aa8a

MAINT make it easier to test the development branch of loky

6797f61

ENH vendor loky 1.1.3

b05be34

Update to loky 1.1.4

6a7b5bd

MAINT setup.cfg should be used as default config file for flake8

6c93c08

DOC memmaping => memmapping

ac08469

DOC fix use_shared_mem variable docstring

7bca86b

STYLE Cosmetics

0625632

Typo

ec942f4

TST improve interactively defined function tests

2741e74

ENH simplify callback aware job insertion + fixed typos

57f9737

ENH further simplification of job handling

f0d5909

FIX flake8 useless imports

8f3fb9f

TST improve test_invalid_njobs

b2b31c9

ogrisel force-pushed the loky_integration branch from 8faf4c6 to eb98b5c Compare August 11, 2017 09:41

lesteve added 4 commits August 11, 2017 12:37

Cosmit

372c582

to leave this file unmodified

Tweak test

edd6428

when running tests from the joblib root foler and joblib is not installed

Remove BatchCompletionCallBack .done attribute

38dcb0b

since it not used anywhere

Tweak comment

fe3f2e3

ogrisel force-pushed the loky_integration branch from eb98b5c to fe3f2e3 Compare August 11, 2017 10:37

TST remove useless warning config in tests

7502017

ogrisel force-pushed the loky_integration branch from 690d9a8 to 7502017 Compare August 11, 2017 10:47

lesteve merged commit e7491c8 into joblib:master Aug 12, 2017

TomDLT mentioned this pull request Mar 13, 2018

Bug when passing a function as parameter in a delayed function #643

Closed

tomMoral deleted the loky_integration branch June 20, 2018 13:17

		return (loads, (dumps(a, protocol=HIGHEST_PROTOCOL),))


		def delete_folder(folder_path):

Conversation

tomMoral commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Apr 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

ogrisel commented Apr 14, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tomMoral commented May 14, 2017

Uh oh!

aabadie left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aabadie commented May 26, 2017

Uh oh!

ogrisel commented Jun 1, 2017

Uh oh!

ogrisel left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ogrisel commented Jun 2, 2017

Uh oh!

ogrisel commented Jun 2, 2017

Uh oh!

tomMoral commented Jun 2, 2017

Uh oh!

aabadie commented Jun 2, 2017

Uh oh!

ogrisel commented Aug 11, 2017

Uh oh!

lesteve commented Aug 12, 2017

Uh oh!

GaelVaroquaux commented Sep 4, 2017 via email

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

tomMoral commented Apr 13, 2017 •

edited

Loading

codecov bot commented Apr 13, 2017 •

edited

Loading

ogrisel commented Apr 14, 2017 •

edited

Loading

ogrisel left a comment •

edited

Loading