joblib / loky

Robust and reusable Executor for joblib
http://loky.readthedocs.io/en/stable/
BSD 3-Clause "New" or "Revised" License
520 stars 47 forks source link

FIX make sure max_workers cannot exceed 61 on windows #390

Closed ogrisel closed 1 year ago

ogrisel commented 1 year ago

Fixes #192.

loky.cpu_count never returns more than 61 on Windows.

I the user explicitly ask for more than 61 workers on Windows, a warning is raised and max_workers is automatically set back to the 61 limit.

I added a new test that checks this by trying to spawn 62 workers. I ran the tests locally on windows and macos and it takes around 10-15s to run locally. I hope this will be ok on the CI, otherwise, I can disable it my default on (some) CI environments. Let's see.

ogrisel commented 1 year ago

I tested locally on Python 3.8 on Windows and the non-regression test passes. So I decided to just skip the test and pretend that Python 3.7 no longer exists which should be the case for loky 3.4.0 once we find a way to fix the broken bumped-up CI in #389.

ogrisel commented 1 year ago

Hum I am confused:

ogrisel commented 1 year ago

I checked again my local windows env, and I made a mistake... It was running Python 3.10. Apparently conda activate does not work with the default windows 11 terminal, I had to use "miniforge prompt" instead.

I can reproduce and apparently 60 workers work well (instead of 61) on Python 3.8.

ogrisel commented 1 year ago

I do not understand why codecov complains about the warning not being covered. There is a test for this and it passes on the windows CI:

https://dev.azure.com/joblib/loky/_build/results?buildId=2845&view=logs&j=d8ad72f6-68b5-5f6a-7b90-6a1eb7e08a79&t=81494c5e-5f2d-54b7-88f1-492165c303fc&l=383

ogrisel commented 1 year ago

Ok I believe I understand why codecov is not 100% up to date:

https://dev.azure.com/joblib/loky/_build/results?buildId=2845&view=logs&j=d8ad72f6-68b5-5f6a-7b90-6a1eb7e08a79&t=c96f4d1e-6183-53d9-b1ea-785b54e3e1ec&l=53

I think we can merge. Hopefully this is a transient problem.

ogrisel commented 1 year ago

A quick review? Maybe @tomMoral @jeremiedbb or @thomasjpfan.

ogrisel commented 1 year ago

The windows py311 failure is already in main and documented at #392. Merging.