FIX Missing num_threads in some HGBT pranges

[jeremiedbb]

Probably the reason of the performance issue reported in INRIA/scikit-learn-mooc#586

in for thread_idx in prange(n_threads, schedule='static', chunksize=1), n_threads is the number of tasks but the number of threads stays unspecified. Like this, the number of tasks might be smaller than the number of threads, but openmp will still spawn all threads even if some of them are idle.

[ogrisel]

Thanks, I wanted to test if that would fix the performance problem on my local machine but unfortunately, I found another bug :)

I reported it here: joblib/loky#354. It should be quite easy to fix.

[ogrisel]

FYI I am currently building this branch in my user container the MOOC platform to give it a try.

[ogrisel]

LGTM! I confirm this fixed the over-subscription problem observed on the MOOC platform!

Please add a changelog entry to document the fix.

[lesteve]

Does that mean that each time prange is used it should be using num_threads or be called within a with cython.parallel(num_threads=...) block?

[jeremiedbb]

Does that mean that each time prange is used it should be using num_threads or be called within a with cython.parallel(num_threads=...) block?

I think so because the num_threads we are passing comes from _openmp_effective_n_threads which takes cgroups quota into account.

[lesteve]

OK, I am not 100% sure but it seems like there are other places where we don't do that, e.g.

scikit-learn/sklearn/cluster/_k_means_elkan.pyx

Line 84 in 998e8f2

for i in prange(n_samples, schedule='static', nogil=True):

This is not using prange(num_threads=...) I tried to follow the code and I don't think this function is called within a cython.parallel(num_thread=...) but I may be wrong. There may be more places.

I was looking visually at git grep with context to try to find potential problems:

git grep -n -C3 prange

[jeremiedbb]

Yes this one should also set num_threads. Let's do that in another PR since this one is specific to HGBT

[lorentzenchr]

Seems to be the right thing to do, but couldn't test it.

[lesteve]

Merging thanks!