Opened 3 years ago

Closed 20 months ago

#26403 closed enhancement (fixed)

Compile Sage with Cython language_level=3str

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-9.0
Component: cython Keywords:
Cc: embray, chapoton Merged in:
Authors: Jeroen Demeyer Reviewers: Frédéric Chapoton
Report Upstream: N/A Work issues:
Branch: 58a30a8 (Commits, GitHub, GitLab) Commit: 58a30a8da95afe7299348443e2669238b5ecfd0d
Dependencies: Stopgaps:

Status badges

Description

Cython 0.29 introduces a new language_level=3str, which is like language_level=3 except that strings are still str instead of unicode. It would be good to use this language_level in Sage to force Python 3 syntax in all Cython files.

Change History (36)

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

I am still confused by the whole language_level discussion. I tried following the discussion on the cython mailing list but I gave up.

If I understand correctly (and this is probably just restating what you wrote in the ticket description) language_level=3str means it's using Python 3 syntax in all Cython files, with the exception that unprefixed string literals are non-unicode strings on Python 2?

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

Regarding your deleted comment from the other ticket (maybe it was meant for this one) about non-ASCII strings in string literals (there was an example you used with μ). If a docstring contains non-ASCII characters then I definitely think we should be explicitly using u''.

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

Replying to embray:

I am still confused by the whole language_level discussion. I tried following the discussion on the cython mailing list but I gave up.

If I understand correctly (and this is probably just restating what you wrote in the ticket description) language_level=3str means it's using Python 3 syntax in all Cython files, with the exception that unprefixed string literals are non-unicode strings on Python 2?

It's not only about string literals. With language_level=3, the global str means unicode:

sage: cython('''
....: # cython: language_level=3
....: print(str)
....: ''')
<type 'unicode'>

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

Replying to embray:

If a docstring contains non-ASCII characters then I definitely think we should be explicitly using u''.

On the other hand, I do consider it a bug that Cython disallows "μ" with language_level=3str since that's valid in both Python 2 and Python 3 (with a different meaning though).

comment:5 in reply to: ↑ 4 Changed 3 years ago by embray

Replying to jdemeyer:

Replying to embray:

If a docstring contains non-ASCII characters then I definitely think we should be explicitly using u''.

On the other hand, I do consider it a bug that Cython disallows "μ" with language_level=3str since that's valid in both Python 2 and Python 3 (with a different meaning though).

Sure it's technically valid on Python 2, but not a great practice in most cases either, and with the different meaning there's a lot of potential for confusion around it so I think they're right to disallow it (even if it's unintentionally disallowed). But I also see your point.

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

Replying to jdemeyer:

Replying to embray:

I am still confused by the whole language_level discussion. I tried following the discussion on the cython mailing list but I gave up.

If I understand correctly (and this is probably just restating what you wrote in the ticket description) language_level=3str means it's using Python 3 syntax in all Cython files, with the exception that unprefixed string literals are non-unicode strings on Python 2?

It's not only about string literals. With language_level=3, the global str means unicode:

sage: cython('''
....: # cython: language_level=3
....: print(str)
....: ''')
<type 'unicode'>

Ok, sure. But "3str" means str is just always str right? I guess I like that approach if so; it meshes well with the approach we've been already taking in Sage.

comment:7 in reply to: ↑ 6 Changed 3 years ago by jdemeyer

Replying to embray:

Ok, sure. But "3str" means str is just always str right?

Yes, str is str and "literals" are also of type str.

comment:8 Changed 3 years ago by jdemeyer

  • Dependencies changed from #25292 to #25292, #26413

comment:9 Changed 3 years ago by jdemeyer

  • Dependencies changed from #25292, #26413 to #25292, #26413, #26414

comment:10 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/compile_sage_with_cython_language_level_3str

comment:11 Changed 3 years ago by git

  • Commit set to 0ea70b5ea17f62a37e601cf6ed9802c9b3d16b22

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
0ea70b5Compile Sage with Cython language_level=3str

comment:12 Changed 3 years ago by git

  • Commit changed from 0ea70b5ea17f62a37e601cf6ed9802c9b3d16b22 to b3a8a8838f6a5fdf5b8bcd6c8c9000a710f10c6c

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

25c25eeVarious fixes to rational_power_parts()
5657422Compile Sage with Cython language_level=3str
b3a8a88Remove __future__ statements from Cython files

comment:13 Changed 2 years ago by git

  • Commit changed from b3a8a8838f6a5fdf5b8bcd6c8c9000a710f10c6c to 172bb57b991f7b90a006a20fb781da38af2e8bb4

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

732e8c5Compile Sage with Cython language_level=3str
172bb57Remove __future__ statements from Cython files

comment:14 Changed 2 years ago by jdemeyer

  • Dependencies #25292, #26413, #26414 deleted
  • Milestone changed from sage-8.4 to sage-8.7

comment:15 Changed 2 years ago by git

  • Commit changed from 172bb57b991f7b90a006a20fb781da38af2e8bb4 to b821f77d81822d2efee48385df5c58a4c619e07c

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

7c36cddExplicitly set language_level=2 for a few modules
8fb3196Compile Sage with Cython language_level=3str
b99e8daRemove __future__ statements from Cython files
b821f77Doctest fixes

