This issue tracker has been migrated to GitHub, and is currently read-only.
For more information, see the GitHub FAQs in the Python's Developer Guide.

classification
Title: Python implementation of `functools.partial` is not a class
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.6
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: ncoghlan Nosy List: abarry, brett.cannon, ncoghlan, ned.deily, python-dev, rhettinger, serhiy.storchaka, vstinner, xiang.zhang
Priority: normal Keywords: patch

Created on 2016-05-27 16:39 by abarry, last changed 2022-04-11 14:58 by admin. This issue is now closed.

Files
File name Uploaded Description Edit
functools_partial_1.patch abarry, 2016-05-28 03:19 review
functools_partial_2.patch abarry, 2016-05-30 15:52 review
functools_partial_3.patch abarry, 2016-06-04 22:31 review
functools_partial_3_1_incomplete.patch abarry, 2016-09-07 13:41 review
functools_partial_4.patch abarry, 2016-09-07 20:59 review
functools_partial_recursive_repr.patch serhiy.storchaka, 2016-09-09 04:39 review
Messages (29)
msg266503 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-27 16:39
functools.partial is a class in C, but the Python implementation is a function. This doesn't matter for most use cases where you only want the end result of the call to partial.

A simple line in the REPL tells me enough (or so I thought) that I wouldn't need to check the implementation:

>>> functools.partial
<class 'functools.partial'>

Oh, it's a class, which means I can subclass it to add a custom repr for my needs.

Unsurprisingly, it works. It may not be the best idea to subclass something that is meant to be final, but I'm simply overriding a single method, what could possibly go wrong? Besides one of the implementations not actually being a class.

I'm suggesting to make the Python implementation also a class, for consistency and making sure that both the C and Python implementation match, in case someone else wants to do that too.

The documentation ( https://docs.python.org/3/library/functools.html#functools.partial ) doesn't state that the Python and C implementations differ, but IMO this isn't a documentation bug.

I haven't written a patch yet, will probably be done by tomorrow.

Note: I haven't actually encountered this issue, but I suspect that it might arise if someone doesn't have access to _functools for whatever reason. And IMO, Python and C implementations of a feature should be fully equivalent (modulo implementation details à la OrderedDict.__root).

Thoughts?
msg266530 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-28 03:19
Attached patch turns the Python implementation of functools.partial into a class. The implementation is as close to the C version as possible, except __repr__ where I went for a different, more Pythonic approach (output is identical).

I haven't yet added tests for this, and I had to fix something (because some errors occur if _functools is not present and that's the only way I know to make the tests run against the Python implementation). One test fails with the patch (and sys.modules["_functools"] = None):

...........................................................E....................
....................................................................
======================================================================
ERROR: test_pickle (test.test_functools.TestPartialC)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "E:\GitHub\cpython\lib\test\test_functools.py", line 224, in test_pickle
    f_copy = pickle.loads(pickle.dumps(f, proto))
_pickle.PicklingError: Can't pickle <class 'functools.partial'>: it's not the sa
me object as functools.partial

----------------------------------------------------------------------
Ran 148 tests in 0.216s

FAILED (errors=1)

I'll try to see what kind of tests I can add to this to reliably test both 'partial' implementations. I'll also see what I can do about that one failing test. Oddly enough, there seems to be a test for subclassing '_functools.partial', which I might extend.

Raymond, what do you think?
msg266534 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-05-28 05:39
-1 I prefer to keep the current clean, easy-to-understand, easy-to-get-right code which complies with the documented API.  I see no real benefit in twisting the code around to match non-guaranteed implementation details.
msg266539 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-28 06:57
One problem with current Python implementation is that it doesn't support pickling. The ability of pickling functools.partial() is used in the pickle module for supporting __newobj_ex__ with pickle protocols 2 and 3 (issue24164) and for supporting pickling of operator.methodcaller() objects with keyword arguments (issue22955).
msg266540 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2016-05-28 07:06
This kind of feature creep has a cost in tern complexity, maintenance, risk of bugs, loss of a clean example for others learn from, and in slower code.
msg266545 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-28 07:33
If the Python implementation is just a rough example, and the functools module always require the _functools module, exact behavior of the Python implementation is not very important, as well as its performance. If the functools module can be used without C implementation, Python implementation is just not completely compatible.
msg266570 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-28 19:36
Serhiy, it seems as though _functools is always required for functools to work - heck, tests start to fail all over the place if it isn't available, because functools.reduce doesn't exist.

Subclassing _functools.partial is already tested for, so I wouldn't qualify it as an implementation detail myself. Moreover, I feel that we should try to (somewhat) keep both implementations identical - or close enough.

It seems that _pickle checks for _functools.partial, and that trying to pickle the pure Python version triggers the error in msg266530.

This probably doesn't matter for most (if not all) of cases where CPython is concerned. But I care about the ability for alternate implementations to work the same way.

