search

marbl / merqury

k-mer based assembly evaluation
Other
272 stars 19 forks source link

Meryl-lookup runs single threaded in phase_block regardless of thread count #50

Closed ASLeonard closed 3 years ago

ASLeonard commented 3 years ago

Hi Arang, I noticed that the different instances of meryl-lookup across merqury run single core, even when env params are set like OMP_NUM_THREADS. I tried manually running the scripts with the meryl-lookup explicitly set with the -threads param it can take, and the results did run parallel and had the same md5sum, so I believe this is a valid behaviour.

I've come across meryl issues in the past (#12) because I was using the meryl tip version, but I was able to recreate this with the meryl tip and v1.3. There is a chance it is just something on my end, but I tried to be as thorough as possible.

I did some testing, and the threading is explicitly set to 1 for the meryl-lookup operation (maybe because of AS_configure in here), while the meryl print operation in the same phase_block file correctly runs with the OMP_NUM_Threads value.

Thanks, Alex

brianwalenz commented 3 years ago

For such a simple bug, the phenotype is surprisingly complicated.

It's only affecting OMP_NUM_THREADS. Using any other environment variable (e.g., SLURM_JOB_CPUS_PER_NODE) to set the number of threads to use works as expected.

Meryl isn't affected because it queries the number of threads allowed before AS_configure() is called, while meryl-lookup calls AS_configure() first.

I think the correct fix is to remove omp_set_num_threads(1) from AS_configure(). This was added to disable OpenMP parallelization of std::sort, but we're now doing that through a different mechanism. For meryl/merqury removing this should be safe; it was canu that had problems here.

ASLeonard commented 3 years ago

Great, thanks Brian.

I commented out that line from AS_configure, and the threading now responds to the OMP variable without the need for the explicit -threads param. Merqury back to being super quick!