Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Pyximport does not use cythonize() internally. #2304

Closed
gabrieldemarmiesse opened this issue May 29, 2018 · 4 comments · Fixed by #4339
Closed

Pyximport does not use cythonize() internally. #2304

gabrieldemarmiesse opened this issue May 29, 2018 · 4 comments · Fixed by #4339

Comments

@gabrieldemarmiesse
Copy link
Contributor

gabrieldemarmiesse commented May 29, 2018

From @scoder 's comment in #2298 about pyximport internals:

Right, and that is totally a bug. It should really use cythonize(). Definitely something to list under "limitations" below.

Since pyximport does not use Cythonize internally, it prevents the use of compiler directives at the top of the .py or .pyx files. Also, using Cythonize could potentially remove quite a bit of code (potentially, all the code in cython/pyximport/pyxbuild.py could be removed).

@scoder scoder added this to the 3.0 milestone Apr 13, 2020
@navytux
Copy link
Contributor

navytux commented Apr 22, 2020

Indeed pyximport does not currently use cythonize internally as it is currently building on old_build_ext:

from Cython.Distutils.old_build_ext import old_build_ext as build_ext

Besides preventing to use compiler directives at the top of .py or .pyx, it also makes pyximport to miss recompiling when build dependency change (see e.g. #3121).

So if we switch pyximport to use new_build_ext, or to use just from Cython.Distutils import build_ext and switch that build_ext to new implementation that is based on cythonize (#3541), then this issue will be automatically fixed.

@ghost
Copy link

ghost commented Oct 30, 2020

I forgot where this was documented but I have had success in getting pyximport to use cythonize by creating a "modulename.pyxbld" file beside the "modulename.pyx" file. This gets pyximport to use the result of cythonize. In my experience, the compiler directives will now work. The resulting .pyxbld file looks like this:

# modulename.pyxbld

from Cython.Build import cythonize

def make_ext(modname, pyxfilename):
    return cythonize(pyxfilename, language_level = 3, annotate = True)[0]

@scoder
Copy link
Contributor

scoder commented Nov 7, 2020

PR very welcome that does what @navytux and @gabrieldemarmiesse have described above.

@mattip
Copy link
Contributor

mattip commented Feb 10, 2021

Setuptools has distutils extensions to allow extending the universe of commands and options available when building an Extension.

For instance, CFFI adds a setuptools entry point distutils.setup_keywords function cffi_modules so you can do this in setup.py:

from setuptools import setup

setup(
    name="example",
    version="0.1",
    py_modules=["readdir"],
    setup_requires=["cffi>=1.0.dev0"],
    cffi_modules=["readdir_build.py:ffi"],
    install_requires=["cffi>=1.0.dev0"],
    zip_safe=False,
)

The function cffi_modules's signature is

def cffi_modules(dist, attr, value):
    assert attr == 'cffi_modules'

where dist is a Distribution, value is ["readdir_build.py:ffi"], and the call adds a new Extension to dist.

Maybe cython could add something similar: add a distutils.setup_keywords function something like cython_modules=["myfile1.pyx", "myfile2.pxd:myfile2.pyx"]. The cython_module function would add a Extension to dist for each value. This would encapsulate the call to cythonize as a separate step, so cython would not override build_ext but could pass it compilation flags.

Another option would be to add a distutils.command for cythonize, but I am not sure how to tie that in so that setup.py build_ext would notice it needs to convert pyx files to c via cythonize.

I willing to put some work into this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants