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

Flatten nested functools.partial #52078

Closed
AlexanderBelopolsky mannequin opened this issue Feb 1, 2010 · 18 comments
Closed

Flatten nested functools.partial #52078

AlexanderBelopolsky mannequin opened this issue Feb 1, 2010 · 18 comments
Assignees
Labels
extension-modules C modules in the Modules dir performance Performance or resource usage stdlib Python modules in the Lib dir

Comments

@AlexanderBelopolsky
Copy link
Mannequin

AlexanderBelopolsky mannequin commented Feb 1, 2010

BPO 7830
Nosy @rhettinger, @abalkin, @pitrou, @vstinner, @jackdied, @MojoVampire
Files
  • no-nested-partial.diff
  • no-nested-partial-exact.diff
  • issue7830.diff
  • issue7830-2.diff
  • 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 = 'https://github.com/abalkin'
    closed_at = <Date 2015-03-01.20:09:11.372>
    created_at = <Date 2010-02-01.19:07:29.413>
    labels = ['extension-modules', 'library', 'performance']
    title = 'Flatten nested functools.partial'
    updated_at = <Date 2015-03-01.20:55:12.337>
    user = 'https://bugs.python.org/AlexanderBelopolsky'

    bugs.python.org fields:

    activity = <Date 2015-03-01.20:55:12.337>
    actor = 'vstinner'
    assignee = 'belopolsky'
    closed = True
    closed_date = <Date 2015-03-01.20:09:11.372>
    closer = 'belopolsky'
    components = ['Extension Modules', 'Library (Lib)']
    creation = <Date 2010-02-01.19:07:29.413>
    creator = 'Alexander.Belopolsky'
    dependencies = []
    files = ['16083', '16091', '17813', '36837']
    hgrepos = []
    issue_num = 7830
    keywords = ['patch']
    message_count = 18.0
    messages = ['98674', '98698', '98706', '108980', '111287', '111289', '111319', '111401', '111508', '122932', '122934', '228735', '228737', '228743', '228747', '228796', '236975', '236979']
    nosy_count = 9.0
    nosy_names = ['rhettinger', 'belopolsky', 'pitrou', 'vstinner', 'jackdied', 'vdupras', 'Christophe Simonis', 'python-dev', 'josh.r']
    pr_nums = []
    priority = 'normal'
    resolution = 'fixed'
    stage = 'resolved'
    status = 'closed'
    superseder = None
    type = 'performance'
    url = 'https://bugs.python.org/issue7830'
    versions = ['Python 3.5']

    @AlexanderBelopolsky
    Copy link
    Mannequin Author

    AlexanderBelopolsky mannequin commented Feb 1, 2010

    Currently applying functools.partial to a callable that is already functools.partial object results in a nested object:

    >>> from functools import partial
    >>> def f(a,b,c): pass
    ... 
    >>> p = partial(partial(f, 1), 2)
    >>> p.func, p.args
    (<functools.partial object at 0x100431d60>, (2,))
    
    
    Proposed patch makes partial(partial(f, 1), 2) return partial(f, 1, 2) instead:
    >>> p.func, p.args
    (<function f at 0x10055d3a8>, (1, 2))

    This patch is partially (no pun intended) motivated by a patch submitted by Christophe Simonis for bpo-4331. Christophe's patch flattens nested partials for a specific case of using partials as bound methods.

    As proposed, the patch will enable flattening for subclasses of functools.partial, but will return a baseclass instance. Flattening will also discard any state attached to the nested partial such as __name__, __doc__, etc or any subclass data. I believe this is the right behavior, but this caveat is the reason I classify this patch as a "feature request" rather than "performance" or "resource usage".

    @AlexanderBelopolsky AlexanderBelopolsky mannequin added type-feature A feature request or enhancement stdlib Python modules in the Lib dir labels Feb 1, 2010
    @pitrou
    Copy link
    Member

    pitrou commented Feb 1, 2010

    Flattening should only happen for instances of the exact type. When people create subclasses, there's usually a reason for it.

    @AlexanderBelopolsky
    Copy link
    Mannequin Author

    AlexanderBelopolsky mannequin commented Feb 2, 2010

    Antoine> Flattening should only happen for instances of the exact type.

    I am attaching a variant of the patch that will only flatten if both nested and wrapping partial is of the exact type. Other possibilities would include flattening partial_subtype(f, ...) if type(f) == partial and if type(f) == partial_subtype, but I'll present only the least and most conservative variants.

    Antoine> When people create subclasses [of partial type], there's usually a reason for it.

    It is hard not to agree with this thesis, but I don't see how it follows that subclass writers will not benefit from flattening their instances. Can you suggest a use case where flattening in functools.partial subtype would be undesirable?

    @abalkin
    Copy link
    Member

    abalkin commented Jun 30, 2010

    I am attaching a patch, bpo-7830.diff, that takes an ultra-concervative approach: partials are only flattened if both outer and inner are of exact functools.partial type and the inner partial does not have __dict__.

    @abalkin abalkin self-assigned this Jun 30, 2010
    @vdupras
    Copy link
    Mannequin

    vdupras mannequin commented Jul 23, 2010

    Applies cleanly on the py3k branch at r83069, the tests work correctly (fail before applying the patch, success afterwards), and, to the best of my C-API knowledge, the C code is alright.

    Oh, and it behaves as described...

    Python 3.2a0 (py3k:83069M, Jul 23 2010, 12:40:49) 
    [GCC 4.2.1 (Apple Inc. build 5659)] on darwin
    Type "help", "copyright", "credits" or "license" for more information.
    >>> def foo(a, b, c): pass
    ... 
    >>> from functools import partial
    >>> p1 = partial(foo, 1)
    >>> p1.func, p1.args
    (<function foo at 0x1004531e8>, (1,))
    >>> p2 = partial(foo, 2)
    >>> p2.func, p2.args
    (<function foo at 0x1004531e8>, (2,))

    @vdupras
    Copy link
    Mannequin

    vdupras mannequin commented Jul 23, 2010

    Oops, used it wrong (but it still works correctly).

    >>> p2 = partial(p1, 2)
    >>> p2.func, p2.args
    (<function foo at 0x10051da68>, (1, 2))

    @abalkin
    Copy link
    Member

    abalkin commented Jul 23, 2010

    Since I am the OP of this patch, I would like a +1 from another developer before checking this in. (Or a couple of -1s before rejecting:-)

    Antoine,

    Does the latest patch address your concerns?

    Virgil,

    If you care enough about this feature, please post a summary on python-dev and ask for comments.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Jul 23, 2010

    Antoine> Flattening should only happen for instances of the exact type.

    FWIW, I agree with Antoine. You cannot know in advance whether a partial-subclass has semantics that need to be preserved when flattening.

    @abalkin
    Copy link
    Member

    abalkin commented Jul 24, 2010

    FWIW, I agree with Antoine. You cannot know in advance whether a
    partial-subclass has semantics that need to be preserved when
    flattening.

    Raymond,

    I have actually conceded this point to Antoine. See msg108980 above. Not only the latest patch preserves partial-subclasses, it also foregoes the optimization if there is anything in __dict__ of the inner partial.

    It looks like we have a consensus on the features, the remaining question is whether this is enough of an optimization to justify adding extra code.

    @rhettinger
    Copy link
    Contributor

    rhettinger commented Nov 30, 2010

    Alexander, I don't see anything wrong with patch, nor anything compelling about it either. It's your choice whether or not to apply.

    @abalkin
    Copy link
    Member

    abalkin commented Nov 30, 2010

    The original motivation for the patch was that if partial() objects are guaranteed to be flat, it would simplify code that deals with them. See bpo-4331 for one example.

    With a "conservative" patch, however, it will still be possible to create nested partials and the code consuming partials should be ready for that.

    @abalkin abalkin closed this as completed Nov 30, 2010
    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Oct 6, 2014

    If it affects the decision, I just had to debug some code at work that triggered a "excessive recursion" bug because they used functools.partial over and over on the same base function, thinking they could safely replace some keyword bindings over and over. This patch would have prevented the problem.

    Any chance of reopening it?

    @abalkin
    Copy link
    Member

    abalkin commented Oct 6, 2014

    Josh,

    Would bpo-7830.diff solve your use-case as is? See msg108980 for the limitations. If so, I don't mind reopening this issue.

    @MojoVampire
    Copy link
    Mannequin

    MojoVampire mannequin commented Oct 6, 2014

    The use case in question, simplified, was:

    from functools import partial
    
    class Foo:
        Bar = othermodule.Bar
    
        def __new__(cls, ...):
            ...
            cls.Bar(...)
            ...
    
        def bind_stuff(cls, *args, **kwargs):
            cls.Bar = partial(Bar, *args, **kwargs)

    Every time they created an instance of Foo, there would be a Foo.bind_stuff call beforehand that fixed some settings they didn't want to make a part of Foo's __new__ profile. And in fact, in practice, they were only binding the same keyword args over and over, so they could have solved the problem by just rebinding the base othermodule.Bar. I'm not defending this design, but from what I can tell, it's a textbook example of where your patch would solve the problem. cls.Bar has no instance variables assigned (hence no __dict__?), and it's always functools.partial that's used, not some special variant.

    A simple way to repro the fundamental problem they were experiencing is to just wrap int a lot:

    >>> for i in range(1001):
            int = partial(int)
    >>> int(5) # Kaboom! Which I assume your patch would prevent

    @abalkin
    Copy link
    Member

    abalkin commented Oct 6, 2014

    I would say that getting "maximum recursion depth exceeded" error from evaluating a deeply nested partial is a bug, but I am not sure we should fix it by flattening partial objects in the constructor or by being smarter at evaluation time.

    @abalkin abalkin added the extension-modules C modules in the Modules dir label Oct 6, 2014
    @abalkin abalkin reopened this Oct 6, 2014
    @vstinner vstinner added performance Performance or resource usage and removed type-feature A feature request or enhancement labels Oct 7, 2014
    @abalkin
    Copy link
    Member

    abalkin commented Oct 8, 2014

    I've updated the patch.

    @python-dev
    Copy link
    Mannequin

    python-dev mannequin commented Mar 1, 2015

    New changeset 7839681ca931 by Alexander Belopolsky in branch 'default':
    Issue bpo-7830: Flatten nested functools.partial.
    https://hg.python.org/cpython/rev/7839681ca931

    @abalkin abalkin closed this as completed Mar 1, 2015
    @vstinner
    Copy link
    Member

    vstinner commented Mar 1, 2015

    I forgot this issue. Thanks for the enhancement. It will help asyncio for
    example. It was surprised the first time i tested nested partial.

    @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
    extension-modules C modules in the Modules dir performance Performance or resource usage stdlib Python modules in the Lib dir
    Projects
    None yet
    Development

    No branches or pull requests

    4 participants