-
-
Notifications
You must be signed in to change notification settings - Fork 30.6k
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
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 |
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 |
@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 = 2 Then 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 instance 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. |
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))? |
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. |
…aining Summary: upstream issue [GH-89519](python/cpython#89519) is about removing classmethod descriptor chaining capability, which was added in 3.9, deprecated in 3.11, and pending removal in 3.13. we have code that breaks due to the "new" (to us, when upgrading from 3.8 ) descriptor chaining behavior, so instead of figuring out how to fix it, considering this is deprecated and being removed in upstream, it seems preferable to "accelerate" that removal internally by backporting the deprecation and removal in cinder 3.10. the stacked two diffs backport the deprecation (D50534764) and removal (D50534886). this diff deletes a couple of tests in cinder that rely on that behavior. Reviewed By: swtaarrs Differential Revision: D50578116 fbshipit-source-id: 2faed200a3a130ff7be7d8c6a231c6431789b330
Summary: backporting upstream commit `ebaf0945f9f` (merged upstream [PR GH-92379](python/cpython#92379)), part of upstream issue [GH-89519](python/cpython#89519) Reviewed By: swtaarrs Differential Revision: D50534764 fbshipit-source-id: e74c8910e56f117b3c1cab3442de7394b0bd8b22
…11 (#110163) Summary: backporting [Raymond Hettinger's `remove_classmethod_descriptor_chaining` branch](https://github.com/rhettinger/cpython/tree/remove_classmethod_descriptor_chaining) (pending upstream PR [GH-110163](python/cpython#110163)), part of upstream issue [GH-89519](python/cpython#89519) Reviewed By: swtaarrs Differential Revision: D50534886 fbshipit-source-id: 05b29193ce220f0d91fd0df8d5cef34d7e923456
…escriptor chaining was introduced.
…ted since 3.11) Summary: backport upstream PRs python/cpython#110163 and python/cpython#113233 upstream issues: python/cpython#89519 (Calling help executes classmethod property decorated methods) and python/cpython#113157 (Changed behavior of <instancemethod>.__get__ in Python 3.11) upstream commits: [`7f9a99e8549b792662f2cd28bf38a4d4625bd402`](python/cpython@7f9a99e) and [`d058eaeed44766a8291013b275ad22f153935d3b`](python/cpython@d058eae) Reviewed By: aleivag Differential Revision: D52014322 fbshipit-source-id: 87de6d9587bd9cc49f053ca340adfc469b041f91
Restore behaviors before classmethod descriptor chaining was introduced.
Restore behaviors before classmethod descriptor chaining was introduced.
Restore behaviors before classmethod descriptor chaining was introduced.
In the next commit we're gonna add a `to_resource` method. As we don't want to have to pass a resource into `to_resource`, the class itself needs to expose what resource class should be built. Thus a type annotation is no longer enough. To solve this we've added a class method to BaseNode which returns the associated resource class. The method on BaseNode will raise a NotImplementedError unless the the inheriting class has overridden the `resouce_class` method to return the a resource class. You may be thinking "Why not a class property"? And that is absolutely a valid question. We used to be able to chain `@classmethod` with `@property` to create a class property. However, this was deprecated in python 3.11 and removed in 3.13 (details on why this happened can be found [here](python/cpython#89519)). There is an [alternate way to setup a class property](python/cpython#89519 (comment)), however this seems a bit convoluted if a class method easily gets the job done. The draw back is that we must do `.resource_class()` instead of `.resource_class` and on classes implementing `BaseNode` we have to override it with a method instead of a property specification. Additionally, making it a class _instance_ property won't work because we don't want to require an _instance_ of the class to get the `resource_class` as we might not have an instance at our dispossal.
* Move `ColumnInfo` to dbt/artifacts * Move `Quoting` resource to dbt/artifacts * Move `TimePeriod` to `types.py` in dbt/artifacts * Move `Time` class to `components` We need to move the data parts of the `Time` definition to dbt/artifacts. That is not what we're doing in this commit. In this commit we're simply moving the functional `Time` definition upstream of `unparsed` and `nodes`. This does two things - Mirrors the import path that the resource `time` definition will have in dbt/artifacts - Reduces the chance of ciricular import problems between `unparsed` and `nodes` * Move data part of `Time` definition to dbt/artifacts * Move `FreshnessThreshold` class to components module We need to move the data parts of the `FreshnessThreshold` definition to dbt/artifacts. That is not what we're doing in this commit. In this commit we're simply moving the functional `FreshnessThreshold` definition upstream of `unparsed` and `nodes`. This does two things - Mirrors the import path that the resource `FreshnessThreshold` definition will have in dbt/artifacts - Reduces the chance of ciricular import problems between `unparsed` and `nodes` * Move data part of `FreshnessThreshold` to dbt/artifacts Note: We had to override some of the attrs of the `FreshnessThreshold` resource because the resource version only has access to the resource version of `Time`. The overrides in the functional definition of `FreshnessThreshold` make it so the attrs use the functional version of `Time`. * Move `ExternalTable` and `ExternalPartition` to `source_definition` module in dbt/artifacts * Move `SourceConfig` to `source_definition` module in dbt/artifacts * Move `HasRelationMetadata` to core `components` module This is a precursor to splitting `HasRelationMetadata` into it's data and functional parts. * Move data portion of `HasRelationMetadata` to dbt/artifacts * Move `SourceDefinitionMandatory` to dbt/artifacts * Move the data parts of `SourceDefinition` to dbt/artifacts Something interesting here is that we had to override the `freshness` property. We had to do this because if we didn't we wouldn't get the functional parts of `FreshnessThreshold`, we'd only get the data parts. Also of note, the `SourceDefintion` has a lot of `@property` methods that on other classes would be actual attribute properties of the node. There is an argument to be made that these should be moved as well, but thats perhaps a separate discussion. Finally, we have not (yet) moved `NodeInfoMixin`. It is an open discussion whether we do or not. It seems primarily functional, as a means to update the source freshness information. As the artifacts primarily deal with the shape of the data, not how it should be set, it seems for now that `NodeInfoMixin` should stay in core / not move to artifacts. This thinking may change though. * Refactor `from_resource` to no longer use generics In the next commit we're gonna add a `to_resource` method. As we don't want to have to pass a resource into `to_resource`, the class itself needs to expose what resource class should be built. Thus a type annotation is no longer enough. To solve this we've added a class method to BaseNode which returns the associated resource class. The method on BaseNode will raise a NotImplementedError unless the the inheriting class has overridden the `resouce_class` method to return the a resource class. You may be thinking "Why not a class property"? And that is absolutely a valid question. We used to be able to chain `@classmethod` with `@property` to create a class property. However, this was deprecated in python 3.11 and removed in 3.13 (details on why this happened can be found [here](python/cpython#89519)). There is an [alternate way to setup a class property](python/cpython#89519 (comment)), however this seems a bit convoluted if a class method easily gets the job done. The draw back is that we must do `.resource_class()` instead of `.resource_class` and on classes implementing `BaseNode` we have to override it with a method instead of a property specification. Additionally, making it a class _instance_ property won't work because we don't want to require an _instance_ of the class to get the `resource_class` as we might not have an instance at our dispossal. * Add `to_resource` method to `BaseNode` Nodes have extra attributes. We don't want these extra attributes to get serialized. Thus we're converting back to resources prior to serialization. There could be a CPU hit here as we're now dictifying and undictifying right before serialization. We can do some complicated and non-straight-forward things to get around this. However, we want to see how big of a perforance hit we actually have before going that route. * Drop `__post_serialize__` from `SourceDefinition` node class The method `__post_serialize__` on the `SourceDefinition` was used for ensuring the property `_event_status` didn't make it to the serialized version of the node. Now that resource definition of `SourceDefinition` handles serialization/deserialization, we can drop `__post_serialize__` as it is no longer needed. * Merge functional parts of `components` into their resource counter parts We discussed this on the PR. It seems like a minimal lift, and minimal to support. Doing so also has the benefit of reducing a bunch of the overriding we were previously doing. * Fixup:: Rename variable `name` to `node_id` in `_map_nodes_to_map_resources` Naming is hard. That is all. * Fixup: Ensure conversion of groups to resources for `WritableManifest`
Hi @SimpleArt , The Argument of type "type[T@classproperty] | type[object]* | None; cannot be assigned to parameter of type ";type[T@classproperty]"; for this slightly modified version of the def __get__(self, instance: Optional[T], owner: Type[T] | None = None) -> RT:
if owner is None:
if instance is not None:
owner = type(instance)
else:
owner = None
return self.__wrapped__(owner) # pyright triggers warning also this assignment self.__module__ = func.__module__ gives Cannot find reference 'module' in '(T) -> RT' Moreover, could you provide a simple example of using abstract class properties for Python>=3.13 ? |
Restore behaviors before classmethod descriptor chaining was introduced.
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: