classification
Title: Deficiencies in multiprocessing/threading API
Type: behavior Stage:
Components: Library (Lib) Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: Nosy List: barry, benjamin.peterson, jnoller, mishok13, ncoghlan, pitrou
Priority: release blocker Keywords: needs review, patch

Created on 2008-07-14 09:57 by ncoghlan, last changed 2008-09-02 09:43 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
mp_api_cleanup.patch jnoller, 2008-08-19 18:46 v2
mp_api_cleanup-v3.patch jnoller, 2008-08-19 18:57
get_name.patch benjamin.peterson, 2008-08-22 19:02
issue3352_remove_threading_deprecation_warnings.diff ncoghlan, 2008-09-01 12:58 3.0 patch to remove deprecation warnings from the threading module
issue3352_tone_down_26_threading_docs.diff ncoghlan, 2008-09-01 13:27 2.6 doc patch to tone down API note in threading docs
issue3352_update_30_threading_docs.diff ncoghlan, 2008-09-01 13:46 3.0 doc patch to adjust API note in threading docs
issue3352_remove_threading_py3k_warnings.diff ncoghlan, 2008-09-01 13:51 2.6 patch to remove py3k warnings from the threading module
Messages (47)
msg69647 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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
msg69648 - (view) 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).
msg69649 - (view) Author: Andrii V. Mishkovskyi (mishok13) Date: 2008-07-14 10:38
Actually, 'getx' -> 'fget'. Sorry for the typo. :)
msg69654 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) 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.
msg69665 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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 "<stdin>", line 1, in <module>
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)
msg69776 - (view) Author: Barry A. Warsaw (barry) * (Python committer) 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.
msg69780 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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 issue874900 which should fix/resolve issue3088
msg69782 - (view) Author: Barry A. Warsaw (barry) * (Python committer) 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 issue874900 which should fix/resolve issue3088

