bpo-44733: Add maxtasksperchild to ProcessPoolExecutor by loganasherjones · Pull Request #27373 · python/cpython · GitHub

https://bugs.python.org/issue44733

Good work Logan! I didn't expect the documentation to be already written. I gave one minor note, and I'll wait for someone who's experienced with this module to add a review. This is probably one of the most complex modules in the standard library, and there are many pitfalls to avoid.

@cool-RR thanks! I've never made a contribution like this so I wasn't sure if I should write docs or not.

And yeah, I was a bit worried about modifying this code because I know it's quite complex. I got all the tests to pass which I figured was a good start, but I'm a bit worried because it seems like worker processes were only originally designed to exit once a shutdown was initiated.

I'm exited for a review :)

I noticed the docs CI is failing, but the error message does not make any sense to me.

@loganasherjones
Confusing indeed. "default role used" might refer to the way you used backticks in your RST file. Look at how other similar files are written, I believe they use a different style.

@pitrou do you have time to look at Logan's PR?

Thank you for posting this PR. Here are some comments.

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

Why not assert that there weren't multiple PIDs up to this point?

I can change around the order here and assert it on the 2nd result, but not the third.

That's good. But why isn't the third run guaranteed to have the same PID?

The problem is it's a race condition.

If I submit the third job, I have to check executor._processes before that worker gets the work item, finishes it and shuts down.

It could either result in an error doing self.assertEqual(len(exectuor._processes), 1) because the worker has already shut down. Even if that doesn't fail, there's no guarantee that the process hasn't already been replaced.

I could get around this by calling sleep_and_print but then it's just introducing a timing-dependent test and I figured we wouldn't want to do that.

That makes sense. Thank you.

@cool-RR think this is ready for me to ping the core devs again? (assuming these tests pass)

Yes, please do.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

@pitrou ping.

I have made the requested changes; please review again.

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

I have made the requested changes; please review again.

Hi @pitrou so this last set of changes I think is probably the best we've come up with. Let me know what you think!

Thanks for making the requested changes!

@pitrou: please review the changes made to this pull request.

friendly ping @pitrou :)

* Add test for number of worker processes

Thanks @loganasherjones ! I pushed some small updates and merged upstream changes.

🤖 New build scheduled with the buildbot fleet by @pitrou for commit e4cbc3a 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

Woohoo!

🤖 New build scheduled with the buildbot fleet by @pitrou for commit cfeefb8 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

Ok, CI is green enough, I'll merge.

…7373) Co-authored-by: Antoine Pitrou <antoine@python.org>

…thonGH-27373)" This reverts commit fdc0e09. This implementation relies on a mechanism for spawning new children dynamically rather than up front that leads to deadlocks due to mixing of threads+fork. See bpo-46464.

bedevere-bot added the awaiting review label Jul 26, 2021

the-knights-who-say-ni added the CLA signed label Jul 26, 2021

cool-RR approved these changes Jul 26, 2021

View reviewed changes

Comment thread Lib/concurrent/futures/process.py Outdated

bedevere-bot added awaiting core review and removed awaiting review labels Jul 26, 2021

pitrou requested changes Aug 7, 2021

View reviewed changes

Comment thread Lib/concurrent/futures/process.py Outdated

Comment thread Lib/concurrent/futures/process.py Outdated

Comment thread Lib/concurrent/futures/process.py Outdated

Comment thread Lib/test/test_concurrent_futures.py Outdated

Comment thread Doc/library/concurrent.futures.rst Outdated

bedevere-bot added awaiting changes and removed awaiting core review labels Aug 7, 2021

loganasherjones force-pushed the issue-44733 branch 2 times, most recently from 0b59566 to ca79201 Compare August 20, 2021 18:14

cool-RR approved these changes Aug 20, 2021

View reviewed changes

loganasherjones force-pushed the issue-44733 branch from ca79201 to a930c21 Compare August 20, 2021 18:48

bedevere-bot added awaiting change review and removed awaiting changes labels Aug 20, 2021

bedevere-bot requested a review from pitrou August 20, 2021 20:04

pitrou reviewed Sep 1, 2021

View reviewed changes

Comment thread Lib/concurrent/futures/process.py Outdated

Comment thread Lib/concurrent/futures/process.py Outdated

