classification
Title: Flatten nested functools.partial
Type: performance Stage: resolved
Components: Extension Modules, Library (Lib) Versions: Python 3.5
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: belopolsky Nosy List: Christophe Simonis, belopolsky, haypo, jackdied, josh.r, pitrou, python-dev, rhettinger, vdupras
Priority: normal Keywords: patch

Created on 2010-02-01 19:07 by Alexander.Belopolsky, last changed 2015-03-01 20:55 by haypo. This issue is now closed.

Files
File name Uploaded Description Edit
no-nested-partial.diff Alexander.Belopolsky, 2010-02-01 19:07
no-nested-partial-exact.diff Alexander.Belopolsky, 2010-02-02 02:04
issue7830.diff belopolsky, 2010-06-30 13:43
issue7830-2.diff belopolsky, 2014-10-08 14:52 review
Messages (18)
msg98674 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-02-01 19:07
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 issue4331. 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".
msg98698 - (view) Author: Antoine Pitrou (pitrou) * (Python committer) Date: 2010-02-01 23:45
Flattening should only happen for instances of the exact type. When people create subclasses, there's usually a reason for it.
msg98706 - (view) Author: Alexander Belopolsky (Alexander.Belopolsky) Date: 2010-02-02 02:04
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?
msg108980 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-06-30 13:43
I am attaching a patch, issue7830.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__.
msg111287 - (view) Author: Virgil Dupras (vdupras) Date: 2010-07-23 12:13
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,))
msg111289 - (view) Author: Virgil Dupras (vdupras) Date: 2010-07-23 12:17
Oops, used it wrong (but it still works correctly).


>>> p2 = partial(p1, 2)
>>> p2.func, p2.args
(<function foo at 0x10051da68>, (1, 2))
msg111319 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-07-23 13:37
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.
msg111401 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-07-23 23:54
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.
msg111508 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-07-24 22:12
> 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.
msg122932 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2010-11-30 19:32
Alexander, I don't see anything wrong with patch, nor anything compelling about it either.   It's your choice whether or not to apply.
msg122934 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2010-11-30 19:51
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 issue4331 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.
msg228735 - (view) Author: Josh Rosenberg (josh.r) * Date: 2014-10-06 21:05
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?
msg228737 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2014-10-06 21:24
Josh,

Would issue7830.diff solve your use-case as is?  See msg108980 for the limitations.  If so, I don't mind reopening this issue.
msg228743 - (view) Author: Josh Rosenberg (josh.r) * Date: 2014-10-06 22:55
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
msg228747 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2014-10-06 23:26
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.
msg228796 - (view) Author: Alexander Belopolsky (belopolsky) * (Python committer) Date: 2014-10-08 14:52
I've updated the patch.
msg236975 - (view) Author: Roundup Robot (python-dev) Date: 2015-03-01 20:08
New changeset 7839681ca931 by Alexander Belopolsky in branch 'default':
Issue #7830: Flatten nested functools.partial.
https://hg.python.org/cpython/rev/7839681ca931
msg236979 - (view) Author: STINNER Victor (haypo) * (Python committer) Date: 2015-03-01 20:55
I forgot this issue. Thanks for the enhancement. It will help asyncio for
example. It was surprised the first time i tested nested partial.
History
Date User Action Args
2015-03-01 20:55:12hayposetmessages: + msg236979
2015-03-01 20:09:11belopolskysetstatus: open -> closed
resolution: fixed
stage: resolved
2015-03-01 20:08:31python-devsetnosy: + python-dev
messages: + msg236975
2014-10-08 14:52:38belopolskysetfiles: + issue7830-2.diff
keywords: + patch
messages: + msg228796
2014-10-07 10:42:27hayposetnosy: + haypo
type: enhancement -> performance
2014-10-06 23:26:55belopolskysetstatus: closed -> open
versions: + Python 3.5, - Python 3.2
messages: + msg228747

components: + Extension Modules
resolution: rejected -> (no value)
stage: resolved -> (no value)
2014-10-06 22:55:19josh.rsetmessages: + msg228743
2014-10-06 21:24:09belopolskysetmessages: + msg228737
2014-10-06 21:05:45josh.rsetnosy: + josh.r
messages: + msg228735
2010-11-30 19:51:11belopolskysetstatus: open -> closed
messages: + msg122934

keywords: - needs review
resolution: rejected
stage: commit review -> resolved
2010-11-30 19:32:04rhettingersetmessages: + msg122932
2010-07-24 22:12:48belopolskysetmessages: + msg111508
2010-07-23 23:54:18rhettingersetnosy: + rhettinger
messages: + msg111401
2010-07-23 13:37:01belopolskysetmessages: + msg111319
stage: patch review -> commit review
2010-07-23 12:17:15vduprassetmessages: + msg111289
2010-07-23 12:13:39vduprassetnosy: + vdupras
messages: + msg111287
2010-06-30 13:43:35belopolskysetfiles: + issue7830.diff

assignee: belopolsky
versions: - Python 2.7
keywords: + needs review, - patch
nosy: + belopolsky, - Alexander.Belopolsky
messages: + msg108980
stage: patch review
2010-02-20 22:53:07Christophe Simonissetnosy: + Christophe Simonis
2010-02-18 21:09:37jackdiedsetnosy: + jackdied
2010-02-02 02:04:18Alexander.Belopolskysetfiles: + no-nested-partial-exact.diff

messages: + msg98706
2010-02-01 23:45:26pitrousetnosy: + pitrou
messages: + msg98698
2010-02-01 19:07:29Alexander.Belopolskycreate