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: Allowing trailing Placeholders #128644

Closed
dg-pb opened this issue Jan 8, 2025 · 11 comments
Closed

functools.partial: Allowing trailing Placeholders #128644

dg-pb opened this issue Jan 8, 2025 · 11 comments
Assignees
Labels
type-feature A feature request or enhancement

Comments

@dg-pb
Copy link
Contributor

dg-pb commented Jan 8, 2025

Feature or enhancement

Proposal:

from functools import partial, Placeholder

def foo(a):
    return None

# Now
partial(foo, Placeholder)   # TypeError

# After
partial(foo, Placeholder)    # Success
# Also, converts all Placeholder arguments to positional only

1. Rationale

So, I considered automatic trimming of trailing Placeholders instead of forbidding such. However it didn't sit well with me.

So I thought this over. Not having trailing Placeholders works well, however both approaches of achieving this have flaws:
a) Forbidding them - results in unexpected errors for the user.
b) Trimming automatically - obfuscates inner workings by implicit input modifications.

Furthermore, it sometimes could be desired to have a trailing Placeholder to convert argument to positional only.

So I think it would be best not to have this restriction at all. This makes things more explicit and predictable. E.g. both would work:

class A:
    def func(self, a):
        pass

    p1 = partial(func)
    p2 = partial(func, Placeholder)

And as expected, self of p1 can be both positional or keyword, while self of p2 becomes restricted to positional only.

2. Implementation

This simplifies the code by not having to do error checks or trimming routines. Existing implementation handles trailing placeholders without modifications (except minor Fast Path fix in C).

3. Relation to partialmethod

This has come out during #124788, but it turned out to be orthogonal.

partialmethod stays as it was before - if no positionals provided argument kind of self is unaffected, while if args are provided then self becomes positional only.

4. Performance

Slightly faster for both cases (with and without Placeholders) when merging args of previous partial with new args.

S="
from functools import partial, Placeholder
def f(a, b):
    pass
p1 = partial(f, 1, 2)
p2 = partial(f, Placeholder, 2)
"
C0='partial(f)'
C1='partial(f, 1, 2)'
C2='partial(p1, 1, 2)'
C3='partial(p2, 1, 2)'
                            # BEFORE | AFTER
$PYEXE -m timeit -s $S $C0  # 135 ns | 135 ns
$PYEXE -m timeit -s $S $C1  # 170 ns | 170 ns
$PYEXE -m timeit -s $S $C2  # 195 ns | 165 ns
$PYEXE -m timeit -s $S $C3  # 195 ns | 165 ns

I think this is more flexible, simple and "straight" (I.e. If base logic is understood, the implications are clear and behaviour is predictable).

Has this already been discussed elsewhere?

This is a minor feature, which does not need previous discussion elsewhere

Or to be more precise, this is an update for new unreleased functionality.

Links to previous discussion of this feature:

No response

Linked PRs

@rhettinger
Copy link
Contributor

Personally, I like that a TypeError is raised. It is a good hint that the partial isn't needed and that more thought needs to be given to what the code is actually doing. ISTM that a trailing Placeholder would almost always be flagged during a code review.

The relevant Zen of Python is "errors should never pass silently" and "in the face of ambiguity, refuse the temptation to guess."

My recommendation is to defer this. Let users gain experience with Placeholder and gather evidence about whether raising a TypeError is a net benefit in practice or whether there actually is a problem that needs to be solved.

If a compelling case for trailing placeholders does arise, we can easily add it later. However, if it was added now and turned out to be a net detriment, it would be difficult to undo (deprecation and removal now takes half a decade).

Also, if the goal is convert all arguments to positional-only (something that I almost never need to do), the trailing placeholder approach is not an obvious way either to do it, either for a code writer or to a code reader. I would not want to see this in production code that I cared about.

@ZeroIntensity
Copy link
Member

Also "There should be one-- and preferably only one --obvious way to do it."

With a trailing Placeholder, it's no different than just using partial as-is, other than the positional-only requirement.

Furthermore, it sometimes could be desired to have a trailing Placeholder to convert argument to positional only.

Speaking of which, I'd like to hear more about this use-case. If something is supposed to be positional-only, then generally that would be done at the function-level, not in partial. Is there a real-world case where it would be desirable to artificially make something positional-only via partial, or are you speculating?

@dg-pb
Copy link
Contributor Author

dg-pb commented Jan 8, 2025

I see it this way. From the perspective of the algorithm, it can be summed up as follows:

"Placeholder indicates that argument will need to be filled on partial.__call__ and is converted to positional-only kind"

If trailing Placeholder is allowed, then the rest of the logic flows out of the statement above without exceptions.

Furthermore, the implementation without restriction:

  1. is more straight forward
  2. is faster
  3. provides more functionality:
    1. Converting arguments to positional-only kind
    2. Removing default as filling it will be required by partial.__call__
  4. Will avoid issues with programatic use, where argument binding would be dynamically constructed with the possibility of leading placeholders. This will sooner or later result in similar programatic usage issues that now arise in itemgetter, where outputs do not naturally flow given inputs and special case handling is needed.

So all of the above is for free without additional effort.


As opposed to restricting them, which is extra work - both on the implementation and mental model aspects.

I did this initially as initial implementation was immature and this restriction made it work, which is now gone and the current one handles leading placeholders appropriately (as it should).


To answer Zen challenges:

I don't think any guessing is needed here as functionality is summed up in 1 statement and everything else is a natural consequence.

This is not an error and I don't think it should be seen as such. If this is an error, then partial(lambda: None) should also be and error, but it is not and I don't think this has ever caused an issue.

Same applies to 1 obvious way. This adds an extra way to do it as much as:

def foo(a):
    return a
def bar(a):
    return foo(a)

And to this specific case it applies even less as this actually has an effect.

I think this case might be more on the side of "consenting adults".
Restricting for the sake of preventing amature mistakes at the expense of extra complexity and absence of benefits might not be the most optimal for this specific case.

@dg-pb
Copy link
Contributor Author

dg-pb commented Jan 8, 2025

Postponing is an option, but given the case is reasonably clear, determining the appropriate path forward now has benefits of releasing better implementation, having closed case and eliminating the possibility of someone depending on handling TypeError (which is less likely than depending on trailing Placehlders, but is backwards compatibility break nevertheless).

@rhettinger
Copy link
Contributor

This is not an error and I don't think it should be seen as such.

In my estimation, partial(foo, Placeholder) would almost always be a thinking error and should be flagged. I really don't want to ever see this in production code. It is a misfeature. I can't think of a single case where I would let this pass a code review.

I see it this way. From the perspective of the algorithm, it can be summed up as follows ...

I've read, re-read, and understood everything you've said but still do not think trailing placeholders are a good idea. To my eyes, they look weird and unnecessary.

