Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Conversation

ericsnowcurrently
Copy link
Member

@ericsnowcurrentlyericsnowcurrently commentedSep 25, 2024
edited
Loading

This is an implementation ofInterpreterPoolExecutor that 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:

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

CC@brianquinlan

temeddix reacted with heart emoji
@ericsnowcurrentlyericsnowcurrently changed the titleAdd concurrent.futures.InterpreterPoolExecutorgh-124694: Add concurrent.futures.InterpreterPoolExecutorSep 27, 2024
@ericsnowcurrentlyericsnowcurrently marked this pull request as ready for reviewSeptember 27, 2024 22:37
@ericsnowcurrently
Copy link
MemberAuthor

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

Copy link
Member

@ZeroIntensityZeroIntensity left a 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 mentiontextwrap.dedent in the docs forinitializer -- I'm worried that users might run into pesky indentation problems when passing scripts, andtextwrap.dedent is a nice way to deal with that.

@ericsnowcurrently
Copy link
MemberAuthor

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

@ericsnowcurrently
Copy link
MemberAuthor

@brianquinlan, what are your thoughts on this?

@brianquinlan
Copy link
Contributor

@brianquinlan, what are your thoughts on this?

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

ericsnowcurrently reacted with thumbs up emoji

@ericsnowcurrentlyericsnowcurrently merged commita5a7f5e intopython:mainOct 16, 2024
37 checks passed
@ericsnowcurrentlyericsnowcurrently deleted the interpreter-pool-executor branchOctober 16, 2024 22:50
@ericsnowcurrently
Copy link
MemberAuthor

ericsnowcurrently commentedOct 17, 2024
edited
Loading

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

willingc reacted with confused emoji

@ZeroIntensity
Copy link
Member

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

@ericsnowcurrently
Copy link
MemberAuthor

In the crash I linked to, it happened when calling_interpqueues.create(), so not_interpreters.

willingc and ZeroIntensity reacted with thumbs up emoji

@ericsnowcurrently
Copy link
MemberAuthor

FWIW, the only other failure I've seen with this on refleak buildbots isAMD64 FreeBSD Refleaks 3.x, with 3 successful runs afterone failure, which looks like it hung.

The other one I mentioned was onAMD64 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
Copy link
MemberAuthor

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

@ZeroIntensity
Copy link
Member

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

@ericsnowcurrently
Copy link
MemberAuthor

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

ZeroIntensity reacted with thumbs up emoji

@ZeroIntensity
Copy link
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
Copy link
Member

ZeroIntensity commentedOct 17, 2024
edited
Loading

Ah,@ericsnowcurrently, I think I found the problem. In most of the_interpqueues methods, something like this is passed toPyArg* 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
Copy link
MemberAuthor

Hmm, I'll take a look.

@ericsnowcurrently
Copy link
MemberAuthor

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

ZeroIntensity reacted with thumbs up emoji

@ericsnowcurrently
Copy link
MemberAuthor

I've openedgh-125667.

@ZeroIntensity
Copy link
Member

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

@ericsnowcurrently
Copy link
MemberAuthor

gh-125668

@ericsnowcurrently
Copy link
MemberAuthor

I also noticed a failure on the Android buildbot:https://buildbot.python.org/#/builders/1594/builds/338. I'm looking into it.

ZeroIntensity reacted with thumbs up emoji

@ericsnowcurrently
Copy link
MemberAuthor

#125708

@ericsnowcurrently
Copy link
MemberAuthor

Looks like there's more to do:https://buildbot.python.org/#/builders/1610/builds/198 (AMD64 CentOS9 NoGIL Refleaks). I'm looking into it.

ZeroIntensity reacted with thumbs up emoji

@ericsnowcurrently
Copy link
MemberAuthor

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

willingc reacted with thumbs up emoji

ebonnal pushed a commit to ebonnal/cpython that referenced this pull requestJan 12, 2025
…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
Sign up for freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@blussblussbluss left review comments

@1st11st11st1 approved these changes

@willingcwillingcwillingc approved these changes

@ZeroIntensityZeroIntensityZeroIntensity approved these changes

@asvetlovasvetlovAwaiting requested review from asvetlovasvetlov is a code owner

@kumaraditya303kumaraditya303Awaiting requested review from kumaraditya303kumaraditya303 is a code owner

@AA-TurnerAA-TurnerAwaiting requested review from AA-Turner

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
@ericsnowcurrently@brianquinlan@ZeroIntensity@kumaraditya303@1st1@willingc@bluss

[8]ページ先頭

©2009-2025 Movatter.jp