Comment thread Lib/test/test_concurrent_futures.py Outdated

Comment thread Lib/test/test_concurrent_futures.py Outdated

loganasherjones force-pushed the issue-44733 branch from a930c21 to 22a259e Compare September 2, 2021 00:00

bedevere-bot requested a review from pitrou September 15, 2021 00:25

bedevere-bot requested a review from pitrou October 31, 2021 00:33

pitrou added 2 commits November 20, 2021 16:15

Merge branch 'main' into issue-44733

52ea6ff

* Process count adjustment must be serialized

e4cbc3a

* Add test for number of worker processes

pitrou approved these changes Nov 20, 2021

View reviewed changes

bedevere-bot added awaiting merge and removed awaiting change review labels Nov 20, 2021

pitrou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 20, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 20, 2021

Remove flaky check

cfeefb8

pitrou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 20, 2021

bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Nov 20, 2021

pitrou added the 🤖 automerge label Nov 20, 2021

pitrou merged commit fdc0e09 into python:main Nov 20, 2021

bedevere-bot removed the awaiting merge label Nov 20, 2021

pitrou removed the 🤖 automerge label Nov 20, 2021

loganasherjones deleted the issue-44733 branch November 22, 2021 00:40

remykarem pushed a commit to remykarem/cpython that referenced this pull request Dec 7, 2021

bpo-44733: Add max_tasks_per_child to ProcessPoolExecutor (pythonGH-2…

504e53f

…7373) Co-authored-by: Antoine Pitrou <antoine@python.org>

cool-RR mannequin mentioned this pull request Apr 10, 2022

Feature request: maxtasksperchild for ProcessPoolExecutor #88896

Closed

gaogaotiantian mentioned this pull request Feb 19, 2024

gh-115634: Force the process pool to adjust when a process worker exits #115642

Closed

gpshead mentioned this pull request Nov 2, 2025

gh-115634: Fix ProcessPoolExecutor deadlock with max_tasks_per_child #140900

Merged

coderabbitai Bot mentioned this pull request Mar 25, 2026

Prevent CUDA error cascade on multi-GPU test runs newton-physics/newton#2234

Merged

3 tasks

bpo-44733: Add maxtasksperchild to ProcessPoolExecutor by loganasherjones · Pull Request #27373 · python/cpython · GitHub

Uh oh!

Uh oh!

bpo-44733: Add maxtasksperchild to ProcessPoolExecutor - #27373

bpo-44733: Add maxtasksperchild to ProcessPoolExecutor#27373pitrou merged 7 commits intopython:mainpython/cpython:mainfrom loganasherjones:issue-44733loganasherjones/cpython:issue-44733Copy head branch name to clipboard

Conversation

loganasherjones commented Jul 26, 2021 • edited by miss-islington Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cool-RR left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

loganasherjones commented Jul 26, 2021

Uh oh!

loganasherjones commented Jul 27, 2021

Uh oh!

cool-RR commented Jul 27, 2021

Uh oh!

cool-RR commented Aug 7, 2021

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

bedevere-bot commented Aug 7, 2021

Uh oh!

Uh oh!

cool-RR Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

loganasherjones Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

cool-RR Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

loganasherjones Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

cool-RR Aug 20, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

loganasherjones commented Aug 20, 2021

Uh oh!

cool-RR commented Aug 20, 2021

Uh oh!

loganasherjones commented Aug 20, 2021

Uh oh!

bedevere-bot commented Aug 20, 2021

Uh oh!

cool-RR commented Sep 1, 2021

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

loganasherjones commented Sep 15, 2021

Uh oh!

bedevere-bot commented Sep 15, 2021

Uh oh!

loganasherjones commented Oct 31, 2021

Uh oh!

bedevere-bot commented Oct 31, 2021

Uh oh!

loganasherjones commented Nov 16, 2021

Uh oh!

pitrou left a comment

Choose a reason for hiding this comment

Uh oh!

bedevere-bot commented Nov 20, 2021

Uh oh!

cool-RR commented Nov 20, 2021

Uh oh!

bedevere-bot commented Nov 20, 2021

bpo-44733: Add maxtasksperchild to ProcessPoolExecutor#27373
pitrou merged 7 commits into
python:mainfrom
loganasherjones:issue-44733

loganasherjones commented Jul 26, 2021 •
edited by miss-islington
Loading