Skip to content

Conversation

tomMoral
Copy link
Contributor

Fix for #674

This PR introspects the BatchCalls and pickles functions and class methods from __main__ using cloudpickle. This is one solution that we discussed with @ogrisel . However, it does not handle deep object introspection. For instance, this would fail if args contains a list of functions that are defined in __main__.

One solution would be to use a try..except when launching the job but I am not sure it is possible as the pickling is handled in a separate thread at least for the ProcessPoolExecutor.
Another solution would be to implement a custom reducer for the function but it might not be possible to fall back on default function pickling in this case.

@codecov
Copy link

codecov bot commented May 10, 2018

Codecov Report

Merging #677 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #677      +/-   ##
=========================================
+ Coverage   95.08%   95.2%   +0.12%     
=========================================
  Files          39      39              
  Lines        5427    5462      +35     
=========================================
+ Hits         5160    5200      +40     
+ Misses        267     262       -5
Impacted Files Coverage Δ
joblib/parallel.py 98.8% <100%> (+0.08%) ⬆️
joblib/test/test_parallel.py 96.39% <100%> (+0.43%) ⬆️
joblib/test/test_memory.py 98.16% <0%> (+0.36%) ⬆️
joblib/_parallel_backends.py 94.39% <0%> (+0.43%) ⬆️

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 cd81f88...51e3b62. Read the comment docs.

@tomMoral
Copy link
Contributor Author

The failure in CI should be fixed with #685 .

I noticed today that the pickling of function in main might be very long compared to the pickling of a function in another module. It can drastically degrade the performance for very large number of small tasks. I think this should be benchmarked and documented in this PR to make sure we are not introducing bottleneck in the computations.

@@ -179,15 +188,33 @@ def __call__(self):
def __len__(self):
return self._size

@staticmethod
def _wrap_non_picklable_objects(obj):
Copy link
Contributor

Choose a reason for hiding this comment

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

Please insert a comment explaining why we do this shallow introspection of objects instead of using the cloudpickle.Pickler class directly to pickle everything.

# The following does a shallow introspection of the Python objects passed as
# args to a parallel call. Interactively defined functions are pre-pickled manually
# using cloudpickle.
# We do not use cloudpickle for all arguments as it would incur a significant
# performance overhead when pickling large cardinality dicts or lists for
# instance.

def _wrap_non_picklable_objects(obj):
need_wrap = "__main__" in getattr(obj, "__module__", "")
if callable(obj):
# Need wrap if the object is a local function
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you please explain what a "local function" is?

print(Parallel(n_jobs=2, backend=backend)(
delayed(run)(square, i) for i in range(5)))
print(Parallel(n_jobs=2, backend=backend)(
delayed(run)(f=square, x=i) for i in range(5)))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please add a test with a lambda expression.

@@ -868,6 +904,11 @@ def __call__(self, iterable):
self.n_dispatched_batches = 0
self.n_dispatched_tasks = 0
self.n_completed_tasks = 0
# Use a caching dict for callable that are pickled with cloudpickle to
Copy link
Contributor

Choose a reason for hiding this comment

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

callables

# Use a caching dict for callable that are pickled with cloudpickle to
# improve performances. This cache is used only in the case of
# functions that are defined in the __main__ module, function that are
# defined locally (inside another function) and lambda expression.
Copy link
Contributor

Choose a reason for hiding this comment

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

lambda expressions.

@@ -868,6 +904,11 @@ def __call__(self, iterable):
self.n_dispatched_batches = 0
self.n_dispatched_tasks = 0
self.n_completed_tasks = 0
# Use a caching dict for callable that are pickled with cloudpickle to
# improve performances. This cache is used only in the case of
# functions that are defined in the __main__ module, function that are
Copy link
Contributor

Choose a reason for hiding this comment

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

functions that are

@tomMoral tomMoral merged commit 9d52110 into joblib:master May 22, 2018
@tomMoral tomMoral deleted the PR_pickle_main branch May 22, 2018 15:54
yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Jul 28, 2018
* tag '0.12': (116 commits)
  Release 0.12
  typo
  typo
  typo
  ENH add initializer limiting n_threads for C-libs (joblib#701)
  DOC better parallel docstring (joblib#704)
  [MRG] Nested parallel call thread bomb mitigation (joblib#700)
  MTN vendor loky2.1.3 (joblib#699)
  Make it possible to configure the reusable executor workers timeout (joblib#698)
  MAINT increase timeouts to make test more robust on travis
  DOC: use the .joblib extension instead of .pkl (joblib#697)
  [MRG] Fix exception handling in nested parallel calls (joblib#696)
  Fix skip test lz4 not installed (joblib#695)
  [MRG] numpy_pickle:  several enhancements (joblib#626)
  Introduce Parallel.__call__ backend callbacks (joblib#689)
  Add distributed on readthedocs (joblib#686)
  Support registration of external backends (joblib#655)
  [MRG] Add a dask.distributed example (joblib#613)
  ENH use cloudpickle to pickle interactively defined callable (joblib#677)
  CI freeze the version of sklearn0.19.1 and scipy1.0.1 (joblib#685)
  ...
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