8000 Instantiate the multiprocessing pool using `with` by ttung · Pull Request #1436 · spacetx/starfish · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Instantiate the multiprocessing pool using with #1436

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jul 9, 2019
Merged

Instantiate the multiprocessing pool using with #1436

merged 1 commit into from
Jul 9, 2019

Conversation

ttung
Copy link
Collaborator
@ttung ttung commented Jul 4, 2019

This ensures we shut down the pool when complete.

Test plan: pytest -v -n4 starfish/core/spots/
Fixes: #1435

@ttung ttung requested a review from shanaxel42 July 4, 2019 00:01
@ttung ttung force-pushed the tonytung-pool-with branch from 67dedce to 984eba3 Compare July 4, 2019 00:01
@codecov-io
Copy link
codecov-io commented Jul 4, 2019 •
edited
Loading

Codecov Report

Merging #1436 into master will decrease coverage by 0.03%.
The diff coverage is 83.33%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1436      +/-   ##
==========================================
- Coverage   89.49%   89.45%   -0.04%     
==========================================
  Files         155      155              
  Lines        5597     5597              
==========================================
- Hits         5009     5007       -2     
- Misses        588      590       +2
Impacted Files Coverage Δ
.../spots/_detect_pixels/combine_adjacent_features.py 84.54% <83.33%> (ø) ⬆️
starfish/core/multiprocessing/pool.py 82.14% <0%> (-7.15%) ⬇️

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 c4f9eb5...31fe190. Read the comment docs.

Instantiate the multiprocessing pool using with
31fe190 
This ensures we shut down the pool when complete.

Test plan: `pytest -v -n4  starfish/core/spots/`
Fixes: #1435
@ttung ttung force-pushed the tonytung-pool-with branch from 984eba3 to 31fe190 Compare July 8, 2019 17:45
ambrosejcarr
ambrosejcarr approved these changes Jul 9, 2019
View reviewed changes
Copy link
Member
@ambrosejcarr ambrosejcarr left a comment

Choose a reason for hiding this comment

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

Really good improvement. This fixes some notebook issues I've had where I ctrl+c a multiprocessing-enabled function and eventually run oom. :-)

I think my problems count as user error, but nice to mitigate nonetheless!

ambrosejcarr
ambrosejcarr approved these changes Jul 9, 2019
View reviewed changes
Copy link
Member
@ambrosejcarr ambrosejcarr left a comment

Choose a reason for hiding this comment

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

Really good improvement. This fixes some notebook issues I've had where I ctrl+c a multiprocessing-enabled function and eventually run oom. :-)

I think my problems count as user error, but nice to mitigate nonetheless!

@berl
Copy link
Collaborator
berl commented Jul 9, 2019 •
edited
Loading

+many
I need this to run a loop over multiple FOVs. It fills up my system volume and errors out with [Errno 28] No space left on device

this looks like a relevant related issue- the with ... should fix it anyway.

@ttung ttung merged commit 5842aca into master Jul 9, 2019
@ttung ttung deleted the tonytung-pool-with branch July 9, 2019 23:27
@ttung ttung restored the tonytung-pool-with branch July 9, 2019 23:31
@ttung ttung deleted the tonytung-pool-with branch July 10, 2019 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ambrosejcarr ambrosejcarr ambrosejcarr approved these changes

@shanaxel42 shanaxel42 Awaiting requested review from shanaxel42

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

multiprocessing pool processes spawned within combine_adjacent_features persist till parent process terminates
4 participants
@ttung @codecov-io @berl @ambrosejcarr
0