Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove multiprocessing code #2472

Merged
merged 1 commit into from Oct 17, 2019
Merged

Remove multiprocessing code #2472

merged 1 commit into from Oct 17, 2019

Conversation

@cdeil
Copy link
Member

cdeil commented Oct 17, 2019

This PR removes the use of multiprocessing Pool in Gammapy.

The reason for this is that multiprocessing isn't reliable, and acutely we had the issue of crashes on macOS: #2453

It's not really clear if CPython or matplotlib or Apple is to blame for these crashes, but overall I think the situation can be characterised like this: multiprocessing mostly works, but sometimes doesn't on some platforms and Python and library versions, and it is tricky to make it work and get good performance consistently. That's why e.g. scikit-learn has written their on replacement, loky and joblib, because they have to not crash and deliver good performance consistently (see https://youtu.be/UVL4LFy8ch0).

@adonath and I discussed whether we should attempt to change to loky or something else like numba or dask or ray or cython now, but agreed to just remove multiprocessing for now (a few line change), and then to bring parallel processing back later, once we have benchmarks and working analyses.

Note that currently we're using multiprocessing for ring convolution and for TS map computation, where really our most CPU-intensive tasks are data reduction and modeling & fitting. When we re-introduce parallelism in Gammapy, we should introduce it for those tasks where it's most relevant first.

@cdeil cdeil added the cleanup label Oct 17, 2019
@cdeil cdeil added this to the 0.15 milestone Oct 17, 2019
@cdeil cdeil self-assigned this Oct 17, 2019
@cdeil cdeil force-pushed the cdeil:rm-mp branch from a6cba16 to 4c66f66 Oct 17, 2019
@cdeil cdeil assigned adonath and unassigned cdeil Oct 17, 2019
@cdeil

This comment has been minimized.

Copy link
Member Author

cdeil commented Oct 17, 2019

@adonath - Please review.

I'm not familiar with that code - maybe there's a quick way to write it in a clearer or more efficient way, using a for loop?

Performance for the detect tutorials looks acceptable to me:

NFO:gammapy.scripts.jupyter:   ... EXECUTING: temp/detect_ts.ipynb
INFO:gammapy.scripts.jupyter:   ... Executing duration: 16.0 seconds

Maybe just merge as-is and move on?

@cdeil cdeil assigned cdeil and unassigned adonath Oct 17, 2019
@cdeil cdeil merged commit 8a52d10 into gammapy:master Oct 17, 2019
9 checks passed
9 checks passed
Codacy/PR Quality Review Up to standards. A positive pull request.
Details
Scrutinizer Analysis: No new issues – Tests: passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
gammapy.gammapy Build #20191017.21 succeeded
Details
gammapy.gammapy (DevDocs) DevDocs succeeded
Details
gammapy.gammapy (Lint) Lint succeeded
Details
gammapy.gammapy (Test Python36) Test Python36 succeeded
Details
gammapy.gammapy (Test Windows36) Test Windows36 succeeded
Details
gammapy.gammapy (Test Windows37) Test Windows37 succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.