comment:16 Changed 2 years ago by embray

  • Milestone changed from sage-8.7 to sage-8.8

Ticket retargeted after milestone closed (if you don't believe this ticket is appropriate for the Sage 8.8 release please retarget manually)

comment:17 Changed 2 years ago by chapoton

red branch

comment:18 Changed 2 years ago by jdemeyer

I know, but it's not ready for review anyway. This needs to wait for Cython 3.0

comment:19 Changed 2 years ago by embray

  • Milestone sage-8.8 deleted

As the Sage-8.8 release milestone is pending, we should delete the sage-8.8 milestone for tickets that are not actively being worked on or that still require significant work to move forward. If you feel that this ticket should be included in the next Sage release at the soonest please set its milestone to the next release milestone (sage-8.9).

comment:20 Changed 22 months ago by git

  • Commit changed from b821f77d81822d2efee48385df5c58a4c619e07c to ae1f7f451251f40f3eb2e73adcfb5055fd273644

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

dbcb78cExplicitly set language_level=2 for a few modules
0f789d8Compile Sage with Cython language_level=3str
aab046dRemove __future__ statements from Cython files
ae1f7f4Doctest fixes

comment:21 Changed 22 months ago by jdemeyer

  • Status changed from new to needs_review

comment:22 Changed 22 months ago by chapoton

red branch. I will review if you merge

comment:23 Changed 22 months ago by chapoton

  • Status changed from needs_review to needs_work

comment:24 Changed 21 months ago by embray

I think this just needs a rebase + testing again. This should be done as a dependency of #28000.

comment:25 Changed 21 months ago by chapoton

  • Branch changed from u/jdemeyer/compile_sage_with_cython_language_level_3str to public/ticket/26403
  • Commit changed from ae1f7f451251f40f3eb2e73adcfb5055fd273644 to 5a330134f1f77a8c6b2d039684584edf8f145d51
  • Status changed from needs_work to needs_review

branch merged with 8.9.rc0


New commits:

5a33013Merge branch 'u/jdemeyer/compile_sage_with_cython_language_level_3str' in 8.9.rc0

comment:26 Changed 21 months ago by chapoton

Three patchbots seems to be happy, both with py2 and py3. Erik, would you approve a positive review ?

comment:27 follow-up: Changed 21 months ago by chapoton

Could please somebody else approve ? Or should I set to positive all by myself ?

comment:28 Changed 21 months ago by chapoton

  • Milestone set to sage-9.0
  • Reviewers set to Frédéric Chapoton
  • Status changed from needs_review to positive_review

In the absence of any reaction whatsoever, I am setting to positive.

comment:29 Changed 21 months ago by vbraun

  • Status changed from positive_review to needs_work

merge conflict...

comment:30 in reply to: ↑ 27 Changed 21 months ago by embray

Replying to chapoton:

Could please somebody else approve ? Or should I set to positive all by myself ?

Yeah why shouldn't you? You reviewed the change and tested it, didn't you? You don't need me to do that.

comment:31 Changed 21 months ago by git

  • Commit changed from 5a330134f1f77a8c6b2d039684584edf8f145d51 to 0a34ee961537652de9e61352670b2b6383be3d70

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

0a34ee9Merge branch 'public/ticket/26403' in 9.0.b1

comment:32 Changed 21 months ago by chapoton

  • Status changed from needs_work to positive_review

conflicts resolved, setting back to positive

comment:33 Changed 20 months ago by vbraun

  • Status changed from positive_review to needs_work

I'm getting the following failure on 32-bit:

File "src/sage/plot/plot3d/shapes.pyx", line 947, in sage.plot.plot3d.shapes.Sphere.get_grid
Failed example:
    Sphere(1).get_grid(100)
Expected:
    ([-10.0, ..., 0.0, ..., 10.0],
     [0.0, ..., 3.141592653589793, ..., 0.0])
Got:
    ([-10.0,
      -1.0471975511965976,
      -0.5235987755982988,
      6.1257422745431e-17,
      0.5235987755982989,
      1.0471975511965979,
      10.0],
     [0.0,
      1.0471975511965979,
      2.0943951023931957,
      3.141592653589793,
      4.188790204786391,
      5.235987755982989,
      0.0])
**********************************************************************
1 item had failures:
   1 of   3 in sage.plot.plot3d.shapes.Sphere.get_grid
    [162 tests, 1 failure, 43.51 s]
----------------------------------------------------------------------
sage -t --long src/sage/plot/plot3d/shapes.pyx  # 1 doctest failed
----------------------------------------------------------------------

comment:34 Changed 20 months ago by git

  • Commit changed from 0a34ee961537652de9e61352670b2b6383be3d70 to 58a30a8da95afe7299348443e2669238b5ecfd0d

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

58a30a8trac 26403 fix 32bit doctest

comment:35 Changed 20 months ago by chapoton

  • Status changed from needs_work to positive_review

ok, fixed

comment:36 Changed 20 months ago by vbraun

  • Branch changed from public/ticket/26403 to 58a30a8da95afe7299348443e2669238b5ecfd0d
  • Resolution set to fixed
  • Status changed from positive_review to closed
Note: See TracTickets for help on using tickets.