Opened 3 years ago

Closed 3 years ago

#25292 closed enhancement (fixed)

Upgrade to Cython 0.29.1

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.5
Component: packages: standard Keywords:
Cc: fbissey, embray, saraedum Merged in:
Authors: Jeroen Demeyer Reviewers: Timo Kaufmann
Report Upstream: Fixed upstream, but not in a stable release. Work issues:
Branch: 4c3e314 (Commits, GitHub, GitLab) Commit: 4c3e31492939d3de3bff61755045afe0dd40ff00
Dependencies: Stopgaps:

Status badges

Description (last modified by jdemeyer)

Tarball: https://files.pythonhosted.org/packages/f0/f8/7f406aac4c6919d5a4ce16509bbe059cd256e9ad94bae5ccac14094b7c51/Cython-0.29.1.tar.gz

Follow-up tickets:

  • #25288: Enable Cython caching again
  • #26403: Compile Sage with Cython language_level=3str

Attachments (1)

cython-0.29.log (1.3 MB) - added by Konrad127123 3 years ago.

Download all attachments as: .zip

Change History (46)

comment:1 Changed 3 years ago by vdelecroix

  • Milestone changed from sage-8.3 to sage-8.4

update milestone 8.3 -> 8.4

comment:2 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:3 Changed 3 years ago by fbissey

  • Cc fbissey added

comment:4 Changed 3 years ago by jdemeyer

  • Authors set to Jeroen Demeyer
  • Dependencies set to #26396

comment:5 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/upgrade_to_cython_0_29

comment:7 Changed 3 years ago by git

  • Commit set to 32756374dc60e6a34730ae05424a3467701b1e9b

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

3275637Explicitly set Cython language level to current Python version

comment:8 Changed 3 years ago by jdemeyer

  • Dependencies changed from #26396 to #26396, #26402
  • Description modified (diff)

comment:9 Changed 3 years ago by jdemeyer

  • Description modified (diff)

comment:10 Changed 3 years ago by jdemeyer

  • Dependencies changed from #26396, #26402 to #26396

comment:11 Changed 3 years ago by git

  • Commit changed from 32756374dc60e6a34730ae05424a3467701b1e9b to f77de1d0e7f90ee12761140500cb8cbbb789ab20

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

bcba4f1Upgrade to cypari2-1.3.1
f77de1dUpgrade to Cython 0.29

comment:12 Changed 3 years ago by jdemeyer

  • Status changed from new to needs_review

Changed 3 years ago by Konrad127123

comment:13 follow-up: Changed 3 years ago by Konrad127123

Running make distclean && make cython with SAGE_CHECK=yes, I'm reproducibly getting one test failing (see log).

