Issue 3352: Deficiencies in multiprocessing/threading API (original) (raw)
Created on 2008-07-14 09:57 by ncoghlan, last changed 2022-04-11 14:56 by admin. This issue is now closed.
Messages (47)
Author: Alyssa Coghlan (ncoghlan) * 
Date: 2008-07-14 09:57
The "PEP 8 compliant" API for multiprocessing and threading needs to be cleaned up before release as per the thread on python-dev. The release manager gave approval for this change during that discussion [1].
Changes needed:
- remove Py3k warnings from the threading module (old API is going to be kept around even in Py3k)
- change get_name/set_name into a "name" property
- change is_daemon/set_daemon into a "daemon" property
(Note that changing "is_alive" to be a property is not on that list - that can be a fairly expensive thing to check for the multiprocessing library. I also left out active_count(), as I think the underscore adds clarity in that case)
[1] http://mail.python.org/pipermail/python-dev/2008-June/080285.html
Author: Andrii V. Mishkovskyi (mishok13)
Date: 2008-07-14 10:36
Does this mean that all multiprocessing get_*/set_* methods should be changed to be properties? For example, multiprocessing.process.Process class defines not only set_name/get_name and is_daemon/set_daemon methods, but also get/set_authkey, get/set_exitcode and get_ident (this method is later used as 'getx' argument of 'pid' property, so we could get rid of this using simple decorator).
Author: Andrii V. Mishkovskyi (mishok13)
Date: 2008-07-14 10:38
Actually, 'getx' -> 'fget'. Sorry for the typo. :)
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-07-14 13:45
I think we should still keep the py3k warnings on (I'll change them to PendingDeprecationWarnings if you want.), so the API doesn't abruptly change on people. With your approval, I'll make it so.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-07-14 22:25
At this stage I'm still inclined to skip the warnings completely - at the very least, any eventual removals will go through a full deprecation cycle in 2.7/3.1 before being removed in 2.8/3.2.
It's also much easier to be sure we aren't adversely affecting performance of the old methods that way (with a PendingDeprecationWarning, performance of the deprecated methods is going to suffer a bit since the warnings machinery will still have to be invoked to discover that the warning has been filtered out). That said, the methods we're changing aren't likely to be something one invokes in speed critical sections of an application, so I wouldn't object if the old names emitted PDW in both 2.6 and 3.0.
Regarding the additional get/set properties on multiprocessing.Process, it depends on whether or not they are cheap to calculate and whether or not they involve any I/O operations. auth_key looked like it may be expensive to set (since I assume there is a crypto calculation involved there), exit_code may involve waiting for the other process to finish, and get_ident may involve waiting for it to start. So my initial reaction is that these are OK to keep as methods rather than trying to turn them into properties.
The following pretty undesirable behaviour should probably be discussed on python-dev before beta3 (if not beta2):
Python 2.6b1+ (trunk:64945, Jul 14 2008, 20:00:46) [GCC 4.1.3 20070929 (prerelease) (Ubuntu 4.1.2-16ubuntu2)] on linux2 Type "help", "copyright", "credits" or "license" for more information.
import multiprocessing as mp isinstance(mp.Lock(), mp.Lock) Traceback (most recent call last): File "", line 1, in TypeError: isinstance() arg 2 must be a class, type, or tuple of classes and types mp.Lock.name 'Lock' mp.Lock.module 'multiprocessing' mp.Lock().class.name 'Lock' mp.Lock().class.module 'multiprocessing.synchronize'
The delayed import functions in multiprocessing.init look like a serious misfeature to me. I'd be inclined to replace them with "from .synchronize import *" and "from .process import *" (leaving anything which isn't covered by those two imports to be retrieved directly from the relevant mp submodule)
Author: Barry A. Warsaw (barry) *
Date: 2008-07-16 12:54
Is anybody working on a patch for this? Nick, I agree with you about undesirable behavior, however if there is no patch currently under development, I'm inclined to defer blocking until beta3.
Author: Jesse Noller (jnoller) *
Date: 2008-07-16 13:12
I don't want to change the API or any other code before we get the change in for which should fix/resolve
Author: Barry A. Warsaw (barry) *
Date: 2008-07-16 13:23
On Jul 16, 2008, at 9:12 AM, Jesse Noller wrote:
I don't want to change the API or any other code before we get the
change in for which should fix/resolve
What's the holdup on 874900?
Author: Jesse Noller (jnoller) *
Date: 2008-07-16 13:25
I don't know, but I am going to ping Antoine and Greg and see if they don't mind me applying it as-is.
Author: Jesse Noller (jnoller) *
Date: 2008-07-16 13:28
This should not block the release of beta 2 IMHO
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-08-02 13:00
Sorry, I'm not going to be able to work on this soon.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-08-18 13:30
I created issue 3589 to cover the problem I mentioned above with the very misleading names for the package level functions in multiprocessing.
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-08-18 16:40
I think I can get this done before the release now. For starters I changed thread.get_ident() to a property in r65818.
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-08-18 18:44
Ok. I've done the threading API and patched up multiprocessing so it actually works. Jesse, can you do multiprocessing?
Also, can we decide about is_alive as a property?
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-08-18 18:45
Oh, and by the way, I would start working on multiprocessing by reverting r65828.
Author: Jesse Noller (jnoller) *
Date: 2008-08-18 18:46
On Mon, Aug 18, 2008 at 2:44 PM, Benjamin Peterson <report@bugs.python.org> wrote:
Benjamin Peterson <musiccomposition@gmail.com> added the comment:
Ok. I've done the threading API and patched up multiprocessing so it actually works. Jesse, can you do multiprocessing?
Also, can we decide about is_alive as a property?
Yup, I can do the MP part.
Author: Jesse Noller (jnoller) *
Date: 2008-08-18 18:47
On Mon, Aug 18, 2008 at 2:45 PM, Benjamin Peterson <report@bugs.python.org> wrote:
Benjamin Peterson <musiccomposition@gmail.com> added the comment:
Oh, and by the way, I would start working on multiprocessing by reverting r65828.
Makes sense.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-08-19 15:34
is_alive appears to be a potentially expensive check for the multiprocessing side of things, which is why I'm inclined to leave it as a method. "if t.is_alive():" actually reads better to me than "if t.alive:" anyway.
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-08-19 15:36
On Tue, Aug 19, 2008 at 10:34 AM, Nick Coghlan <report@bugs.python.org> wrote:
Nick Coghlan <ncoghlan@gmail.com> added the comment:
is_alive appears to be a potentially expensive check for the multiprocessing side of things, which is why I'm inclined to leave it as a method. "if t.is_alive():" actually reads better to me than "if t.alive:" anyway.
So be it. I don't really want to do any renaming anyway. :)
Python tracker <report@bugs.python.org> <http://bugs.python.org/issue3352>
-- Cheers, Benjamin Peterson "There's no place like 127.0.0.1."
Author: Jesse Noller (jnoller) *
Date: 2008-08-19 15:36
On Tue, Aug 19, 2008 at 11:34 AM, Nick Coghlan <report@bugs.python.org> wrote:
Nick Coghlan <ncoghlan@gmail.com> added the comment:
is_alive appears to be a potentially expensive check for the multiprocessing side of things, which is why I'm inclined to leave it as a method. "if t.is_alive():" actually reads better to me than "if t.alive:" anyway.
Dang, I already cut that one over.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-08-19 15:51
My preference actually isn't particularly strong either way, so I suggest asking Barry for a casting vote.
Author: Jesse Noller (jnoller) *
Date: 2008-08-19 15:52
Nick/Ben - here's a rough draft patch for the attribute changes for mp - could you both take a look and let me know what you think?
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-08-19 16:05
The patch looks pretty good. In a few places, you replaced x.set_x(True) with x.x(True). The docs also will need a few tweaks. Otherwise, if the tests pass, I say go ahead.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-08-19 16:09
Just saw a couple of docs checkins go by suggesting that the camelCase names will be unavailable in 3.x. Remember that the intent isn't to remove the old names from threading.py - we're just adding the PEP 8 names so that multiprocessing can have a PEP 8 compliant API while still being close to a drop-in replacement for the 2.6 version of the threading module.
This means that all of the DeprecationWarnings and so forth can be left out completely.
Regarding your patch Jesse:
- if I remember rightly, get_exitcode() can block waiting for the other process to finish, which makes turning it into a property a fairly questionable idea (p.get_exitcode() blocking is a lot less surprising than "p.exitcode" doing so)
- the get/set documentation for the new properties can probably be consolidated, but that isn't a major problem as far as this week's beta is concerned.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-08-19 16:10
Note regarding those comments - only the exitcode one is something we should try to get sorted for the beta. Backing out the deprecation warnings and cleaning up the documentation can wait for the first release candidate.
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-08-19 16:52
Barry said that is_alive should remain a method following in Guido's footsteps.
Author: Jesse Noller (jnoller) *
Date: 2008-08-19 18:20
Nick Coghlan <ncoghlan@gmail.com> added the comment:
Note regarding those comments - only the exitcode one is something we should try to get sorted for the beta. Backing out the deprecation warnings and cleaning up the documentation can wait for the first release candidate.
Would it be acceptable to add a doc note mentioning .exitcode() may block rather than changing it back to the get_exitcode method? It's not that it's hard - but I do like the consistency of the property-style interface.
Author: Jesse Noller (jnoller) *
Date: 2008-08-19 18:43
Nick Coghlan <ncoghlan@gmail.com> added the comment:
Note regarding those comments - only the exitcode one is something we should try to get sorted for the beta. Backing out the deprecation warnings and cleaning up the documentation can wait for the first release candidate.
Actually, re-examining .exitcode - it wraps the custom forking.Popen class .poll() method, this method should not block (forking.py, line 104) - the WNOHANG call passed to os.waitpid means it should also not hang.
Author: Jesse Noller (jnoller) *
Date: 2008-08-19 18:45
Here is a cleaned up patch (ben was right, I typo'ed a few spots) - I did not clean up the docs from the original patch except to correct the x.x(Foo) accidents.
Author: Jesse Noller (jnoller) *
Date: 2008-08-19 18:46
I attached the wrong patch, here is v2
Author: Jesse Noller (jnoller) *
Date: 2008-08-19 18:57
And here is v3 - I fixed all of the Docs/include/mp* scripts to reflect the new API
Author: Jesse Noller (jnoller) *
Date: 2008-08-19 19:07
committed the v3 patch as-is in r65864 - moving from release blocker to deferred blocker pending the Docs cleanup, and outstanding debate points.
Author: Antoine Pitrou (pitrou) *
Date: 2008-08-22 18:59
There are still some instances of get_name() in threading.py itself, which gives errors like the following:
Unhandled exception in thread started by <bound method Thread.__bootstrap of <Thread(reader 5, stopped daemon -1285227632)>> Traceback (most recent call last): File "/home/antoine/cpython/cpickle/Lib/threading.py", line 499, in __bootstrap self.__bootstrap_inner() File "/home/antoine/cpython/cpickle/Lib/threading.py", line 537, in __bootstrap_inner (self.get_name(), _format_exc())) AttributeError: 'Thread' object has no attribute 'get_name'
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-08-22 19:02
Here's a patch.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 11:42
I found further PEP 8 non-compliances in the multiprocessing API while working on a patch for issue 3589, mainly in the area of function names that start with a capital letter, making them look like classes when they definitely are not.
After noticing a few of these, I went through checked more thoroughly, and found all of the following to be functions that claimed to be classes by way of their naming convention (a far worse sin than using camelCase instead of underscores): multiprocessing.Pipe (aka multiprocessing.connection.Pipe) multiprocessing.RawValue (aka multiprocessing.sharedctypes.RawValue) multiprocessing.RawArray (aka multiprocessing.sharedctypes.RawArray) multiprocessing.Value (aka multiprocessing.sharedctypes.Value) multiprocessing.Array (aka multiprocessing.sharedctypes.Array) multiprocessing.connection.Client multiprocessing.connection.SocketClient multiprocessing.connection.PipeClient multiprocessing.connection.XmlClient multiprocessing.managers.RebuildProxy multiprocessing.managers.MakeProxyType multiprocessing.managers.AutoProxy multiprocessing.managers.Array
These should all be converted to start with a lowercase letter and use underscores, otherwise people are going to assume they can be treated like classes.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 12:02
Benjamin's patch was applied in r65982
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 12:29
Patch added that removes the incorrect Py3k warnings from the threading module (also restores the methods to the same name attributes as they had in 2.5).
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 12:58
Second patch added that removes the deprecation warnings from the Py3k version of the threading module.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 13:19
It turns out threading uses the odd "class-that-is-not-a-class" naming scheme as well: threading.Lock threading.RLock threading.Condition threading.Semaphore threading.BoundedSemaphore threading.Event threading.Timer
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 13:27
Patch added to tone down note regarding the PEP 8 compliant aliases that have been added to the threading module.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 13:46
And one last patch to adjust the threading docs in Py3k to reflect the fact that the 2.x API is still supported, even if it is no longer documented.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 13:51
Updated the 2.6 threading patch to also remove the warnings from the methods that are being replaced by properties.
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-09-01 13:54
The patches look good to me. Please apply.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 21:32
Regarding the factory functions that are named as if they were classes, Fredrik noted on python-dev that the ones from the threading module are explicitly documented as being factory functions, and the multiprocessing API really just follows that example (note that without applying the patch from issue 3589, all of the names that are factory functions in the threading API are also factory functions in the multiprocessing API).
So perhaps the best course at this stage is to just leave these alone for 2.6/3.0?
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-01 21:53
Ben, if you get a chance to apply those patches, feel free, otherwise I should be able to get to them this evening (my time - about 10 hours from now).
Author: Benjamin Peterson (benjamin.peterson) *
Date: 2008-09-01 23:13
Ok. patches applied in r66126 and r66127.
Author: Alyssa Coghlan (ncoghlan) *
Date: 2008-09-02 09:43
Thanks!
History
Date
User
Action
Args
2022-04-11 14:56:36
admin
set
github: 47602
2008-09-02 09:43:26
ncoghlan
set
messages: +
2008-09-01 23:13:33
benjamin.peterson
set
status: open -> closed
resolution: fixed
messages: +
2008-09-01 21:53:21
ncoghlan
set
messages: +
2008-09-01 21:32:35
ncoghlan
set
messages: +
2008-09-01 13:54:22
benjamin.peterson
set
messages: +
2008-09-01 13:51:55
ncoghlan
set
files: + issue3352_remove_threading_py3k_warnings.diff
messages: +
2008-09-01 13:50:32
ncoghlan
set
files: - issue3352_remove_threading_py3k_warnings.diff
2008-09-01 13:46:19
ncoghlan
set
files: + issue3352_update_30_threading_docs.diff
messages: +
2008-09-01 13:27:20
ncoghlan
set
files: + issue3352_tone_down_26_threading_docs.diff
messages: +
2008-09-01 13:19:35
ncoghlan
set
messages: +
2008-09-01 12:58:58
ncoghlan
set
keywords: + patch
2008-09-01 12:58:41
ncoghlan
set
assignee: jnoller ->
2008-09-01 12:58:31
ncoghlan
set
keywords: + needs review, - patch
2008-09-01 12:58:14
ncoghlan
set
files: + issue3352_remove_threading_deprecation_warnings.diff
messages: +
2008-09-01 12:29:48
ncoghlan
set
files: + issue3352_remove_threading_py3k_warnings.diff
messages: +
2008-09-01 12:02:12
ncoghlan
set
messages: +
2008-09-01 11:42:46
ncoghlan
set
messages: +
2008-08-22 19:02:02
benjamin.peterson
set
files: + get_name.patch
messages: +
2008-08-22 18:59:36
pitrou
set
nosy: + pitrou
messages: +
2008-08-21 03:07:04
barry
set
priority: deferred blocker -> release blocker
2008-08-19 19:07:27
jnoller
set
priority: release blocker -> deferred blocker
messages: +
2008-08-19 18:57:11
jnoller
set
files: + mp_api_cleanup-v3.patch
messages: +
2008-08-19 18:46:25
jnoller
set
files: + mp_api_cleanup.patch
messages: +
2008-08-19 18:45:35
jnoller
set
files: - mp_nohang_jnoller.patch
2008-08-19 18:45:28
jnoller
set
files: - mp_api_cleanup.patch
2008-08-19 18:45:22
jnoller
set
files: + mp_nohang_jnoller.patch
messages: +
2008-08-19 18:43:23
jnoller
set
messages: +
2008-08-19 18:20:53
jnoller
set
messages: +
2008-08-19 16:52:33
benjamin.peterson
set
messages: +
2008-08-19 16:10:45
ncoghlan
set
messages: +
2008-08-19 16:09:18
ncoghlan
set
messages: +
2008-08-19 16:05:44
benjamin.peterson
set
messages: +
2008-08-19 15:52:42
jnoller
set
files: + mp_api_cleanup.patch
keywords: + patch
messages: +
2008-08-19 15:51:50
ncoghlan
set
messages: +
2008-08-19 15:36:29
jnoller
set
messages: +
2008-08-19 15:36:11
benjamin.peterson
set
messages: +
2008-08-19 15:34:55
ncoghlan
set
messages: +
2008-08-18 18:47:44
jnoller
set
messages: +
2008-08-18 18:46:54
jnoller
set
messages: +
2008-08-18 18:45:48
benjamin.peterson
set
messages: +
2008-08-18 18:44:09
benjamin.peterson
set
assignee: benjamin.peterson -> jnoller
messages: +
2008-08-18 16:40:34
benjamin.peterson
set
assignee: benjamin.peterson
messages: +
2008-08-18 13:30:26
ncoghlan
set
messages: +
2008-08-02 13:00:08
benjamin.peterson
set
assignee: benjamin.peterson -> (no value)
messages: +
2008-07-18 03:46:43
barry
set
priority: deferred blocker -> release blocker
2008-07-16 13:52:15
benjamin.peterson
set
priority: release blocker -> deferred blocker
2008-07-16 13:28:28
jnoller
set
messages: +
2008-07-16 13:25:10
jnoller
set
messages: +
2008-07-16 13:23:28
barry
set
messages: +
2008-07-16 13:12:04
jnoller
set
messages: +
2008-07-16 12:54:43
barry
set
nosy: + barry
messages: +
2008-07-14 22:25:53
ncoghlan
set
messages: +
2008-07-14 13:45:10
benjamin.peterson
set
assignee: benjamin.peterson
messages: +
nosy: + benjamin.peterson
2008-07-14 10:38:55
mishok13
set
messages: +
2008-07-14 10:36:37
mishok13
set
nosy: + jnoller, mishok13
messages: +
2008-07-14 09:57:55
ncoghlan
create