gh-124652: partialmethod simplifications - #124788
Conversation
a04c14f to
d217592
Compare
|
Removed keyword Placeholder restriction from this and will issue a separate PR after. Felt like too much is packed into 1 PR. |
|
@dg-pb Is this PR still relevant or have you opened PRs for the different components? If so, can we close this one? |
|
It is still relevant. I could factor "allowing trailing placeholders" into a separate one if it is preferred. |
I have not looked at all the changes in detail, but the PR seems big and that could be a reason this PR has not yet been reviewed. In the description at least 3 changes are mentioned (allowing placeholders, performance, refactor for |
There are 2 really. Performance benefit is a consequence of "allowing trailing placeholders". I don't mind making changes, splitting as desired etc, but I would like these to be called by reviewer. Otherwise, I already have experience by trying to guess what reviewer might prefer, making changes per suggestions of others, etc and when final reviewer comes he desires to be different again and I need to keep changing things more times than necessary. And either way these would need to be considered at the same time. I.e. allowing or not allowing trailing placeholders are both ok. There is a slight advantage for allowing them as it makes it a bit more flexible and explicit. While looking at Also, if I split now, then I have PRs hanging on unmerged code.
It isn't that big. Most of it is Pure Python rewrite of |
|
As this series of |
|
@serhiy-storchaka, just a gentle reminder. |
|
@dg-pb, I am reviewing, I just was very busy last days with other issue. |
|
@serhiy-storchaka, I think it would be good to merge this. Otherwise, it will be sitting there for another version. |
|
This PR is stale because it has been open for 30 days with no activity. |
|
@serhiy-storchaka, I think this can be merged regardless of #128644. And maybe we could reopen #128644 so that it has another chance for consideration? |
eendebakpt
left a comment
There was a problem hiding this comment.
There are some behavior changes that need to be addressed (or documented).
Reproducers:
from functools import partialmethod, Placeholder
class C: # main: TypeError here
m = partialmethod(print, Placeholder) # PR: accepted silently
C().m # PR: TypeError raised only now,
# from inside functools.__make_method
And
from functools import partialmethod
class D: # a descriptor that needs no binding
def __get__(self, obj, cls=None): return self
def __call__(self, *a, **kw): return a
class C:
m = partialmethod(D(), 42)
C().m() # main: (<C object>, 42) PR: (42,) <- instance gone
And
Freezing of partialmethod
"""Reproducer 3 for PR 124788 (gh-124652).
On current main, partialmethod.__get__ rebuilds the bound callable on every
access, and the generated closure reads self.func / self.args / self.keywords
at CALL time -- so mutating those public attributes is reflected immediately.
With the PR, the method object is built once and cached (self.method), so the
partialmethod is effectively frozen after its first access: later mutation is
silently ignored. Note the cache is a *public* attribute (`.method`), which
makes such mutation easy to reach.
"""
from functools import partialmethod
def base(self, *args, **kwargs):
return ('base', args, kwargs)
def other(self, *args, **kwargs):
return ('other', args, kwargs)
class C:
m = partialmethod(base, 1, kw='before')
pm = C.__dict__['m']
c = C()
first = c.m()
print(f'1st access (baseline) : {first!r}')
# --- mutate every documented public attribute of partialmethod -------------
pm.keywords = {'kw': 'after'}
after_keywords = c.m()
print(f'after pm.keywords = ... : {after_keywords!r}')
pm.args = (99,)
after_args = c.m()
print(f'after pm.args = (99,) : {after_args!r}')
pm.func = other
after_func = c.m()
print(f'after pm.func = other : {after_func!r}')
frozen = (after_keywords == first
and after_args == first
and after_func == first)
# Is the stale callable being reused, and is the cache publicly reachable?
cached = getattr(pm, 'method', None)
print()
print(f'public .method cache attr : {cached!r}')
print(f'declared __slots__ : '
f'{getattr(partialmethod, "__slots__", None)!r}')
if cached is not None:
# The slot named in __slots__ is "wrapper", but the code stores the cache
# in "method" -- so it lands in __dict__ and the slot goes unused.
print(f'cache stored in __dict__ : '
f'{"method" in getattr(pm, "__dict__", {})}')
|
Thank you for looking into this.
I added the check so that after this PR this is also raised at definition. I think it is good to keep it in sync with |
Made this PR to behave the same as it was. |
If you look at pure python To make I made it this way purely for performance - reconstructing on each call is fairly expensive.
Would be good to know if being able to modify Perspective of someone with historical context would be very helpful here. |
Lets also see what others think. |
|
So ok, I looked around a bit and it seems that ability to modify keywords of So in terms of restriction and not breaking expected (even though undocumented) contract likely the most sensible thing to do here is to replicate what So my solution is as follows: class partialmethod:
...
@property
def func(self):
return self._func
@property
def args(self):
return self._args
@property
def keywords(self):
method = self._cachedmethod
if method is None:
return self._keywords
if isinstance(method, (staticmethod, classmethod)):
method = method.__wrapped__
return method.keywordsSo in case the method is cached, give Subclass optimization also works correctly via: temp = partial(lambda *_, **__: None, *func._args, **func.keywords) |
Made changes. Now:
|
partial(makes use ofpartialinstead of containing any complexities of partial)inspectpartialobjects)partialmethodBenchmarks:Setup
Updated: 2026-09-03T16:09:16
functools.partialmethodsimplification #124652