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

Add __get__ to the partial object #121027

Closed
serhiy-storchaka opened this issue Jun 26, 2024 · 10 comments
Closed

Add __get__ to the partial object #121027

serhiy-storchaka opened this issue Jun 26, 2024 · 10 comments
Labels
3.14 new features, bugs and security fixes type-feature A feature request or enhancement

Comments

@serhiy-storchaka
Copy link
Member

serhiy-storchaka commented Jun 26, 2024

Feature or enhancement

In #119827 (comment), @rhettinger proposed to add the __get__ method to the partial object in functools. This is a breaking change, although the impact may be much lesser than of adding __get__ to builtin functions. But we should follow the common procedure for such changes: first add __get__ that emits FutureWarning with suggestion to wrap partial into staticmethod and return the partial object unchanged, then change the behavior few releases later.

Linked PRs

@serhiy-storchaka serhiy-storchaka added type-feature A feature request or enhancement 3.14 new features, bugs and security fixes labels Jun 26, 2024
@ncoghlan
Copy link
Contributor

The "Does practicality beat purity?" question also applies here.

Yes, if we'd had the placeholder functionality all along we might never have added partialmethod, but given that we did add it, what are we gaining by adding partial.__get__ that justifies the migration cost of having to wrap instances in staticmethod to get the old behaviour back when it was actually desired?

@serhiy-storchaka
Copy link
Member Author

I do not know. I have not found any issue which could be solved by adding __get__ to partial. The migration cost could be not high, but the benefit is unclear to me. It may simplify the partialmethod implementation in long run, but I did not check this.

@serhiy-storchaka
Copy link
Member Author

For clarification, I do not oppose this feature. But there is a procedure for making breaking changes. It should not bit buried as a side effect of adding other feature.

This can be worked around with significant core developer support if we decide the benefit of immediate change outweighs the risk. Or if we reclassify it as a bugfix.

@rhettinger
Copy link
Contributor

what are we gaining by adding partial.get

We are avoiding a problem that formerly didn't present itself. Partial is almost entirely understood as being an equivalent to a lambda or def. Before `Placeholder' was added, no one would encounter this. Now, it is a certitude.

Without adding __get__, one user after another will have to discover the hard way that these two are not equivalent:

set_alive2 = lambda self: self.set_state(True)  
set_alive3 = partial(set_state, Placeholder, True)

When they learn about the non-equivalence, it will break their mental model of partial() as it did for me.

Next, they will have to discover a workaround with partialmethod() which is mostly unused, mostly unknown, and really shouldn't be necessary anymore.

I don't think it should be acceptable to leave partial() in a non-harmonious state just because partialmethod() exists. The mental model of equivalence with lambda and def is too important.

@rhettinger
Copy link
Contributor

rhettinger commented Jun 26, 2024

Also, I don't think there is much of a downside. The docs for partial() don't promise to not act like a regular function in a class. It is certainly isn't an advertised feature of partial() that "I am just like an equivalent def or lambda but can't be used as a method."

There might be some minor disruption but it leads to a better designed more clear outcome than being trapped into a surprising behavior that was never intended.

If __get__ is not added, I'm thinking of withdrawing my support for Placeholder because it would make this wart prominent and it would limit natural uses of the new feature.

@serhiy-storchaka
Copy link
Member Author

As an option, we can add __get__ that emits a FutureWarning in 3.13 and change the behavior in 3.14. This will not delay introducion of Placeholder.

It may be late for a beta stage, but the premise is that it will not affect much user code. If this is true, adding it in 3.13 is safe. If it is not true, it is still better than breaking that code without a warning.

Surprisingly, it only breaks few recently added tests in the CPython test suite, and these tests were added specially to test how the code handles such weird corner case. They do not establish the correct behavior, they test that the current behavior will not be changed unintentionally.

cc @Yhg1s as the RM

@rhettinger
Copy link
Contributor

rhettinger commented Jun 26, 2024

+1 for adding a FutureWarning to 3.13 if it is still possible. I expect that people with encounter it rarely and that the workaround is to just wrap partial with staticmethod making the behavior explicit.

It would also be reasonable to just put a note in the docs. This wasn't a behavior previously promised in the docs and isn't something that people would typically encounter. I've taught partial to thousands of people and only noticed the absence of __get__ when the new Placeholder option became available.

get the old behaviour back when it was actually desired?
As near as I can tell, this is almost never desired. And as a code reviewer if someone wanted this behavior, it should be made explicit because almost no one knows about it and it is surprising.

serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jun 27, 2024
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jun 27, 2024
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jun 27, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jun 27, 2024
serhiy-storchaka added a commit to serhiy-storchaka/cpython that referenced this issue Jun 27, 2024
Yhg1s pushed a commit that referenced this issue Jun 27, 2024
…H-121086) (#121092)

gh-121027: Add a future warning in functools.partial.__get__ (GH-121086)
(cherry picked from commit db96edd)

Co-authored-by: Serhiy Storchaka <[email protected]>
@rhettinger
Copy link
Contributor

Thanks for doing this.

@PeterJCLaw
Copy link
Contributor

PeterJCLaw commented Jul 4, 2024

In #119827 (comment), @rhettinger proposed to add the __get__ method to the partial object in functools. This is a breaking change, although the impact may be much lesser than of adding __get__ to builtin functions. But we should follow the common procedure for such changes: first add __get__ that emits FutureWarning with suggestion to wrap partial into staticmethod and return the partial object unchanged, then change the behavior few releases later.

I'm not sure whether your comment "This is a breaking change" is intended to refer to the future change to make the __get__ method return something other than self, or to the change as currently implemented, though I'm guessing the former. Either way, I wanted to highlight that the change as implemented may already be breaking for some use-cases. In particular tools which rely on introspection of code objects may be caught by this, as Jedi has been (it renders function signatures for method descriptors differently than other functions or class instances). Jedi can almost certainly cope with this, so I'm not arguing either for or against this change -- just wanted to share the impact that this has already had in case there might be other consequences of functools.partial being detected to be a descriptor.

@serhiy-storchaka
Copy link
Member Author

It referred to the future change, but, unfortunately, adding even trivial __get__() that returns self is a breaking change. It required changing the inspect.signature() code, and may also affect the third-party code. There is no good solution for this.

noahbkim pushed a commit to hudson-trading/cpython that referenced this issue Jul 11, 2024
estyxx pushed a commit to estyxx/cpython that referenced this issue Jul 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
3.14 new features, bugs and security fixes type-feature A feature request or enhancement
Projects
None yet
Development

No branches or pull requests

4 participants