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

[sampling] use shared memory for all large arrays #521

Merged
merged 5 commits into from Jun 19, 2017

Conversation

Projects
None yet
4 participants
@cdiener
Member

cdiener commented Jun 9, 2017

Sorry for the delay. Took me quite a while to figure out how to manage class attributes in shared memory with multiprocessing.Pool. This PR moves all of those to shared memory so additional process in the OptGPSampler do not duplicate those anymore. This has the following effects on OptGPSampler:

  1. constant memory use independent of the number of processes
  2. a bit faster sample since the initial memory copy is gone
  3. now allows sampling of models with arbitrary size in memory (as long as you have enough RAM for the built matrices), no more 4GB limit due to pickle constraints
  4. runs much smoother in jupyter now, no idea why though

Also added a section to the docstrings giving the rough order of memory usage.

Benchmark and convergence analysis

Fixes #516.

@cdiener cdiener added the ready label Jun 9, 2017

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 12, 2017

Codecov Report

Merging #521 into devel will decrease coverage by 0.02%.
The diff coverage is 80.76%.

Impacted file tree graph

@@            Coverage Diff            @@
##            devel    #521      +/-   ##
=========================================
- Coverage   69.43%   69.4%   -0.03%     
=========================================
  Files          64      64              
  Lines        8476    8494      +18     
  Branches     1447    1451       +4     
=========================================
+ Hits         5885    5895      +10     
- Misses       2334    2339       +5     
- Partials      257     260       +3
Impacted Files Coverage Δ
cobra/flux_analysis/sampling.py 86.95% <80.76%> (-2.67%) ⬇️

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 20519a5...7010fc7. Read the comment docs.

codecov-io commented Jun 12, 2017

Codecov Report

Merging #521 into devel will decrease coverage by 0.02%.
The diff coverage is 80.76%.

Impacted file tree graph

@@            Coverage Diff            @@
##            devel    #521      +/-   ##
=========================================
- Coverage   69.43%   69.4%   -0.03%     
=========================================
  Files          64      64              
  Lines        8476    8494      +18     
  Branches     1447    1451       +4     
=========================================
+ Hits         5885    5895      +10     
- Misses       2334    2339       +5     
- Partials      257     260       +3
Impacted Files Coverage Δ
cobra/flux_analysis/sampling.py 86.95% <80.76%> (-2.67%) ⬇️

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 20519a5...7010fc7. Read the comment docs.

@hredestig

Very nice! And educational to review. Some small discretionary comments.

variable_bounds=shared_np_array(var_bounds.shape, var_bounds),
projection=shared_np_array(projection.shape, projection),
homogeneous=homogeneous
)
def generate_fva_warmup(self):
"""Generate the warmup points for the sampler.

This comment has been minimized.

@hredestig

hredestig Jun 13, 2017

Contributor

some stale parameters listed in the docstring, I guess better remove those

@hredestig

hredestig Jun 13, 2017

Contributor

some stale parameters listed in the docstring, I guess better remove those

This comment has been minimized.

@cdiener

cdiener Jun 13, 2017

Member

Sure will check.

@cdiener

cdiener Jun 13, 2017

Member

Sure will check.

@@ -56,6 +56,40 @@
"""
def mp_init(obj):
"""Initialize the multiprocessing pool."""
global sampler

This comment has been minimized.

@hredestig

hredestig Jun 13, 2017

Contributor

since it is to be looked up at the module level, perhaps also should be defined there?

@hredestig

hredestig Jun 13, 2017

Contributor

since it is to be looked up at the module level, perhaps also should be defined there?

This comment has been minimized.

@cdiener

cdiener Jun 13, 2017

Member

Only needed for OptGP and I didn't want to pollute the module name space. But no strong feelings either way so whatever you prefer.

@cdiener

cdiener Jun 13, 2017

Member

Only needed for OptGP and I didn't want to pollute the module name space. But no strong feelings either way so whatever you prefer.

Show outdated Hide outdated cobra/flux_analysis/sampling.py Outdated
Show outdated Hide outdated cobra/flux_analysis/sampling.py Outdated
@cdiener

This comment has been minimized.

Show comment
Hide comment
@cdiener

cdiener Jun 14, 2017

Member

Okay added your suggestions @hredestig. Hope it's okay for you 😄

Member

cdiener commented Jun 14, 2017

Okay added your suggestions @hredestig. Hope it's okay for you 😄

@hredestig

Yep, all good, cheers!

chains = mp.map(
_sample_chain,
zip([self] * self.np, [n_process] * self.np, range(self.np)),
zip([n_process] * self.np, range(self.np)),
chunksize=1)

This comment has been minimized.

@Midnighter

Midnighter Jun 15, 2017

Member

Have you played with imap_unordered and a larger chunksize at some point? Or is the communication cost negligible compared to in-process computation time?

@Midnighter

Midnighter Jun 15, 2017

Member

Have you played with imap_unordered and a larger chunksize at some point? Or is the communication cost negligible compared to in-process computation time?

This comment has been minimized.

@cdiener

cdiener Jun 15, 2017

Member

So, no pool in the process is ever recycled. A pool of size n will receive exactly n problems to sample, so imap_unordered does not really make sense IMO. A chunksize of one guarantees that each process will run one sampling chain otherwise it would theoretically be possible that a process received two chains and another none (unlikely).

@cdiener

cdiener Jun 15, 2017

Member

So, no pool in the process is ever recycled. A pool of size n will receive exactly n problems to sample, so imap_unordered does not really make sense IMO. A chunksize of one guarantees that each process will run one sampling chain otherwise it would theoretically be possible that a process received two chains and another none (unlikely).

This comment has been minimized.

@Midnighter

Midnighter Jun 15, 2017

Member

So what's a typical size for n? My strategy usually is to create a pool of size equal to the number of processors and not equal to the size of the problem. Maybe those numbers are equal here, I may have overlooked that.

@Midnighter

Midnighter Jun 15, 2017

Member

So what's a typical size for n? My strategy usually is to create a pool of size equal to the number of processors and not equal to the size of the problem. Maybe those numbers are equal here, I may have overlooked that.

This comment has been minimized.

@cdiener

cdiener Jun 15, 2017

Member

The n is user supplied. Sometimes you might not want to block the entire system for the sampling (e.g. shared server). So we let the user pick. The idea is that the problem is really easy to divide into completely independent sub-problems so you can create the optimal workflow for any number of processes easily.

@cdiener

cdiener Jun 15, 2017

Member

The n is user supplied. Sometimes you might not want to block the entire system for the sampling (e.g. shared server). So we let the user pick. The idea is that the problem is really easy to divide into completely independent sub-problems so you can create the optimal workflow for any number of processes easily.

cdiener added some commits Jun 15, 2017

@hredestig hredestig merged commit 1bd2ea8 into opencobra:devel Jun 19, 2017

4 checks passed

codecov/patch 80.76% of diff hit (target 69.43%)
Details
codecov/project Absolute coverage decreased by -0.02% but relative coverage increased by +11.33% compared to 20519a5
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@hredestig hredestig removed the ready label Jun 19, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment