classification
Title: functools.total_ordering doesn't work with metaclasses
Type: behavior Stage: resolved
Components: Library (Lib) Versions: Python 3.11, Python 3.10, Python 3.9
process
Status: closed Resolution: fixed
Dependencies: Superseder:
Assigned To: rhettinger Nosy List: Dennis Sweeney, glyph, lukasz.langa, miss-islington, rhettinger
Priority: normal Keywords: patch

Created on 2021-07-12 05:21 by glyph, last changed 2021-08-06 20:14 by rhettinger. This issue is now closed.

Pull Requests
URL Status Linked Edit
PR 27633 merged rhettinger, 2021-08-06 14:49
PR 27640 merged miss-islington, 2021-08-06 19:33
PR 27641 merged miss-islington, 2021-08-06 19:33
Messages (14)
msg397278 - (view) Author: Glyph Lefkowitz (glyph) (Python triager) Date: 2021-07-12 05:21
Consider the following example that attempts to make a series of types sortable by their class name:




from functools import total_ordering

@total_ordering
class SortableMeta(type):
    def __new__(cls, name, bases, ns):
        return super().__new__(cls, name, bases, ns)

    def __lt__(self, other):
        if not isinstance(other, SortableMeta):
            pass
        return self.__name__ < other.__name__

    def __eq__(self, other):
        if not isinstance(other, SortableMeta):
            pass
        return self.__name__ == other.__name__

class B(metaclass=SortableMeta):
    pass

class A(metaclass=SortableMeta):
    pass


print(A < B)
print(A > B)


This should just print "True", "False", but instead the second comparison raises this exception:

Traceback (most recent call last):
  File "total_ordering_metaclass.py", line 27, in <module>
    print(A > B)
  File "lib/python3.9/functools.py", line 91, in _gt_from_lt
    op_result = self.__lt__(other)
TypeError: expected 1 argument, got 0


The problem here is that functools considers .__special__ to be equivalent to operator.special.  I'm pretty sure this should be invoking "self < other" rather than self.__lt__(other).
msg397283 - (view) Author: Glyph Lefkowitz (glyph) (Python triager) Date: 2021-07-12 05:53
Adding versions after confirming the bug is the same on 3.10
msg397350 - (view) Author: Dennis Sweeney (Dennis Sweeney) * (Python triager) Date: 2021-07-12 19:22
The one twist is that if type(b) is a strict subtype of type(a), then "a < b" first calls type(b).__gt__(b, a), then falls back to type(a).__lt__(a, b). Example:

>>> class Int(int):
...     def __gt__(self, other):
...         print("Here!")
...         return int(self) > int(other)
... 
...     
>>> 5 < Int(6)
Here!
True

see https://github.com/python/cpython/blob/da2e673c53974641a0e13941950e7976bbda64d5/Objects/object.c#L683

So I think replacing "a.__lt__(b)" with "a < b" would be an unnecessary change in behavior, since "a < b" still has to decide which method to use.

I think instead "a.__lt__(b)" could be replaced with "type(a).__lt__(a, b)"
msg398187 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-07-25 16:37
To add support for metaclasses, replacing 'self.__lt__(other)' with 'type(self).__lt__(self, other)' will work. 

The performance degradation for the common case is about 25%:

$ py -m timeit -s 'from tmp import a, b' 'a >= b'     # op_result = type(self).__lt__(self, other)
1000000 loops, best of 5: 283 nsec per loop

$ py -m timeit -s 'from tmp import a, b' 'a >= b'     # op_result = self.__lt__(other)
1000000 loops, best of 5: 227 nsec per loop
msg399023 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-08-05 18:15
Do you all have preference for 1) expanding the range of use cases to cover metaclasses but incurring a performance hit for common cases, or 2) leaving it as-is and documenting that it doesn't work for metaclasses?
msg399028 - (view) Author: Glyph Lefkowitz (glyph) (Python triager) Date: 2021-08-05 18:39
> Do you all have preference for 1) expanding the range of use cases to cover metaclasses but incurring a performance hit for common cases, or 2) leaving it as-is and documenting that it doesn't work for metaclasses?

If we do need the slow approach for the meta-type case, (i.e.: type(self).__lt__(other)) then why not decide on implementation at decoration time? The decorator has enough information to know if this strategy is necessary and can put different special methods in place.

