Calling help executes @classmethod @property decorated methods
#89519
Comments
|
I noticed some strange behaviour when calling from time import sleep
from abc import ABC, ABCMeta, abstractmethod
class MyMetaClass(ABCMeta):
@classmethod
@property
def expensive_metaclass_property(cls):
"""This may take a while to compute!"""
print("computing metaclass property"); sleep(3)
return "Phew, that was a lot of work!"
class MyBaseClass(ABC, metaclass=MyMetaClass):
@classmethod
@property
def expensive_class_property(cls):
"""This may take a while to compute!"""
print("computing class property .."); sleep(3)
return "Phew, that was a lot of work!"
@property
def expensive_instance_property(self):
"""This may take a while to compute!"""
print("computing instance property ..."); sleep(3)
return "Phew, that was a lot of work!"
class MyClass(MyBaseClass):
"""Some subclass of MyBaseClass"""
help(MyClass)Calling The other two properties, Secondly, only The problem is also present in '3.10.0rc2 (default, Sep 28 2021, 17:57:14) [GCC 10.2.1 20210110]' Stack Overflow thread: https://stackoverflow.com/questions/69426309 |
randolf-scholz
mannequin
added
3.9
|
I updated the script with dome more info. The class-property gets actually executed 5 times when calling |
|
See also: https://bugs.python.org/issue44904 |
|
Randolf, what specific behaviors do you consider to be bugs that should be fixed. What would a test of the the changed behavior look like? This should perhaps be closed as a duplicate of bpo-44904. Randolf, please check and say what you thing. |
|
On current 3.9, 3.10, 3.11, on Windows running in IDLE, I see |
terryjreedy
added
3.11
|
@terry I think the problem boils down to the fact that Calling mappingproxy({'__module__': '__main__',
'expensive_class_property': <classmethod at 0x7f893e95dd60>,
'expensive_instance_property': <property at 0x7f893e8a5860>,
'__dict__': <attribute '__dict__' of 'MyBaseClass' objects>,
'__weakref__': <attribute '__weakref__' of 'MyBaseClass' objects>,
'__doc__': None,
'__abstractmethods__': frozenset(),
'_abc_impl': <_abc._abc_data at 0x7f893fb98740>})Two main issues:
isinstance(func, property) or (`isinstance(func, classmethod)` and `isinstance(func.__func__, property)`
MyBaseClass.expensive_class_property = 2
MyBaseClass().expensive_instance_property = 2Then the first line erroneously executes, such that MyBaseClass.dict['expensive_class_property'] is now |
|
If fact, in the current state it seem that it is impossible to implement real class-properties, for a simple reason: descriptor.__set__ is only called when setting the attribute of an instance, but not of a class!! import math
class TrigConst:
const = math.pi
def __get__(self, obj, objtype=None):
print("__get__ called")
return self.const
def __set__(self, obj, value):
print("__set__ called")
self.const = value
class Trig:
const = TrigConst() # Descriptor instanceTrig().const # calls TrigConst.__get__
Trig().const = math.tau # calls TrigConst.__set__
Trig.const # calls TrigConst.__get__
Trig.const = math.pi # overwrites TrigConst attribute with float. |
|
I'm merging bpo-44904 into this one because they are related. |
|
It may have been a mistake to allow this kind of decorator chaining.
We either need to undo the Python 3.9 feature from bpo-19072, or we need to put serious thought into making all these descriptors composable in a way that would match people's expectations. |
|
Dear Raymond, I think decorator chaining is a very useful concept. At the very least, if all decorators are functional (following the The question is: can we get similar behaviour when allowing decoration with stateful objects, i.e. classes? This seems a lot more difficult. At the very least, in the case of ### Expectation
Using your pure-python versions of property / classmethod from https://docs.python.org/3.9/howto/descriptor.html, I was able to write down a variant of def ClassMethod(func):
class Wrapped(type(func)):
def __get__(self, obj, cls=None):
if cls is None:
cls = type(obj)
if hasattr(type(self.__func__), '__get__'):
return self.__func__.__get__(cls)
return MethodType(self.__func__, cls)
return Wrapped(func)I attached a full MWE. Unfortunately, this doesn't fix the ### Some further Proposals / Ideas
|
|
Some thoughts from me, as an unqualified but interested party: Like Randolph, I very much like having class properties in the language, and have used them in several projects since their introduction in 3.9. I find they're especially useful with Enums. However, while the bug in doctest, for example, is relatively trivial to fix (see my PR in bpo-44904), it seems to me plausible that bugs might crop up in other standard library modules as well. As such, leaving class properties in the language might mean that several more bugfixes relating to this feature would have to be made in the future. So, I can see the argument for removing them. It seems to me that Randolph's idea of changing This would at least mean that Note that this change wouldn't fix the bugs in abc and doctest, nor does it fix the issue with class-property setter logic that Randolph identified. With regards to the point that |
|
@alex Regarding my proposal, the crucial part is the desiderata, not the hacked up implementation I presented. And I really believe that at least that part I got hopefully right. After all, what does Regarding code breakage - yes that is a problem, but code is already broken and imo Python needs to make a big decision going forward:
At this point however I want to emphasize that I am neither a CS major nor a python expert (I only started working with the language 3 years ago), so take everything I say as what it is - the opinion of some random stranger from the internet (: |
|
I've put up a PR; I'm not sure it's the best way to fix it. I will look more into it and will try to post some details about the PR later today. |
|
I missed that this is assigned to Raymond, hope we didn't duplicate any effort (it only took me a short while to do the PR). Apologies.. |
|
I've looked more into it, the issue is that even before an object can be tested with So I think my PR is going in the right direction, adding guards in the inspect function and in two |
|
added Graham Dumpleton to nosy list in case he has some useful insight here, I think the PR from bpo-19072 may have been adapted from grahamd's patch originally? |
|
Too much to grok right now. There is already a convention for what a decorator wraps. It is __wrapped__. Line 70 in 3405792 Don't use __func__ as that has other defined meaning in Python related to bound methods and possibly other things as well and overloading on that will break other stuff. In part I suspect a lot of the problems here are because things like classmethod and functools style decorators are not proper transparent object proxies, which is the point of what the wrapt package was trying to solve so that accessing stuff on the wrapper behaves as much as possible as if it was done on what was wrapped, including things like isinstance checks. |
|
Also see: https://bugs.python.org/issue42073 The classmethod pass through broke some existing code and the "fix" for it looks dubious: if hasattr(type(self.f), '__get__'):
return self.f.__get__(cls, cls) |
|
I propose deprecating classmethod chaining. It has become clear that it doesn't really do what people wanted and can't easily be made to work. By even suggesting that some stateful decorators are composable, we've ventured onto thin ice. Wrapping property in a classmethod doesn't produce something that behaves like a real property. Mixing staticmethod and property doesn't work at all. Putting abstractmethod in the mix doesn't work well either. The ecosystem of code inspection tools, like help() in this issue, is wholly unprepared for recognizing and working around these combinations. The latest "fix" for classmethod chaining looks weird and worriesome as well: self.f.__get__(cls, cls). Classmethod chaining is relatively new, so we will affect very little code by deprecating it. Any of the possible use cases can be served in other ways like the wrapt package or by explicit code in __getattribute__. |
|
It makes me sad that the stdlib will no longer provide a way to compose classmethods with other descriptors. However, I agree that deprecating classmethod chaining is probably the correct course of action, given the complications this feature has caused, and the backwards-compatibility issues it raises. This is probably a conversation for another BPO issue or the python-ideas mailing list, but I hope some consideration can be given in the future as to whether a new classmethod-like feature could possibly be added to functools that would enable this kind of decorator chaining without the same code-breakage concerns that this feature has had. |
|
Probably >90% of the use-cases for chaining classmethod are a read-only class property. Perhaps the general case of classmethod(descriptor(...)) should be treated separately from the common case?
If classmethod(property(f)) is going to be removed, can an implementation of classproperty be considered as a replacement? Or perhaps support via the other ordering, property(classmethod(f))? |
|
Seems the problem with the deprecation is that Python functions are descriptors, so in case of normal class methods we pass the same path as for properties. Am I right? I think that instead of emitting a deprecation warning when read a class property, it should be emitted when the class property is created. We can start with emitting warning if the |
|
@randolf-scholz I also did experiments on class properties, but with some differences:
My observations are:
Expectations from user side are:
The following code demonstrates how a descriptor is replaced by an integer: from typing import Callable, Any
def classproperty(method):
"""A decorator adding properties to classes."""
class ClassPropertyDescriptor:
"""A descriptor for class properties.."""
_name: str
_getter: Callable
_setter: Callable
_deleter: Callable
def __init__(self, getter: Callable = None, setter: Callable = None, deleter: Callable = None, doc: str = None):
self._getter = getter
self._setter = setter
self._deleter = deleter
self._name = ''
if doc is None and getter is not None:
doc = getter.__doc__
self.__doc__ = doc
def __set_name__(self, owner: Any, name: str):
self._name = name
def __get__(self, instance: Any, objtype: type = None):
if self._getter is None:
raise AttributeError(f'unreadable attribute {self._name}')
return self #._getter(objtype)
def __set__(self, instance: Any, value: Any):
print(f"__set__: {value}")
if self._setter is None:
raise AttributeError(f"can't set attribute {self._name}")
self._setter(instance, value)
def __delete__(self, instance: Any):
if self._deleter is None:
raise AttributeError(f"can't delete attribute {self._name}")
self._deleter(type(instance))
def getter(self, getter: Callable):
prop = type(self)(getter, self._setter, self._deleter, self.__doc__)
prop._name = self._name
return prop
def setter(self, setter: Callable):
prop = type(self)(self._getter, setter, self._deleter, self.__doc__)
prop._name = self._name
print(f"setter: self: {id(self)}; prop:{id(prop)}")
return prop
def deleter(self, deleter: Callable):
prop = type(self)(self._getter, self._setter, deleter, self.__doc__)
prop._name = self._name
return prop
return ClassPropertyDescriptor(method)
class TestClass:
_member: int = 1
@classproperty
def Member(cls) -> int:
"""Class_1.Member"""
return cls._member
@Member.setter
def Member(cls, value: int) -> None:
print(f"value: {value} {cls} {id(cls)}")
cls._member = value
def main():
print(f"TestClass.Member: {TestClass.Member} {id(TestClass.Member)}")
TestClass.Member.__set__(TestClass, 12)
TestClass.Member = 13
TestClass.Member.__set__(TestClass, 14)
print(f"TestClass.Member: {TestClass.Member} {id(TestClass.Member)}")
if __name__ == "__main__":
main()Output: Error message: By changing how the descriptor works for Tested with Python 3.10. |
@rhettinger - Could you share an example of how you would do this as a replacement for chaining |
|
It's really disappointing that this behavior would be changed without the standard deprecation cycle. Surely there should be an alternative introduced so people have time to transition? In addition, would this break the pattern described as an alternative to |
|
Wrapping Since |
|
@KevinMGranger @MattF-NSIDC classmethod docs says that
def abstractmethod(funcobj):
"""A decorator indicating abstract methods.
Requires that the metaclass is ABCMeta or derived from it. A
class that has a metaclass derived from ABCMeta cannot be
instantiated unless all of its abstract methods are overridden.
The abstract methods can be called using any of the normal
'super' call mechanisms. abstractmethod() may be used to declare
abstract methods for properties and descriptors.
Usage:
class C(metaclass=ABCMeta):
@abstractmethod
def my_abstract_method(self, ...):
...
"""
funcobj.__isabstractmethod__ = True
return funcobj |
See the discussion here: python/cpython#89519
See the discussion here: python/cpython#89519
See the discussion here: python/cpython#89519
@zduvall Here is an implementation for a read-only class property that is backwards/forwards compatible: import sys
from typing import Generic, Optional, TypeVar
if sys.version_info < (3, 9):
from typing import Callable, Type
else:
from builtins import type as Type
from collections.abc import Callable
T = TypeVar("T")
RT = TypeVar("RT")
class classproperty(Generic[T, RT]):
"""
Class property attribute (read-only).
Same usage as @property, but taking the class as the first argument.
class C:
@classproperty
def x(cls):
return 0
print(C.x) # 0
print(C().x) # 0
"""
def __init__(self, func: Callable[[Type[T]], RT]) -> None:
# For using `help(...)` on instances in Python >= 3.9.
self.__doc__ = func.__doc__
self.__module__ = func.__module__
self.__name__ = func.__name__
self.__qualname__ = func.__qualname__
# Consistent use of __wrapped__ for wrapping functions.
self.__wrapped__: Callable[[Type[T]], RT] = func
def __set_name__(self, owner: Type[T], name: str) -> None:
# Update based on class context.
self.__module__ = owner.__module__
self.__name__ = name
self.__qualname__ = owner.__qualname__ + "." + name
def __get__(self, instance: Optional[T], owner: Optional[Type[T]] = None) -> RT:
if owner is None:
owner = type(instance)
return self.__wrapped__(owner)Additionally, as shown in this issue thread, running Note also that as shown by @Paebbels, the descriptor protocol ONLY supports class getters, it does not support class setters or class deleters. As such, only read-only class properties are possible. This isn't to say you can't have mutable class properties however, only that you can't have them follow the descriptor protocol for the nicer syntax. You can instead have the class getter return objects with import sys
from types import MethodType
from typing import Any, Callable, Generic, Optional, TypeVar, Union
if sys.version_info < (3, 9):
from typing import Type
else:
from builtins import type as Type
if sys.version_info < (3, 11):
Self = TypeVar("Self", bound="classproperty")
else:
from typing import Self
T = TypeVar("T")
class BoundClassDescriptor(Generic[T]):
def __init__(
self,
owner: Type[T],
descriptor: property,
module: Optional[str],
name: Optional[str],
qualname: Optional[str],
) -> None:
self.__doc__ = descriptor.__doc__
self.__module__ = module
self.__name__ = name
self.__qualname__ = qualname
self.__wrapped__: property = descriptor
self.get: Callable[[], Any]
self.set: Callable[[Any], object]
self.delete: Callable[[], object]
if descriptor.fget is not None:
self.get = MethodType(descriptor.fget, owner)
if descriptor.fset is not None:
self.set = MethodType(descriptor.fset, owner)
if descriptor.fdel is not None:
self.delete = MethodType(descriptor.fdel, owner)
class classproperty(Generic[T]):
def __init__(
self,
fget: Optional[Callable[[Type[T]], Any]] = None,
fset: Optional[Callable[[Type[T], Any], object]] = None,
fdel: Optional[Callable[[Type[T]], object]] = None,
doc: Optional[str] = None,
) -> None:
self.__module__ = None
self.__name__ = None
self.__qualname__ = None
self.__wrapped__: property = property(fget, fset, fdel, doc)
def __set_name__(self, owner: Type[T], name: str) -> None:
self.__module__ = owner.__module__
self.__name__ = name
self.__qualname__ = owner.__qualname__ + "." + name
def __get__(
self, instance: Optional[T], owner: Optional[Type[T]] = None
) -> BoundClassDescriptor[T]:
if owner is None:
owner = type(instance)
return BoundClassDescriptor(
owner,
self.__wrapped__,
self.__module__,
self.__name__,
self.__qualname__,
)
def getter(self: Self, func: Callable[[Type[T]], Any]) -> Self:
self.__wrapped__ = self.__wrapped__.getter(func)
return self
def setter(self: Self, func: Callable[[Type[T], Any], object]) -> Self:
self.__wrapped__ = self.__wrapped__.setter(func)
return self
def deleter(self: Self, func: Callable[[Type[T]], object]) -> Self:
self.__wrapped__ = self.__wrapped__.deleter(func)
return selfExample: class A:
_foo = 0
@classproperty
def foo(cls):
"""Description of foo class property."""
return cls._foo
@foo.setter
def foo(cls, value):
cls._foo = value
class B(A):
passUsage: >>> # Getters/setters/deleters work as you'd expect using `A._foo` directly.
>>> A.foo.get()
0
>>> B.foo.get()
0
>>>
>>> A.foo.set(1)
>>> A.foo.get()
1
>>> B.foo.get()
1
>>>
>>> B.foo.set(2)
>>> A.foo.get()
1
>>> B.foo.get()
2
>>>
>>> help(A.foo) # Instance documentation works in Python >= 3.9.
Help on BoundClassDescriptor in A:
A.foo = <BoundClassDescriptor object>
Description of foo class property.
>>> help(A.foo.get)
Help on method foo in module __main__:
foo() method of builtins.type instance
Description of foo class property.
>>> help(A.foo.set) # You could have custom setter documentation.
Help on method foo in module __main__:
foo(value) method of builtins.type instance
>>> help(A.foo.delete) # Only has a delete method if one was set.
Traceback (most recent call last):
...
AttributeError: 'BoundClassDescriptor' object has no attribute 'delete'Of course the documentation won't work on static analyzers, but you'll at least be able to dynamically use |
|
One minor detail concerning abstract properties is that removing from abc import ABC, abstractmethod
class C(ABC):
@property
@classmethod
@abstractmethod
def expensive_class_property(cls):
...I don't think anyone's going to miss this though, and if you're going so far then the workaround is to use Correction:You'll need to wrap it with from typing import ClassVar, Protocol
class C(Protocol):
class_property: ClassVar[int]
class D(C):
class_property = 5Otherwise you'll need to use from typing import ClassVar, Protocol, TypeVar, Union
T = TypeVar("T")
class Attribute(Protocol[T]):
def __get__(self, instance, owner=None) -> T:
...
def __set__(self, instance, value: int) -> None:
...
class C(Protocol):
foo: ClassVar[Union[Attribute[int], int]]
class C1(C):
pass
class C2(C):
foo = 3
C1() # Cannot instantiate abstract class "D" with abstract attribute "foo"
C2() # Works fine.
reveal_type(C2.foo) # builtins.int
reveal_type(C2().foo) # builtins.int |
@SimpleArt Thus:
/cc @skoehler |
|
@Paebbels To clarify, the inconsistency is assuming "read-only property" translates to "read-only class property" or "read-only descriptor". There's simply no such thing as an actual "read-only" thing, all a read-only property does is define a Quite frankly, changing it so that |
After writing the previous comment, I realized that there's a much simpler alternative that avoids all of the complicated code. Metaclasses. class MetaC(type):
@property
def expensive_class_property(self):
...
class C(metaclass=MetaC):
passUnlike properties which are defined within Just be wary of metaclass conflicts. |
|
I can't help but wonder, not being very familiar with the C side of things here to know how Indeed this original problem is not necessarily the fault of class Descriptor:
"""Description of this descriptor."""
def __get__(self, instance, owner=None):
print("expensive descriptor getter")
class Foo:
foo = Descriptor()Running I assume the fact that the getter is ran 4 times instead of once is definitely a bug that is getting fixed. I also assume the point of running the getter is so that the help documentation can display the value, as it does at runtime: However, compare the documentation to that of >>> class Bar:
... @property
... def foo():
... """Description of this property."""
...
>>> help(Bar)
Help on class Bar in module __main__:
class Bar(builtins.object)
| Readonly properties defined here:
|
| foo
| Description of this property.
|
| ----------------------------------------------------------------------
| Data descriptors defined here:
|
| __dict__
| dictionary for instance variables (if defined)
|
| __weakref__
| list of weak references to the object (if defined)From the documentation specifically, the way I see it is that there are two differences:
Personally, I don't find displaying the value very useful. At the point that it's something like a descriptor, there's a reason it's not just a value. It may not be the same value every time or I may not want to calculate it if I don't have to. My expectation is that using This approach would also let one display a value if they want by implementing |
|
Concerning the chaining of class property:
def __init__(self, fget=None, fset=None, fdel=None, doc=None, cget=None):
if doc is None and fget is not None:
doc = fget.__doc__
if isinstance(fget, classmethod):
if cget is not None:
raise TypeError("got class getter with classmethod fget")
cget = fget
fget = None
self.fget = fget
self.fset = fset
self.fdel = fdel
self.__doc__ = doc
self.cget = cget
def __get__(self, instance, owner=None):
if instance is None:
return self.cget(owner)
elif self.cget is None: # The default behavior is to return the property object.
return self
else:
return self.fget(owner)
def getter(self, fget):
if isinstance(fget, classmethod):
return property(self.fget, self.fset, self.fdel, self.__doc__, fget)
else:
return property(fget, self.fset, self.fdel, self.__doc__, self.cget)Example usage: class C:
@property
@classmethod
def is_instance(cls) -> bool:
return False
@is_instance.getter
def is_instance(self) -> bool:
return True
print(C.is_instance) # False
print(C().is_instance) # TrueAlthough I can't imagine a case where you'd want to use both, this would definitely let you consistently use each approach individually. The drawback that I see from this however is the fact that people might get confused and think that you can use |
|
Now 3.13 is in the works, so this can be changed according to the TODO item in |
|
I notice that this feature will be removed by #110163 in Python 3.13. So are there any workaround? If so, that may be great if it could be added to the Descriptor HowTo Guide, because it has been commonly used. |
|
Here is the sample of the implementation. You can also use a metaclass to add a property. |
Note: these values reflect the state of the issue at the time it was migrated and might not reflect the current state.
Show more details
GitHub fields:
bugs.python.org fields:
Linked PRs
The text was updated successfully, but these errors were encountered: