Skip to content

bpo-17140: Document multiprocessing's ThreadPool #23812

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.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 1 commit into from
Dec 18, 2020
Merged

bpo-17140: Document multiprocessing's ThreadPool #23812

merged 1 commit into from
Dec 18, 2020

Conversation

godlygeek
Copy link
Contributor

@godlygeek godlygeek commented Dec 17, 2020 •
edited by bedevere-bot
Loading

Up until now the multiprocessing.pool.ThreadPool class has gone
undocumented, despite being a public class in multiprocessing that is
included in multiprocessing.pool.__all__.

https://bugs.python.org/issue17140

@bedevere-bot bedevere-bot added docs Documentation in the Doc dir awaiting review labels Dec 17, 2020
Fidget-Spinner
Fidget-Spinner approved these changes Dec 17, 2020
View reviewed changes
Copy link
Member

@Fidget-Spinner Fidget-Spinner left a comment

Choose a reason for hiding this comment

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

This LGTM. The only comment I have is that it seems strange to place ThreadPool under the Process Pools section, though I understand that it makes sense since it's a subclass of Pool. Maybe we can create a Thread Pools subsection for it. What do you think?

@godlygeek
Copy link
Contributor Author

godlygeek commented Dec 17, 2020 •
edited
Loading

I almost did that, and don't mind changing it to be that way if you prefer. Would you like the new section directly below the "Process Pools" section?

I also considered putting it in the "The :mod:multiprocessing.dummy module" section, which is slightly weird since it's in multiprocessing.pool, but also is arguably the place where it best belongs since it is related to the "multiprocessing but with threads instead of processes" thing that multiprocessing.dummy does, and because multiprocessing.dummy.Pool does return ThreadPool instances (though it is a wrapper function around multiprocessing.Pool.ThreadPool for some reason, not an alias for it).

@Fidget-Spinner
Copy link
Member

I almost did that, and don't mind changing it to be that way if you prefer. Would you like the new section directly below the "Process Pools" section?

Hmm I think, I'll wait and see what the core dev who reviews this prefers. Thanks for working on this :).

@pablogsal pablogsal requested a review from pitrou December 17, 2020 16:19
pablogsal
pablogsal reviewed Dec 17, 2020
View reviewed changes
@pablogsal pablogsal self-assigned this Dec 17, 2020
@godlygeek
Copy link
Contributor Author

@pablogsal OK, I've force-pushed a new commit that yanks it down into the multiprocessing.dummy section, and explains that ThreadPoolExecutor is generally preferable. Take a look and let me know if you think there's anything else it needs, please.

pablogsal
pablogsal approved these changes Dec 17, 2020
View reviewed changes
Copy link
Member

@pablogsal pablogsal left a comment •
edited
Loading

Choose a reason for hiding this comment

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

LGTM

I will leave some days in case Antoine has time to review this before landing in case he has some new insights or he wants some changes.

@godlygeek
Copy link
Contributor Author

I just made one final change - I had the note about preferring ThreadPoolExecutor above the class definition, and I've moved it down into the documentation for the class instead.

@godlygeek
bpo-17140: Document multiprocessing's ThreadPool
351ccc1 
Up until now the `multiprocessing.pool.ThreadPool` class has gone
undocumented, despite being a public class in multiprocessing that is
included in `multiprocessing.pool.__all__`.
@godlygeek
Copy link
Contributor Author

One more force push - I noticed a missing word after re-reading this.

@pablogsal
Copy link
Member

One more force push - I noticed a missing word after re-reading this.

Note the there is no need for force pushes: the commits will be squashed and the core Dev merging will write the final commit anyway.

@pitrou
Copy link
Member

pitrou commented Dec 17, 2020

This is fine with me. Thanks for reviewing @pablogsal !

@pablogsal pablogsal merged commit 84ebcf2 into python:master Dec 18, 2020
@miss-islington
Copy link
Contributor

