Skip to content

Conversation

ogrisel
Copy link
Contributor

@ogrisel ogrisel commented Jun 15, 2018

When doing nested parallel calls, all exception would be mapped to the generic JoblibException therefore losing the original exception type information (e.g. ValueError).

Furthermore, Python 3 already has a builtin __cause__ exception nesting system that is used by default to trace exception that occurred on a remote worker process. The combination of the Python 3 __cause__ nesting and the Joblib system yields overwhelmingly verbose and redundant exception reports.

To fix those two problems, we:

  • disable useless exception wrapping under Python 3 to only rely on the built-in exception nesting mechanism;
  • fix nested exception wrapping under Python 2 to preserve type information.

@ogrisel ogrisel force-pushed the fix-nested-exceptions branch from 4f95bdc to 37e9d1e Compare June 15, 2018 14:49
@joblib joblib deleted a comment from codecov bot Jun 15, 2018
@codecov
Copy link

codecov bot commented Jun 15, 2018

Codecov Report

Merging #696 into master will decrease coverage by 0.04%.
The diff coverage is 98.14%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #696      +/-   ##
==========================================
- Coverage   95.12%   95.08%   -0.05%     
==========================================
  Files          40       40              
  Lines        5663     5692      +29     
==========================================
+ Hits         5387     5412      +25     
- Misses        276      280       +4
Impacted Files Coverage Δ
joblib/format_stack.py 83.25% <0%> (ø) ⬆️
joblib/test/test_my_exceptions.py 89.28% <100%> (ø) ⬆️
joblib/parallel.py 97.39% <100%> (-0.03%) ⬇️
joblib/my_exceptions.py 98.11% <100%> (+0.19%) ⬆️
joblib/_parallel_backends.py 94.49% <100%> (-2.09%) ⬇️
joblib/test/test_parallel.py 96.41% <100%> (+0.32%) ⬆️
joblib/backports.py 93.75% <0%> (-2.09%) ⬇️
joblib/_store_backends.py 90.47% <0%> (-0.53%) ⬇️
... and 1 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 da01924...d90ce4b. Read the comment docs.

@ogrisel ogrisel force-pushed the fix-nested-exceptions branch from 84446f9 to a7fc8f4 Compare June 15, 2018 16:04
ogrisel added 2 commits June 15, 2018 18:31
When doing nested parallel calls, all exception would be mapped to the generic
JoblibException therefore loosing the original exception type information (e.g.
ValueError).

Furthermore Python 3 already has a builtin __cause__ exception nesting system
that is used by default to trace exception that occured on a remote worker
process. The combination of the Python 3 __cause__ nesting and the Joblib
system would yield overwhelmingly verbose and redundant exception reports.

To fix those two problems, we:

- disable useless exception wrapping under Python 3 to only rely on the
  built-in nesting mechanism;
- fix nested exception wrapping under Python 2 to preserve type information.
@ogrisel ogrisel force-pushed the fix-nested-exceptions branch from a7fc8f4 to 2ecb9e1 Compare June 15, 2018 16:32
@ogrisel ogrisel changed the title Fix exception handling in nested parallel calls [MRG] Fix exception handling in nested parallel calls Jun 15, 2018
@ogrisel ogrisel requested review from GaelVaroquaux and lesteve June 15, 2018 16:53
@ogrisel
Copy link
Contributor Author

ogrisel commented Jun 18, 2018

I had a quick chat with @GaelVaroquaux about this PR. He seemed not opposed to the general idea. As it's very easy to revert in case of a problem, let me merge this to make it part of the next release.

@ogrisel ogrisel merged commit 48ae8f6 into joblib:master Jun 18, 2018
@ogrisel ogrisel deleted the fix-nested-exceptions branch June 18, 2018 13:17
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)
  ...
yarikoptic added a commit to yarikoptic/joblib that referenced this pull request Jul 28, 2018
* releases: (121 commits)
  Release 0.12.1
  fix kwonlydefaults key error in filter_args (joblib#715)
  MNT fix some "undefined name" flake8 warnings (joblib#713).
  from importlib import reload for Python 3 (joblib#675)
  MTN vendor loky2.1.4 (joblib#708)
  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)
  ...
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.

1 participant