Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#18927 closed enhancement (fixed)

Upgrade Cython to 0.23.1

Reported by: jdemeyer Owned by:
Priority: major Milestone: sage-6.9
Component: packages: standard Keywords:
Cc: robertwb, fbissey, borassi Merged in:
Authors: Jeroen Demeyer Reviewers: François Bissey
Report Upstream: N/A Work issues:
Branch: b6e57c4 (Commits, GitHub, GitLab) Commit:
Dependencies: Stopgaps:

Status badges

Description (last modified by vbraun)

Upstream: http://cython.org/release/Cython-0.23.1.tar.gz


Features added

  • Support for coverage.py 4.0+ can be enabled by adding the plugin "Cython.Coverage" to the ".coveragerc" config file.
  • Annotated HTML source pages can integrate (XML) coverage reports.
  • Tracing is supported in nogil functions/sections and module init code.
  • When generators are used in a Cython module and the module imports the modules "inspect" and/or "asyncio", Cython enables interoperability by patching these modules during the import to recognise Cython's internal generator and coroutine types. This can be disabled by C compiling the module with "-D CYTHON_PATCH_ASYNCIO=0" or "-D CYTHON_PATCH_INSPECT=0"
  • When generators or coroutines are used in a Cython module, their types are registered with the Generator and Coroutine ABCs in the collections or collections.abc stdlib module at import time to enable interoperability with code that needs to detect and process Python generators/coroutines. These ABCs were added in CPython 3.5 and are available for older Python versions through the backports_abc module on PyPI. See https://bugs.python.org/issue24018
  • Adding/subtracting/dividing/modulus and equality comparisons with constant Python floats and small integers are faster.
  • Binary and/or/xor/rshift operations with small constant Python integers are faster.
  • When called on generator expressions, the builtins all(), any(), dict(), list(), set(), sorted() and unicode.join() avoid the generator iteration overhead by inlining a part of their functionality into the for-loop.
  • Keyword argument dicts are no longer copied on function entry when they are not being used or only passed through to other function calls (e.g. in wrapper functions).
  • wraparound() and boundscheck() are available as no-ops in pure Python mode.
  • Const iterators were added to the provided C++ STL declarations.
  • NULL is allowed as default argument when embedding signatures. This fixes ticket 843.
  • When compiling with --embed, the internal module name is changed to main to allow arbitrary program names, including those that would be invalid for modules. Note that this prevents reuse of the generated C code as an importable module.
  • External C++ classes that overload the assignment operator can be used. Patch by Ian Henriksen.

Bugs fixed

  • Language level 3 did not enable true division (a.k.a. float division) for integer operands.
  • Functions with fused argument types that included a generic 'object' fallback could end up using that fallback also for other explicitly listed object types.
  • Relative cimports could accidentally fall back to trying an absolute cimport on failure.
  • The result of calling a C struct constructor no longer requires an intermediate assignment when coercing to a Python dict.
  • C++ exception declarations with mapping functions could fail to compile when pre-declared in .pxd files.
  • cpdef void methods are now permitted.
  • abs(cint) could fail to compile in MSVC and used sub-optimal code in C++. Patch by David Vierra, original patch by Michael Enßlin.
  • Buffer index calculations using index variables with small C integer types could overflow for large buffer sizes. Original patch by David Vierra.
  • C unions use a saner way to coerce from and to Python dicts.
  • When compiling a module foo.pyx, the directories in sys.path are no longer searched when looking for foo.pxd. Patch by Jeroen Demeyer.
  • Memory leaks in the embedding main function were fixed. Original patch by Michael Enßlin.
  • Some complex Python expressions could fail to compile inside of finally clauses.

Other changes

  • Changed mangling scheme in header files generated by cdef api declarations.

Change History (31)

comment:1 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:2 Changed 6 years ago by jdemeyer

  • Branch set to u/jdemeyer/upgrade_cython_to_0_23

comment:3 Changed 6 years ago by jdemeyer

  • Commit set to 8fb35d747ae62e9655c832d79df2584b342e70bc
  • Description modified (diff)

comment:4 Changed 6 years ago by fbissey

  • Cc fbissey added

comment:5 Changed 6 years ago by jdemeyer

  • Description modified (diff)

comment:6 Changed 6 years ago by git

  • Commit changed from 8fb35d747ae62e9655c832d79df2584b342e70bc to 4d35d080f69f6c702f7fba5de19a201e984d5858

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

4d35d08Upgrade to Cython 0.23 + add testsuite [work in progress]

comment:7 Changed 6 years ago by fbissey

And it is now officially released. I am ready to put this to positive review once we have a tarball.

comment:8 Changed 6 years ago by jdemeyer

  • Description modified (diff)
  • Milestone changed from sage-6.8 to sage-6.9

comment:9 Changed 6 years ago by git

  • Commit changed from 4d35d080f69f6c702f7fba5de19a201e984d5858 to 1185b2c839d3bb06b35a7b7f07061fb0fc6dde0f

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

1185b2cUpgrade to Cython 0.23 + add testsuite

comment:10 Changed 6 years ago by jdemeyer

  • Status changed from new to needs_review

