Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 34k
gh-124694: Add concurrent.futures.InterpreterPoolExecutor#124548
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
gh-124694: Add concurrent.futures.InterpreterPoolExecutor #124548
Uh oh!
There was an error while loading. Please reload this page.
Conversation
ericsnowcurrently commented Sep 25, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
46b5388 to 84993a5Compare28f948b to 45d584dCompareUh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
ericsnowcurrently commented Sep 30, 2024
@ZeroIntensity, I've fixed all those Docs things. |
ZeroIntensity left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. A small nitpick is that it might be a good idea to mention textwrap.dedent in the docs for initializer -- I'm worried that users might run into pesky indentation problems when passing scripts, and textwrap.dedent is a nice way to deal with that.
ericsnowcurrently commented Sep 30, 2024
Good point. It would make sense to automatically call it for users. |
ericsnowcurrently commented Sep 30, 2024
@brianquinlan, what are your thoughts on this? |
brianquinlan commented Oct 1, 2024
@ericsnowcurrently I haven't been active for yours but this seems like an excellent addition. |
ericsnowcurrently commented Oct 17, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
This may have introduced a |
ZeroIntensity commented Oct 17, 2024
Yeah, I noticed the tests for |
ericsnowcurrently commented Oct 17, 2024
In the crash I linked to, it happened when calling |
ericsnowcurrently commented Oct 17, 2024
FWIW, the only other failure I've seen with this on refleak buildbots is AMD64 FreeBSD Refleaks 3.x, with 3 successful runs after one failure, which looks like it hung. The other one I mentioned was on AMD64 RHEL8 Refleaks 3.x, and has had multiple successful runs to go with that one crash. I don't see any other failures for this on other stable buildbots. |
ericsnowcurrently commented Oct 17, 2024
I also haven't been able to reproduce any crash or hang locally yet. |
ZeroIntensity commented Oct 17, 2024
I guess it's possible that this only affects AMD? |
ericsnowcurrently commented Oct 17, 2024
It's probably a race due to some load profile that has only shown up there. |
ZeroIntensity commented Oct 17, 2024
I'll run the tests under valgrind to see if it picks anything up. That will take a while, though. Do you want to revert this in the meantime? |
ZeroIntensity commented Oct 17, 2024 • edited
Loading Uh oh!
There was an error while loading. Please reload this page.
edited
Uh oh!
There was an error while loading. Please reload this page.
Ah, @ericsnowcurrently, I think I found the problem. In most of the As far as I can tell, default struct initialization is not C standard, so it's values are just junk stack memory. I'm guessing that x86 has some sort of detail that sets stack-allocated structs to NULL, but not on ARM--that's why it's only failing there. Edit: Oh wait, it's AMD, not ARM. I guess it's a chip-specific issue then, but the point is that it's UB. |
ericsnowcurrently commented Oct 17, 2024
Hmm, I'll take a look. |
ericsnowcurrently commented Oct 17, 2024
Yeah, that could very well be it. The "label" field would sometimes be a problem if not initialized. I'll fix that. |
ericsnowcurrently commented Oct 17, 2024
I've opened gh-125667. |
ZeroIntensity commented Oct 17, 2024
Great, I'll review your PR whenever you get to it. |
ericsnowcurrently commented Oct 17, 2024
ericsnowcurrently commented Oct 18, 2024
I also noticed a failure on the Android buildbot: https://buildbot.python.org/#/builders/1594/builds/338. I'm looking into it. |
ericsnowcurrently commented Oct 18, 2024
ericsnowcurrently commented Oct 18, 2024
Looks like there's more to do: https://buildbot.python.org/#/builders/1610/builds/198 (AMD64 CentOS9 NoGIL Refleaks). I'm looking into it. |
ericsnowcurrently commented Oct 18, 2024
I've opened a new issue to deal with this, so we don't keep going on here: #125716. |
…ongh-124548) This is an implementation of InterpreterPoolExecutor that builds on ThreadPoolExecutor. (Note that this is not tied to PEP 734, which is strictly about adding a new stdlib module.) Possible future improvements: * support passing a script for the initializer or to submit() * support passing (most) arbitrary functions without pickling * support passing closures * optionally exec functions against __main__ instead of the their original module
This is an implementation of
InterpreterPoolExecutorthat builds onThreadPoolExecutor.This assumes that we're okay adding the executor separately from PEP 734. That PEP is about adding a new stdlib module, which is a separate matter from adding the new executor.
Possible future improvements:
__main__instead of the their original moduleCC @brianquinlan