| msg397278 - (view) | Author: Glyph Lefkowitz (glyph)  | 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)  | 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) *  | 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) *  | 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) *  | 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)  | 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) *  | 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)  | 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) *  | 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)  | 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) *  | 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) *  | 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) *  | 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) *  | 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 |
|
| Date | User | Action | Args |
| 2022-04-11 14:59:47 | admin | set | github: 88771 |
| 2021-08-06 20:14:15 | rhettinger | set | status: open -> closed stage: patch review -> resolved resolution: fixed versions: + Python 3.9, Python 3.10 |
| 2021-08-06 20:11:51 | rhettinger | set | messages: + msg399138 |
| 2021-08-06 19:58:10 | rhettinger | set | messages: + msg399136 |
| 2021-08-06 19:33:42 | miss-islington | set | pull_requests: + pull_request26135 |
| 2021-08-06 19:33:40 | rhettinger | set | messages: + msg399134 |
| 2021-08-06 19:33:38 | miss-islington | set | nosy: + miss-islington pull_requests: + pull_request26134
|
| 2021-08-06 17:51:16 | lukasz.langa | set | nosy: + lukasz.langa messages: + msg399112
|
| 2021-08-06 14:49:17 | rhettinger | set | keywords: + patch stage: needs patch -> patch review pull_requests: + pull_request26127 |
| 2021-08-06 06:12:35 | glyph | set | messages: + msg399058 |
| 2021-08-06 04:21:08 | rhettinger | set | messages: + msg399056 |
| 2021-08-06 00:06:56 | glyph | set | messages: + msg399053 |
| 2021-08-05 23:29:05 | rhettinger | set | messages: + msg399048 |
| 2021-08-05 18:39:09 | glyph | set | messages: + msg399028 |
| 2021-08-05 18:15:29 | rhettinger | set | messages: + msg399023 |
| 2021-07-25 16:37:03 | rhettinger | set | messages: + msg398187 |
| 2021-07-15 15:22:26 | rhettinger | set | versions: - Python 3.7, Python 3.8, Python 3.9, Python 3.10 |
| 2021-07-15 15:22:09 | rhettinger | set | assignee: rhettinger
nosy: + rhettinger |
| 2021-07-12 19:22:56 | Dennis Sweeney | set | nosy: + Dennis Sweeney messages: + msg397350
|
| 2021-07-12 05:53:59 | glyph | set | messages: + msg397283 versions: + Python 3.10, Python 3.11 |
| 2021-07-12 05:21:04 | glyph | create | |