comment:11 Changed 6 years ago by fbissey

  • Reviewers set to François Bissey
  • Status changed from needs_review to positive_review

comment:12 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/upgrade_cython_to_0_23 to 1185b2c839d3bb06b35a7b7f07061fb0fc6dde0f
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:13 Changed 6 years ago by vbraun

  • Commit 1185b2c839d3bb06b35a7b7f07061fb0fc6dde0f deleted

The testsuite fails on 32-bit for the record: http://trac.cython.org/ticket/857

comment:14 Changed 6 years ago by vbraun

  • Resolution fixed deleted
  • Status changed from closed to new

Reopening as it fails on some buildbots with error: reference to ‘uint64_t’ is ambiguous. I asked at https://groups.google.com/d/msg/cython-users/nOaoFjt8BLo/RI3vtsOMDwAJ for advice.

comment:15 Changed 6 years ago by vbraun

  • Branch changed from 1185b2c839d3bb06b35a7b7f07061fb0fc6dde0f to u/jdemeyer/upgrade_cython_to_0_23
  • Commit set to 1185b2c839d3bb06b35a7b7f07061fb0fc6dde0f

New commits:

1185b2cUpgrade to Cython 0.23 + add testsuite

comment:16 Changed 6 years ago by vbraun

  • Branch changed from u/jdemeyer/upgrade_cython_to_0_23 to u/vbraun/upgrade_cython_to_0_23

comment:17 Changed 6 years ago by vbraun

  • Cc borassi added
  • Commit changed from 1185b2c839d3bb06b35a7b7f07061fb0fc6dde0f to 6608075748469195a6d63dcae5651db6cb8ec9f4
  • Status changed from new to needs_review

Regardless of how the cython list answers we should get rid of the global using namespace statements.


New commits:

6608075global "using namespace foo" statements are bad practice

comment:18 Changed 6 years ago by fbissey

I think it is a good idea but I cannot check the impact on a 32bit platform.

comment:19 Changed 6 years ago by vbraun

This shouldn't have any impact on 32-bit platforms, it just avoids kneecapping the compilation unit from the get-go.

comment:20 Changed 6 years ago by vbraun

  • Description modified (diff)
  • Summary changed from Upgrade Cython to 0.23 to Upgrade Cython to 0.23.1

comment:21 Changed 6 years ago by git

  • Commit changed from 6608075748469195a6d63dcae5651db6cb8ec9f4 to 420cdf22f3c34dee0fbf03d96aecf4ac28f84faa

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

420cdf2Update to Cython-0.23.1

comment:22 Changed 6 years ago by vbraun

Can somebody click on positive review?

comment:23 Changed 6 years ago by fbissey

  • Status changed from needs_review to positive_review

There.

comment:24 Changed 6 years ago by git

  • Commit changed from 420cdf22f3c34dee0fbf03d96aecf4ac28f84faa to b6e57c46b714b1eb811a3a62fccfc37b6b5ff2ff
  • Status changed from positive_review to needs_review

Branch pushed to git repo; I updated commit sha1 and set ticket back to needs_review. New commits:

8d8504cMerge Sage-6.9.beta3 into t/18927/upgrade_cython_to_0_23
b6e57c4more boost interface namespace fixes

comment:25 Changed 6 years ago by vbraun

  • Status changed from needs_review to positive_review

In the meantime more stuff without namespace appeared...

comment:26 Changed 6 years ago by fbissey

Marginally related in a way. Should we have a separate audit of namespace and include a bit about them in the review section of the documentation?

comment:27 Changed 6 years ago by vbraun

Really we should put C++ Sage stuff into a sage namespace, then its safe to pull symbols in via using declarations

namespace sage {
    namespace graph {
        using std::vector;
        using boost:make_iterator_property_map;
        [...]
    }
}

comment:28 Changed 6 years ago by fbissey

hum... Definitely need the namespace(s), that's just asking to be clobbered.

comment:29 Changed 6 years ago by borassi

Hello!

Thank you very much for telling me that using namespace is bad practice, and for correcting my code in boost_interface.cpp.

However, I have used vectors also in Cython, in files boost_graph.pyx and boost_graph.pxd, and I haven't used namespaces (for instance, by writing vector and not std::vector). Is it fine? Otherwise, what is the correct way to do it?

Best,

Michele

comment:30 Changed 6 years ago by vbraun

  • Branch changed from u/vbraun/upgrade_cython_to_0_23 to b6e57c46b714b1eb811a3a62fccfc37b6b5ff2ff
  • Resolution set to fixed
  • Status changed from positive_review to closed

comment:31 Changed 6 years ago by vbraun

  • Commit b6e57c46b714b1eb811a3a62fccfc37b6b5ff2ff deleted

In Python its ok to import into the module namespace "from libcpp.vector cimport vector". Since there are no #include statements this will not cause conflicts in third-party headers. Its only in C++ that

using namespace foo;
#include <bar>

will then make bar see all of foo's symbols (and potentially trip over some).

Actuall in Cython you have pxi files which behave like #include, but you shouldn't use them if you can avoid it; they are mostly for historical reasons.

Note: See TracTickets for help on using tickets.