Repository: joblib/loky

Issue:
loky is broken on windows with max_workers>62
As reported in scikit-learn/scikit-learn#12263, `loky` hangs when using many workers on windows. The following traceback is given:

```python-traceback
Exception in thread QueueManagerThread:

  File "<<elided>>\lib\site-packages\sklearn\externals\joblib\externals\loky\process_executor.py", line 617, in _queue_management_worker
    ready = wait(readers + worker_sentinels)
  File "<<elided>>\lib\multiprocessing\connection.py", line 859, in wait
  File "<<elided>>\lib\multiprocessing\connection.py", line 791, in _exhaustive_wait
    res = _winapi.WaitForMultipleObjects(L, False, timeout)
ValueError: need at most 63 handles, got a sequence of length 129
```

It looks like the probelm is  caused by our implementation of `wait`, which relies on [`winapi.WaitForMultipleObjects`](https://docs.microsoft.com/en-us/windows/desktop/api/synchapi/nf-synchapi-waitformultipleobjects). It seems that this function cannot wait for more than `MAXIMUM_WAIT_OBJECTS=64` objects.

Discussion:
ogrisel: 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: Hum I am confused:

- the failing CI job `test_loky windows-py37` is actually launching the tests with Python 3.8 instead of Python 3.7;
- I cannot reproduce the problem locally with Python 3.8 on Windows... This tests passes on my Windows VM in a Python 3.8 conda env.
ogrisel: 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: 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

Patch:
diff --git a/.azure_pipeline.yml b/.azure_pipeline.yml
index 47178e4..1e2648c 100644
--- a/.azure_pipeline.yml
+++ b/.azure_pipeline.yml
@@ -2,7 +2,7 @@ jobs:
 - job: linting
   displayName: Linting
   pool:
-    vmImage: ubuntu-20.04
+    vmImage: ubuntu-latest
   steps:
     - task: UsePythonVersion@0
       inputs:
@@ -19,46 +19,46 @@ jobs:
   strategy:
     matrix:
 
-      windows-py310:
+      windows-py311:
         imageName: windows-latest
-        python.version: "3.10"
-        tox.env: py310
-      windows-py37:
+        python.version: "3.11"
+        tox.env: py311
+      windows-py38:
         imageName: windows-latest
         python.version: "3.8"
         tox.env: py38
 
-      macos-py310:
+      macos-py311:
         imageName: "macos-latest"
-        python.version: "3.10"
-        tox.env: py310
-      macos-py37:
+        python.version: "3.11"
+        tox.env: py311
+      macos-py38:
         imageName: "macos-latest"
-        python.version: "3.7"
-        tox.env: py37
+        python.version: "3.8"
+        tox.env: py38
 
       linux-pypy3:
-        imageName: "ubuntu-20.04"
+        imageName: "ubuntu-latest"
         python.version: "pypy3"
         tox.env: pypy3
         LOKY_MAX_CPU_COUNT: "2"
 
-      linux-py310:
-        imageName: "ubuntu-20.04"
-        python.version: "3.10"
-        tox.env: py310
+      linux-py311:
+        imageName: "ubuntu-latest"
+        python.version: "3.11"
+        tox.env: py311
       linux-py39-joblib-tests:
-        imageName: "ubuntu-20.04"
+        imageName: "ubuntu-latest"
         python.version: "3.9"
         tox.env: "py39"
         joblib.tests: "true"
       linux-python-py39-high-memory:
-        imageName: "ubuntu-20.04"
+        imageName: "ubuntu-latest"
         python.version: "3.9"
         tox.env: py39
         RUN_MEMORY: "true"
       linux-py38:
-        imageName: "ubuntu-20.04"
+        imageName: "ubuntu-latest"
         python.version: "3.8"
         tox.env: py38
 
diff --git a/CHANGES.md b/CHANGES.md
index 7b56908..0dff38f 100644
--- a/CHANGES.md
+++ b/CHANGES.md
@@ -6,6 +6,10 @@
 - Fix handling of CPU affinity  by using `psutil`'s `cpu_affinity` on platforms
   that do not implement `os.sched_getaffinity`, such as PyPy (#381).
 
+- Fix crash when using `max_workers > 61` on Windows. Loky will no longer
+  attempt to use more than 61 workers on that platform (or 60 depending on the
+  Python version). (#390).
+
 ### 3.3.0 - 2022-09-15
 
 - Fix worker management logic in `get_reusable_executor` to ensure
diff --git a/loky/backend/context.py b/loky/backend/context.py
index af7be78..433d6f5 100644
--- a/loky/backend/context.py
+++ b/loky/backend/context.py
@@ -18,9 +18,15 @@ import warnings
 import multiprocessing as mp
 from multiprocessing import get_context as mp_get_context
 from multiprocessing.context import BaseContext
+from concurrent.futures.process import _MAX_WINDOWS_WORKERS
 
 from .process import LokyProcess, LokyInitMainProcess
 
+# Apparently, on older Python versions, loky cannot work 61 workers on Windows
+# but instead 60: ¯\_(ツ)_/¯
+if sys.version_info < (3, 10):
+    _MAX_WINDOWS_WORKERS = _MAX_WINDOWS_WORKERS - 1
+
 START_METHODS = ["loky", "loky_init_main", "spawn"]
 if sys.platform != "win32":
     START_METHODS += ["fork", "forkserver"]
@@ -88,10 +94,21 @@ def cpu_count(only_physical_cores=False):
     or the LOKY_MAX_CPU_COUNT environment variable. If the number of physical
     cores is not found, return the number of logical cores.
 
+    Note that on Windows, the returned number of CPUs cannot exceed 61 (or 60 for
+    Python < 3.10), see:
+    https://bugs.python.org/issue26903.
+
     It is also always larger or equal to 1.
     """
     # Note: os.cpu_count() is allowed to return None in its docstring
     os_cpu_count = os.cpu_count() or 1
+    if sys.platform == "win32":
+        # On Windows, attempting to use more than 61 CPUs would result in a
+        # OS-level error. See https://bugs.python.org/issue26903. According to
+        # https://learn.microsoft.com/en-us/windows/win32/procthread/processor-groups
+        # it might be possible to go beyond with a lot of extra work but this
+        # does not look easy.
+        os_cpu_count = min(os_cpu_count, _MAX_WINDOWS_WORKERS)
 
     cpu_count_user = _cpu_count_user(os_cpu_count)
     aggregate_cpu_count = max(min(os_cpu_count, cpu_count_user), 1)
diff --git a/loky/process_executor.py b/loky/process_executor.py
index 9176afe..6e42258 100644
--- a/loky/process_executor.py
+++ b/loky/process_executor.py
@@ -79,7 +79,7 @@ from multiprocessing.connection import wait
 
 from ._base import Future
 from .backend import get_context
-from .backend.context import cpu_count
+from .backend.context import cpu_count, _MAX_WINDOWS_WORKERS
 from .backend.queues import Queue, SimpleQueue
 from .backend.reduction import set_loky_pickler, get_loky_pickler_name
 from .backend.utils import kill_process_tree, get_exitcodes_terminated_worker
@@ -1064,6 +1064,16 @@ class ProcessPoolExecutor(Executor):
                 raise ValueError("max_workers must be greater than 0")
             self._max_workers = max_workers
 
+        if (
+            sys.platform == "win32"
+            and self._max_workers > _MAX_WINDOWS_WORKERS
+        ):
+            warnings.warn(
+                f"On Windows, max_workers cannot exceed {_MAX_WINDOWS_WORKERS} "
+                "due to limitations of the operating system."
+            )
+            self._max_workers = _MAX_WINDOWS_WORKERS
+
         if context is None:
             context = get_context()
         self._context = context
diff --git a/tox.ini b/tox.ini
index 650b7dc..91bb53e 100644
--- a/tox.ini
+++ b/tox.ini
@@ -1,6 +1,6 @@
 # content of: tox.ini , put in same dir as setup.py
 [tox]
-envlist = py37, py38, py39, py310, pypy3
+envlist = py38, py39, py310, py311, pypy3
 skip_missing_interpreters=True
 
 [testenv]


Test patch:
diff --git a/tests/test_loky_module.py b/tests/test_loky_module.py
index 474f49d..b9734ea 100644
--- a/tests/test_loky_module.py
+++ b/tests/test_loky_module.py
@@ -10,7 +10,7 @@ import pytest
 
 import loky
 from loky import cpu_count
-from loky.backend.context import _cpu_count_user
+from loky.backend.context import _cpu_count_user, _MAX_WINDOWS_WORKERS
 
 
 def test_version():
@@ -34,6 +34,11 @@ def test_cpu_count():
     assert 1 <= cpus_physical <= cpus
 
 
+@pytest.mark.skipif(sys.platform != "win32", reason="Windows specific test")
+def test_windows_max_cpu_count():
+    assert cpu_count() <= _MAX_WINDOWS_WORKERS
+
+
 cpu_count_cmd = (
     "from loky.backend.context import cpu_count;" "print(cpu_count({args}))"
 )
diff --git a/tests/test_reusable_executor.py b/tests/test_reusable_executor.py
index f45e7ed..f9b99d4 100644
--- a/tests/test_reusable_executor.py
+++ b/tests/test_reusable_executor.py
@@ -20,6 +20,7 @@ from loky import get_reusable_executor
 from loky.process_executor import _RemoteTraceback, TerminatedWorkerError
 from loky.process_executor import BrokenProcessPool, ShutdownExecutorError
 from loky.reusable_executor import _ReusablePoolExecutor
+from loky.backend.context import _MAX_WINDOWS_WORKERS
 
 try:
     import psutil
@@ -220,7 +221,6 @@ class CExitAtGCInWorker:
 
 
 class TestExecutorDeadLock(ReusableExecutorMixin):
-
     crash_cases = [
         # Check problem occuring while pickling a task in
         (id, (ExitAtPickle(),), PicklingError, None),
@@ -1013,3 +1013,41 @@ class TestExecutorInitializer(ReusableExecutorMixin):
         out, err = p.communicate()
         assert p.returncode == 1, out.decode()
         assert b"resource_tracker" not in err, err.decode()
+
+
+def test_no_crash_max_workers_on_windows():
+    # Check that loky's reusable process pool executor does not crash when the
+    # user asks for more workers than the maximum number of workers supported
+    # by the platform.
+
+    # Note: on overloaded CI hosts, spawning many processes can take a long
+    # time. We need to increase the timeout to avoid spurious failures when
+    # making assertions on `len(executor._processes)`.
+    idle_worker_timeout = 10 * 60
+    with warnings.catch_warnings(record=True) as record:
+        executor = get_reusable_executor(
+            max_workers=_MAX_WINDOWS_WORKERS + 1, timeout=idle_worker_timeout
+        )
+        assert executor.submit(lambda: None).result() is None
+    if sys.platform == "win32":
+        assert len(record) == 1
+        assert "max_workers" in str(record[0].message)
+        assert len(executor._processes) == _MAX_WINDOWS_WORKERS
+    else:
+        assert len(record) == 0
+        assert len(executor._processes) == _MAX_WINDOWS_WORKERS + 1
+
+    # Downsizing should never raise a warning.
+    before_downsizing_executor = executor
+    with warnings.catch_warnings(record=True) as record:
+        executor = get_reusable_executor(
+            max_workers=_MAX_WINDOWS_WORKERS, timeout=idle_worker_timeout
+        )
+        assert executor.submit(lambda: None).result() is None
+
+    # No warning on any OS when max_workers does not exceed the limit.
+    assert len(record) == 0
+    assert before_downsizing_executor is executor
+    assert len(executor._processes) == _MAX_WINDOWS_WORKERS
+
+    executor.shutdown()