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

functools.partial placeholders #119127

Open
dg-pb opened this issue May 17, 2024 · 5 comments
Open

functools.partial placeholders #119127

dg-pb opened this issue May 17, 2024 · 5 comments
Assignees
Labels
extension-modules C modules in the Modules dir type-feature A feature request or enhancement

Comments

@dg-pb
Copy link
Contributor

dg-pb commented May 17, 2024

Feature or enhancement

Proposal:

I know that this has been brought up in the past, but I would appreciate if this was reconsidered. I would be happy to bring this up to a nice level.

from functools import partial
from partial2 import partial as partial2, Placeholder

_ = VOID = Placeholder()

%timeit partial(opr.sub, 1, 2)     # 130 ns
%timeit partial2(opr.sub, 1, 2)    # 155 ns

p1 = partial(opr.sub, 1)
p2 = partial2(opr.sub, 1)
p3 = partial2(opr.sub, _, 1)
p4 = partial2(opr.sub, VOID, 1)

print(p1(2))    # -1
print(p2(2))    # -1
print(p3(2))    # 1
print(p4(2))    # 1

%timeit p1(2)   # 48 ns
%timeit p2(2)   # 52 ns
%timeit p3(2)   # 54 ns
%timeit p4(2)   # 54 ns

The logic is as follows:

def partial(func, *p_args, **p_kwds):
    p_args = list(p_args)
    nvoid = sum(a is VOID for a in p_args)
    arng = range(len(p_args))

    def wrapper(*args, **kwds):
        if len(args) < nvoid:
            raise ValueError('Not all VOIDs are filled')
        t_args = p_args.copy()
        j = 0
        for i in arng:
            if p_args[i] is VOID:
                t_args[i] = args[j]
                j += 1
        t_args.extend(args[j:])
        return func(*t_args, **{**p_kwds, **kwds})
    return wrapper

vectorcall change looks like:

if (np) {
        nargs_new = pto_nargs + nargs - np;
        Py_ssize_t ip = 0;
        for (Py_ssize_t i=0; i < pto_nargs; i++) {
            if (PyObject_TypeCheck(pto_args[i], &placeholderType)){
                memcpy(stack + i, args + ip, 1 * sizeof(PyObject*));
                ip += 1;
            } else {
                memcpy(stack + i, pto_args + i, 1 * sizeof(PyObject*));
            }
        }
        if (nargs_total > np){
            memcpy(stack + pto_nargs, args + np, (nargs_total - np) * sizeof(PyObject*));
        }
    // EO Placeholders ------
    } else {

Has this already been discussed elsewhere?

I have already discussed this feature proposal on Discourse

Links to previous discussion of this feature:

https://discuss.python.org/t/functools-partial-placeholder-arguments/53487

Linked PRs

@dg-pb dg-pb added the type-feature A feature request or enhancement label May 17, 2024
@rhettinger rhettinger self-assigned this May 17, 2024
@Eclips4 Eclips4 added the extension-modules C modules in the Modules dir label May 17, 2024
@rhettinger
Copy link
Contributor

rhettinger commented May 17, 2024

@dg-pb
Copy link
Contributor Author

dg-pb commented May 17, 2024

[quote="Raymond Hettinger, post:20, topic:19163, username:rhettinger"]
I’m 100% opposed to including better-partial or anything like it in the standard library. At best, it would be an attractive nuisance.
[/quote]

In https://discuss.python.org/t/functools-partial-placeholder-arguments/53487 I have laid out several examples and use cases for this. But the main gain here is speed improvement by being able to efficiently utilise functions from operator module to use as predicates to functionals, where argument order is not convenient. I have been lately working on iterator recipes and from my experience the partial wrap is 20 ns vs lambda call being ~50ns can result in considerable speed-ups. Especially if it is used more than once.

[quote="Raymond Hettinger, post:9, topic:19163, username:rhettinger"]
Every variant I’ve seen is more complex, less speedy, and harder to debug than a simple def or lambda. Every variant required something extra to learn and remember, unlike def or lambda which are Python basics.
[/quote]

  • More complex. I acknowledge that complexity here is an issue, but I believe that I have found sufficiently simple solution, which can be expressed in 17 lines of python code (see above). The code is fully working and includes all needed error checks.
  • Less speedy. The solution has minimal call overhead. Only few nanoseconds and even less if it does not contain placeholders. It did add 25ns to initialisation, but I am still figuring out why it is so much as id doesn't look right. See initial comparisons above.
  • Harder to debug. It is inevitably harder to debug than simple lambda. But this is also the case for functools.partial as it is now.
  • Requires new learning. From my experience it is very easy to learn and understand. It is visually intuitive. All needed attributes are exposed so that the user can easily investigate and understand.
p3 = partial2(opr.sub, _, 1)
print(inspect.signature(p3.func))    # <Signature (a, b, /)>
print(p3.args)                # (<partial2.Placeholder object at 0x111e95ad0>, 1)

Placeholder sentinel still needs work, it would have much nicer representation. E.g: (functools.PPHoldder, 1)

Personally, I would make use of this functionality in algorithm recipes, iterator recipes, utility functions and similar places of lower level code where performance is important. And I don't see this as lambda or def replacement, but rather functionality that is well suited for a clear purpose.

Also, partial already exists with many of the problems that are being used against this functionality. But given the fact that partial exists already why not make it better and make use of it in the area where it excels (performance)?

Why I think this deserves reconsideration is that this hasn't been implemented and benchmarked before. All the previous attempts were in python, where instead of performance gain which has led me to this point, the result was a significant performance loss - up to 20x slower than current standard library implementation. So naturally, those are not used, because lambda/def is a simpler, much faster than better_partial or any other 3rd party implementation and has all the flexibility.

While before supporting reasoning was convenience, Linters, MyPy issues and similar, my main argument is performance.

@rhettinger
Copy link
Contributor

rhettinger commented Jun 6, 2024

I've left this open for comment and no one else has stepped in to opine, so I'm going to go ahead and approve it.

The API is a minimal addition that gives more flexibility without venturing into creating a mini-DSL. It is unclear whether it will present any typing challenges beyond what partial() already does, but I expect that will only be a minor obstacle.

The PR needs a lot of work but the concept is sound. I've been trying it out for a while and it looks reasonable in real code. It supports multiple placeholders but in the examples I've found only one is needed.

I'm dubious about the motivation as an optimization (PyPy doesn't need this and CPython may yet optimize a plain lambda version). However, it does make partial more broadly useful and that is a sufficient reason.

@rhettinger
Copy link
Contributor

rhettinger commented Jun 6, 2024

FWIW, the example I most enjoyed is bin8 = partial(format, Placeholder, '08b').

@picnixz
Copy link
Contributor

picnixz commented Jun 8, 2024

Personally, most of the time I need a placeholder for is when I want right partialization, not left partialization. While there are times where partialization in arbitrary order makes sense, an alternative would to provide rpartial interface without introducing new singletons or immortal objects. The specs on how to use rpartial would still remain an open question (i.e., is rpartial(f, a, b)(x) for f(x, a, b) or f(x, b, a)?)

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 type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants