search

pnlbwh / CNN-Diffusion-MRIBrain-Segmentation

CNN based brain masking
Other
14 stars 10 forks source link

Uncontrolled multiprocessing is in place in this program!! #39

Closed tashrifbillah closed 2 months ago

tashrifbillah commented 9 months ago

This block is not connected to args.cr:

https://github.com/pnlbwh/CNN-Diffusion-MRIBrain-Segmentation/blob/8caf97fc6b423d778c0b6b9988f50eb1825b1604/pipeline/dwi_masking.py#L627-L675

https://github.com/pnlbwh/CNN-Diffusion-MRIBrain-Segmentation/blob/8caf97fc6b423d778c0b6b9988f50eb1825b1604/pipeline/dwi_masking.py#L571

Hence, it spawns as many processes as the number of cases in caselist.txt. This will blow out the CPU. It was not discovered so far because test pipeline included in this project has only three cases. Tashrif discovered it while trying to utilize its inherent looping facility over 220 cases. All 220 cases spawned uncontrollably!!

It needs to be fixed using a standard multiprocessing coding e.g. https://github.com/pnlbwh/TBSS/blob/2fb1bc776b07013b450c24057db876486fc4bb2b/lib/tbss_single.py#L112-L121

tashrifbillah commented 9 months ago

cc @RyanZurrin

RyanZurrin commented 9 months ago

Hello Tashrif,

I can take a look at this and help come up with a fix later this week, maybe tomorrow, if not then Thrusday and Frideay for sure.

If you fix it before that let make know otherwise I will reach out to you soon.

RyanZurrin commented 5 months ago

I have updated the code section to use a Pool, and it is now connected to the -nproc flag, which gets saved as args.cr. I have successufully tested with a few subjects setting nproc to 1 and 2 and it would only allow that many processes to spawn at any given time. I will create a pull request with this fix.

tashrifbillah commented 2 months ago
  1. Tashrif changed the inappropriate name cr to nproc.
  2. Tashrif also created a block that working with nproc=1. This new block helps when debugging is needed.