What's the holdup on 874900?
msg69783 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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.
msg69786 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2008-07-16 13:28
This should not block the release of beta 2 IMHO
msg70630 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-02 13:00
Sorry, I'm not going to be able to work on this soon.
msg71329 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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.
msg71349 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) 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.
msg71357 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) 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?
msg71358 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-18 18:45
Oh, and by the way, I would start working on multiprocessing by
reverting r65828.
msg71359 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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.
msg71360 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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.
msg71424 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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.
msg71425 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) 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."
msg71426 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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.
msg71431 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-08-19 15:51
My preference actually isn't particularly strong either way, so I
suggest asking Barry for a casting vote.
msg71432 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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?
msg71435 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) 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.
msg71437 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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.
msg71439 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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.
msg71444 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-19 16:52
Barry said that is_alive should remain a method following in Guido's
footsteps.
msg71456 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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.
msg71460 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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.
msg71461 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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.
msg71462 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2008-08-19 18:46
I attached the wrong patch, here is v2
msg71463 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2008-08-19 18:57
And here is v3 - I fixed all of the Docs/include/mp* scripts to reflect 
the new API
msg71464 - (view) Author: Jesse Noller (jnoller) * (Python committer) 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.
msg71767 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) 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'
msg71768 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-08-22 19:02
Here's a patch.
msg72226 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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.
msg72229 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-09-01 12:02
Benjamin's patch was applied in r65982
msg72232 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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).
msg72236 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-09-01 12:58
Second patch added that removes the deprecation warnings from the Py3k
version of the threading module.
msg72238 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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
msg72239 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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.
msg72240 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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.
msg72241 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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.
msg72242 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-09-01 13:54
The patches look good to me. Please apply.
msg72298 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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?
msg72300 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) 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).
msg72304 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-09-01 23:13
Ok. patches applied in r66126 and r66127.
msg72320 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-09-02 09:43
Thanks!
History
Date User Action Args
2008-09-02 09:43:26ncoghlansetmessages: + msg72320
2008-09-01 23:13:33benjamin.petersonsetstatus: open -> closed
resolution: fixed
messages: + msg72304
2008-09-01 21:53:21ncoghlansetmessages: + msg72300
2008-09-01 21:32:35ncoghlansetmessages: + msg72298
2008-09-01 13:54:22benjamin.petersonsetmessages: + msg72242
2008-09-01 13:51:55ncoghlansetfiles: + issue3352_remove_threading_py3k_warnings.diff
messages: + msg72241
2008-09-01 13:50:32ncoghlansetfiles: - issue3352_remove_threading_py3k_warnings.diff
2008-09-01 13:46:19ncoghlansetfiles: + issue3352_update_30_threading_docs.diff
messages: + msg72240
2008-09-01 13:27:20ncoghlansetfiles: + issue3352_tone_down_26_threading_docs.diff
messages: + msg72239
2008-09-01 13:19:35ncoghlansetmessages: + msg72238
2008-09-01 12:58:58ncoghlansetkeywords: + patch
2008-09-01 12:58:41ncoghlansetassignee: jnoller ->
2008-09-01 12:58:31ncoghlansetkeywords: + needs review, - patch
2008-09-01 12:58:14ncoghlansetfiles: + issue3352_remove_threading_deprecation_warnings.diff
messages: + msg72236
2008-09-01 12:29:48ncoghlansetfiles: + issue3352_remove_threading_py3k_warnings.diff
messages: + msg72232
2008-09-01 12:02:12ncoghlansetmessages: + msg72229
2008-09-01 11:42:46ncoghlansetmessages: + msg72226
2008-08-22 19:02:02benjamin.petersonsetfiles: + get_name.patch
messages: + msg71768
2008-08-22 18:59:36pitrousetnosy: + pitrou
messages: + msg71767
2008-08-21 03:07:04barrysetpriority: deferred blocker -> release blocker
2008-08-19 19:07:27jnollersetpriority: release blocker -> deferred blocker
messages: + msg71464
2008-08-19 18:57:11jnollersetfiles: + mp_api_cleanup-v3.patch
messages: + msg71463
2008-08-19 18:46:25jnollersetfiles: + mp_api_cleanup.patch
messages: + msg71462
2008-08-19 18:45:35jnollersetfiles: - mp_nohang_jnoller.patch
2008-08-19 18:45:28jnollersetfiles: - mp_api_cleanup.patch
2008-08-19 18:45:22jnollersetfiles: + mp_nohang_jnoller.patch
messages: + msg71461
2008-08-19 18:43:23jnollersetmessages: + msg71460
2008-08-19 18:20:53jnollersetmessages: + msg71456
2008-08-19 16:52:33benjamin.petersonsetmessages: + msg71444
2008-08-19 16:10:45ncoghlansetmessages: + msg71439
2008-08-19 16:09:18ncoghlansetmessages: + msg71437
2008-08-19 16:05:44benjamin.petersonsetmessages: + msg71435
2008-08-19 15:52:42jnollersetfiles: + mp_api_cleanup.patch
keywords: + patch
messages: + msg71432
2008-08-19 15:51:50ncoghlansetmessages: + msg71431
2008-08-19 15:36:29jnollersetmessages: + msg71426
2008-08-19 15:36:11benjamin.petersonsetmessages: + msg71425
2008-08-19 15:34:55ncoghlansetmessages: + msg71424
2008-08-18 18:47:44jnollersetmessages: + msg71360
2008-08-18 18:46:54jnollersetmessages: + msg71359
2008-08-18 18:45:48benjamin.petersonsetmessages: + msg71358
2008-08-18 18:44:09benjamin.petersonsetassignee: benjamin.peterson -> jnoller
messages: + msg71357
2008-08-18 16:40:34benjamin.petersonsetassignee: benjamin.peterson
messages: + msg71349
2008-08-18 13:30:26ncoghlansetmessages: + msg71329
2008-08-02 13:00:08benjamin.petersonsetassignee: benjamin.peterson -> (no value)
messages: + msg70630
2008-07-18 03:46:43barrysetpriority: deferred blocker -> release blocker
2008-07-16 13:52:15benjamin.petersonsetpriority: release blocker -> deferred blocker
2008-07-16 13:28:28jnollersetmessages: + msg69786
2008-07-16 13:25:10jnollersetmessages: + msg69783
2008-07-16 13:23:28barrysetmessages: + msg69782
2008-07-16 13:12:04jnollersetmessages: + msg69780
2008-07-16 12:54:43barrysetnosy: + barry
messages: + msg69776
2008-07-14 22:25:53ncoghlansetmessages: + msg69665
2008-07-14 13:45:10benjamin.petersonsetassignee: benjamin.peterson
messages: + msg69654
nosy: + benjamin.peterson
2008-07-14 10:38:55mishok13setmessages: + msg69649
2008-07-14 10:36:37mishok13setnosy: + jnoller, mishok13
messages: + msg69648
2008-07-14 09:57:55ncoghlancreate