Compromise: rename the current partial function to partial_func and keep it around ;-)
msg266587 - (view) Author: STINNER Victor (vstinner) * (Python committer) Date: 2016-05-29 06:45
The PEP 399 requires that the Python implementation behaves like the C
accelerator, no? I didn't check the specific case of this issue.
msg266588 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-05-29 07:02
PEP 399 requires that the C code must pass the test suite used for the pure Python code.
msg266599 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-29 13:36
Functionally, both versions are equivalent, but one is a class and the other is not. From PEP 399:

"Technical details of the VM providing the accelerated code are allowed to differ as necessary, e.g., a class being a `type` when implemented in C."

It clearly states that it's an implementation detail. I've been beaten! However, the next paragraph also says this:

"Acting as a drop-in replacement also dictates that no public API be provided in accelerated code that does not exist in the pure Python code."

I think this contradicts itself, since IMO an object being a type is part of the public API (but obviously this thought isn't universal). Whatever happens with the pure Python implementation, shouldn't the PEP be clarified on that point?
msg266700 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-05-30 15:52
New patch after Serhiy's comments. I kept the __dict__ in pickling to properly pickle attributes that aren't defined by 'partial' itself; but if 'func', 'args' or 'keywords' are present and don't match the provided arguments, an error will be raised, as it should not be partial's responsibility to handle this; this is for the caller to do.
msg267003 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2016-06-03 00:22
As Raymond notes, the main downside here is in terms of code complexity. However, the concrete gain is that APIs that rely on callable pickling, such as concurrent.futures with a ProcessPoolExecutor, would be consistently compatible with functools.partial:

>>> from concurrent.futures import ProcessPoolExecutor
>>> from functools import partial
>>> with ProcessPoolExecutor() as pool:
...     pool.submit(print, "hello")
...     pool.submit(partial(print, "hello"))
... 
<Future at 0x7f4fdb47ce48 state=running>
<Future at 0x7f4fd4f9cb00 state=pending>
hello
hello

At the moment, such code will fail if _functools is unavailable, since closures don't support pickling (unpickling functions involves looking them up by name, which isn't possible with closures)

The other main benefit is retaining the custom __repr__ when falling back to the Python implementation:

>>> partial(print, "hello")
functools.partial(<built-in function print>, 'hello')

At the moment, the closure-based version instead gives:

>>> partial(print, "hello")
<function functools.partial.<locals>.newfunc at 0x7f4fd6e0aea0>

Preserving those two capabilities seems sufficiently worthwhile to me to justify the extra code complexity and the greater speed penalty when the accelerator module isn't available (I'm assuming that in runtimes using a JIT compiler the speed difference should be negligible in practice)
msg267334 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-06-04 22:31
New patch, now almost all the tests that are used for the C version are also used for the Python version (the only exception is the one checking that trying to change/delete attributes would raise an error). Some tests needed a bit of tweaking to work with the implementation details of both versions, but now all of the tests pass (even the pickle one, which I found how to make it pass while working on #27220).
msg269899 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-07-06 19:08
Is there anything still preventing this from being merged? I think the ability to consistently get the same behaviour, with or without _functools available, outweighs the performance and simplicity loss (and as far as performance loss goes, it appears that _functools is always available as far as CPython is concerned, which makes it a non-issue).
msg269917 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2016-07-07 02:29
I should be able to take a look at the updated patch this coming weekend.
msg270043 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2016-07-09 13:59
Emanuel's latest patch looks good to me, but after seeing yet another behaviour discrepancy between the two versions(static method vs instance method behaviour), I've proposed disallowing function/class mismatches as an explicit policy clarification in PEP 399: https://mail.python.org/pipermail/python-dev/2016-July/145556.html

Since this change can land any time before the first 3.6 beta in September, I'm going to wait and see want kind of responses we get to that thread before merging it.
msg274476 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-09-06 00:16
We're one week from the feature freeze, seems like a good time to merge this :)
msg274522 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2016-09-06 04:59
Thanks for the nudge, and sorry for getting distracted! I won't be able to get to this today, but I'll definitely handle it in time for the deadline :)
msg274768 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2016-09-07 06:11
Attempting to apply this, I found that the pure Python fallback fails the new recursive repr tests added in https://bugs.python.org/issue25455

The recursive pickling test is also failing, but I suspect that may be due to an error in my attempt to apply parts of the patch manually after they failed to apply due to the #25455 changes.

Emanuel, would you be able to take another pass at this and resolve those inconsistencies? Otherwise I should be able to get to it on the weekend (and hence still in time for beta 1)
msg274809 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-09-07 13:41
Hi Nick, thank you for letting me know! I started trying to fix this, however I found it very hard to fix the recursive repr issue. I've whipped up an incomplete (but yet working) patch that fixes all but the recursive repr issue. Only those two tests fail (once for functools.partial and once for the subclass). I have to go for now, but feel free to play with this patch and see if you can fix it. The problem is that the way it's handled is inconsistent within the C implementation, and is incompatible with reprlib.recursive_repr. I might try my hand at it later today or tomorrow.

In the meantime, I think maybe the C implementation cares too much about the special cases for that.
msg274827 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-07 16:22
I think we can change C implementation of __repr__ to match straightforward Python implementation.
msg274890 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-09-07 20:59
Thank you Serhiy for the comments! Here's a new patch. I'm fine with the recursive repr tests failing for now; it's something I believe we can fix during the beta phase if we don't have time before (fix as in modify the C implementation to match the Python version instead of the other way around).
msg275232 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-09-09 02:02
Can this be merged even with the two failing tests? I have little to no time available to fix it properly before the feature freeze. We can skip/silence those tests for a bit; I'll open a new issue and fix it in time for beta 2.
msg275240 - (view) Author: Brett Cannon (brett.cannon) * (Python committer) Date: 2016-09-09 03:20
Ned will make the call about committing broken tests decorated with the expected failure decorator, but if you view this as a bug then this can go into b2 w/ passing tests instead.
msg275251 - (view) Author: Serhiy Storchaka (serhiy.storchaka) * (Python committer) Date: 2016-09-09 04:39
Here is a patch that makes C implementation repr matching Python implementation repr. It should be applied to all Python versions (the code is new).
msg275305 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-09-09 11:06
The patch LGTM and applies fine. Looks like there's no need to wait for beta 2 after all; thanks Serhiy!
msg275609 - (view) Author: Roundup Robot (python-dev) (Python triager) Date: 2016-09-10 10:00
New changeset cdc91b6ae3b2 by Nick Coghlan in branch 'default':
Issue #27137: align Python & C implementations of functools.partial
https://hg.python.org/cpython/rev/cdc91b6ae3b2
msg275610 - (view) Author: Alyssa Coghlan (ncoghlan) * (Python committer) Date: 2016-09-10 10:02
Thanks folks - for 3.6, the pure Python and accelerated C implementations of functools.partial are now both multiprocessing, subclassing, and REPL friendly :)
msg275629 - (view) Author: Anilyka Barry (abarry) * (Python triager) Date: 2016-09-10 12:13
Thank you Nick! I just opened #28062 to fix the repr inconsistency between functools.partial and any subclass :)
History
Date User Action Args
2022-04-11 14:58:31adminsetgithub: 71324
2016-09-10 12:13:36abarrysetmessages: + msg275629
2016-09-10 10:02:04ncoghlansetstatus: open -> closed
resolution: fixed
messages: + msg275610

stage: commit review -> resolved
2016-09-10 10:00:17python-devsetnosy: + python-dev
messages: + msg275609
2016-09-09 11:06:26abarrysetmessages: + msg275305
2016-09-09 04:39:10serhiy.storchakasetfiles: + functools_partial_recursive_repr.patch

messages: + msg275251
2016-09-09 03:20:50brett.cannonsetnosy: + ned.deily
messages: + msg275240
2016-09-09 02:02:48abarrysetmessages: + msg275232
2016-09-07 20:59:08abarrysetfiles: + functools_partial_4.patch

messages: + msg274890
2016-09-07 16:22:38serhiy.storchakasetmessages: + msg274827
2016-09-07 13:41:58abarrysetfiles: + functools_partial_3_1_incomplete.patch

messages: + msg274809
2016-09-07 06:11:14ncoghlansetmessages: + msg274768
2016-09-06 04:59:16ncoghlansetmessages: + msg274522
2016-09-06 00:16:41abarrysetmessages: + msg274476
2016-07-09 13:59:52ncoghlansetmessages: + msg270043
2016-07-07 02:29:46ncoghlansetmessages: + msg269917
stage: patch review -> commit review
2016-07-06 19:08:33abarrysetmessages: + msg269899
2016-06-04 22:31:28abarrysetfiles: + functools_partial_3.patch

messages: + msg267334
2016-06-03 00:22:43ncoghlansetmessages: + msg267003
2016-05-30 15:52:28abarrysetfiles: + functools_partial_2.patch

messages: + msg266700
2016-05-30 13:02:51ppperrysettitle: functools.partial: Inconsistency between Python and C implementations -> Python implementation of `functools.partial` is not a class
2016-05-29 14:16:05serhiy.storchakasetnosy: + brett.cannon
2016-05-29 13:36:43abarrysetmessages: + msg266599
2016-05-29 07:02:17serhiy.storchakasetmessages: + msg266588
2016-05-29 06:45:35vstinnersetmessages: + msg266587
2016-05-28 19:36:26abarrysetmessages: + msg266570
2016-05-28 07:33:16serhiy.storchakasetmessages: + msg266545
2016-05-28 07:06:12rhettingersetassignee: rhettinger -> ncoghlan
messages: + msg266540
2016-05-28 06:57:36serhiy.storchakasetnosy: + serhiy.storchaka
messages: + msg266539
2016-05-28 05:39:53rhettingersetmessages: + msg266534
2016-05-28 03:19:54abarrysetfiles: + functools_partial_1.patch
keywords: + patch
messages: + msg266530

stage: needs patch -> patch review
2016-05-27 18:07:17xiang.zhangsetnosy: + xiang.zhang
2016-05-27 17:03:52serhiy.storchakasetassignee: rhettinger
2016-05-27 17:01:55vstinnersetnosy: + vstinner
2016-05-27 16:39:18abarrycreate