classification
Title: Misleading names for multiprocessing "convenience" functions
Type: Stage:
Components: Library (Lib) Versions: Python 3.0, Python 2.6
process
Status: closed Resolution: wont fix
Dependencies: Superseder:
Assigned To: jnoller Nosy List: benjamin.peterson, jnoller, ncoghlan, pitrou
Priority: release blocker Keywords: needs review

Created on 2008-08-18 13:27 by ncoghlan, last changed 2008-09-02 14:02 by ncoghlan. This issue is now closed.

Files
File name Uploaded Description Edit
issue3589_true_aliases_in_multiprocessing.diff ncoghlan, 2008-09-01 11:57 Replace convenience functions with absolute imports
Messages (13)
msg71327 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-08-18 13:27
The package level imports from the new multiprocessing package exhibit
some very strange behaviour because they are actually functions
pretending to be classes:

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)
msg71328 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-08-18 13:28
Setting to deferred blocker, since this really needs to be dealt with
for RC1 (probably too close to b3 to get it discussed and dealt with for
that).
msg71334 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-08-18 14:30
> 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 *"

+1
(or, even better, qualified than wildcard imports)
msg72228 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-09-01 11:57
Patch attached that removes the misleading "convenience" functions,
replacing them with explicit imports of the appropriate names.

The patch also adds docstrings to some of the original class definitions
that were missing them.

No changes were needed to the multiprocessing tests - they all still
passed with this change, and the docs are still accurate as well (I
would actually say this change makes the docs MORE accurate).

Python 2.6b3+ (trunk:66083, Aug 31 2008, 19:00:32)
[GCC 4.2.3 (Ubuntu 4.2.3-2ubuntu7)] on linux2
Type "help", "copyright", "credits" or "license" for more information.
>>> import multiprocessing as mp
>>> isinstance(mp.Lock(), mp.Lock)
True
>>> mp.Lock.__name__
'Lock'
>>> mp.Lock.__module__
'multiprocessing.synchronize'
msg72237 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-09-01 13:07
Interesting - in some of the other work I was doing regarding the PEP 8
compliant alternative threading API, I noticed that the threading module
contains similar gems such as:

def Event(*args, **kwds):
  return _Event(*args, **kwds)

Using a factory function to discourage subclassing is one thing, but why
name the factory function as if it was a still a class?
msg72244 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2008-09-01 14:03
This is why multiprocessing had them nick - the threading module does

On Sep 1, 2008, at 9:07 AM, Nick Coghlan <report@bugs.python.org> wrote:

>
> Nick Coghlan <ncoghlan@gmail.com> added the comment:
>
> Interesting - in some of the other work I was doing regarding the  
> PEP 8
> compliant alternative threading API, I noticed that the threading  
> module
> contains similar gems such as:
>
> def Event(*args, **kwds):
>  return _Event(*args, **kwds)
>
> Using a factory function to discourage subclassing is one thing, but  
> why
> name the factory function as if it was a still a class?
>
> _______________________________________
> Python tracker <report@bugs.python.org>
> <http://bugs.python.org/issue3589>
> _______________________________________
msg72274 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2008-09-01 16:47
Patch reviewed/tested and I also confirmed that this doesn't affect the 
examples. I submitted the patch in r66114
msg72277 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2008-09-01 17:09
Reopening, there's a bug that the tests/examples/etc didn't catch (and 
nor did I), after the patch application:

woot:python-trunk jesse$ ./python.exe 
Python 2.6b3+ (trunk:66112:66114M, Sep  1 2008, 13:00:19) 
[GCC 4.0.1 (Apple Inc. build 5484)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import _multiprocessing
Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/Users/jesse/open_source/subversion/python-
trunk/Lib/multiprocessing/__init__.py", line 148, in <module>
    from multiprocessing.synchronize import (Lock, RLock, Condition, 
Event,
  File "/Users/jesse/open_source/subversion/python-
trunk/Lib/multiprocessing/synchronize.py", line 29, in <module>
    SEM_VALUE_MAX = _multiprocessing.SemLock.SEM_VALUE_MAX
AttributeError: 'module' object has no attribute 'SemLock'
>>>
msg72278 - (view) Author: Jesse Noller (jnoller) * (Python committer) Date: 2008-09-01 17:09
Ben is backing out the patch now
msg72294 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-09-01 21:22
Given how long I've been using the threading module without realising it
does the same thing, I'm actually prepared to live with the wrapper
functions rather than messing with this so close to release.

As Fredrik noted in the python-dev thread, the threading versions of
these are already explicitly documented as being factory functions
rather than classes (and as a reference to _thread.allocate_lock,
threading.Lock has good reason to be a factory function rather than a
class), so it may be appropriate to do the same thing for multiprocessing.
msg72297 - (view) Author: Benjamin Peterson (benjamin.peterson) * (Python committer) Date: 2008-09-01 21:31
Sounds good to me. :)
msg72303 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2008-09-01 22:11
Le lundi 01 septembre 2008 à 21:22 +0000, Nick Coghlan a écrit :
> As Fredrik noted in the python-dev thread, the threading versions of
> these are already explicitly documented as being factory functions
> rather than classes (and as a reference to _thread.allocate_lock,
> threading.Lock has good reason to be a factory function rather than a
> class), so it may be appropriate to do the same thing for multiprocessing.

"Foolish consistency, etc.". :-)
I think we should fix this before the releases rather than live with a
stupid indirection that doesn't seem to serve any useful purposes.
Unless the fix is too complicated of course.
msg72338 - (view) Author: Nick Coghlan (ncoghlan) * (Python committer) Date: 2008-09-02 14:02
Not so much "too complicated" as "potentially touches a lot of code and
not something I want to fiddle with this close to release".

As I noted on python-dev, it's actually a change that can easily be
handled through the normal API deprecation process, so it can be
reconsidered at a later date.
History
Date User Action Args
2008-09-02 14:02:21ncoghlansetmessages: + msg72338
2008-09-01 22:11:25pitrousetmessages: + msg72303
2008-09-01 21:31:44benjamin.petersonsetstatus: open -> closed
resolution: wont fix
messages: + msg72297
nosy: + benjamin.peterson
2008-09-01 21:22:16ncoghlansetmessages: + msg72294
2008-09-01 17:09:19jnollersetmessages: + msg72278
2008-09-01 17:09:03jnollersetstatus: closed -> open
resolution: fixed -> (no value)
messages: + msg72277
2008-09-01 16:47:53jnollersetstatus: open -> closed
resolution: fixed
messages: + msg72274
2008-09-01 16:45:59jnollersetkeywords: + needs review, - patch
2008-09-01 14:03:00jnollersetmessages: + msg72244
2008-09-01 13:07:52ncoghlansetmessages: + msg72237
2008-09-01 11:57:53ncoghlansetfiles: + issue3589_true_aliases_in_multiprocessing.diff
keywords: + patch
messages: + msg72228
2008-08-21 03:06:26barrysetpriority: deferred blocker -> release blocker
2008-08-18 14:30:41pitrousetnosy: + pitrou
messages: + msg71334
2008-08-18 13:28:25ncoghlansetpriority: critical -> deferred blocker
messages: + msg71328
2008-08-18 13:27:13ncoghlancreate