bpo-44733: Add maxtasksperchild to ProcessPoolExecutor #27373
Conversation
|
@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 |
|
@pitrou do you have time to look at Logan's PR? |
|
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 |
loganasherjones
force-pushed
the
issue-44733
branch
2 times, most recently
from
0b59566
to
ca79201
Compare
Lib/test/test_concurrent_futures.py
Outdated
| f2 = executor.submit(mul, 6, 7) | ||
| f3 = executor.submit(mul, 6, 7) | ||
| self.assertEqual(f2.result(), 42) | ||
| self.assertEqual(f3.result(), 42) |
There was a problem hiding this comment.
Why not assert that there weren't multiple PIDs up to this point?
There was a problem hiding this comment.
I can change around the order here and assert it on the 2nd result, but not the third.
There was a problem hiding this comment.
That's good. But why isn't the third run guaranteed to have the same PID?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
pitrou
added
the
bedevere-bot
removed
the
|
Woohoo! |
pitrou
added
the
bedevere-bot
removed
the
|
Ok, CI is green enough, I'll merge. |
pitrou
added
the
pitrou
removed
the
…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.
…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.
https://bugs.python.org/issue44733