wright-group / WrightSim

A simulation package for multidimensional spectroscopy.
MIT License
4 stars 0 forks source link

consider loky for CPU parallelization #18

Open untzag opened 6 years ago

untzag commented 6 years ago

loky

among other advantages... no need for if __name__ == "__main__" (on windows machines...)

ksunden commented 6 years ago

Can you explain what you mean by not needing __name__ == "__main__", and why this problem only affects Windows? I see that they claim this in their README, but still don't quite understand what the actual problem is (perhaps this is just me not using windows regularly to know that there is a common problem solved by checking for the main thread)

Right now it seems that the overhead of CPU parallelism is small, and i'm initially inclined to stick with the standard library solution, but am open to arguments to change it. I have not personally had any problem with the current solution.

untzag commented 6 years ago

gosh, I just mentioned this in a comment on PR #20 as well (totally forgetting about loky at that time)

using multiprocessing on windows causes a runaway effect where each child process spawns its own children, and those children have children etc

hiding the multiprocessing step behind if __name__ == '__main__' enforces correct behaviour: only the main process creates children

I do not know why this is only a problem on windows machines, but I can say from personal experience that the effect is real (just try running NISE in mp mode)

for reasons that I also do not understand, I get the impression that the builtin concurrent.futures.ProcessPoolExecutor somehow escapes this problem

it's all over my head right now, so I don't know what the advantages or disadvantages of moving to loky or concurrent.futures might be---I do know that I hate the user-hostile name=main gotcha

we could always switch our CPU multiprocessing implementation conditional of os.name

untzag commented 6 years ago

actually, concurrent.futures.ProcessPoolExecutor has the same problem as multiprocessing: see this example

I think I was getting confused about examples I saw that were using ThreadPoolExecutor (classic rookie mistake)

so yeah, at the moment it seems like loky is the only library that magically escapes the bug---thanks to something called a cloudpickle, apparently

@ksunden if you think it's best to just live with the gotcha that's fine with me---after all this seems to be understood expected behaviour on windows, and it seems foolish to make things vastly more complicated under the hood in the pursuit of a slightly simpler api

if we leave the gotcha in, I think mp should be off by default :disappointed:

ksunden commented 6 years ago

I am starting to understand why this happens, has to do with the way forking is handled. POSIX-like systems have well defined process forking at the kernel level, allowing for an immediate fork. Windows' implementation relies on spawning new instances of the interpreter from scratch. What I'm unclear of is whether this remains a problem if the multiprocessing call is done within layers of function calls in different namespaces. It sound's like you are saying it does present itself as such.

ksunden commented 6 years ago

It is pretty common, and I believe considered standard practice, to surround code in files to be run as scripts with if __name__ == "__main__": This is probably due to exactly this problem (among others, often you will see this line for testing scripts at the bottom of a file which is intended to be imported, for instance, the stuff behind this guard is not run upon import.) This is considered good practice. (Don't always follow it, e.g. the current version of target.py does not include this, and therefore may exhibit this problem if run on windows.)

My current inclination remains to encourage this behavior, as it ensures that users' learn good habits. This is not a particularly firm inclination, however, and I am still mulling over what makes the most sense to me.

Perhaps when I get the chance to run on Windows, I may change my mind...