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

Misleading names for multiprocessing "convenience" functions #47839

Closed
ncoghlan opened this issue Aug 18, 2008 · 13 comments
Closed

Misleading names for multiprocessing "convenience" functions #47839

ncoghlan opened this issue Aug 18, 2008 · 13 comments
Labels
release-blocker stdlib Python modules in the Lib dir

Comments

@ncoghlan
Copy link
Contributor

BPO 3589
Nosy @ncoghlan, @pitrou, @benjaminp
Files
  • issue3589_true_aliases_in_multiprocessing.diff: Replace convenience functions with absolute imports
  • Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.

    Show more details

    GitHub fields:

    assignee = None
    closed_at = <Date 2008-09-01.21:31:44.348>
    created_at = <Date 2008-08-18.13:27:13.261>
    labels = ['library', 'release-blocker']
    title = 'Misleading names for multiprocessing "convenience" functions'
    updated_at = <Date 2008-09-02.14:02:21.313>
    user = 'https://github.com/ncoghlan'

    bugs.python.org fields:

    activity = <Date 2008-09-02.14:02:21.313>
    actor = 'ncoghlan'
    assignee = 'jnoller'
    closed = True
    closed_date = <Date 2008-09-01.21:31:44.348>
    closer = 'benjamin.peterson'
    components = ['Library (Lib)']
    creation = <Date 2008-08-18.13:27:13.261>
    creator = 'ncoghlan'
    dependencies = []
    files = ['11323']
    hgrepos = []
    issue_num = 3589
    keywords = ['needs review']
    message_count = 13.0
    messages = ['71327', '71328', '71334', '72228', '72237', '72244', '72274', '72277', '72278', '72294', '72297', '72303', '72338']
    nosy_count = 4.0
    nosy_names = ['ncoghlan', 'pitrou', 'benjamin.peterson', 'jnoller']
    pr_nums = []
    priority = 'release blocker'
    resolution = 'wont fix'
    stage = None
    status = 'closed'
    superseder = None
    type = None
    url = 'https://bugs.python.org/issue3589'
    versions = ['Python 2.6', 'Python 3.0']

    @ncoghlan
    Copy link
    Contributor Author

    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)

    @ncoghlan ncoghlan added the stdlib Python modules in the Lib dir label Aug 18, 2008
    @ncoghlan
    Copy link
    Contributor Author

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

    @pitrou
    Copy link
    Member

    pitrou commented Aug 18, 2008

    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)

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Sep 1, 2008

    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'

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Sep 1, 2008

    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?

    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Sep 1, 2008

    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\>


    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Sep 1, 2008

    Patch reviewed/tested and I also confirmed that this doesn't affect the
    examples. I submitted the patch in r66114

    @jnoller jnoller mannequin closed this as completed Sep 1, 2008
    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Sep 1, 2008

    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'
    >>>

    @jnoller jnoller mannequin reopened this Sep 1, 2008
    @jnoller
    Copy link
    Mannequin

    jnoller mannequin commented Sep 1, 2008

    Ben is backing out the patch now

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Sep 1, 2008

    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.

    @benjaminp
    Copy link
    Contributor

    Sounds good to me. :)

    @pitrou
    Copy link
    Member

    pitrou commented Sep 1, 2008

    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.

    @ncoghlan
    Copy link
    Contributor Author

    ncoghlan commented Sep 2, 2008

    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.

    @ezio-melotti ezio-melotti transferred this issue from another repository Apr 10, 2022
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    release-blocker stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants