Add pool decorator to functions #489
Conversation
|
the decorator does nicely streamline the code! any ideas on a potential tests to add for the |
|
I think the new tests can be made more concise-- e.g. for each tool can just refactor to keep the insulation.equals(insulation_pooled2) inside of existing earlier test to minmize the boilerplate code |
|
note this would close #431 |
|
hi @sergpolly & @nvictus -- could either of you check out this PR? Thanks! |
There was a problem hiding this comment.
The pool decorator is a great idea and this overall looks good to me! Just one important thing missing.
As it stands right now, applying @pool_decorator to a function removes the flexibility that the map= parameter provides in the first place, which is to support alternative and third party map functors (e.g. from concurrent.futures or loky). This decorator forces the function to always use multiprocess.Pool.imap or builtins.map when nproc is 1.
The decorator should provide a sane default, like imap (or could even be parameterized in the future @pool_decorator("imap_unordered")). But it should allow the client to explicitly pass in map=<my_custom_functor> which will override the defaults and ignore nproc.
… 3. add explicit docstring to pool_decorator 4. all pool.terminate() 5. support third-party map functor
|
these changes look great @Yaoyx ! two qs:
e.g. @pool_decorator
def cooltools_function_to_decorate(
clr, ...
nproc=None,
map_functor=my_custom_map_func,
):
|
... no strong opinion ... I think, good docs would be sufficient - sort of like - "as soon as you start messing with the
in the same vein - I'd say, ultimately, provide the convenience of |
|
oh , i just looked at the overall code - and there is 1 more tiny thing - I think it would be also nice to clean up all the remaining |
There was a problem hiding this comment.
let's try to have the same defnition for the nproc & map_functor arguments across functions. I propose:
nproc : int, optional
How many processes to use for calculation. Ignored if map_functor is passed.
map_functor : callable, optional
Map function to dispatch the matrix chunks to workers. If left unspecified, pool_decorator applies the following defaults: if nproc>1 this defaults to multiprocess.Pool; If nproc=1 this defaults the builtin map.
does this clear @sergpolly ?
|
yes, I think, it is clear The last thing on my side, @Yaoyx - i've noticed few remaining |
|
ok to merge now @nvictus ? |
There was a problem hiding this comment.
Other than the docstring comment, this LGTM. One issue is that the nproc parameters lingering in the wrapped functions aren't necessary because nproc is provided by the decorator and doesn't get passed down to the wrapped function. We can consider removing them in the future, but maybe retaining them in docstrings.
|
how would you then suggest CLI compatibility @nvictus ? could the map_functor argument be an int in which case the decorator interprets it as |
|
Yeah, assuming the CLI is not customizing the map implementation, you only need to expose a parameter to set the number of processes. |
|
I like it! That would make the arguments a little cleaner by not having 2 arguments to control 1 behavior... Though providing an int to an argument named map_functor could be a bit confusing... any suggestions? |
|
@gfudenberg a function wrapped into pool_decorator - has both nproc and map-functor, but the function being wrapped does not have to have nproc as a kwargs, only map_functor - I think this was Nezar point ... At least this is how understand it now - took time to understand the interplay of the wrapper and the wrapped |
|
yes, this is now my understanding too @sergpolly -- so I'm wondering if @pool_decorator
def cooltools_function_to_decorate(
clr,
... other args,
map_functor=1 or int or my_custom_map_func(),
):would be clear, or if this would make the map_functor argument confusing (or better to rename it something else) |
|
Hm , I don't know if passing map_functor=8 or int would improve readability... IMHO. I very much like the current state + Nezars suggestion - the way I understand it is
The only part missing for me now - is how do you organize docstrings... - cooltools_func drops nproc as kwarg , together with the docstrings for it - ok, and then what ? Do we specify docstring for nproc in the wrapper ? Can one combine the docstrings of wrapper the wrapped up in a nice way ? |
|
@gfudenberg I'm still a bit confused about this nproc/map_functor thing ... am I missing something obvious ? or do you agree with the previous comment ? |
|
sorry for the confusion @sergpolly !
|
|
perfect ! it is nice to have a consensus ! i'm sure there is some python/decorator/wrapping magic that would enable docstring combination |
* add pool decorator to previous functions * Fix pool_decorator * add pool_decorator to dotfinder.py and expected.py * combined import command for pool_decorator with others * add tests for pooled functions to check if outputs are the same as using regular map * changed lambda func to a def func * try to set start method for multiprocessing * test pool.map insteand of pool.imap * switch multiprocessing to multiprocess * swith to multiprocess * change job back to lambda * change transforms back to lambda function * streamlined tests for multiprocess * move multiprocess code from cli/coverage and cli/sample to api * add nproc to test_sample and reduce the random sampling size * decrease test_sample sampling value * change frac to 0.1 in test_sample * regulate tolerance in test_sample * 1.move logging into decorator 2. use map_functor as the argument name 3. add explicit docstring to pool_decorator 4. all pool.terminate() 5. support third-party map functor * remove unused imports * remove unused imports in cli * unified definitions of nproc and map_functor * Update pool_decorator docstring --------- Co-authored-by: Nezar Abdennur <nabdennur@gmail.com>
No description provided.