(The two failing tests in #26450 don't seem to be an issue anymore in 0.29.)

comment:14 Changed 3 years ago by git

  • Commit changed from f77de1d0e7f90ee12761140500cb8cbbb789ab20 to 7ffa94631d3a74861bdde7c467757822297c24e7

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

7ffa946Upgrade to Cython 0.29

comment:15 Changed 3 years ago by jdemeyer

  • Report Upstream changed from N/A to Fixed upstream, but not in a stable release.

comment:16 in reply to: ↑ 13 ; follow-up: Changed 3 years ago by jdemeyer

Replying to Konrad127123:

Running make distclean && make cython with SAGE_CHECK=yes, I'm reproducibly getting one test failing (see log).

Fixed by https://github.com/cython/cython/pull/2674

comment:17 Changed 3 years ago by jdemeyer

  • Dependencies #26396 deleted

comment:18 in reply to: ↑ 16 Changed 3 years ago by Konrad127123

Replying to jdemeyer:

Replying to Konrad127123:

Running make distclean && make cython with SAGE_CHECK=yes, I'm reproducibly getting one test failing (see log).

Fixed by https://github.com/cython/cython/pull/2674

I can confirm that cython now builds with SAGE_CHECK=yes. I've not tested it apart from that.

comment:19 Changed 3 years ago by jdemeyer

  • Cc embray saraedum added

Can somebody review this please? There are follow-up tickets depending on this one.

comment:20 Changed 3 years ago by embray

Does this mean Cython 0.29 will become a hard dependency of Sage? I don't mind that ofc, but maybe the tickets that depend on it can be done in such a way that checks this (e.g., checks the Cython version before enabling those features).

I'm going to give this a quick check on Cygwin, but I doubt there will be any problem, so positive_review from me if it that works.

comment:21 Changed 3 years ago by embray

  • Milestone changed from sage-8.4 to sage-8.5

comment:22 Changed 3 years ago by tscrim

Erik, any updates? This works for me, and I am ready to set a positive review.

comment:23 follow-up: Changed 3 years ago by embray

Works for me, AFAICT. I think my previous question "Does this mean Cython 0.29 will become a hard dependency of Sage?" should be answered though.

comment:24 Changed 3 years ago by saraedum

I can only really talk about the cycache ticket. There we could of course only enable caching when we find that it actually works.

I guess the same could be done for the other ticket but I do not know enough about it.

Anyway, sure, let's keep that in mind!

comment:25 in reply to: ↑ 23 Changed 3 years ago by jdemeyer

Replying to embray:

Does this mean Cython 0.29 will become a hard dependency of Sage?

Yes. But I don't see why that should be a problem.

comment:26 Changed 3 years ago by embray

It's not necessarily a problem. It just means that the Cython package on any system that packages Sage must also be updated to 0.29. This would be a good thing to do anyways: it's just not obvious that it's so simple. Sage tests Cython very well, but rebuilding every debian package that relies on Cython is an even bigger test. I think it will probably go fine but we do need to make sure it gets done...

comment:27 follow-ups: Changed 3 years ago by jdemeyer

With only this ticket applied (not the follow-up tickets), it requires only minor adjustments to support both Cython 0.28 and Cython 0.29.

So what you are saying is maybe more an argument to postpone the follow-up tickets until we know for sure that Debian is ready for Cython 0.29.

comment:28 in reply to: ↑ 27 Changed 3 years ago by embray

Replying to jdemeyer:

With only this ticket applied (not the follow-up tickets), it requires only minor adjustments to support both Cython 0.28 and Cython 0.29.

So what you are saying is maybe more an argument to postpone the follow-up tickets until we know for sure that Debian is ready for Cython 0.29.

That might be wise, as annoying as it might be. Hopefully it won't be long though. Also I'm just using Debian as a general barometer.

comment:29 follow-ups: Changed 3 years ago by gh-timokau

For what its worth, here is the status of cython on various distros and these are the distros that package sage. Debian Unstable hasn't upgraded yet. Arch and Nix have. Gentoo isn't listed for some reason.

comment:30 in reply to: ↑ 29 Changed 3 years ago by fbissey

Replying to gh-timokau:

For what its worth, here is the status of cython on various distros and these are the distros that package sage. Debian Unstable hasn't upgraded yet. Arch and Nix have. Gentoo isn't listed for some reason.

Gentoo main tree, doesn't have it yet. But the sage-on-gentoo repo does have a package ready for when this ticket makes it regardless of the main tree.

comment:31 Changed 3 years ago by Konrad127123

Note that Cython 0.29.1 has now been released.

comment:32 Changed 3 years ago by fbissey

Actually the Gentoo main tree caught up to me yesterday and 0.29.1 is in the main tree.

comment:33 Changed 3 years ago by jdemeyer

  • Description modified (diff)
  • Status changed from needs_review to needs_work
  • Summary changed from Upgrade to Cython 0.29 to Upgrade to Cython 0.29.1

comment:34 Changed 3 years ago by git

  • Commit changed from 7ffa94631d3a74861bdde7c467757822297c24e7 to 4c3e31492939d3de3bff61755045afe0dd40ff00

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

4c3e314Upgrade to Cython 0.29.1

comment:35 Changed 3 years ago by jdemeyer

  • Status changed from needs_work to needs_review

Not tested yet...

comment:36 in reply to: ↑ 29 ; follow-up: Changed 3 years ago by embray

Replying to gh-timokau:

For what its worth, here is the status of cython on various distros and these are the distros that package sage. Debian Unstable hasn't upgraded yet. Arch and Nix have. Gentoo isn't listed for some reason.

That's a cool site--I was not aware of it before. It doesn't look like it tracks conda-forge or cygwin packages either. I wonder how hard it would be to add trackers for new repositories to the site...

comment:37 in reply to: ↑ 36 Changed 3 years ago by gh-timokau

Replying to embray:

Replying to gh-timokau:

For what its worth, here is the status of cython on various distros and these are the distros that package sage. Debian Unstable hasn't upgraded yet. Arch and Nix have. Gentoo isn't listed for some reason.

That's a cool site--I was not aware of it before. It doesn't look like it tracks conda-forge or cygwin packages either. I wonder how hard it would be to add trackers for new repositories to the site...

Yeah its pretty neat. Nix uses a bot that automatically generates trivial package update PRs based on repology data. I think adding support for another repo would basically sonsist of (1) finding a relibale data source that gives current metadata about all packages and (2) converting that datasource into the format repology expects. There are already issues for conda-forge and cygwin.

comment:38 in reply to: ↑ 27 ; follow-up: Changed 3 years ago by gh-timokau

Replying to jdemeyer:

With only this ticket applied (not the follow-up tickets), it requires only minor adjustments to support both Cython 0.28 and Cython 0.29.

So what you are saying is maybe more an argument to postpone the follow-up tickets until we know for sure that Debian is ready for Cython 0.29.

Back to topic: So in its current state this ticket is not backwards compatible?

comment:39 in reply to: ↑ 29 Changed 3 years ago by fbissey

Replying to gh-timokau:

For what its worth, here is the status of cython on various distros and these are the distros that package sage. Debian Unstable hasn't upgraded yet. Arch and Nix have. Gentoo isn't listed for some reason.

The curating is imperfect and for some distros it is listed under "cython" and for other under "python:cython" https://repology.org/metapackage/python:cython/versions not much you can do about that.

comment:40 Changed 3 years ago by gh-timokau

You can report it (I just did). Every time I've done that it has been resolved within a couple of hours.

comment:41 in reply to: ↑ 38 ; follow-up: Changed 3 years ago by jdemeyer

Replying to gh-timokau:

Back to topic: So in its current state this ticket is not backwards compatible?

No, it is not. The difference is really small though, packagers that insist on Cython 0.28 can easily revert this patch.

comment:42 Changed 3 years ago by gh-timokau

Okay since the majority have already switched (all but debian I think) and its very possible that debian will have switched by the time 8.5 is released, I think we can go forward. Any other opinions?

comment:43 in reply to: ↑ 41 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to gh-timokau:

Back to topic: So in its current state this ticket is not backwards compatible?

No, it is not. The difference is really small though, packagers that insist on Cython 0.28 can easily revert this patch.

I think that's a bit presumptuous. But I agree there's no reason for anyone to hold back up on updating Cython that that it should/will happen before long so I don't object to moving forward on this.

Version 0, edited 3 years ago by embray (next)

comment:44 Changed 3 years ago by gh-timokau

  • Reviewers set to Timo Kaufmann
  • Status changed from needs_review to positive_review

comment:45 Changed 3 years ago by vbraun

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