Opened 3 years ago

Closed 3 years ago

#26402 closed enhancement (fixed)

Set language_level for cython() command

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-8.4
Component: python3 Keywords:
Cc: embray, chapoton Merged in:
Authors: Jeroen Demeyer Reviewers: Travis Scrimshaw
Report Upstream: N/A Work issues:
Branch: 3bb37c1 (Commits, GitHub, GitLab) Commit: 3bb37c135c55197ac824c7b019065f9a438acd5e
Dependencies: Stopgaps:

Status badges

Description

Explicitly set the default language_level for the cython() to that of the currently running Python version. This is needed because Cython 0.29 will warn if no language_level is given.

It makes sense to use language_level=3 when running Sage on Python 3 to have the user's Cython code run more consistently with the user's Python code. By keeping language_level=2 on Python 2, this is not a breaking change.

Change History (15)

comment:1 Changed 3 years ago by jdemeyer

  • Branch set to u/jdemeyer/set_language_level_for_cython___command

comment:2 Changed 3 years ago by jdemeyer

  • Commit set to 3bb37c135c55197ac824c7b019065f9a438acd5e
  • Status changed from new to needs_review

New commits:

​3bb37c1Explicitly set Cython language level to current Python version

comment:3 Changed 3 years ago by jdemeyer

(edit: wrong ticket)

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

comment:4 Changed 3 years ago by tscrim

  • Reviewers set to Travis Scrimshaw
  • Status changed from needs_review to positive_review

Makes sense.

comment:5 Changed 3 years ago by embray

I thought you were against this, last I proposed it. What changed your mind? Are you sure this won't cause a huge regression in the ongoing Python 3 port?

comment:6 Changed 3 years ago by embray

Nevermind, I misunderstood. This is just for cython(...), not the Sage sources themselves.

comment:7 Changed 3 years ago by chapoton

Has this been tested against a python3 build of sage, just to be sure ?

comment:8 Changed 3 years ago by tscrim

No, I don't have a python3 build.

comment:9 Changed 3 years ago by tscrim

Well, to be specific, I did not test it on python3. I don't know if Jeroen did or not.

comment:10 Changed 3 years ago by jdemeyer

I haven't really been following the Python 3 porting effort lately, so I don't even know what testing against a python3 build of sage would mean.

comment:11 Changed 3 years ago by chapoton

This would mean

1) check that sage still builds with python3 with this branch applied. If not, I would not agree with the ticket.

2) check that the number of failing modules and doctests does not grow. If it grows or explodes, I would be rather unhappy. There is a script on #26212 that can extract the data from a doctest log.

Instructions for building sage with python3 are at the bottom of

​https://wiki.sagemath.org/Python3-compatible%20code

comment:12 Changed 3 years ago by jdemeyer

Can't we just apply this branch and then fix any failing doctests later? There are only a few tests using cython(), so there cannot be a lot of breakage.

comment:13 Changed 3 years ago by jdemeyer

In Python 3, I manually checked all doctests from the regular expression sage:.*cython[(] and didn't see any breakage.

comment:14 Changed 3 years ago by embray

I agree; I think it's low probability of breaking much.

comment:15 Changed 3 years ago by vbraun

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