Skip to content

gh-124652: partialmethod simplifications - #124788

Open
dg-pb wants to merge 28 commits into
python:mainfrom
dg-pb:gh-124652-partialmethod
Open

gh-124652: partialmethod simplifications#124788
dg-pb wants to merge 28 commits into
python:mainfrom
dg-pb:gh-124652-partialmethod

Conversation

@dg-pb

@dg-pb dg-pb commented Sep 30, 2024

Copy link
Copy Markdown
Contributor
  1. it is now untangled from partial (makes use of partial instead of containing any complexities of partial)
  2. it has no need for any special-casing in inspect
  3. its performance is now reasonable (for standard methods and partial objects)

partialmethod Benchmarks:

Setup

S="
from functools import partialmethod, Placeholder

@staticmethod
def s(a, b):
    pass
@classmethod
def c(self, a, b):
    pass
def m(self, a, b):
    pass

class D:
    def __get__(self, obj, cls=None):
        return (lambda a, b: None)
d = D()

class A:
    @staticmethod
    def s(a, b):
        pass
    @classmethod
    def c(self, a, b):
        pass
    def m(self, a, b):
        pass
    d = D()

    ps = partialmethod(s, 1)
    pc = partialmethod(c, 1)
    pm = partialmethod(m, 1)
    pd = partialmethod(d, 1)

a = A()
"

C1='partialmethod(s, 1)'    # staticmethod
C2='partialmethod(c, 1)'    # classmethod
C3='partialmethod(m, 1)'    # instance method
C4='partialmethod(d, 1)'    # unknown descriptor

C5='A.ps(2)'
C6='A.pc(2)'
C7='a.pm(2)'
C8='a.pd(2)'

NOTE: construction cost for standard methods is obfuscated 
      as they are constructed and cached on 1st call.

                            #  BEFORE | AFTER
--------------------------- #-----------------
$PYEXE -m timeit -s $S $C1  #  920 ns |  390 ns
$PYEXE -m timeit -s $S $C2  # 1000 ns |  420 ns
$PYEXE -m timeit -s $S $C3  #  900 ns |  440 ns
$PYEXE -m timeit -s $S $C4  # 1000 ns |  600 ns
--------------------------- #-----------------
$PYEXE -m timeit -s $S $C5  # 1300 ns |  330 ns
$PYEXE -m timeit -s $S $C6  #  830 ns |  390 ns
$PYEXE -m timeit -s $S $C7  #  800 ns |  390 ns
$PYEXE -m timeit -s $S $C8  # 1300 ns |  890 ns

Updated: 2026-09-03T16:09:16

--------------------------- #-----------------
$PYEXE -m timeit -s $S $C1  # 1000 ns |  650 ns
$PYEXE -m timeit -s $S $C2  # 1300 ns |  680 ns
$PYEXE -m timeit -s $S $C3  # 1000 ns |  800 ns
$PYEXE -m timeit -s $S $C4  # 1100 ns |  880 ns
--------------------------- #-----------------
$PYEXE -m timeit -s $S $C5  # 1000 ns |  380 ns
$PYEXE -m timeit -s $S $C6  # 1100 ns |  410 ns
$PYEXE -m timeit -s $S $C7  # 1000 ns |  410 ns
$PYEXE -m timeit -s $S $C8  # 1450 ns | 1500 ns

@rhettinger
rhettinger removed their request for review September 30, 2024 16:31
Comment thread Lib/functools.py Outdated
@dg-pb
dg-pb force-pushed the gh-124652-partialmethod branch from a04c14f to d217592 Compare October 6, 2024 16:56
@dg-pb

dg-pb commented Oct 6, 2024

Copy link
Copy Markdown
Contributor Author

Removed keyword Placeholder restriction from this and will issue a separate PR after. Felt like too much is packed into 1 PR.

Comment thread Lib/functools.py Outdated
@eendebakpt

Copy link
Copy Markdown
Contributor

@dg-pb Is this PR still relevant or have you opened PRs for the different components? If so, can we close this one?

@dg-pb

dg-pb commented Jan 4, 2025

Copy link
Copy Markdown
Contributor Author

It is still relevant.
On my part this is ready for review.
Given partial.Placeholder has been merged recently and not in production I wasn't sure if splitting is necessary.

I could factor "allowing trailing placeholders" into a separate one if it is preferred.

Comment thread Lib/functools.py Outdated
@eendebakpt

Copy link
Copy Markdown
Contributor

It is still relevant. On my part this is ready for review. Given partial.Placeholder has been merged recently and not in production I wasn't sure if splitting is necessary.

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 partialmethod). If possible, I would advice to split the PR into multiple PRs.

@dg-pb

dg-pb commented Jan 4, 2025

Copy link
Copy Markdown
Contributor Author

at least 3 changes are mentioned

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 partial from the POV of using it on methods, the advantages and rationale for allowing them can be seen more clearly putting it on a favourable side (at least this is my conclusion).

Also, if I split now, then I have PRs hanging on unmerged code.

the PR seems big

