Always use EM_PYTHON_MULTIPROCESSING after win32 bugfix #17881
Conversation
sbc100
force-pushed
the
windows_python_max_multiprocessing
branch
2 times, most recently
from
d816f6b
to
5a47a81
Compare
sbc100
changed the title
sbc100
force-pushed
the
windows_python_max_multiprocessing
branch
from
5a47a81
to
48d7a64
Compare
This codepath is not normally used since once currently needs to opt into using via EM_PYTHON_MULTIPROCESSING=1. However, I'm hoping to remove that limitation in #17881. This cleaanup/fix should land before #17881 does. The primary change here is to switch from `subprocessrun.Popen` to `subprocess.run` which supports the `check=True` argument. As well as avoiding the need for the call to `communicate()`
This codepath is not normally used since once currently needs to opt into using via EM_PYTHON_MULTIPROCESSING=1. However, I'm hoping to remove that limitation in #17881. This cleaanup/fix should land before #17881 does. The primary change here is to switch from `subprocessrun.Popen` to `subprocess.run` which supports the `check=True` argument. As well as avoiding the need for the call to `communicate()`
This codepath is not normally used since once currently needs to opt into using via EM_PYTHON_MULTIPROCESSING=1. However, I'm hoping to remove that limitation in #17881. This cleaanup/fix should land before #17881 does. The primary change here is to switch from `subprocessrun.Popen` to `subprocess.run` which supports the `check=True` argument. As well as avoiding the need for the call to `communicate()`
This codepath is not normally used since once currently needs to opt into using via EM_PYTHON_MULTIPROCESSING=1. However, I'm hoping to remove that limitation in #17881. This cleaanup/fix should land before #17881 does. The primary change here is to switch from `subprocessrun.Popen` to `subprocess.run` which supports the `check=True` argument. As well as avoiding the need for the call to `communicate()`
tools/shared.py
Outdated
| If True, an array of stdouts is returned, for each subprocess. | ||
| """ | ||
| # On windows, there is limit on the total number of tasks which is | ||
| # enforced bu multiprocessing, but only on newer version of python: |
There was a problem hiding this comment.
| # enforced bu multiprocessing, but only on newer version of python: | |
| # enforced by multiprocessing, but only on newer version of python: |
| if not multiprocessing_pool: | ||
| max_workers = get_num_cores() | ||
| if WINDOWS: | ||
| max_workers = min(max_workers, 61) |
There was a problem hiding this comment.
Is the fix just this, to limit to 61? So this was only ever a problem for machines with >61 cores..?
There was a problem hiding this comment.
Yes, that is my understanding. see the python bug for details. I guess for emcc users it would also effect users who set EMCC_CORES > 61.. since you can create more threads that cores if you want to using that variable.b
There was a problem hiding this comment.
Hmm, I'm a little surprised then that we got but reports about this. How many people have that many cores..?
If we can confirm this works, or we can ask someone that has encountered the bug, that would make me more confident here. But, maybe I'm being overly cautious, if this is in upstream Python it's probably fine..?
There was a problem hiding this comment.
Yes, but it makes sense it you look at the original bug report : "On the other Windows system, this issue does not seem to ever occur. The affected system is a 64-thread Threadripper 2990WX, and the numbers are quite close to 64, so I wonder if that has a meaning here. The unaffected system is a 16-thread system."
| @@ -20,6 +20,8 @@ See docs/process.md for more on how version tagging works. | |||
|
|
|||
| 3.1.22 (in development) | |||
| ----------------------- | |||
| - Remove EM_PYTHON_MULTIPROCESSING=1 environment variable which is no longer | |||
| required. (#17881) | |||
sbc100
changed the title
|
Actually I decided it would be safer to do this incrementally and first land #17892 |
sbc100
force-pushed
the
windows_python_max_multiprocessing
branch
2 times, most recently
from
8a28283
to
9022814
Compare
sbc100
force-pushed
the
windows_python_max_multiprocessing
branch
from
9022814
to
7c20ffe
Compare
This removes the need for the alternative implementation now that the primary windows issue has been resolved.
sbc100
force-pushed
the
windows_python_max_multiprocessing
branch
from
7c20ffe
to
35cad0d
Compare
|
Closing this change for now until we can measure and decide if we want to go with EM_PYTHON_MULTIPROCESSING by default, or not. |
This removes the need for the aternative implementation and mirrors the upstream bugfix in python 3.8.0.
Fixes: #13785