sagemath / sage

Main repository of SageMath
https://www.sagemath.org
Other
1.43k stars 479 forks source link

Upgrade to Cython 0.29.1 #25292

Closed jdemeyer closed 5 years ago

jdemeyer commented 6 years ago

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

Follow-up tickets:

Upstream: Fixed upstream, but not in a stable release.

CC: @kiwifb @embray @saraedum

Component: packages: standard

Author: Jeroen Demeyer

Branch/Commit: 4c3e314

Reviewer: Timo Kaufmann

Issue created by migration from https://trac.sagemath.org/ticket/25292

videlec commented 6 years ago
comment:1

update milestone 8.3 -> 8.4

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -2,3 +2,5 @@

 - #25284: Move some declarations from Sage to Cython
 - #25288: Enable Cython caching again
+
+**Tarball**: https://github.com/cython/cython/archive/0.29b1.tar.gz
jdemeyer commented 6 years ago

Author: Jeroen Demeyer

jdemeyer commented 6 years ago

Dependencies: #26396

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -3,4 +3,4 @@
 - #25284: Move some declarations from Sage to Cython
 - #25288: Enable Cython caching again

-**Tarball**: https://github.com/cython/cython/archive/0.29b1.tar.gz
+**Tarball**: https://github.com/cython/cython/archive/0.29rc2.tar.gz
jdemeyer commented 6 years ago

Branch: u/jdemeyer/upgrade_to_cython_0_29

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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

3275637Explicitly set Cython language level to current Python version
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Commit: 3275637

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,8 @@
-There is no Cython 0.29 yet, but consider this as a metaticket for Cython issues which would be fixed by that.
-
-- #25284: Move some declarations from Sage to Cython
-- #25288: Enable Cython caching again
+There is no Cython 0.29 yet, but there is a release candidate

 **Tarball**: https://github.com/cython/cython/archive/0.29rc2.tar.gz
+
+Follow-up tickets:
+
+- #25288: Enable Cython caching again
+- #26403: Compile Sage with Cython language_level=3str
jdemeyer commented 6 years ago

Changed dependencies from #26396 to #26396, #26402

jdemeyer commented 6 years ago

Description changed:

--- 
+++ 
@@ -1,6 +1,4 @@
-There is no Cython 0.29 yet, but there is a release candidate
-
-**Tarball**: https://github.com/cython/cython/archive/0.29rc2.tar.gz
+**Tarball**: https://files.pythonhosted.org/packages/f0/66/6309291b19b498b672817bd237caec787d1b18013ee659f17b1ec5844887/Cython-0.29.tar.gz

 Follow-up tickets:
jdemeyer commented 6 years ago

Changed dependencies from #26396, #26402 to #26396

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from 3275637 to f77de1d

Konrad127123 commented 6 years ago
comment:13

Attachment: cython-0.29.log

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.)

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

Changed commit from f77de1d to 7ffa946

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 6 years ago

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

7ffa946Upgrade to Cython 0.29
jdemeyer commented 6 years ago

Upstream: Fixed upstream, but not in a stable release.

jdemeyer commented 6 years ago
comment:16

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

jdemeyer commented 6 years ago

Changed dependencies from #26396 to none

Konrad127123 commented 6 years ago
comment:18

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.

jdemeyer commented 6 years ago
comment:19

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

embray commented 6 years ago
comment:20

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.

tscrim commented 5 years ago
comment:22

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

embray commented 5 years ago
comment:23

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.

saraedum commented 5 years ago
comment:24

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!

jdemeyer commented 5 years ago
comment:25

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.

embray commented 5 years ago
comment:26

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

jdemeyer commented 5 years ago
comment:27

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.

embray commented 5 years ago
comment:28

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.

timokau commented 5 years ago
comment:29

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.

kiwifb commented 5 years ago
comment:30

Replying to @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.

Konrad127123 commented 5 years ago
comment:31

Note that Cython 0.29.1 has now been released.

kiwifb commented 5 years ago
comment:32

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

jdemeyer commented 5 years ago

Description changed:

--- 
+++ 
@@ -1,4 +1,4 @@
-**Tarball**: https://files.pythonhosted.org/packages/f0/66/6309291b19b498b672817bd237caec787d1b18013ee659f17b1ec5844887/Cython-0.29.tar.gz
+**Tarball**: https://files.pythonhosted.org/packages/f0/f8/7f406aac4c6919d5a4ce16509bbe059cd256e9ad94bae5ccac14094b7c51/Cython-0.29.1.tar.gz

 Follow-up tickets:
7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

Changed commit from 7ffa946 to 4c3e314

7ed8c4ca-6d56-4ae9-953a-41e42b4ed313 commented 5 years ago

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

4c3e314Upgrade to Cython 0.29.1
jdemeyer commented 5 years ago
comment:35

Not tested yet...

embray commented 5 years ago
comment:36

Replying to @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...

timokau commented 5 years ago
comment:37

Replying to @embray:

Replying to @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.

timokau commented 5 years ago
comment:38

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?

kiwifb commented 5 years ago
comment:39

Replying to @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.

timokau commented 5 years ago
comment:40

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

jdemeyer commented 5 years ago
comment:41

Replying to @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.

timokau commented 5 years ago
comment:42

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?

embray commented 5 years ago
comment:43

Replying to @jdemeyer:

Replying to @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 and that it should/will happen before long so I don't object to moving forward on this.

timokau commented 5 years ago

Reviewer: Timo Kaufmann

vbraun commented 5 years ago

Changed branch from u/jdemeyer/upgrade_to_cython_0_29 to 4c3e314