[itamarst]

Fixes #299

[itamarst]

Plausibly there's a second half missing, where the top-level restore of threadpoolctl, the one where it relinquishes control, should always be to the original level of the process, because when it grabs the original level it might grab the wrong one as a race condition.

[itamarst]

Or, another way of saying it, if thread A stores values to restore to at a time when thread B is the controlling thread, the restore value is wrong, and should instead be "all cores" (zero? or a level stored at startup). Will do that or equivalent tomorrow.

[itamarst]

The above is a real race condition (I have implemented a test and fix) but it is still not enough to fix the joblib race condition reproducer!

[itamarst]

Will continue trying to figure out the joblib race condition next week.

[itamarst]

@ogrisel ok I think this is ready for review.

[ogrisel]

Some more feedback, this time about API design concerns.

[itamarst]

The reproducer seems like basically the known limitation, that using the imperative API doesn't participate in automatic process? Last time it came up we decided to just document it. And it's why joblib has to increment/decrement manually itself.

[ogrisel]

Yeah sorry, I forgot about that. Let's ignore this part of my review. The second part on the public API could be fixed though. What do you think?

[itamarst]

Yep, on it.

[itamarst]

Note that CI was failing because ubuntu-latest is now 26.04, and LLVM apt repo doesn't LLVM 18. In order to continue testing with LLVM 18 (which is presumably still pretty common) I switched to ubuntu-24.04 explicitly, but could alternatively switch to newer LLVM instead (21, 22, or 23).

[itamarst]

@ogrisel ok back to you

[ogrisel]

Thanks @itamarst for the follow-up. The fix itself LGTM, an maybe is the correct one for a 3.8.1 bug fix release.

However, I have the feeling that our API design makes OpenBLASController leak its peculiarities in too many places:

• in the _set_threadpool_limits method (more details below);
• in joblib itself (notably in _get_threadpoolctl_limits) (see https://github.com/joblib/joblib/pull/1871/changes#r4524197578 in particular).

I think this is a sign we should consider exposing ownership control semantics more explicitly in controllers. WDYT?

[itamarst]

On the one hand it is getting more complicated, yeah. On the other hand this only makes a difference when:

• Using nested Python thread pools.
• The parent thread pool is less than the number of cores.

Which isn't that common outside of scikit-learn, probably?

And then add in the fact that half the time OpenBLAS parallelism slows things down and plausibly a lot of more sophisticated performance-sensitive users will just set the limit to 1 and call it a day (or use MKL if they have complex parallelism).

[itamarst]

Switching to isinstance(..., OpenBLASController) maybe made a bunch of tests fail with FlexiBLAS, or maybe it was just random failures unrelated to that, as there is still one test failing when I reverted.

Probably because with FlexiBLAS you actually set limits twice, once directly on OpenBLAS and once indirectly with FlexiBLAS and they now have conflicting semantics. So will think about it and hopefully fix that today.

[itamarst]

I made the failing test more constrained, so it should hopefully pass even on FlexiBLAS, and just left FlexiBLAS in its previous state of 🤷.

[itamarst]

My basic thought is, to summarize: none of this is perfect but it's probably better than what we have now, and the edge cases (which are real!) probably don't affect that many people. And edge cases are probably no worse than the status quo before 3.8.0.