It isn't that big. Most of it is Pure Python rewrite of partialmethod and the implementation is much simpler to follow than the previous one.

@dg-pb

dg-pb commented Jan 8, 2025

Copy link
Copy Markdown
Contributor Author

As this series of partial related PRs started slowly moving, the path became a bit clearer and splitting this into 2 seems to be the best option to me now. Will do that shortly.

Comment thread Lib/functools.py Outdated
Comment thread Lib/functools.py
Comment thread Lib/functools.py
Comment thread Lib/functools.py
Comment thread Lib/functools.py Outdated
Comment thread Lib/functools.py Outdated
Comment thread Lib/functools.py Outdated
Comment thread Lib/functools.py Outdated
Comment thread Lib/functools.py Outdated
Comment thread Lib/functools.py Outdated
Comment thread Misc/NEWS.d/next/Library/2024-10-17-00-50-32.gh-issue-124652.AK3PDp.rst Outdated
@dg-pb

dg-pb commented Nov 23, 2025

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka, just a gentle reminder.
This is ready for final review.

@serhiy-storchaka

Copy link
Copy Markdown
Member

@dg-pb, I am reviewing, I just was very busy last days with other issue.

@dg-pb

dg-pb commented Feb 23, 2026

Copy link
Copy Markdown
Contributor Author

@serhiy-storchaka, I think it would be good to merge this. Otherwise, it will be sitting there for another version. inspect simplification is what I think has most value here.

@github-actions

Copy link
Copy Markdown

This PR is stale because it has been open for 30 days with no activity.

@github-actions github-actions Bot added the stale Stale PR or inactive for long period of time. label Apr 18, 2026
@dg-pb

dg-pb commented Aug 20, 2026

Copy link
Copy Markdown
Contributor Author

@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?

@github-actions github-actions Bot removed the stale Stale PR or inactive for long period of time. label Aug 22, 2026

@eendebakpt eendebakpt left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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__", {})}')

We might want to add tests for these cases.

Comment thread Lib/functools.py Outdated
@dg-pb

dg-pb commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Thank you for looking into this.

from functools import partialmethod, Placeholder
class C: # main: TypeError here
m = partialmethod(print, Placeholder) # PR: accepted silently

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 partial and it is an easy one to catch early.

@dg-pb

dg-pb commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

C().m() # main: (, 42) PR: (42,) <- instance gone

Made this PR to behave the same as it was.

@dg-pb

dg-pb commented Sep 1, 2026

Copy link
Copy Markdown
Contributor Author

Freezing of partialmethod

If you look at pure python partial - these are writable as well, but in C implementation these are read only.

To make partial the same in both versions, all what would be needed is to make read only properties. In this case one can still modify contents of partial.keywords['kw'] = new_val and see the effect.

I made it this way purely for performance - reconstructing on each call is fairly expensive.
I haven't really seen any use cases for mutating args / kwds so I think this change in behaviour is reasonable cost for perf? I see 3 options:

  1. Remove caching to keep behaviour in sync
  2. Leave it as is and document the behaviour.
  3. Make func,args,keywords private with read-only properties. And make keywords return proxy object, which on write actions (setitem/delitem/update/...) clears cache. This would make partialmethod behave the same way as C version of partial. But is it worth the effort?

Would be good to know if being able to modify partial.keywords has found any use cases or is it purely the consequence of not having frozedict at a time?
I would go with (2) and address mutability of both partial and partialmethod separately. I would guess that mutability is not really desirable here and it might be worth considering making partialmethod.args/keywords read only and also make keywords of both partial and partialmethod frozendict.

Perspective of someone with historical context would be very helpful here.

@eendebakpt

Copy link
Copy Markdown
Contributor

Freezing of partialmethod

If you look at pure python partial - these are writable as well, but in C implementation these are read only.
That would be a good argument for making the attributes of partial read-only (python and C versions should behave the same, I consider the C version leading here).

  1. is an option (not sure about the performance impact)
  2. is not a good option for me: users setting the argument will not get the original behaviour (e.g. they will get the cached object), but also not an exception
  3. Indeed more effort, but acceptable from a backwards compatibility viewpoint: keep attributes as is, but via a proxy or dict watcher clear the cache on mutation (maybe in combination with a deprecation warning)

Lets also see what others think.

@dg-pb

dg-pb commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

So ok, I looked around a bit and it seems that ability to modify keywords of partial has become a feature.

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 partial does. If someone has been depending on the fact that func/args/keywords can be replaced by new objects, I think this is something that is ok to put a stop to.

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.keywords

So in case the method is cached, give keywords of underlying partial object. An in case descriptor is not yet built or is in state of being built on every call, then return partialmethod._keywords.

Subclass optimization also works correctly via:

temp = partial(lambda *_, **__: None, *func._args, **func.keywords)

@dg-pb

dg-pb commented Sep 3, 2026

Copy link
Copy Markdown
Contributor Author

Freezing of partialmethod

Made changes. Now:

  1. Backwards compatibility is preserved. Provided script now gives same answers when run with main and this branch (except __slots__).
  2. Performance benefit is retained (updated timings in description).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants