Opened 7 years ago
Closed 6 years ago
#20686 closed enhancement (fixed)
Refactor getattr_from_other_class() for lookup of methods in categories
Reported by:  Nicolas M. Thiéry  Owned by:  

Priority:  major  Milestone:  sage7.4 
Component:  categories  Keywords:  
Cc:  Jeroen Demeyer, Travis Scrimshaw  Merged in:  
Authors:  Jeroen Demeyer  Reviewers:  Vincent Delecroix 
Report Upstream:  Fixed upstream, in a later stable release.  Work issues:  
Branch:  74041f3 (Commits, GitHub, GitLab)  Commit:  74041f30d8de4c15055345f14340a4f443236ffa 
Dependencies:  #21030, #21409  Stopgaps: 
Description (last modified by )
For parents and elements implemented in Cython, category lookup is emulated in __getattr__
using getattr_from_other_class
. In this ticket, this is refactored a bit:
 We should not pick up special attributes from
type
, there is no point in returning the type name of the category here (similarly for__dict__
,__mro__
, ...):sage: ZZ(1).__name__ 'JoinCategory.element_class' sage: ZZ.__name__ 'JoinCategory.parent_class'
This change causes a few failures which need to be fixed.
 The implementation of
getattr_from_other_class
is not very robust. For example, static methods are not supported. We fix this by using lowlevel Python functions to get the attribute and we manually call the descriptor__get__
if needed.
 We shouldn't do anything special with doubleunderscore private attributes: in plain Python, this is implemented by the parser and not by
getattr()
. So__getattr__
would already receive the mangled private name.
 Some of the changes broke
_sage_src_lines_
, so we also change that: currently,_sage_src_lines_()
is used both to get the source lines of a class (e.g. dynamic classes) and an instance (e.g. cached functions). We change this to always mean the source lines of an instance, which makes things clearer.
 The lookup using
getattr_from_other_class
is about 9 times slower than a normal method lookup::sage: def foo(self): return sage: Sets().element_class.foo = foo sage: def g(x): ....: for i in range(1000): ....: x.foo() sage: x = Semigroups().example().an_element() sage: y = 1 sage: %timeit g(x) 10000 loops, best of 3: 115 µs per loop sage: %timeit g(y) 1000 loops, best of 3: 1.18 ms per loop
We improve on this by roughly a factor 2 (but even then, it's still a lot slower than usual method lookup).
NOTE: This needs a trivial Cython patch, see #21030 for the Cython upgrade.
Change History (81)
comment:1 Changed 7 years ago by
Cc:  Jeroen Demeyer added 

comment:2 Changed 7 years ago by
comment:4 Changed 6 years ago by
Authors:  → Jeroen Demeyer 

Description:  modified (diff) 
comment:5 Changed 6 years ago by
Branch:  → u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes 

comment:6 Changed 6 years ago by
Commit:  → bbd0b05c11b0431f02b34c1b55bed6b775de7fa8 

Dependencies:  → #20825 
comment:7 Changed 6 years ago by
Commit:  bbd0b05c11b0431f02b34c1b55bed6b775de7fa8 → 3cd449f14dc1b8c07d326ae1192067f4fdf7e644 

comment:8 Changed 6 years ago by
Description:  modified (diff) 

Report Upstream:  N/A → Reported upstream. No feedback yet. 
comment:9 Changed 6 years ago by
Description:  modified (diff) 

comment:10 Changed 6 years ago by
Report Upstream:  Reported upstream. No feedback yet. → Fixed upstream, but not in a stable release. 

comment:11 Changed 6 years ago by
Commit:  3cd449f14dc1b8c07d326ae1192067f4fdf7e644 → e906b7e6201c92438e41055b08a2ac085f9aa6b7 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
e906b7e  Improve getattr_from_other_class

comment:12 Changed 6 years ago by
Description:  modified (diff) 

comment:13 Changed 6 years ago by
Commit:  e906b7e6201c92438e41055b08a2ac085f9aa6b7 → 92f4520d01640379590d6c5974622f3d6f396e9b 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
92f4520  Improve getattr_from_other_class

comment:14 Changed 6 years ago by
Description:  modified (diff) 

comment:15 Changed 6 years ago by
Description:  modified (diff) 

comment:16 Changed 6 years ago by
Commit:  92f4520d01640379590d6c5974622f3d6f396e9b → 9f529b0c4c8fd3523ebb557c9821e39799c41476 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
9f529b0  Improve getattr_from_other_class

comment:17 Changed 6 years ago by
Commit:  9f529b0c4c8fd3523ebb557c9821e39799c41476 → ff3a496289bbb6611d0feaf583d031522d69c90f 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
ff3a496  Improve getattr_from_other_class

comment:18 Changed 6 years ago by
Status:  new → needs_review 

comment:19 Changed 6 years ago by
Description:  modified (diff) 

Summary:  Optimize method lookup from the categories for instances of Cython classes → Refactor getattr_from_other_class() for lookup of methods in categories 
comment:20 Changed 6 years ago by
Commit:  ff3a496289bbb6611d0feaf583d031522d69c90f → d49b67a34d416f17285c25d576c6444ad977f7b0 

Branch pushed to git repo; I updated commit sha1. New commits:
d49b67a  Test that static methods work

comment:21 Changed 6 years ago by
Cc:  Travis Scrimshaw added 

comment:22 Changed 6 years ago by
Status:  needs_review → needs_work 

comment:23 Changed 6 years ago by
Commit:  d49b67a34d416f17285c25d576c6444ad977f7b0 → 30594cce7e9bdb3e8a8ff5bfb601e93a52da175b 

comment:24 Changed 6 years ago by
Commit:  30594cce7e9bdb3e8a8ff5bfb601e93a52da175b → 851fa16587c1d74797b6ce66e101915e5ebbe339 

Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
851fa16  Improve _sage_src_lines_()

comment:25 Changed 6 years ago by
Commit:  851fa16587c1d74797b6ce66e101915e5ebbe339 → f5361d972a61dcb60a76425264baa9e6d134e54b 

comment:26 Changed 6 years ago by
Keywords:  test added 

comment:27 Changed 6 years ago by
Keywords:  test removed 

comment:28 Changed 6 years ago by
Description:  modified (diff) 

comment:29 Changed 6 years ago by
Description:  modified (diff) 

comment:30 Changed 6 years ago by
Description:  modified (diff) 

comment:31 Changed 6 years ago by
Commit:  f5361d972a61dcb60a76425264baa9e6d134e54b → 8a4f48d047a06ef9fcb2a0e01b5198f7ef439f9b 

comment:32 Changed 6 years ago by
Branch:  u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes → u/nthiery/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes 

comment:33 Changed 6 years ago by
Branch:  u/nthiery/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes → u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes 

comment:34 Changed 6 years ago by
Commit:  8a4f48d047a06ef9fcb2a0e01b5198f7ef439f9b → 7140ee389ac63921f356d5abc4cdc94a6bf48580 

Description:  modified (diff) 
comment:35 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:37 Changed 6 years ago by
Description:  modified (diff) 

comment:38 Changed 6 years ago by
Dependencies:  #20825 → #21030 

Description:  modified (diff) 
Report Upstream:  Fixed upstream, but not in a stable release. → Fixed upstream, in a later stable release. 
Status:  needs_review → needs_work 
comment:39 Changed 6 years ago by
Commit:  7140ee389ac63921f356d5abc4cdc94a6bf48580 → fc6487cb02514a811cb1f7ccfd92bed177bb2dea 

comment:40 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:41 Changed 6 years ago by
Commit:  fc6487cb02514a811cb1f7ccfd92bed177bb2dea → 16d474363c051b081383dcdbc1944828e735c41e 

Branch pushed to git repo; I updated commit sha1. New commits:
16d4743  EvaluationMethods should be a newstyle class

comment:42 Changed 6 years ago by
Commit:  16d474363c051b081383dcdbc1944828e735c41e → 54ccd828cb20b1254aeaf55bf7edab4a73f1032a 

Branch pushed to git repo; I updated commit sha1. New commits:
54ccd82  Remove __dict__ attribute from ElementWrapper

comment:43 followup: 45 Changed 6 years ago by
why is this test removed
 We test that "private" attributes are not requested from the element class
 of the category (:trac:`10467`)::

 sage: C = EuclideanDomains()
 sage: P.<x> = QQ[]
 sage: C.element_class.__foo = 'bar'
 sage: x.parent() in C
 True
 sage: x.__foo
 Traceback (most recent call last):
 ...
 AttributeError: 'sage.rings.polynomial.polynomial_rational_flint.Polynomial_rational_flint' object has no attribute '__foo'
comment:44 Changed 6 years ago by
similarly
 We test that "private" attributes are not requested from the element class
 of the category (:trac:`10467`)::

 sage: P = Parent(QQ, category=CommutativeRings())
 sage: P.category().parent_class.__foo = 'bar'
 sage: P.__foo
 Traceback (most recent call last):
 ...
 AttributeError: 'sage.structure.parent.Parent' object has no attribute '__foo'
comment:45 Changed 6 years ago by
Replying to vdelecroix:
why is this test removed
Because #10467 was a mistake, see point 3 in the ticket description.
comment:46 Changed 6 years ago by
More precisely: see https://trac.sagemath.org/ticket/10467#comment:18
comment:47 Changed 6 years ago by
Aargh....... this whole "category lookup" mechanism is causing me so much frustration with #20767. It is a very fragile hack which just happens to work correctly in the current setting but when you change anything in the coercion model it breaks.
I am starting to think we should reconsider the whole category approach.
comment:48 followup: 49 Changed 6 years ago by
In case this could be useful, we could have a live chat about this tomorrow between 11pm and 2pm, or latter in the week.
Cheers,
comment:49 Changed 6 years ago by
Replying to nthiery:
In case this could be useful, we could have a live chat about this tomorrow between 11pm and 2pm, or latter in the week.
You meant 11am2pm? Given that Santiago is 6h from Paris, it will be hard for me before 2pm (=8am in Santiago).
comment:51 Changed 6 years ago by
Anyway, I am also hitting Cython bugs/oddities, so there are other battles to fight.
comment:52 Changed 6 years ago by
In #20767, I "solved" the category problem (see 47) in the following way:
 doubleunderscore methods (e.g.
__add__
) are never taken from the category.
 anything deriving directly from
Element
fully supports singleunderscore methods (e.g._add_
) from the category.
 anything deriving from
ModuleElement
,RingElement
,... has only partial support for singleunderscore methods from the category.
 nothing changes for nonarithmetic methods (e.g.
base_ring
).
The above rules do not break any existing usage of categories. I hope you can live with this...
comment:53 followup: 54 Changed 6 years ago by
The following example of getattr_from_other_class
line 249253 in sage/structure/misc.pyx
seems broken
sage: n = 1 sage: ip = get_ipython() sage: ip.magic_psearch('n.N') Traceback (most recent call last): ... AttributeError: 'SageTerminalInteractiveShell' object has no attribute 'magic_psearch'
comment:54 Changed 6 years ago by
Replying to vdelecroix:
The following example of
getattr_from_other_class
line 249253 insage/structure/misc.pyx
seems broken
True, but surely not because of this ticket. Most likely, it has to do with the IPython 5 upgrade.
comment:55 followup: 58 Changed 6 years ago by
Is that what we want
if self._category is None: # Usually, this will just raise AttributeError in # getattr_from_other_class(). cls = type else: cls = self._category.parent_class
In other words, are there use case where the category is not initialized but we still might need to go through __getattr__
?
comment:56 followup: 57 Changed 6 years ago by
Why in Element
there is a two stage __getattr__ > getattr_from_category > getattr_from_other_class
whereas in Parent
there is no intermediate getattr_from_category
?
comment:57 followup: 59 Changed 6 years ago by
Replying to vdelecroix:
Why in
Element
there is a two stage__getattr__ > getattr_from_category > getattr_from_other_class
whereas inParent
there is no intermediategetattr_from_category
?
Mainly because I care more about Element
than Parent
(with #20767 in mind). The idea of getattr_from_category
was to allow direct access to the category methods without normal methods. In the end, I am not using this in #20767 but I kept it because it looked potentially useful. And, since this a cdef
method, there is essentially no performance penalty.
comment:58 Changed 6 years ago by
Replying to vdelecroix:
In other words, are there use case where the category is not initialized but we still might need to go through
__getattr__
?
Probably not, but the old code also supported that (directly raising an AttributeError
). But I just let getattr_from_other_class
raise the error.
comment:59 followup: 60 Changed 6 years ago by
Replying to jdemeyer:
Replying to vdelecroix:
Why in
Element
there is a two stage__getattr__ > getattr_from_category > getattr_from_other_class
whereas inParent
there is no intermediategetattr_from_category
?Mainly because I care more about
Element
thanParent
(with #20767 in mind). The idea ofgetattr_from_category
was to allow direct access to the category methods without normal methods. In the end, I am not using this in #20767 but I kept it because it looked potentially useful. And, since this acdef
method, there is essentially no performance penalty.
It just looks weird to have twice the same thing implemented differently. I am just complaining about readability.
comment:60 Changed 6 years ago by
Replying to vdelecroix:
It just looks weird to have twice the same thing implemented differently. I am just complaining about readability.
OK, I will make the requested changes. I will also move the category lookup stuff from Parent
to CategoryObject
where it belongs.
comment:61 Changed 6 years ago by
Milestone:  sage7.3 → sage7.4 

Status:  needs_review → needs_work 
comment:62 Changed 6 years ago by
Commit:  54ccd828cb20b1254aeaf55bf7edab4a73f1032a → a501048a8480fd66300c37ff614969469509d189 

Branch pushed to git repo; I updated commit sha1. New commits:
a501048  Move category attribute lookup to CategoryObject.getattr_from_category()

comment:63 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:64 followup: 67 Changed 6 years ago by
Do you know whether it is worth it to keep this __cached_method
dictionary? I would be happy to see timings here. (though this is rather disjoint from the main preoccupation of this ticket)
comment:65 Changed 6 years ago by
In CategoryObject.__getattr__
it would better to do
try: return self.__cached_methods[name] except KeyError: if self._category is None: cls = type else: cls = self._category.parent_class attr = getattr_from_other_class(self, cls, name) self.__cached_methods[name] = attr return attr
comment:66 Changed 6 years ago by
Reviewers:  → Vincent Delecroix 

comment:67 Changed 6 years ago by
Reviewers:  Vincent Delecroix 

Replying to vdelecroix:
Do you know whether it is worth it to keep this
__cached_method
dictionary?
I would guess so since a lookup in a dictionary should be faster than whatever getattr_from_other_class
is doing.
comment:68 Changed 6 years ago by
Reviewers:  → Vincent Delecroix 

Status:  needs_review → needs_work 
comment:69 Changed 6 years ago by
Commit:  a501048a8480fd66300c37ff614969469509d189 → 0fabb7a5dc50518946d1913488bf4a0444730c0f 

Branch pushed to git repo; I updated commit sha1. New commits:
0fabb7a  Optimize CategoryObject.getattr_from_category()

comment:70 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:71 Changed 6 years ago by
Status:  needs_review → needs_work 

Got two failures with ptestlong
File "src/sage/combinat/root_system/integrable_representations.py", line 189, in sage.combinat.root_system.integrable_representations.IntegrableRepresentation.__init__ Failed example: TestSuite(V).run() Expected nothing Got: Failure in _test_additive_associativity: Traceback (most recent call last): ... AttributeError: 'IntegrableRepresentation' object has no attribute '_an_element_' ...
and similarly
********************************************************************** File "src/sage/homology/hochschild_complex.py", line 98, in sage.homology.hochschild_complex.HochschildComplex.__init__ Failed example: TestSuite(H).run(skip="_test_category") Expected nothing Got: Failure in _test_additive_associativity: Traceback (most recent call last): ... AttributeError: 'HochschildComplex' object has no attribute '_an_element_' ...
comment:72 Changed 6 years ago by
And also not related to _an_element_
********************************************************************** File "src/sage/combinat/root_system/integrable_representations.py", line 189, in sage.combinat.root_system.integrable_representations.IntegrableRepresentation. __init__ Failed example: TestSuite(V).run() Expected nothing Got: ... Failure in _test_not_implemented_methods: Traceback (most recent call last): ... AssertionError: Not implemented method: __contains__ ...
comment:73 Changed 6 years ago by
The last patchbot report was green, so this must be a recentlyintroduced problem.
comment:74 Changed 6 years ago by
Interestingly, these failures are for classes inheriting from CategoryObject
but not Parent
.
comment:75 Changed 6 years ago by
Those failures are true bugs in HochschildComplex
and IntegrableRepresentation
: they pretend to be in some category but do not actually implement everything needed to be in that category. This ticket causes the category tests to be run (which is good) but then the tests fail.
sage: SGA = SymmetricGroupAlgebra(QQ, 3) sage: T = SGA.trivial_representation() sage: H = SGA.hochschild_complex(T) sage: H in CommutativeAdditiveGroups() True sage: H.zero() ... TypeError: 'HochschildComplex' object is not callable
and
sage: Lambda = RootSystem(['A',3,1]).weight_lattice(extended=true).fundamental_weights() sage: V = IntegrableRepresentation(Lambda[1]+Lambda[2]+Lambda[3]) sage: V in CommutativeAdditiveGroups() True sage: V.zero() ... TypeError: 'IntegrableRepresentation' object is not callable
I think the right thing to do here is to mark those tests as # known bug
.
comment:76 Changed 6 years ago by
Commit:  0fabb7a5dc50518946d1913488bf4a0444730c0f → 74041f30d8de4c15055345f14340a4f443236ffa 

Branch pushed to git repo; I updated commit sha1. New commits:
74041f3  Mark broken testsuites as # known bug

comment:77 Changed 6 years ago by
Status:  needs_work → needs_review 

comment:80 Changed 6 years ago by
Dependencies:  #21030 → #21030, #21409 

This ticket seems to cause #21409
comment:81 Changed 6 years ago by
Branch:  u/jdemeyer/optimize_method_lookup_from_the_categories_for_instances_of_cython_classes → 74041f30d8de4c15055345f14340a4f443236ffa 

Resolution:  → fixed 
Status:  positive_review → closed 
I might have some ideas... discuss in Meudon?