search

bids-apps / freesurfer

BIDS app wrapping recon-all from FreeSurfer
Apache License 2.0
40 stars 35 forks source link

Parellelization #58

Open araikes opened 4 years ago

araikes commented 4 years ago

Thanks to those who are updating this (@Shotgunosine, @PeerHerholz, and anyone I didn't notice).

Given that the openmp flag is being passed to recon-all should this also include -parallel (i.e. https://github.com/BIDS-Apps/freesurfer/blob/d51c7e4505c15bd34b3a62be1cb233bdf36c18fb/run.py#L247

Thanks

Shotgunosine commented 4 years ago

Hi @araikes thanks for the suggestion. If I understand their documentation right, that parallel option is for parallelization over subjects. Is that right?

PeerHerholz commented 4 years ago

Depending on the implementation and scope, I think we could also check out nipype's ReconAll implementation as we could nicely build upon the workflow architecture and already supported plugin options.

PeerHerholz commented 4 years ago

I just saw that the -parallel flag was removed in v6.0.1-4 (see corresponding PR here), apparently because of stability issues. However, I think this was related to OpenNeuro and since they don't support running of BIDS apps anymore, we could reintroduce it. However, the symlink problem is still a thing depending on the setup I guess. WDYT?

araikes commented 4 years ago

@Shotgunosine I'm fairly certain that the -parallel flag enables parallel processing of the hemispheres while also using the openmp parallelization. However, if the implementation is switched to nipype, I don't know if there would be noticeable performance gains for most.

Shotgunosine commented 4 years ago

@PeerHerholz I think switching to a nipype backend might be a good thing to try, but would be fairly challenging. We'd need someone with time, skill, and interest to work on that problem. Now that I think about it, I may have had similar race conditions using the parallel flag on our HPC. Unless there is a strong demand for the feature, I'm not inclined to reintroduce it.

Shotgunosine commented 4 years ago

@PeerHerholz If we were going to switch to a nipype backend, would we basically just be looking at creating a wrapper around https://github.com/nipreps/smriprep?

PeerHerholz commented 4 years ago

Hi gang,

sorry I didn't mean to re-route the discussion to a potential nipype implementation, just thought that wrt parallelization (among other things) it's an interesting opportunity. However, I agree with you @Shotgunosine: this would change the entire setup and implementation we have so far. smriprep is obviously great, but I rather thought of it as it's own thing that incorporates ReconAll but also does other things. Thus, I guess if we discuss this further the nipype ReconAll interface would be a good starting point. Of course: always happy to discuss this! That might be a nice hackathon project!?

@araikes could you maybe follow up on @Shotgunosine's point re running into problems on HPC? Did you ever noticed a comparable problems (using other ReconAll implementations than this BIDS app)?

araikes commented 4 years ago

@PeerHerholz and @Shotgunosine,

I can't say that I've noticed race conditions using either non-BIDS-app recon-all or this one (I forked the repository like a year ago and added the flag in locally). I'll do some more testing this/next week and see if it any issues crop up or if it seems to actually improve speed noticeably.

PeerHerholz commented 4 years ago

Thx @araikes, that sounds cool. The level of detail is highly appreciated!

PeerHerholz commented 3 years ago

Hi @araikes,

just wanted to ask if you have any updates here?