FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

gh-124652: partialmethod simplifications by dg-pb · Pull Request #124788 · python/cpython · GitHub

/ cpython Public

gh-124652: partialmethod simplifications - #124788

Open
dg-pb wants to merge 23 commits into
python:mainfrom
dg-pb:gh-124652-partialmethod
ConversationCommits (23)ChecksFiles changed
Open

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

Conversation

dg-pb commented Sep 30, 2024 •
edited
Loading

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

dg-pb requested a review from rhettinger as a code owner September 30, 2024 15:12
bedevere-app Bot mentioned this pull request Sep 30, 2024
Open
rhettinger removed their request for review September 30, 2024 16:31
tomasr8 reviewed Sep 30, 2024
View reviewed changes
Comment thread Lib/functools.py Outdated
dg-pb force-pushed the gh-124652-partialmethod branch from a04c14f to d217592 Compare October 6, 2024 16:56

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.

dg-pb mentioned this pull request Oct 17, 2024
Closed
eendebakpt reviewed Jan 4, 2025
View reviewed changes
Comment thread Lib/functools.py Outdated

eendebakpt commented Jan 4, 2025

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

eendebakpt reviewed Jan 4, 2025
View reviewed changes
Comment thread Lib/functools.py

eendebakpt commented Jan 4, 2025

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 commented Jan 4, 2025 •
edited
Loading

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

dg-pb mentioned this pull request Jan 8, 2025
Closed
dg-pb mentioned this pull request May 8, 2025
Merged
serhiy-storchaka self-requested a review May 8, 2025 08:02
eendebakpt reviewed Sep 9, 2025
View reviewed changes
Comment thread Lib/functools.py Outdated
Comment thread Lib/functools.py Outdated
picnixz reviewed Sep 9, 2025
View reviewed changes
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
Comment thread Lib/functools.py Outdated
Comment thread Lib/functools.py Outdated
Comment thread Lib/functools.py Outdated

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 commented Nov 28, 2025

Copy link
Copy Markdown
Member

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

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 Bot commented Apr 18, 2026

Copy link
Copy Markdown

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

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

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 Bot removed the stale Stale PR or inactive for long period of time. label Aug 22, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

eendebakpt eendebakpt left review comments
tomasr8 tomasr8 left review comments
picnixz picnixz left review comments
serhiy-storchaka Awaiting requested review from serhiy-storchaka

Assignees

No one assigned

Labels

awaiting review

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

Back | FazBrowse Home | New Git URL