attrs
attrs copied to clipboard
python-attrs
`__getattr__` in child class gets called when mixed with `cached_property`
from functools import cached_property
from attrs import define
@define
class Bob:
@cached_property
def howdy(self):
return 3
class Sup(Bob):
def __getattr__(self, name):
raise AttributeError(name)
b = Sup()
# reaches __getattr__ where I raise AttributeError
b.howdy
This previously did not error. I would expect __getattr__ to not be called since the attribute exists.
I believe his is fixed in #1253. I wanted to push a release before PyCon/travel but sadly #1267 blocked me. :(
I just checked against latest master and it still exists. It looks like 23.2.0 is when the error started, 23.1.0 works.
The fix from #1253 does not cover this inheritance case, indeed. I'll see if this can be fixed.
#1289 fixes a slightly different version of the original problem, namely when the child class is also a (slotted) attr class, thus from the original example:
@define
class Sup(Bob):
...
I'm not sure if the original problem may be resolved. Or at least, not in the same manner as we have no mean to inspect the plain subclass.
https://github.com/python-attrs/attrs/pull/1291 is another attempt, which resolves both situations mentioned above.
Just noticed this issue, and thought I could give my insight for what it's worth.
As noted the problem listed is unfixable with the approach, because __getattr__ on the attrs class is never getting called. This is to some extent an inherent problem with __getattr__ from multiple sources, and there's a fairly reasonable argument that __getattr__ in the derived class should be calling super().__getattr__(name), as a general practice, not just in this case, because you don't necessarily know how a class will be further sub-classed.
from functools import cached_property
import attrs
@attrs.define
class Bob:
@cached_property
def howdy(self):
return 3
class Sup(Bob):
def __getattr__(self, name):
try:
return super().__getattr__(name)
except AttributeError:
raise AttributeError(name)
b = Sup()
print(b.howdy)
Runs with no errors (and prints 3).
That said, obviously it's surprising to encounter this error, and it would be better if it could be avoided.
IMHO, the __getattribute__ proposal doesn't really fix this issue, it just shifts it onto __getattribute__ instead of __getattr__. (Though it probably gets used less often than __getattr__, so it mitigates it somewhat).
It also comes at the significant (IMHO) cost of adding an overhead to every attribute access. That overhead might be relatively small, but given that both slots and cached_property are often used in performance-sensitive use-cases, it may be intolerable for some users.
I think there's an option of adding a descriptor to the class for cached properties that wraps the slots descriptor, something like:
try:
super().__get__(name)
except AttributeError:
result = self._compute_value(name)
super().__set__(name, result)
return result
I also rejected this approach in favour of the __getattr__ approach, because of the performance overhead (for repeated access), but it is at least only on the cached_properties (rather than every attribute).
I do think the performance characteristics are important to consider here, because adding an overhead to each attribute access arguably breaks the performance contract that cached_property promises.
Oof, so this is all a lot more complicated than hoped.
I kinda agree @diabolo-dan here I think – if you're gonna inherit and overwrite __getattr__, it's your responsibility to play fair with the class you're subclassing, or am I missing something here?
That said, I suspect the descriptor-based approach's performance impact could be harmless enough to justify it (one usually doesn't use cached properties to save a local attribute lookup). However, unless I'm missing something, that would be a bit of magic we'd be adding, that could be confusing to our users's mental model (attrs tries to stay primarily out of the way). I fear we're on a trajectory of a whack-a-mole and increasingly adding more and more kludges.
I've just added CodSpeed to attrs to make decisions like this easier, but I didn't get around writing really meaningful tests yet (I've mostly squatted at their EuroPython booth and spend most of the time getting it to work).
@dlax do you habe thoughts? ISTM this is more of a documentation issue? Does the super approach from @diabolo-dan work for you?
I missed the impact of using __getattribute__() over __getattr__() on performance and I agree it's important to consider and perhaps makes the change I suggested unacceptable.
Having hacked significantly on this feature[^1], I also don't really feel comfortable with the implementation because it's fairly complex (sharing your "whack-a-mole" fear somehow). So I'm totally fine moving the resolution to documentation.
[^1]: I originally picked an issue with no particular personal interest, just as a way to get involved in a project I appreciate.
Thank you @dlax it’s appreciated!
I guess I should’ve asked @dalejung about the super/documentation issue? Long ticket history is long. 😅