Opened 3 years ago
Closed 3 years ago
#23227 closed defect (fixed)
Add cpdef declarations in pxd files when overwriting a cdef method
Reported by:  roed  Owned by:  

Priority:  major  Milestone:  sage8.0 
Component:  cython  Keywords:  
Cc:  saraedum  Merged in:  
Authors:  David Roe  Reviewers:  Jeroen Demeyer 
Report Upstream:  Fixed upstream, but not in a stable release.  Work issues:  
Branch:  919de01 (Commits)  Commit:  919de01b8f0d0a5a2ee32d722d4c5ff8b018880a 
Dependencies:  Stopgaps: 
Description (last modified by )
Change History (28)
comment:1 Changed 3 years ago by
 Report Upstream changed from N/A to Reported upstream. No feedback yet.
comment:2 Changed 3 years ago by
 Description modified (diff)
comment:3 followup: ↓ 4 Changed 3 years ago by
comment:4 in reply to: ↑ 3 ; followup: ↓ 5 Changed 3 years ago by
Replying to jdemeyer:
How did you come up with this? Is there a specific concrete problem or just a theoretical bug?
Yeah, this caused a segfault that saraedum and I spent 10 hours yesterday debugging, while working on #23218. Because the vtable is screwed up, function calls behave unpredictably.
By the way, the gist link talks about "
cpdef
'd without a corresponding entry in their pxd file" while the Cython bug talks about "cpdef
override of acdef
method". These are obviously different things, so what is the problem exactly? If the problem is onlycpdef
overrides ofcdef
methods, I guess that a large majority of the entries in the gist link are false positives, making that link not very informative.
Yeah, I agree that the link is not that informative. I wrote something easy to try to get an idea of the scope of the problem, then determined that it wasn't easy to actually find the cases where the problem actually occurs (which is, indeed, where a cdef method is overwritten by a cpdef method without a corresponding entry in the pxd file). I decided that I wanted to move on and work on other stuff, so I uploaded the gist as a place to start looking.
comment:5 in reply to: ↑ 4 Changed 3 years ago by
Replying to roed:
Replying to jdemeyer:
How did you come up with this? Is there a specific concrete problem or just a theoretical bug?
Yeah, this caused a segfault that saraedum and I spent 10 hours yesterday debugging, while working on #23218. Because the vtable is screwed up, function calls behave unpredictably.
In particular, we noticed the problem for Polynomial_generic_dense
, where _add_
, _mul_
and _floordiv_
have this issue.
comment:6 followup: ↓ 7 Changed 3 years ago by
Honestly, I don't think it is realistic to fix the general problem in Sage since the underlying problem is in upstream Cython. Of course, we can fix specific cases like the Polynomial_generic_dense
that you mentioned.
comment:7 in reply to: ↑ 6 ; followup: ↓ 8 Changed 3 years ago by
Replying to jdemeyer:
Honestly, I don't think it is realistic to fix the general problem in Sage since the underlying problem is in upstream Cython. Of course, we can fix specific cases like the
Polynomial_generic_dense
that you mentioned.
Sure, and I'm fine waiting to see what Cython does. But one possible thing for them to do is to require a cpdef declaration in the pxd file, in which case this ticket can track those additions.
comment:8 in reply to: ↑ 7 ; followup: ↓ 10 Changed 3 years ago by
Replying to roed:
Replying to jdemeyer:
Honestly, I don't think it is realistic to fix the general problem in Sage since the underlying problem is in upstream Cython. Of course, we can fix specific cases like the
Polynomial_generic_dense
that you mentioned.Sure, and I'm fine waiting to see what Cython does. But one possible thing for them to do is to require a cpdef declaration in the pxd file, in which case this ticket can track those additions.
Cython will soon raise an error in these cases. I think the best thing to do is to wait until we update to a new version of Cython and change these pxd files then. I think it's worth leaving this ticket open until that point in case someone else runs into a similar problem.
comment:9 Changed 3 years ago by
 Report Upstream changed from Reported upstream. No feedback yet. to Fixed upstream, but not in a stable release.
comment:10 in reply to: ↑ 8 Changed 3 years ago by
Replying to roed:
Cython will soon raise an error in these cases.
I convinced Cython upstream to downgrade this to a warning for now. That will make our life easier, since we can first upgrade to Cython 0.26 (in beta now) and then fix this gradually.
comment:11 Changed 3 years ago by
 Dependencies set to #23360
comment:12 Changed 3 years ago by
 Branch set to u/roed/fix_cpdef
comment:13 Changed 3 years ago by
 Commit set to 99b1e3eb9e6c30eb97072a628594cca88a2ca097
Branch pushed to git repo; I updated commit sha1. New commits:
99b1e3e  Add cpdef _add_(self, other) and cpdef _mul_(self, other) all over

comment:15 Changed 3 years ago by
Why restrict to _add_
and _mul_
? (edit: you are not doing that, it's just the commit message implying that you do)
comment:16 followup: ↓ 18 Changed 3 years ago by
Yeah, I just got lazy going through and figuring out all the functions that I'd actually changed. I used the log of warnings to find the places to edit, so I think I got them all, but I haven't rebuilt from scratch to check.
comment:17 Changed 3 years ago by
 Status changed from new to needs_review
Yeah, I think this is ready for review. I don't have anything else to add to it.
comment:18 in reply to: ↑ 16 Changed 3 years ago by
Replying to roed:
Yeah, I just got lazy going through and figuring out all the functions that I'd actually changed. I used the log of warnings to find the places to edit, so I think I got them all, but I haven't rebuilt from scratch to check.
Even if you didn't catch them all, that's not really a problem.
comment:19 Changed 3 years ago by
 Branch changed from u/roed/fix_cpdef to u/jdemeyer/fix_cpdef
comment:20 Changed 3 years ago by
 Commit changed from 99b1e3eb9e6c30eb97072a628594cca88a2ca097 to e6f5cc5c4d50fdbe8d97e22e38c27118a29cc8be
 Reviewers set to Jeroen Demeyer
Rebased to 8.1.beta1
New commits:
e6f5cc5  Add cpdef _add_(self, other) and cpdef _mul_(self, other) all over

comment:21 Changed 3 years ago by
Why this?
 cpdef normalize(self) + cpdef normalize(self, include_zeroth_moment = *)
comment:22 Changed 3 years ago by
In some cases, I am wondering whether it is better to add "abstract" _add_
and _mul_
methods on a lower level. For example, instead of adding those functions to QuaternionAlgebraElement_generic
, QuaternionAlgebraElement_number_field
and QuaternionAlgebraElement_rational_field
, add them to QuaternionAlgebraElement_abstract
raising NotImplementedError
.
Concretely for this ticket, I would like to fix only the totally uncontroversial cases (for example, undoing the changes to quaternion algebra elements) and leave the rest for a followup.
comment:23 Changed 3 years ago by
 Commit changed from e6f5cc5c4d50fdbe8d97e22e38c27118a29cc8be to 919de01b8f0d0a5a2ee32d722d4c5ff8b018880a
Branch pushed to git repo; I updated commit sha1. This was a forced push. New commits:
919de01  Add declarations for cpdef arithmetic functions in .pxd files

comment:24 Changed 3 years ago by
 Status changed from needs_review to positive_review
positive_review to this strict subset of the original patch.
comment:25 Changed 3 years ago by
 Description modified (diff)
comment:26 Changed 3 years ago by
See #23600 for the followup.
comment:27 Changed 3 years ago by
 Dependencies #23360 deleted
comment:28 Changed 3 years ago by
 Branch changed from u/jdemeyer/fix_cpdef to 919de01b8f0d0a5a2ee32d722d4c5ff8b018880a
 Resolution set to fixed
 Status changed from positive_review to closed
How did you come up with this? Is there a specific concrete problem or just a theoretical bug?
By the way, the gist link talks about "
cpdef
'd without a corresponding entry in their pxd file" while the Cython bug talks about "cpdef
override of acdef
method". These are obviously different things, so what is the problem exactly? If the problem is onlycpdef
overrides ofcdef
methods, I guess that a large majority of the entries in the gist link are false positives, making that link not very informative.