bpo-44733: Add maxtasksperchild to ProcessPoolExecutor #27373

loganasherjones · 2021-07-26T17:39:37Z

https://bugs.python.org/issue44733

cool-RR

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.

Lib/concurrent/futures/process.py

loganasherjones · 2021-07-26T17:50:32Z

@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 :)

loganasherjones · 2021-07-27T00:32:57Z

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

cool-RR · 2021-07-27T08:01:33Z

@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.

cool-RR · 2021-08-07T09:47:37Z

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

pitrou

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

Lib/concurrent/futures/process.py

Lib/test/test_concurrent_futures.py

Doc/library/concurrent.futures.rst

bedevere-bot · 2021-08-07T10:43:45Z

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.

Lib/concurrent/futures/process.py

cool-RR · 2021-08-20T18:32:58Z

Lib/test/test_concurrent_futures.py

+        f2 = executor.submit(mul, 6, 7)
+        f3 = executor.submit(mul, 6, 7)
+        self.assertEqual(f2.result(), 42)
+        self.assertEqual(f3.result(), 42)


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.

Doc/library/concurrent.futures.rst

loganasherjones · 2021-08-20T19:02:10Z

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

cool-RR · 2021-08-20T19:26:48Z

Yes, please do.

loganasherjones · 2021-08-20T20:04:07Z

I have made the requested changes; please review again.

bedevere-bot · 2021-08-20T20:04:10Z

Thanks for making the requested changes!

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

cool-RR · 2021-09-01T16:40:03Z

@pitrou ping.

Lib/concurrent/futures/process.py

Lib/test/test_concurrent_futures.py

loganasherjones · 2021-09-15T00:25:53Z

I have made the requested changes; please review again.

bedevere-bot · 2021-09-15T00:25:56Z

Thanks for making the requested changes!

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

loganasherjones · 2021-10-31T00:33:55Z

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!

bedevere-bot · 2021-10-31T00:33:57Z

Thanks for making the requested changes!

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

loganasherjones · 2021-11-16T01:12:08Z

friendly ping @pitrou :)

* Add test for number of worker processes

pitrou

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

bedevere-bot · 2021-11-20T15:41:05Z

🤖 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.

cool-RR · 2021-11-20T15:46:40Z

Woohoo!

bedevere-bot · 2021-11-20T17:36:57Z

🤖 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.

pitrou · 2021-11-20T20:18:55Z

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

…7373) Co-authored-by: Antoine Pitrou <[email protected]>

…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

Lib/concurrent/futures/process.py Outdated Show resolved Hide resolved

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

pitrou requested changes Aug 7, 2021

View reviewed changes

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

Lib/concurrent/futures/process.py Outdated Show resolved Hide resolved

Lib/concurrent/futures/process.py Outdated Show resolved Hide resolved

Lib/test/test_concurrent_futures.py Outdated Show resolved Hide resolved

Lib/test/test_concurrent_futures.py Show resolved Hide resolved

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

loganasherjones force-pushed the issue-44733 branch from 22a259e to 948a855 Compare October 30, 2021 19:36

bpo-44733: Ensuring process is cleaned up

a6495ff

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 <[email protected]>

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

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

bpo-44733: Add maxtasksperchild to ProcessPoolExecutor #27373

bpo-44733: Add maxtasksperchild to ProcessPoolExecutor #27373

loganasherjones commented Jul 26, 2021 •

edited by miss-islington

Loading

cool-RR left a comment

loganasherjones commented Jul 26, 2021

loganasherjones commented Jul 27, 2021

cool-RR commented Jul 27, 2021

cool-RR commented Aug 7, 2021

pitrou left a comment

bedevere-bot commented Aug 7, 2021

cool-RR Aug 20, 2021

loganasherjones Aug 20, 2021

cool-RR Aug 20, 2021

loganasherjones Aug 20, 2021

cool-RR Aug 20, 2021

loganasherjones commented Aug 20, 2021

cool-RR commented Aug 20, 2021

loganasherjones commented Aug 20, 2021

bedevere-bot commented Aug 20, 2021

cool-RR commented Sep 1, 2021

loganasherjones commented Sep 15, 2021

bedevere-bot commented Sep 15, 2021

loganasherjones commented Oct 31, 2021

bedevere-bot commented Oct 31, 2021

loganasherjones commented Nov 16, 2021

pitrou left a comment

bedevere-bot commented Nov 20, 2021

cool-RR commented Nov 20, 2021

bedevere-bot commented Nov 20, 2021

pitrou commented Nov 20, 2021

bpo-44733: Add maxtasksperchild to ProcessPoolExecutor #27373

bpo-44733: Add maxtasksperchild to ProcessPoolExecutor #27373

Conversation

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

cool-RR left a comment

Choose a reason for hiding this comment

loganasherjones commented Jul 26, 2021

loganasherjones commented Jul 27, 2021

cool-RR commented Jul 27, 2021

cool-RR commented Aug 7, 2021

pitrou left a comment

Choose a reason for hiding this comment

bedevere-bot commented Aug 7, 2021

cool-RR Aug 20, 2021

Choose a reason for hiding this comment

loganasherjones Aug 20, 2021

Choose a reason for hiding this comment

cool-RR Aug 20, 2021

Choose a reason for hiding this comment

loganasherjones Aug 20, 2021

Choose a reason for hiding this comment

cool-RR Aug 20, 2021

Choose a reason for hiding this comment

loganasherjones commented Aug 20, 2021

cool-RR commented Aug 20, 2021

loganasherjones commented Aug 20, 2021

bedevere-bot commented Aug 20, 2021

cool-RR commented Sep 1, 2021

loganasherjones commented Sep 15, 2021

bedevere-bot commented Sep 15, 2021

loganasherjones commented Oct 31, 2021

bedevere-bot commented Oct 31, 2021

loganasherjones commented Nov 16, 2021

pitrou left a comment

Choose a reason for hiding this comment

bedevere-bot commented Nov 20, 2021

cool-RR commented Nov 20, 2021

bedevere-bot commented Nov 20, 2021

pitrou commented Nov 20, 2021

loganasherjones commented Jul 26, 2021 •

edited by miss-islington

Loading