Issue45356
This issue tracker has been migrated to GitHub,
and is currently read-only.
For more information,
see the GitHub FAQs in the Python's Developer Guide.
Created on 2021-10-03 19:45 by randolf.scholz, last changed 2022-04-11 14:59 by admin.
Files | ||||
---|---|---|---|---|
File name | Uploaded | Description | Edit | |
classmethod_property.py | randolf.scholz, 2021-10-03 19:45 | |||
classmethod_property.py | randolf.scholz, 2021-10-03 20:21 | |||
ClassPropertyIdea.py | randolf.scholz, 2021-10-11 12:55 |
Pull Requests | |||
---|---|---|---|
URL | Status | Linked | Edit |
PR 29239 | open | andrei.avk, 2021-10-27 14:14 |
Messages (23) | |||
---|---|---|---|
msg403109 - (view) | Author: Randolf Scholz (randolf.scholz) | Date: 2021-10-03 19:45 | |
I noticed some strange behaviour when calling `help` on a class inheriting from a class or having itself @classmethod @property decorated methods. ```python 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 `help(MyClass)` will cause `expensive_class_property` to be executed 4 times (!) The other two properties, `expensive_instance_property` and `expensive_metaclass_property` are not executed. Secondly, only `expensive_instance_property` is listed as a read-only property; `expensive_class_property` is listed as a classmethod and `expensive_metaclass_property` is unlisted. 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 |
|||
msg403110 - (view) | Author: Randolf Scholz (randolf.scholz) | Date: 2021-10-03 20:21 | |
I updated the script with dome more info. The class-property gets actually executed 5 times when calling `help(MyClass)` ``` Computing class property of <class '__main__.MyClass'> ...DONE! Computing class property of <class '__main__.MyClass'> ...DONE! Computing class property of <class '__main__.MyClass'> ...DONE! Computing class property of <class '__main__.MyBaseClass'> ...DONE! Computing class property of <class '__main__.MyClass'> ...DONE! ``` |
|||
msg403111 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2021-10-03 21:03 | |
See also: https://bugs.python.org/issue44904 |
|||
msg403504 - (view) | Author: Terry J. Reedy (terry.reedy) * | Date: 2021-10-08 20:47 | |
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 #44904. Randolf, please check and say what you thing. |
|||
msg403505 - (view) | Author: Terry J. Reedy (terry.reedy) * | Date: 2021-10-08 20:53 | |
On current 3.9, 3.10, 3.11, on Windows running in IDLE, I see computing class property .. computing class property .. computing class property .. computing class property .. computing class property .. Help ... |
|||
msg403578 - (view) | Author: Randolf Scholz (randolf.scholz) | Date: 2021-10-10 09:57 | |
@Terry I think the problem boils down to the fact that `@classmethod @property` decorated methods end up not being real properties. Calling `MyBaseClass.__dict__` reveals: ```python 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: 1. Any analytics or documentation tool that has special treatment for properties may not identify 'expensive_class_property' as a property if they simply check via `isinstance(func, property)`. Of course, this could be fixed by the tool-makers by doing a conditional check: ```python isinstance(func, property) or (`isinstance(func, classmethod)` and `isinstance(func.__func__, property)` ``` 2. `expensive_class_property` does not implement `getter`, `setter`, `deleter`. This seems to be the real dealbreaker, for example, if we do ```python MyBaseClass.expensive_class_property = 2 MyBaseClass().expensive_instance_property = 2 ``` Then the first line erroneously executes, such that MyBaseClass.__dict__['expensive_class_property'] is now `int` instead of `classmethod`, while the second line correctly raises `AttributeError: can't set attribute` since the setter method is not implemented. |
|||
msg403582 - (view) | Author: Randolf Scholz (randolf.scholz) | Date: 2021-10-10 10:37 | |
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!! ```python 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 instance ``` ```python Trig().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. ``` |
|||
msg403611 - (view) | Author: Raymond Hettinger (rhettinger) * | Date: 2021-10-10 22:17 | |
I'm merging issue 44904 into this one because they are related. |
|||
msg403613 - (view) | Author: Raymond Hettinger (rhettinger) * | Date: 2021-10-10 22:38 | |
It may have been a mistake to allow this kind of decorator chaining. * As Randolf and Alex have noticed, it invalidates assumptions made elsewhere in the standard library and presumably in third-party code as well. * And as Randolf noted in his last post, the current descriptor logic doesn't make it possible to implement classmethod properties with setter logic. * In issue 44973, we found that staticmethod and property also don't compose well, nor does abstract method. We either need to undo the Python 3.9 feature from issue 19072, or we need to put serious thought into making all these descriptors composable in a way that would match people's expectations. |
|||
msg403653 - (view) | Author: Randolf Scholz (randolf.scholz) | Date: 2021-10-11 12:55 | |
Dear Raymond, I think decorator chaining is a very useful concept. At the very least, if all decorators are functional (following the `functools.wraps` recipe), things work out great -- we even get associativity of function composition if things are done properly! 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 `@classmethod` I think one can formulate a straightforward desiderata: ### Expectation - `classmethod(property)` should still be a `property`! - More generally: `classmethod(decorated)` should always be a subclass of `decorated`! 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 `classmethod` that works mostly as expected in conjunction with `property`. The main idea is rewrite the `classmethod` to dynamically be a subclass of whatever it wrapped; roughly: ```python 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 `help` bug, though it is kind of weird because the decorated class-property now correctly shows up under the "Readonly properties" section. Maybe `help` internally checks `isinstance(cls.func, property)` at some point instead of `isinstance(cls.__dict__["func"], property)`? ### Some further Proposals / Ideas 1. Decorators could always have an attribute that points to whatever object they wrapped. For the sake of argument, let's take `__func__`. ⟹ raise Error when typing `@decorator` if `not hasattr(decorated, "__func__")` ⟹ Regular functions/methods should probably by default have `__func__` as a pointer to themselves? ⟹ This could hae several subsidiary benefits, for example, currently, how would you implement a pair of decorators `@print_args_and_kwargs` and `@time_execution` such that both of them only apply to the base function, no matter the order in which they are decorating it? The proposed `__func__` convention would make this very easy, almost trivial. 2. `type.__setattr__` could support checking if `attr` already exists and has `__set__` implemented. ⟹ This would allow true class-properties with `getter`, `setter` and `deleter`. I provide a MWE here: <https://mail.google.com/mail/u/0/#label/Python+Ideas/FMfcgzGlkPRbJVRkHHtkRPhMCxNsFHpl> 3. I think an argument can be made that it would be really, really cool if `@` could become a general purpose function composition operator? ⟹ This is already kind of what it is doing with decorators ⟹ This is already exacltly what it is doing in numpy -- matrix multiplication \*is\* the composition of linear functions. ⟹ In fact this is a frequently requested feature on python-ideas! ⟹ But here is probably the wrong place to discuss this. |
|||
msg403699 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2021-10-11 22:43 | |
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 #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 `classmethod` from a class into a function would break a lot of existing code. As an alternative, one small adjustment that could be made would be to special-case `isinstance()` when it comes to class properties. In pure Python, you could achieve this like this: ``` oldproperty = property class propertymeta(type): def __instancecheck__(cls, obj): return super().__instancecheck__(obj) or ( isinstance(obj, classmethod) and super().__instancecheck__(obj.__func__) ) class property(oldproperty, metaclass=propertymeta): pass ``` This would at least mean that `isinstance(classmethod(property(lambda cls: 42)), property)` and `isinstance(classmethod(property(lambda cls: 42)), classmethod)` would both evaluate to `True`. This would be a bit of a lie (an instance of `classmethod` isn't an instance of `property`), but would at least warn the caller that the object they were dealing with had some propertylike qualities to it. 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 `@staticmethod` cannot be stacked on top of `@property`: it strikes me that this feature isn't really needed. You can achieve the same effect just by stacking `@classmethod` on top of `@property` and not using the `cls` parameter. |
|||
msg403713 - (view) | Author: Randolf Scholz (randolf.scholz) | Date: 2021-10-12 07:32 | |
@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 `@classmethod` functionally do? It makes the first argument of the function receive the class type instead of the object instance and makes it possible to call it directly from the class without instantiating it. I would still expect `MyClass.__dict__["themethod"]` to behave, as an object, much the same, regardless if it was decorated with `@classmethod` or not. Regarding code breakage - yes that is a problem, but code is already broken and imo Python needs to make a big decision going forward: 1. Embrace decorator chaining and try hard to make it work universally (which afaik was never intended originally when decorators were first introduced). As a mathematician I would love this, also adding `@` as a general purpose function composition operator would add quite some useful functional programming aspects to python. 2. Revert changes from 3.9 and generally discourage decorator chaining. 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 (: |
|||
msg405100 - (view) | Author: Andrei Kulakov (andrei.avk) * | Date: 2021-10-27 14:26 | |
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. |
|||
msg405115 - (view) | Author: Andrei Kulakov (andrei.avk) * | Date: 2021-10-27 17:39 | |
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.. |
|||
msg405121 - (view) | Author: Andrei Kulakov (andrei.avk) * | Date: 2021-10-27 20:21 | |
I've looked more into it, the issue is that even before an object can be tested with `isinstance()`, both inspect.classify_class_attrs and in pydoc, classdoc local function `spill()` use a `getattr()` call, which triggers the property. So I think my PR is going in the right direction, adding guards in the inspect function and in two `spill()` functions. If `isinstance()` can be fixed to detect class properties correctly, these guards can be simplified but they still need to be there, I think. |
|||
msg405142 - (view) | Author: wim glenn (wim.glenn) * | Date: 2021-10-28 00:16 | |
added Graham Dumpleton to nosy list in case he has some useful insight here, I think the PR from issue19072 may have been adapted from grahamd's patch originally? |
|||
msg405143 - (view) | Author: Graham Dumpleton (grahamd) | Date: 2021-10-28 00:42 | |
Too much to grok right now. There is already a convention for what a decorator wraps. It is __wrapped__. https://github.com/python/cpython/blob/3405792b024e9c6b70c0d2355c55a23ac84e1e67/Lib/functools.py#L70 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. |
|||
msg406600 - (view) | Author: Raymond Hettinger (rhettinger) * | Date: 2021-11-19 17:31 | |
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) |
|||
msg406605 - (view) | Author: Raymond Hettinger (rhettinger) * | Date: 2021-11-19 18:12 | |
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__. |
|||
msg406606 - (view) | Author: Alex Waygood (AlexWaygood) * | Date: 2021-11-19 18:29 | |
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. |
|||
msg407493 - (view) | Author: Stephen Rosen (sirosen0) | Date: 2021-12-01 21:51 | |
Probably >90% of the use-cases for chaining classmethod are a read-only class property. It's important enough that some tools (e.g. sphinx) even have special-cased support for classmethod(property(...)). Perhaps the general case of classmethod(descriptor(...)) should be treated separately from the common case? > 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. 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))? |
|||
msg407499 - (view) | Author: Raymond Hettinger (rhettinger) * | Date: 2021-12-01 22:19 | |
In Python 3.10, classmethod() added a __wrapped__ attribute. Presumably, any use case for implicit chaining can now be accomplished in an explicit and controlled manner. |
|||
msg413451 - (view) | Author: Raymond Hettinger (rhettinger) * | Date: 2022-02-17 20:03 | |
See also: https://bugs.python.org/issue46764 |
History | |||
---|---|---|---|
Date | User | Action | Args |
2022-04-11 14:59:50 | admin | set | github: 89519 |
2022-02-17 20:03:40 | rhettinger | set | messages: + msg413451 |
2021-12-01 22:19:48 | rhettinger | set | messages: + msg407499 |
2021-12-01 21:51:07 | sirosen0 | set | nosy:
+ sirosen0 messages: + msg407493 |
2021-11-19 18:29:25 | AlexWaygood | set | messages: + msg406606 |
2021-11-19 18:12:27 | rhettinger | set | nosy:
+ lukasz.langa, pablogsal messages: + msg406605 |
2021-11-19 17:31:55 | rhettinger | set | messages: + msg406600 |
2021-10-28 00:42:49 | grahamd | set | messages: + msg405143 |
2021-10-28 00:16:06 | wim.glenn | set | nosy:
+ grahamd messages: + msg405142 |
2021-10-27 20:21:54 | andrei.avk | set | messages: + msg405121 |
2021-10-27 17:39:12 | andrei.avk | set | messages: + msg405115 |
2021-10-27 14:26:00 | andrei.avk | set | messages: + msg405100 |
2021-10-27 14:14:56 | andrei.avk | set | keywords:
+ patch nosy: + andrei.avk pull_requests: + pull_request27502 stage: test needed -> patch review |
2021-10-12 07:32:22 | randolf.scholz | set | messages: + msg403713 |
2021-10-11 22:43:14 | AlexWaygood | set | messages: + msg403699 |
2021-10-11 12:55:14 | randolf.scholz | set | files:
+ ClassPropertyIdea.py messages: + msg403653 |
2021-10-10 22:38:26 | rhettinger | set | assignee: rhettinger messages: + msg403613 nosy: + berker.peksag |
2021-10-10 22:18:34 | rhettinger | link | issue44904 superseder |
2021-10-10 22:17:30 | rhettinger | set | nosy:
+ rhettinger messages: + msg403611 |
2021-10-10 19:08:12 | serhiy.storchaka | set | nosy:
+ serhiy.storchaka |
2021-10-10 10:37:36 | randolf.scholz | set | messages: + msg403582 |
2021-10-10 09:57:52 | randolf.scholz | set | messages: + msg403578 |
2021-10-08 20:53:12 | terry.reedy | set | messages:
+ msg403505 versions: + Python 3.11, - Python 3.9, Python 3.10 |
2021-10-08 20:47:43 | terry.reedy | set | nosy:
+ terry.reedy messages: + msg403504 stage: test needed |
2021-10-04 20:14:40 | wim.glenn | set | nosy:
+ wim.glenn |
2021-10-04 07:26:14 | corona10 | set | nosy:
+ corona10 |
2021-10-03 21:03:34 | AlexWaygood | set | nosy:
+ AlexWaygood messages: + msg403111 |
2021-10-03 20:21:20 | randolf.scholz | set | files:
+ classmethod_property.py messages: + msg403110 |
2021-10-03 19:45:55 | randolf.scholz | create |