FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-124694: Add concurrent.futures.InterpreterPoolExecutor by ericsnowcurrently · Pull Request #124548 · python/cpython · GitHub

/ cpython Public

gh-124694: Add concurrent.futures.InterpreterPoolExecutor - #124548

Merged
ericsnowcurrently merged 39 commits into
python:mainfrom
ericsnowcurrently:interpreter-pool-executor
Oct 16, 2024
ConversationCommits (39)ChecksFiles changed
Merged

gh-124694: Add concurrent.futures.InterpreterPoolExecutor#124548
ericsnowcurrently merged 39 commits into
python:mainfrom
ericsnowcurrently:interpreter-pool-executor

Conversation

ericsnowcurrently commented Sep 25, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

This is an implementation of InterpreterPoolExecutor that builds on ThreadPoolExecutor.

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:

  • support passing (most) arbitrary functions without pickling
  • support passing closures
  • optionally exec functions against __main__ instead of the their original module

CC @brianquinlan

ericsnowcurrently changed the title Add concurrent.futures.InterpreterPoolExecutor gh-124694: Add concurrent.futures.InterpreterPoolExecutor Sep 27, 2024
bedevere-app Bot mentioned this pull request Sep 27, 2024
Closed
ericsnowcurrently marked this pull request as ready for review September 27, 2024 22:37
ZeroIntensity reviewed Sep 29, 2024
View reviewed changes
Comment thread Doc/library/concurrent.futures.rst Outdated
Comment thread Doc/library/concurrent.futures.rst Outdated
Comment thread Doc/library/concurrent.futures.rst Outdated
Comment thread Doc/library/concurrent.futures.rst Outdated
Comment thread Doc/library/concurrent.futures.rst Outdated
Comment thread Doc/library/concurrent.futures.rst Outdated
Comment thread Doc/library/concurrent.futures.rst Outdated
Comment thread Lib/concurrent/futures/interpreter.py Outdated

ericsnowcurrently commented Sep 30, 2024

Copy link
Copy Markdown
Member Author

@ZeroIntensity, I've fixed all those Docs things.

ZeroIntensity approved these changes Sep 30, 2024
View reviewed changes

ZeroIntensity left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

Copy link
Copy Markdown
Member Author

Good point. It would make sense to automatically call it for users.

ericsnowcurrently commented Sep 30, 2024

Copy link
Copy Markdown
Member Author

@brianquinlan, what are your thoughts on this?

brianquinlan commented Oct 1, 2024

Copy link
Copy Markdown
Contributor

@brianquinlan, what are your thoughts on this?

@ericsnowcurrently I haven't been active for yours but this seems like an excellent addition.

ericsnowcurrently deleted the interpreter-pool-executor branch October 16, 2024 22:50

ericsnowcurrently commented Oct 17, 2024 •
edited
Loading

Copy link
Copy Markdown
Member Author

This may have introduced a refleak crash on one of the refleak buildbots: https://buildbot.python.org/#/builders/259/builds/1528. I'm taking a look.

ZeroIntensity commented Oct 17, 2024

Copy link
Copy Markdown
Member

Yeah, I noticed the tests for InterpreterPoolExecutor segfaulting on someone elses PR yesterday on something seemingly unrelated. Since this is pure-Python, _interpreters might be buggier than we think :(

ericsnowcurrently commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

In the crash I linked to, it happened when calling _interpqueues.create(), so not _interpreters.

ericsnowcurrently commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

I also haven't been able to reproduce any crash or hang locally yet.

ZeroIntensity commented Oct 17, 2024

Copy link
Copy Markdown
Member

I guess it's possible that this only affects AMD?

ericsnowcurrently commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

It's probably a race due to some load profile that has only shown up there.

ZeroIntensity commented Oct 17, 2024

Copy link
Copy Markdown
Member

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

Copy link
Copy Markdown
Member

Ah, @ericsnowcurrently, I think I found the problem. In most of the _interpqueues methods, something like this is passed to PyArg* as a converter:

qidarg_converter_data qidarg;

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

Copy link
Copy Markdown
Member Author

Hmm, I'll take a look.

ericsnowcurrently commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

Yeah, that could very well be it. The "label" field would sometimes be a problem if not initialized. I'll fix that.

ericsnowcurrently mentioned this pull request Oct 17, 2024
Closed

ericsnowcurrently commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

I've opened gh-125667.

ZeroIntensity commented Oct 17, 2024

Copy link
Copy Markdown
Member

Great, I'll review your PR whenever you get to it.

ericsnowcurrently commented Oct 17, 2024

Copy link
Copy Markdown
Member Author

gh-125668

ericsnowcurrently commented Oct 18, 2024

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

#125708

ericsnowcurrently commented Oct 18, 2024

Copy link
Copy Markdown
Member Author

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

Copy link
Copy Markdown
Member Author

I've opened a new issue to deal with this, so we don't keep going on here: #125716.

jakirkham mentioned this pull request Oct 22, 2024
Closed
This was referenced Oct 23, 2024
Open
Closed
Closed
ericsnowcurrently mentioned this pull request Jan 7, 2025
Closed
ebonnal pushed a commit to ebonnal/cpython that referenced this pull request Jan 12, 2025
pythongh-124694: Add concurrent.futures.InterpreterPoolExecutor (pyth…
843c78d 
…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
donbarbos mentioned this pull request Apr 4, 2025
Open
ZeroIntensity mentioned this pull request May 31, 2025
Merged
cdce8p mentioned this pull request Jun 11, 2025
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1st1 1st1 approved these changes
willingc willingc approved these changes
ZeroIntensity ZeroIntensity approved these changes
asvetlov Awaiting requested review from asvetlov asvetlov is a code owner
kumaraditya303 Awaiting requested review from kumaraditya303 kumaraditya303 is a code owner
AA-Turner Awaiting requested review from AA-Turner AA-Turner is a code owner
+1 more reviewer
bluss bluss left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

7 participants

Back | FazBrowse Home | New Git URL