(But also, wouldn't 'from operator import lt ... lt(self, other)' be a bit faster, and also more general, just because it's doing specialized dispatch in C rather than an additional Python function call / method dispatch?  I have not benchmarked myself, so please ignore if you've already tested this.)
msg399048 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-08-05 23:29
[Glyph]
> why not decide on implementation at decoration time?

We could do that and not incur performance issues.  However, it would expand the API and double the size of the code.  

We've had no other reports about this issue since total_ordering() was added in Python 2.7, nor have there been any questions about it on StackOverflow.  Do you think anyone other than yourself would use this feature?

It seems like a rare niche use case, and the workaround is simple.

For production code, I usually recommend writing out the other three methods (that is less magical, runs faster, easier to test, has fewer  dependencies, and makes the tracebacks more readable.

> (But also, wouldn't 'from operator import lt ... lt(self, other)'
> be a bit faster, and also more general, 

The lt() function emulates the '<' operator, so we still have the problems with reflection that we were trying to avoid by calling the __lt__ method directly.
msg399053 - (view) Author: Glyph Lefkowitz (glyph) (Python triager) Date: 2021-08-06 00:06
> We could do that and not incur performance issues.  However, it would expand the API and double the size of the code.  

If the preference is to not support this use-case, then I'd rather the decorator simply raise an exception and tell me this is going to be a problem so I can eagerly implement the workaround.

> It seems like a rare niche use case, and the workaround is simple.

It does seem like there are various times that one might want to make classes orderable, for example if they are bound to IDL objects or database tables; the use-case where I ran into this was something like that.

> For production code, I usually recommend writing out the other three methods (that is less magical, runs faster, easier to test, has fewer  dependencies, and makes the tracebacks more readable.

Wait, is the suggestion here that @total_ordering is not suitable for production?
msg399056 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-08-06 04:21
> If the preference is to not support this use-case ,,,

I don't really have a preference.  Was just letting you know the pros and cons.  I'll put together a PR with the "type(self).__lt__(self, other)" substitutions.  Let me know if that is the outcome you want.

> Wait, is the suggestion here that @total_ordering
> is not suitable for production?

It works fine, but as noted in the docs, the indirect code isn't as good as just writing-out the methods by hand.
msg399058 - (view) Author: Glyph Lefkowitz (glyph) (Python triager) Date: 2021-08-06 06:12
My preference would be to fix it one way or another; the stdlib should be correct before it's performant.  If, as you say, it's better to write the methods longhand anyway for ease of debugging, then it makes sense to write them out for performance, too.
msg399112 - (view) Author: Ɓukasz Langa (lukasz.langa) * (Python committer) Date: 2021-08-06 17:51
IMO the PR should be accepted and have approved it accordingly.

@total_ordering, as mentioned by Raymond, wasn't ever the most efficient way to implement ordering on a class. It is the most readable and easy way to get a correct implementation though so it's preferred where performance isn't the highest priority.
msg399134 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-08-06 19:33
New changeset 1f7d64608b5c7f4c3d96b01b33e18ebf9dec8490 by Raymond Hettinger in branch 'main':
bpo-44605: Teach @total_ordering() to work with metaclasses (GH-27633)
https://github.com/python/cpython/commit/1f7d64608b5c7f4c3d96b01b33e18ebf9dec8490
msg399136 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-08-06 19:58
New changeset fde84170d06f74afd6f95d5b18cf3f733018191a by Miss Islington (bot) in branch '3.9':
bpo-44605: Teach @total_ordering() to work with metaclasses (GH-27633) (GH-27641)
https://github.com/python/cpython/commit/fde84170d06f74afd6f95d5b18cf3f733018191a
msg399138 - (view) Author: Raymond Hettinger (rhettinger) * (Python committer) Date: 2021-08-06 20:11
New changeset 66dd1a0e645f26b074547dccc92448169cb32410 by Miss Islington (bot) in branch '3.10':
bpo-44605: Teach @total_ordering() to work with metaclasses (GH-27633) (GH-27640)
https://github.com/python/cpython/commit/66dd1a0e645f26b074547dccc92448169cb32410
History
Date User Action Args
2021-08-06 20:14:15rhettingersetstatus: open -> closed
stage: patch review -> resolved
resolution: fixed
versions: + Python 3.9, Python 3.10
2021-08-06 20:11:51rhettingersetmessages: + msg399138
2021-08-06 19:58:10rhettingersetmessages: + msg399136
2021-08-06 19:33:42miss-islingtonsetpull_requests: + pull_request26135
2021-08-06 19:33:40rhettingersetmessages: + msg399134
2021-08-06 19:33:38miss-islingtonsetnosy: + miss-islington
pull_requests: + pull_request26134
2021-08-06 17:51:16lukasz.langasetnosy: + lukasz.langa
messages: + msg399112
2021-08-06 14:49:17rhettingersetkeywords: + patch
stage: needs patch -> patch review
pull_requests: + pull_request26127
2021-08-06 06:12:35glyphsetmessages: + msg399058
2021-08-06 04:21:08rhettingersetmessages: + msg399056
2021-08-06 00:06:56glyphsetmessages: + msg399053
2021-08-05 23:29:05rhettingersetmessages: + msg399048
2021-08-05 18:39:09glyphsetmessages: + msg399028
2021-08-05 18:15:29rhettingersetmessages: + msg399023
2021-07-25 16:37:03rhettingersetmessages: + msg398187
2021-07-15 15:22:26rhettingersetversions: - Python 3.7, Python 3.8, Python 3.9, Python 3.10
2021-07-15 15:22:09rhettingersetassignee: rhettinger

nosy: + rhettinger
2021-07-12 19:22:56Dennis Sweeneysetnosy: + Dennis Sweeney
messages: + msg397350
2021-07-12 05:53:59glyphsetmessages: + msg397283
versions: + Python 3.10, Python 3.11
2021-07-12 05:21:04glyphcreate