Opened 2 years ago

Closed 2 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: sage-8.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:

Change History (28)

comment:1 Changed 2 years ago by roed

  • Report Upstream changed from N/A to Reported upstream. No feedback yet.

comment:2 Changed 2 years ago by roed

  • Description modified (diff)

comment:3 follow-up: Changed 2 years ago by jdemeyer

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 a cdef method". These are obviously different things, so what is the problem exactly? If the problem is only cpdef overrides of cdef methods, I guess that a large majority of the entries in the gist link are false positives, making that link not very informative.

comment:4 in reply to: ↑ 3 ; follow-up: Changed 2 years ago by 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.

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 a cdef method". These are obviously different things, so what is the problem exactly? If the problem is only cpdef overrides of cdef 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 2 years ago by roed

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 follow-up: Changed 2 years ago by 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.

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:7 in reply to: ↑ 6 ; follow-up: Changed 2 years ago by 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.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 2 years ago by roed

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 2 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • 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 2 years ago by jdemeyer

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.

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:11 Changed 2 years ago by roed

  • Dependencies set to #23360

comment:12 Changed 2 years ago by roed

  • Branch set to u/roed/fix_cpdef

comment:13 Changed 2 years ago by git

  • Commit set to 99b1e3eb9e6c30eb97072a628594cca88a2ca097

Branch pushed to git repo; I updated commit sha1. New commits:

99b1e3eAdd cpdef _add_(self, other) and cpdef _mul_(self, other) all over

comment:14 Changed 2 years ago by jdemeyer

  • Authors changed from Jeroen Demeyer to David Roe

Ready for review?

comment:15 Changed 2 years ago by jdemeyer

Why restrict to _add_ and _mul_? (edit: you are not doing that, it's just the commit message implying that you do)

Last edited 2 years ago by jdemeyer (previous) (diff)

comment:16 follow-up: Changed 2 years ago by 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.

comment:17 Changed 2 years ago by roed

  • 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 2 years ago by jdemeyer

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 2 years ago by jdemeyer

  • Branch changed from u/roed/fix_cpdef to u/jdemeyer/fix_cpdef

comment:20 Changed 2 years ago by jdemeyer

  • Commit changed from 99b1e3eb9e6c30eb97072a628594cca88a2ca097 to e6f5cc5c4d50fdbe8d97e22e38c27118a29cc8be
  • Reviewers set to Jeroen Demeyer

Rebased to 8.1.beta1


New commits:

e6f5cc5Add cpdef _add_(self, other) and cpdef _mul_(self, other) all over

comment:21 Changed 2 years ago by jdemeyer

Why this?

-    cpdef normalize(self)
+    cpdef normalize(self, include_zeroth_moment = *)

comment:22 Changed 2 years ago by jdemeyer

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 follow-up.

comment:23 Changed 2 years ago by git

  • Commit changed from e6f5cc5c4d50fdbe8d97e22e38c27118a29cc8be to 919de01b8f0d0a5a2ee32d722d4c5ff8b018880a

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

919de01Add declarations for cpdef arithmetic functions in .pxd files

comment:24 Changed 2 years ago by jdemeyer

  • Status changed from needs_review to positive_review

positive_review to this strict subset of the original patch.

comment:25 Changed 2 years ago by roed

  • Description modified (diff)

comment:26 Changed 2 years ago by roed

See #23600 for the followup.

comment:27 Changed 2 years ago by jdemeyer

  • Dependencies #23360 deleted

comment:28 Changed 2 years ago by vbraun

  • Branch changed from u/jdemeyer/fix_cpdef to 919de01b8f0d0a5a2ee32d722d4c5ff8b018880a
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.