Always use EM_PYTHON_MULTIPROCESSING after win32 bugfix#17881
Always use EM_PYTHON_MULTIPROCESSING after win32 bugfix#17881
Conversation
d816f6b to
5a47a81
Compare
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."
kripken
left a comment
There was a problem hiding this comment.
Ok, makes sense. Sounds like this really is a 61+ cores issue then...
| 3.1.22 (in development) | ||
| ----------------------- | ||
| - Remove EM_PYTHON_MULTIPROCESSING=1 environment variable which is no longer | ||
| required. (#17881) |
|
Actually I decided it would be safer to do this incrementally and first land #17892 |
8a28283 to
9022814
Compare
9022814 to
7c20ffe
Compare
This removes the need for the alternative implementation now that the primary windows issue has been resolved.
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