AFAICT, no one needs this. If someone is programmatically adding a variable number of placeholders (why, I'm not sure), then is easy to explicitly strip the rightmost placeholders. If someone wants to force a positional-only signature (again not sure why), they should do so in a more explicit manner rather than indirectly with a partial/placeholder combination. The common and intended use case doesn't need this at all.

"Placeholder indicates that argument will need to be filled on partial.__call__ and is converted to positional-only kind"

I don't like that definition. It feels contrived to justify trailing placeholders, and it expresses an implementation centric viewpoint rather than how users would actually think about placeholders (and how we should advertise them).

The actual intended purpose of Placeholder as I originally approved it was not aimed at that definition. We were solving a specific problem, holding open substitution points between the constants. The trailing inputs were already handled naturally.

functools.partial: Allowing trailing Placeholders

Note the plural here. The proposal also allows partial(func, Placeholder, Placeholder, Placeholder). Let's not do this.

Postponing is an option

I will leave this open for a while in case other core-devs feel like this is something they want users to be doing. But unless strong support is expressed, we should pass on this one.

@dg-pb
Copy link
Contributor Author

dg-pb commented Jan 9, 2025

It is kind of implementation centric, but that is not necessarily a bad thing.

Note the plural here. The proposal also allows partial(func, Placeholder, Placeholder, Placeholder). Let's not do this.

def func(a=1, b=2, c=3):
    return a + b + c
signature(func)    # (a=1, b=2, c=3)

p = partial(func, Placeholder, Placeholder, Placeholder)
signature(p)    # (a, b, c, /)

Not sure if there are actual use-cases for this, but maybe others will find this useful. E.g. ensuring that parameters such as interpolation-mode are explicit for an extra insurance against API changes.

The last time I advocated for not restricting something ended up realising that restriction was a good idea. Although this case feels a bit different, I don't want to make the same mistake.

I am undecided now, need more time to think about it.
Even if this was favoured I would leave this sit for a few months.
I push this down to the bottom of partial-related PR list (#124652 (comment)).

@rhettinger rhettinger self-assigned this Jan 9, 2025
@dg-pb
Copy link
Contributor Author

dg-pb commented Jan 9, 2025

By the way, this is another possibility for keyword Placeholders.

Keyword Placeholder could be extended for this.

I.e. Keyword placeholder would hold a place which needs to be filled on __call__. It would also convert to "keyword only".


I have pretty much concluded that kwpartial and impartial in https://discuss.python.org/t/future-of-functools-partial/71175 are simply too complex for the functionalities they offer.

This is much simpler and is realistic (given this is a desirable feature).

@dg-pb
Copy link
Contributor Author

dg-pb commented Jan 9, 2025

The path of converting keyword arguments to positional, reordering arguments, etc is complex and partial extensions would be always lacking in one way or another - the gap between partial and def would never be completely closed and it is a rabbit hole with increasing complexity-to-benefit ratio. After having explored kwpartial and impartial this road took me to considering completely new concepts as such would provide much more functionality/benefit with much lower complexity.

While this one is simple and would be pretty much final. The complete concept would read like:

"Placeholder indicates that argument will need to be filled on partial.__call__. Positional Placeholders are converted to positional-only kind and keyword Placeholders are converted to keyword-only kind."

@dg-pb
Copy link
Contributor Author

dg-pb commented Jan 9, 2025

This would allow things such as:

def update_dicts(d, **kwds):
    d.update(kwds)

names = dict()

update_names_compulsory_key_a = partial(update_dict, names, a=Placeholder)

This could be a reasonable final step for partial which has intuitive behaviour, straight forward and simple concept and provides moderately useful functionality while keeping implementation simple.

I think it is worthwhile to keep this open as IMO in time (with increasing conviction that possible alternative extensions are better covered by other concepts/tools) this has a non-negligible chance to turn out to be a good way forward for partial.

@rhettinger
Copy link
Contributor

ISTM that the core problem we set out to solve has been solved and it would be best to just stop here.

It now feels like we're now entering the territory of imagination and invented problems. The risk is of damaging something that is currently clean.

The entire premise of approving the Placeholder extension was that it was minimal and avoided creating a new mini-language for rewriting function signatures (see better-partial for example).

It is my strong recommendation that you enjoy and use what has already been landed. In art school, they say that the hardest thing to teach is knowing when to stop painting.

For now, I'm going to mark this as closed. Now other commenter has stepped forward in support of trailing placeholders and it is my belief that it is would be detrimental to users. If a compelling use case arises, we can re-open this and discuss it more.

@rhettinger rhettinger closed this as not planned Won't fix, can't repro, duplicate, stale Jan 10, 2025
@rhettinger
Copy link
Contributor

rhettinger commented Jan 10, 2025

If someday you reopen this, it would be helpful to have examined code in real projects and found places (no pun intended) where trailing placeholders or a keyword placeholder would have improved the code, making it clearer, more obvious, and easier to maintain.

Another suggestion is to show those proposed code edits to other users to see if they can figure out what it does (a readability test). Can a cold reader figure out that trailing placeholders and keyword argument placeholders are intended to force optional arguments to be required.

Perhaps, poll a few programmers to see which of these three they would most want to see when reading or reviewing code:

update_names_compulsory_key_a = partial(update_dict, names, a=Placeholder)

update_names_compulsory_key_a = lambda *, a: update_dict(names, a=a)

def update_names_compulsory_key_a(*, a):
    return update_dict(names, a=a)

For me at least, the last one has the least cognitive load. It is easy to write and isn't mysterious. Also, I would keep the flexibility of easily adding type annotations, changing the parameter names, and/or adding docstrings:

def update_names_compulsory_key_a(*, first_word: str):
    "Update first_word in the names dictionary."
    return update_dict(names, a=first_word)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

3 participants