Skip to content
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 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

bpo-17140: Document multiprocessing's ThreadPool #23812

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
Fidget-Spinner
Fidget-Spinner approved these changes Dec 17, 2020
View changes
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner left a comment

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?

Loading

@godlygeek
Copy link
Contributor Author

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

Loading

@Fidget-Spinner
Copy link
Contributor

@Fidget-Spinner Fidget-Spinner commented Dec 17, 2020

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

Loading

@pablogsal pablogsal requested a review from pitrou Dec 17, 2020
pablogsal
pablogsal reviewed Dec 17, 2020
View changes
Doc/library/multiprocessing.rst Outdated Show resolved Hide resolved
Loading
@pablogsal pablogsal self-assigned this Dec 17, 2020
@godlygeek godlygeek force-pushed the document_threadpool branch from a2bd609 to dc97d38 Dec 17, 2020
@godlygeek
Copy link
Contributor Author

@godlygeek godlygeek commented Dec 17, 2020

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

Loading

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

@pablogsal pablogsal left a comment •
edited
Loading

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.

Loading

@godlygeek godlygeek force-pushed the document_threadpool branch from dc97d38 to 0f1dca8 Dec 17, 2020
@godlygeek
Copy link
Contributor Author

@godlygeek godlygeek commented Dec 17, 2020

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.

Loading

@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 godlygeek force-pushed the document_threadpool branch from 0f1dca8 to 351ccc1 Dec 17, 2020
@godlygeek
Copy link
Contributor Author

@godlygeek godlygeek commented Dec 17, 2020

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

Loading

@pablogsal
Copy link
Member

@pablogsal pablogsal commented Dec 17, 2020

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.

Loading

@pitrou
Copy link
Member

@pitrou pitrou commented Dec 17, 2020

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

Loading

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

@miss-islington miss-islington commented Dec 18, 2020

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.
🐍🍒⛏🤖

Loading

@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Dec 18, 2020

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

Loading

miss-islington added a commit to miss-islington/cpython that referenced this issue 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 <mwozniski@bloomberg.net>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Dec 18, 2020

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

Loading

miss-islington added a commit to miss-islington/cpython that referenced this issue 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 <mwozniski@bloomberg.net>
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Dec 18, 2020

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

Loading

miss-islington added a commit to miss-islington/cpython that referenced this issue 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 <mwozniski@bloomberg.net>
miss-islington added a commit that referenced this issue Dec 18, 2020
@godlygeek @miss-islington
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 <mwozniski@bloomberg.net>
miss-islington added a commit that referenced this issue Dec 18, 2020
@godlygeek @miss-islington
[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 <mwozniski@bloomberg.net>
ned-deily pushed a commit that referenced this issue Dec 18, 2020
@godlygeek @miss-islington
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 <mwozniski@bloomberg.net>
adorilson added a commit to adorilson/cpython that referenced this issue 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

@Fidget-Spinner Fidget-Spinner

@pitrou pitrou

Assignees

@pablogsal pablogsal

Labels
CLA signed type-documentation
Projects
None yet
Milestone
No milestone
7 participants
@godlygeek @Fidget-Spinner @pablogsal @pitrou @miss-islington @bedevere-bot @the-knights-who-say-ni