Thanks @godlygeek for the PR, and @pablogsal for merging it 🌮🎉.. I'm working now to backport this PR to: 3.7, 3.8, 3.9.
🐍🍒⛏🤖

@bedevere-bot
Copy link

GH-23834 is a backport of this pull request to the 3.9 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 18, 2020
@godlygeek @miss-islington
bpo-17140: Document multiprocessing's ThreadPool (pythonGH-23812)
f382ed3 
Up until now, the `multiprocessing.pool.ThreadPool` class has gone
undocumented, despite being a public class in multiprocessing that is
included in `multiprocessing.pool.__all__`.
(cherry picked from commit 84ebcf2)

Co-authored-by: Matt Wozniski <[email protected]>
@bedevere-bot
Copy link

GH-23835 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 18, 2020
@godlygeek @miss-islington
bpo-17140: Document multiprocessing's ThreadPool (pythonGH-23812)
2c52729 
Up until now, the `multiprocessing.pool.ThreadPool` class has gone
undocumented, despite being a public class in multiprocessing that is
included in `multiprocessing.pool.__all__`.
(cherry picked from commit 84ebcf2)

Co-authored-by: Matt Wozniski <[email protected]>
@bedevere-bot
Copy link

GH-23836 is a backport of this pull request to the 3.7 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Dec 18, 2020
@godlygeek @miss-islington
bpo-17140: Document multiprocessing's ThreadPool (pythonGH-23812)
a40ce53 
Up until now, the `multiprocessing.pool.ThreadPool` class has gone
undocumented, despite being a public class in multiprocessing that is
included in `multiprocessing.pool.__all__`.
(cherry picked from commit 84ebcf2)

Co-authored-by: Matt Wozniski <[email protected]>
miss-islington added a commit that referenced this pull request Dec 18, 2020
@miss-islington @godlygeek
bpo-17140: Document multiprocessing's ThreadPool (GH-23812)
1461992 
Up until now, the `multiprocessing.pool.ThreadPool` class has gone
undocumented, despite being a public class in multiprocessing that is
included in `multiprocessing.pool.__all__`.
(cherry picked from commit 84ebcf2)

Co-authored-by: Matt Wozniski <[email protected]>
miss-islington added a commit that referenced this pull request Dec 18, 2020
@miss-islington @godlygeek
[3.8] bpo-17140: Document multiprocessing's ThreadPool (GH-23812) (GH…
d21d29a 
…-23835)

Up until now, the `multiprocessing.pool.ThreadPool` class has gone
undocumented, despite being a public class in multiprocessing that is
included in `multiprocessing.pool.__all__`.
(cherry picked from commit 84ebcf2)


Co-authored-by: Matt Wozniski <[email protected]>
ned-deily pushed a commit that referenced this pull request Dec 18, 2020
@miss-islington @godlygeek
bpo-17140: Document multiprocessing's ThreadPool (GH-23812) (GH-23836)
00278d4 
Up until now, the `multiprocessing.pool.ThreadPool` class has gone
undocumented, despite being a public class in multiprocessing that is
included in `multiprocessing.pool.__all__`.
(cherry picked from commit 84ebcf2)

Co-authored-by: Matt Wozniski <[email protected]>
adorilson pushed a commit to adorilson/cpython that referenced this pull request Mar 13, 2021
@godlygeek @adorilson
bpo-17140: Document multiprocessing's ThreadPool (pythonGH-23812)
730f273 
Up until now, the `multiprocessing.pool.ThreadPool` class has gone
undocumented, despite being a public class in multiprocessing that is
included in `multiprocessing.pool.__all__`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pablogsal pablogsal pablogsal approved these changes

@Fidget-Spinner Fidget-Spinner Fidget-Spinner approved these changes

@pitrou pitrou Awaiting requested review from pitrou

Assignees

@pablogsal pablogsal

Labels
docs Documentation in the Doc dir
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@godlygeek @Fidget-Spinner @pablogsal @pitrou @miss-islington @bedevere-bot @the-knights-who-say-ni