New function to get attribute without binding and use it instead of __func__

[jdemeyer]

There are various instances in Sage of code like

someclass.ParentMethods.method.__func__

This is because we typically want to get the actual function from the ParentMethods body without binding to ParentMethods. This may apply to more general descriptors besides functions.

To solve this, I propose a new function rawattr() which is like getattr() but without binding.

CC: @nthiery

Component: categories

Author: Jeroen Demeyer

Branch/Commit: 854eb48

Reviewer: Erik Bray

Issue created by migration from https://trac.sagemath.org/ticket/25179

[nthiery]

comment:1

Hi Jeroen,

I agree that the idiom is not great :-)
(though it makes explicit that something non trivial is happening)

Do you have a way to implement your proposition without having to specify a super class or metaclass in each and every XXXMethods class?

[jdemeyer]

comment:2

Replying to @nthiery:

Do you have a way to implement your proposition without having to specify a super class or metaclass in each and every XXXMethods class?

I don't think so. My idea would be to replace

class ParentMethods:

by

class ParentMethods(Namespace):

Of course, the current code works so it's not required to do that change everywhere. I would suggest to do the change only when needed.

[jdemeyer]

Branch: u/jdemeyer/parentmethods__use_a_metaclass_which_does_not_bind_methods

[sagetrac-git]

Commit: 39bb476

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

a674ad0	NamespaceMeta for namespace classes
39bb476	Use Namespace for ParentMethods/ElementMethods where needed

[jdemeyer]

comment:5

These are the minimal changes needed to get rid of the existing __func__ accesses related to ParentMethods and ElementMethods.

[jdemeyer]

Author: Jeroen Demeyer

[nthiery]

comment:6

Thanks for investigating this. I definitely see the point of trying to improve the idiom which expose an implementation detail about bound/unbound methods. However I am reluctant with the current solution. Somehow it's too unlocal and non explicit. The risk is that a developper will see the idiom (without __func__) in the target class, try to replicate it, without noticing that there is something special to be done in the original class.

I would be more in favor of a local and explicit idiom like:

    foo = unbind(Bars.ParentMethods.foo)

or

    foo = get_unbound(Bars.ParentMethod, "foo")

where we would use unbind to hide the implementation details and support any kind of object,
not just methods.

What do you think? Opinions anyone else?

[jdemeyer]

comment:7

Replying to @nthiery:

where we would use unbind to hide the implementation details and support any kind of object,
not just methods.

This wouldn't simply because you cannot "unbind" arbitrary objects. Imagine

class X:
    @property
    def answer(self):
        return 42

There is no way to unbind 42 to the property object X.answer.

get_unbound would work: it's essentially a one-liner to implement that because it's exactly what the C API function _PyType_Lookup does. It gets the bare attribute from a class and it doesn't fancy stuff like __get__ or __getattr__ or metaclass shenanigans.

[jdemeyer]

comment:8

I'll try to implement that on a different branch.

[sagetrac-git]

Changed commit from 39bb476 to 3c0b127

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

bf4c33d	New function rawattr to get attributes without binding
3c0b127	Use rawattr where needed

[sagetrac-git]

Changed commit from 3c0b127 to 680a1c3

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

ffb07a2	New function rawattr to get attributes without binding
c182d50	Use rawattr where needed
ff1c4d7	f1
680a1c3	f2

[sagetrac-git]

Changed commit from 680a1c3 to 8598cc1

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

8165f15	New function rawattr to get attributes without binding
8598cc1	Use rawattr where needed

[embray]

comment:15

This is neat, but it seems like extreme overkill for a relatively minor issue. Were there any other use cases for this you had in mind? Because the current use case is handled much more simply by other means.

[embray]

comment:16

Replying to @jdemeyer:

These are the minimal changes needed to get rid of the existing __func__ accesses related to ParentMethods and ElementMethods.

I'm not sure what you mean by this. While I'm not particularly fond of the solution in #24955 either, it's not clear how this is any better, much less simpler.

[embray]

comment:17

I'm also -1 on immense mucking around with Python interpreter internals for seemingly little need/benefit. Although I agree it may be a useful capability to have and might be worth proposing upstream.

As a different way of replacing the current idiom for copying methods from another class's ParentMethods, I had also floated the idea in #24955 of a class decorator that does the same thing. This avoids the need for a metaclass or inheritence--just something like @inherit_methods_from(<cls>, [<list of method names>]). Maybe "inherit" is not the best word choice though if we want to avoid allusion to class inheritance.

[embray]

comment:29

I think my problem just comes down to how I read "binding the attribute to the object" (and maybe it's my reading that's wrong). I think I would have worded this just subtly differently like

"Like getattr() but without invoking the binding behavior of descriptors under normal attribute access."

[nthiery]

comment:30

Replying to @embray:

I think my problem just comes down to how I read "binding the attribute to the object" (and maybe it's my reading that's wrong).

I guess my own reading is influenced by the first line of the
Descriptor How To:

    In general, a descriptor is an object attribute with “binding behavior”

No idea what the average reading will be though.

I think I would have worded this just subtly differently like

"Like getattr() but without invoking the binding behavior of descriptors under normal attribute access."

Sounds good to me!

[jdemeyer]

comment:31

Replying to @embray:

Replying to @jdemeyer:

If your question is "why does rawattr look both in the instance and in the class?", then my answer is: it makes it convenient to replace __func__ regardless of whether the method was bound or unbound.

I don't understand this statement.

Suppose X is a class. I meant that we should support both rawattr(X, name) (getting the attribute from X itself) and rawattr(X(), name) (getting the attribute from X().__class__)

[jdemeyer]

comment:32

Replying to @embray:

I just don't know why the implementation has to be so complicated.

I don't think that the implementation is complicated at all. I actually think it would be more complicated in pure Python since pure Python has no equivalent for _PyType_Lookup.

Also note that most of the complications come from the support for old-style instances and classes. Once you remove that, it's basically three lines of code.

[jdemeyer]

comment:33

Replying to @embray:

I just don't know why the implementation has to be so complicated.

Do you mean that the functionality of raw_getattr() should be simplified (that would make it less analogous to getattr) or that the implementation of that functionality can be simplified? Or is it just that you consider it complicated only because it's implemented in C?

[sagetrac-git]

Changed commit from 8598cc1 to 854eb48

[sagetrac-git]

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:

b605557	New function raw_getattr to get attributes without binding
854eb48	Use raw_getattr where needed

[jdemeyer]

comment:35

Renamed rawattr -> raw_getattr

[embray]

comment:36

Replying to @jdemeyer:

Replying to @embray:

I just don't know why the implementation has to be so complicated.

Do you mean that the functionality of raw_getattr() should be simplified (that would make it less analogous to getattr) or that the implementation of that functionality can be simplified? Or is it just that you consider it complicated only because it's implemented in C?

Sorry, yes, I should clarify "complicated". In part I mean that it's implemented in C, though I don't believe that alone makes it complicated. I'm more concerned about the over-reliance on undocumented internals.

I could just as easily write this function in pure Python, and I would be interested in a performance comparison between a pure python version and this C version, but I could try doing that myself.

To be clear I'm otherwise +1 on this--that's my only misgiving at this point.

[tscrim]

comment:37

FYI - I have a new use-case for this on #25146 where I want to treat functions as class-level attributes, but Python by default binds them as methods.

[jdemeyer]

comment:38

Replying to @embray:

I'm more concerned about the over-reliance on undocumented internals.

Well, the internals of old-style instances and classes aren't likely going to change in Python 2.7.16. And _PyType_Lookup and _PyObject_GetDictPtr are indeed private, but pretty stable: they have existed for a long time and still do in Python master.

[jdemeyer]

comment:39

I am starting to think that a super-like syntax might be better: something like namespace(obj).attr instead of raw_getattr(obj, "attr").

Advantages:

1. It allows specifying the attribute name in a more natural way, not as string.

2. namespace(obj) is an object which can be created once (and even stored or passed around) in case that you need to access many raw attributes of the same object.

3. Usable as class decorator to create namespace "classes" (whether this would still be a class or not remains to be seen).

The main disadvantage is that it would be more complicated to implement and slower.

[embray]

comment:40

For Python 3.3+ (and I think for practical purposes we're not interested in Python versions below 3.3) it looks like you want to use PyObject_GenericGetDict. This does practically the same thing you're doing with _PyObject_GetDictPtr but it also correctly implements the semantics of PEP-412.

For Python 2.7 I agree it should be safe-enough since it's not going to change.

As far as _PyType_Lookup is concerned, while I'm still wary of using any function that's not part of the limited API, I see now that we were already using it anyways so I'm not going to bother about it here, and I don't really see a better alternative--ISTM this might be a good candidate for adding to the documented API since it is important for implementing custom attribute access on extension types.

[embray]

comment:41

I like the idea of the namespace type, but it's also useful to have the stand-alone raw_getattr. namespace would then just be built on top of that.

[jdemeyer]

comment:42

Replying to @embray:

for practical purposes we're not interested in Python versions below 3.3

Did you forget that 2.7 is still the default and only supported version?

EDIT: I guess you meant 3.x with x < 3.

[embray]

comment:43

Yes, Python 3 versions below 3.3 (and really 3.6, but I'm thinking about what Python 3 versions are still in some distros...)

[jdemeyer]

comment:44

I don't think that there is anything wrong with using _PyObject_GetDictPtr. It does exactly what I want. I don't need to create the dict when it's not there and I don't need exceptions thrown when the object does not support __dict__.

And it only works on Python 3, so it would only complicate the code.

If you insist, I will use it though.

[embray]

comment:45

No, it's okay. I just think it goes without saying that internal functions should be avoided. This whole thing could be written in pure Python and it seems like an overwrought pre-mature optimization not to. OTOH I feel like it's a fault on the Python side that a) this function doesn't just come built-in--sometimes it is useful, especially for introspection, to bypass some of the high-level attribute machinery, and b) that this can't currently be easily implemented by third-party C code without reliance on internal functions. Which in turn gives more credence to the idea that this should be a built-in.

I think I would like to propose that to python-ideas if you don't mind, and use your code as an example.

[jdemeyer]

comment:46

Replying to @embray:

This whole thing could be written in pure Python

Yes, it could be written in Python. It could also be written in C.

You seem to find it a problem that it's implemented in C but I don't understand why. I would argue that the C implementation is conceptually simpler than the potential Python implementation because Python doesn't have the equivalent of _PyType_Lookup.

[jdemeyer]

comment:47

Replying to @embray:

I think I would like to propose that to python-ideas if you don't mind, and use your code as an example.

Feel free. But do remove the Python 2 stuff if you quote my code.

[embray]

comment:48

Python is practically pseudocode IMO. It's almost always conceptually simpler than C (especially if you're also relying on undocumented internals to do something).

[embray]

Reviewer: Erik Bray

[vbraun]

Changed branch from u/jdemeyer/parentmethods__use_a_metaclass_which_does_not_bind_methods to 854eb48