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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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) *  |
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 (vstinner) *  |
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.
|
|
Date |
User |
Action |
Args |
2022-04-11 14:56:57 | admin | set | github: 52078 |
2015-03-01 20:55:12 | vstinner | set | messages:
+ msg236979 |
2015-03-01 20:09:11 | belopolsky | set | status: open -> closed resolution: fixed stage: resolved |
2015-03-01 20:08:31 | python-dev | set | nosy:
+ python-dev messages:
+ msg236975
|
2014-10-08 14:52:38 | belopolsky | set | files:
+ issue7830-2.diff keywords:
+ patch messages:
+ msg228796
|
2014-10-07 10:42:27 | vstinner | set | nosy:
+ vstinner type: enhancement -> performance
|
2014-10-06 23:26:55 | belopolsky | set | status: 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:19 | josh.r | set | messages:
+ msg228743 |
2014-10-06 21:24:09 | belopolsky | set | messages:
+ msg228737 |
2014-10-06 21:05:45 | josh.r | set | nosy:
+ josh.r messages:
+ msg228735
|
2010-11-30 19:51:11 | belopolsky | set | status: open -> closed messages:
+ msg122934
keywords:
- needs review resolution: rejected stage: commit review -> resolved |
2010-11-30 19:32:04 | rhettinger | set | messages:
+ msg122932 |
2010-07-24 22:12:48 | belopolsky | set | messages:
+ msg111508 |
2010-07-23 23:54:18 | rhettinger | set | nosy:
+ rhettinger messages:
+ msg111401
|
2010-07-23 13:37:01 | belopolsky | set | messages:
+ msg111319 stage: patch review -> commit review |
2010-07-23 12:17:15 | vdupras | set | messages:
+ msg111289 |
2010-07-23 12:13:39 | vdupras | set | nosy:
+ vdupras messages:
+ msg111287
|
2010-06-30 13:43:35 | belopolsky | set | files:
+ 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:07 | Christophe Simonis | set | nosy:
+ Christophe Simonis
|
2010-02-18 21:09:37 | jackdied | set | nosy:
+ jackdied
|
2010-02-02 02:04:18 | Alexander.Belopolsky | set | files:
+ no-nested-partial-exact.diff
messages:
+ msg98706 |
2010-02-01 23:45:26 | pitrou | set | nosy:
+ pitrou messages:
+ msg98698
|
2010-02-01 19:07:29 | Alexander.Belopolsky | create | |